Re: [PATCH 01/10] gdth: refactor ioc_general

2018-12-06 Thread Finn Thain 


On Thu, 6 Dec 2018, Christoph Hellwig wrote:

> This function is a huge mess with duplicated error handling.  Split out
> a few useful helpers and use goto labels to untangle the error handling
> and no-data ioctl handling.
> 
> Signed-off-by: Christoph Hellwig 
> ---
>  drivers/scsi/gdth.c | 244 +++-
>  1 file changed, 126 insertions(+), 118 deletions(-)
> 
> diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
> index 16709735b546..45e67d4cb3af 100644
> --- a/drivers/scsi/gdth.c
> +++ b/drivers/scsi/gdth.c
> @@ -4155,131 +4155,139 @@ static int ioc_resetdrv(void __user *arg, char 
> *cmnd)
>  return 0;
>  }
>  
> -static int ioc_general(void __user *arg, char *cmnd)
> +static void gdth_ioc_addr32(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
>  {
> -gdth_ioctl_general gen;
> -char *buf = NULL;
> -u64 paddr; 
> -gdth_ha_str *ha;
> -int rval;
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache.DestAddr = 0x;
> + gen->command.u.cache.sg_canz = 1;
> + gen->command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> + gen->command.u.cache.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache.DestAddr = paddr;
> + gen->command.u.cache.sg_canz = 0;
> + }
> +}
>  
> -if (copy_from_user(, arg, sizeof(gdth_ioctl_general)))
> -return -EFAULT;
> -ha = gdth_find_ha(gen.ionode);
> -if (!ha)
> -return -EFAULT;
> +static void gdth_ioc_addr64(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & SCATTER_GATHER) {
> + gen->command.u.cache64.DestAddr = (u64)-1;
> + gen->command.u.cache64.sg_canz = 1;
> + gen->command.u.cache64.sg_lst[0].sg_ptr = paddr;
> + gen->command.u.cache64.sg_lst[0].sg_len = gen->data_len;
> + gen->command.u.cache64.sg_lst[1].sg_len = 0;
> + } else {
> + gen->command.u.cache64.DestAddr = paddr;
> + gen->command.u.cache64.sg_canz = 0;
> + }
> +}
>  
> -if (gen.data_len > INT_MAX)
> -return -EINVAL;
> -if (gen.sense_len > INT_MAX)
> -return -EINVAL;
> -if (gen.data_len + gen.sense_len > INT_MAX)
> -return -EINVAL;
> +static void gdth_ioc_cacheservice(gdth_ha_str *ha, gdth_ioctl_general *gen,
> + u64 paddr)
> +{
> + if (ha->cache_feat & GDT_64BIT) {
> + /* copy elements from 32-bit IOCTL structure */
> + gen->command.u.cache64.BlockCnt = gen->command.u.cache.BlockCnt;
> + gen->command.u.cache64.BlockNo = gen->command.u.cache.BlockNo;
> + gen->command.u.cache64.DeviceNo = gen->command.u.cache.DeviceNo;
>  
> -if (gen.data_len + gen.sense_len != 0) {
> -if (!(buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len,
> - FALSE, )))
> -return -EFAULT;
> -if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),  
> -   gen.data_len + gen.sense_len)) {
> -gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
> -return -EFAULT;
> -}
> + gdth_ioc_addr64(ha, gen, paddr);
> + } else {
> + gdth_ioc_addr32(ha, gen, paddr);
> + }
> +}
>  
> -if (gen.command.OpCode == GDT_IOCTL) {
> -gen.command.u.ioctl.p_param = paddr;
> -} else if (gen.command.Service == CACHESERVICE) {
> -if (ha->cache_feat & GDT_64BIT) {
> -/* copy elements from 32-bit IOCTL structure */
> -gen.command.u.cache64.BlockCnt = 
> gen.command.u.cache.BlockCnt;
> -gen.command.u.cache64.BlockNo = gen.command.u.cache.BlockNo;
> -gen.command.u.cache64.DeviceNo = 
> gen.command.u.cache.DeviceNo;
> -/* addresses */
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache64.DestAddr = (u64)-1;
> -gen.command.u.cache64.sg_canz = 1;
> -gen.command.u.cache64.sg_lst[0].sg_ptr = paddr;
> -gen.command.u.cache64.sg_lst[0].sg_len = gen.data_len;
> -gen.command.u.cache64.sg_lst[1].sg_len = 0;
> -} else {
> -gen.command.u.cache64.DestAddr = paddr;
> -gen.command.u.cache64.sg_canz = 0;
> -}
> -} else {
> -if (ha->cache_feat & SCATTER_GATHER) {
> -gen.command.u.cache.DestAddr = 0x;
> -gen.command.u.cache.sg_canz = 1;
> -gen.command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
> -gen.command.u.cache.sg_lst[0].sg_len = gen.data_len;
> -

[PATCH 01/10] gdth: refactor ioc_general

2018-12-06 Thread Christoph Hellwig 
This function is a huge mess with duplicated error handling.  Split out
a few useful helpers and use goto labels to untangle the error handling
and no-data ioctl handling.

Signed-off-by: Christoph Hellwig 
---
 drivers/scsi/gdth.c | 244 +++-
 1 file changed, 126 insertions(+), 118 deletions(-)

diff --git a/drivers/scsi/gdth.c b/drivers/scsi/gdth.c
index 16709735b546..45e67d4cb3af 100644
--- a/drivers/scsi/gdth.c
+++ b/drivers/scsi/gdth.c
@@ -4155,131 +4155,139 @@ static int ioc_resetdrv(void __user *arg, char *cmnd)
 return 0;
 }
 
-static int ioc_general(void __user *arg, char *cmnd)
+static void gdth_ioc_addr32(gdth_ha_str *ha, gdth_ioctl_general *gen,
+   u64 paddr)
 {
-gdth_ioctl_general gen;
-char *buf = NULL;
-u64 paddr; 
-gdth_ha_str *ha;
-int rval;
+   if (ha->cache_feat & SCATTER_GATHER) {
+   gen->command.u.cache.DestAddr = 0x;
+   gen->command.u.cache.sg_canz = 1;
+   gen->command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
+   gen->command.u.cache.sg_lst[0].sg_len = gen->data_len;
+   gen->command.u.cache.sg_lst[1].sg_len = 0;
+   } else {
+   gen->command.u.cache.DestAddr = paddr;
+   gen->command.u.cache.sg_canz = 0;
+   }
+}
 
-if (copy_from_user(, arg, sizeof(gdth_ioctl_general)))
-return -EFAULT;
-ha = gdth_find_ha(gen.ionode);
-if (!ha)
-return -EFAULT;
+static void gdth_ioc_addr64(gdth_ha_str *ha, gdth_ioctl_general *gen,
+   u64 paddr)
+{
+   if (ha->cache_feat & SCATTER_GATHER) {
+   gen->command.u.cache64.DestAddr = (u64)-1;
+   gen->command.u.cache64.sg_canz = 1;
+   gen->command.u.cache64.sg_lst[0].sg_ptr = paddr;
+   gen->command.u.cache64.sg_lst[0].sg_len = gen->data_len;
+   gen->command.u.cache64.sg_lst[1].sg_len = 0;
+   } else {
+   gen->command.u.cache64.DestAddr = paddr;
+   gen->command.u.cache64.sg_canz = 0;
+   }
+}
 
-if (gen.data_len > INT_MAX)
-return -EINVAL;
-if (gen.sense_len > INT_MAX)
-return -EINVAL;
-if (gen.data_len + gen.sense_len > INT_MAX)
-return -EINVAL;
+static void gdth_ioc_cacheservice(gdth_ha_str *ha, gdth_ioctl_general *gen,
+   u64 paddr)
+{
+   if (ha->cache_feat & GDT_64BIT) {
+   /* copy elements from 32-bit IOCTL structure */
+   gen->command.u.cache64.BlockCnt = gen->command.u.cache.BlockCnt;
+   gen->command.u.cache64.BlockNo = gen->command.u.cache.BlockNo;
+   gen->command.u.cache64.DeviceNo = gen->command.u.cache.DeviceNo;
 
-if (gen.data_len + gen.sense_len != 0) {
-if (!(buf = gdth_ioctl_alloc(ha, gen.data_len + gen.sense_len,
- FALSE, )))
-return -EFAULT;
-if (copy_from_user(buf, arg + sizeof(gdth_ioctl_general),  
-   gen.data_len + gen.sense_len)) {
-gdth_ioctl_free(ha, gen.data_len+gen.sense_len, buf, paddr);
-return -EFAULT;
-}
+   gdth_ioc_addr64(ha, gen, paddr);
+   } else {
+   gdth_ioc_addr32(ha, gen, paddr);
+   }
+}
 
-if (gen.command.OpCode == GDT_IOCTL) {
-gen.command.u.ioctl.p_param = paddr;
-} else if (gen.command.Service == CACHESERVICE) {
-if (ha->cache_feat & GDT_64BIT) {
-/* copy elements from 32-bit IOCTL structure */
-gen.command.u.cache64.BlockCnt = gen.command.u.cache.BlockCnt;
-gen.command.u.cache64.BlockNo = gen.command.u.cache.BlockNo;
-gen.command.u.cache64.DeviceNo = gen.command.u.cache.DeviceNo;
-/* addresses */
-if (ha->cache_feat & SCATTER_GATHER) {
-gen.command.u.cache64.DestAddr = (u64)-1;
-gen.command.u.cache64.sg_canz = 1;
-gen.command.u.cache64.sg_lst[0].sg_ptr = paddr;
-gen.command.u.cache64.sg_lst[0].sg_len = gen.data_len;
-gen.command.u.cache64.sg_lst[1].sg_len = 0;
-} else {
-gen.command.u.cache64.DestAddr = paddr;
-gen.command.u.cache64.sg_canz = 0;
-}
-} else {
-if (ha->cache_feat & SCATTER_GATHER) {
-gen.command.u.cache.DestAddr = 0x;
-gen.command.u.cache.sg_canz = 1;
-gen.command.u.cache.sg_lst[0].sg_ptr = (u32)paddr;
-gen.command.u.cache.sg_lst[0].sg_len = gen.data_len;
-gen.command.u.cache.sg_lst[1].sg_len = 0;
-} else {
-gen.command.u.cache.DestAddr = paddr;
-gen.command.u.cache.sg_canz = 0;
-}
-}
-} else if