Re: [PATCH 26/28] visorhba: sanitze private device data allocation

[Hannes Reinecke]

On 07/19/2017 07:45 PM, Kershner, David A wrote:
> 
> 
>> -Original Message-
>> From: Hannes Reinecke [mailto:h...@suse.de]
>> Sent: Wednesday, June 28, 2017 4:25 AM
>> To: Christoph Hellwig <h...@lst.de>
>> Subject: [PATCH 26/28] visorhba: sanitze private device data allocation
>>
>> There's no need to keep the private data for a device in a separate
>> list; better to store it in ->hostdata and do away with the additional
>> list.
>>
>> Signed-off-by: Hannes Reinecke <h...@suse.com>
>> Cc: David Kershner <david.kersh...@unisys.com>
> 
> I'm not sure what the process for this patch was to get into the upstream, do
> you know the forecast for it? I.e. when it will be available in Linus's tree? 
> 
> We have several more changes that I would like to make to this driver and
> to simplify them, I would like to put them on top of this change, would you
> mind if I submitted it to GregKH for the staging branch if it is going to take
> some time?
> 
> Sorry for the newbie questions.
> 
typically the maintainer (that'll be you :-) does a review, and replies
with something like:

Patch is okay;

Reviewed-by: David Kershner <x...@yyy.com>

Then the subsystem maintainer (ie Martin Petersen) will be picking it
up, and queueing it in his subsystem tree for eventual merge with linux
upstream.

The first step has been completed, so we're waiting for Martin to pick
it up. It might be some time, though; depending on the merge window and
his overall workload.

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

RE: [PATCH 26/28] visorhba: sanitze private device data allocation

[Kershner, David A]

> -Original Message-
> From: Hannes Reinecke [mailto:h...@suse.de]
> Sent: Wednesday, June 28, 2017 4:25 AM
> To: Christoph Hellwig <h...@lst.de>
> Cc: Martin K. Petersen <martin.peter...@oracle.com>; James Bottomley
> <james.bottom...@hansenpartnership.com>; linux-scsi@vger.kernel.org;
> Hannes Reinecke <h...@suse.de>; Hannes Reinecke <h...@suse.com>;
> Kershner, David A <david.kersh...@unisys.com>
> Subject: [PATCH 26/28] visorhba: sanitze private device data allocation
> 
> There's no need to keep the private data for a device in a separate
> list; better to store it in ->hostdata and do away with the additional
> list.
> 
> Signed-off-by: Hannes Reinecke <h...@suse.com>
> Cc: David Kershner <david.kersh...@unisys.com>

Thanks for the patch, works great!  Tested it on s-Par. 

Acked-by: David Kershner <david.kersh...@unisys.com>

Thanks, 
David Kershner

> ---
>  drivers/staging/unisys/visorhba/visorhba_main.c | 123 ++--
> 
>  1 file changed, 53 insertions(+), 70 deletions(-)
> 
> diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c
> b/drivers/staging/unisys/visorhba/visorhba_main.c
> index 6997b16..811abfc 100644
> --- a/drivers/staging/unisys/visorhba/visorhba_main.c
> +++ b/drivers/staging/unisys/visorhba/visorhba_main.c
> @@ -47,8 +47,8 @@
>  MODULE_ALIAS("visorbus:"
> SPAR_VHBA_CHANNEL_PROTOCOL_UUID_STR);
> 
>  struct visordisk_info {
> + struct scsi_device *sdev;
>   u32 valid;
> - u32 channel, id, lun;   /* Disk Path */
>   atomic_t ios_threshold;
>   atomic_t error_count;
>   struct visordisk_info *next;
> @@ -101,12 +101,6 @@ struct visorhba_devices_open {
>   struct visorhba_devdata *devdata;
>  };
> 
> -#define for_each_vdisk_match(iter, list, match) \
> - for (iter = >head; iter->next; iter = iter->next) \
> - if ((iter->channel == match->channel) && \
> - (iter->id == match->id) && \
> - (iter->lun == match->lun))
> -
>  /*
>   *   visor_thread_start - starts a thread for the device
>   *   @threadfn: Function the thread starts
> @@ -296,10 +290,9 @@ static void cleanup_scsitaskmgmt_handles(struct idr
> *idrtable,
>   *   Returns whether the command was queued successfully or not.
>   */
>  static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
> - struct scsi_cmnd *scsicmd)
> + struct scsi_device *scsidev)
>  {
>   struct uiscmdrsp *cmdrsp;
> - struct scsi_device *scsidev = scsicmd->device;
>   struct visorhba_devdata *devdata =
>   (struct visorhba_devdata *)scsidev->host->hostdata;
>   int notifyresult = 0x;
> @@ -347,12 +340,6 @@ static int forward_taskmgmt_command(enum
> task_mgmt_types tasktype,
>   dev_dbg(>sdev_gendev,
>   "visorhba: taskmgmt type=%d success; result=0x%x\n",
>tasktype, notifyresult);
> - if (tasktype == TASK_MGMT_ABORT_TASK)
> - scsicmd->result = DID_ABORT << 16;
> - else
> - scsicmd->result = DID_RESET << 16;
> -
> - scsicmd->scsi_done(scsicmd);
>   cleanup_scsitaskmgmt_handles(>idr, cmdrsp);
>   return SUCCESS;
> 
> @@ -376,17 +363,20 @@ static int visorhba_abort_handler(struct scsi_cmnd
> *scsicmd)
>   /* issue TASK_MGMT_ABORT_TASK */
>   struct scsi_device *scsidev;
>   struct visordisk_info *vdisk;
> - struct visorhba_devdata *devdata;
> + int rtn;
> 
>   scsidev = scsicmd->device;
> - devdata = (struct visorhba_devdata *)scsidev->host->hostdata;
> - for_each_vdisk_match(vdisk, devdata, scsidev) {
> - if (atomic_read(>error_count) <
> VISORHBA_ERROR_COUNT)
> - atomic_inc(>error_count);
> - else
> - atomic_set(>ios_threshold,
> IOS_ERROR_THRESHOLD);
> + vdisk = scsidev->hostdata;
> + if (atomic_read(>error_count) < VISORHBA_ERROR_COUNT)
> + atomic_inc(>error_count);
> + else
> + atomic_set(>ios_threshold,
> IOS_ERROR_THRESHOLD);
> + rtn = forward_taskmgmt_command(TASK_MGMT_ABORT_TASK,
> scsidev);
> + if (rtn == SUCCESS) {
> + scsicmd->result = DID_ABORT << 16;
> + scsicmd->scsi_done(scsicmd);
>   }
> - return forward_taskmgmt_command(TASK_MGMT_ABORT_TASK,
> scsicmd);
> + return rtn;
>  }
> 
>  /*
> @@ -400,17 +390,20 @@ static int visorhba_device_reset_handler(s

[PATCH 26/28] visorhba: sanitze private device data allocation

[Hannes Reinecke]

There's no need to keep the private data for a device in a separate
list; better to store it in ->hostdata and do away with the additional
list.

Signed-off-by: Hannes Reinecke 
Cc: David Kershner 
---
 drivers/staging/unisys/visorhba/visorhba_main.c | 123 ++--
 1 file changed, 53 insertions(+), 70 deletions(-)

diff --git a/drivers/staging/unisys/visorhba/visorhba_main.c 
b/drivers/staging/unisys/visorhba/visorhba_main.c
index 6997b16..811abfc 100644
--- a/drivers/staging/unisys/visorhba/visorhba_main.c
+++ b/drivers/staging/unisys/visorhba/visorhba_main.c
@@ -47,8 +47,8 @@
 MODULE_ALIAS("visorbus:" SPAR_VHBA_CHANNEL_PROTOCOL_UUID_STR);
 
 struct visordisk_info {
+   struct scsi_device *sdev;
u32 valid;
-   u32 channel, id, lun;   /* Disk Path */
atomic_t ios_threshold;
atomic_t error_count;
struct visordisk_info *next;
@@ -101,12 +101,6 @@ struct visorhba_devices_open {
struct visorhba_devdata *devdata;
 };
 
-#define for_each_vdisk_match(iter, list, match) \
-   for (iter = >head; iter->next; iter = iter->next) \
-   if ((iter->channel == match->channel) && \
-   (iter->id == match->id) && \
-   (iter->lun == match->lun))
-
 /*
  * visor_thread_start - starts a thread for the device
  * @threadfn: Function the thread starts
@@ -296,10 +290,9 @@ static void cleanup_scsitaskmgmt_handles(struct idr 
*idrtable,
  * Returns whether the command was queued successfully or not.
  */
 static int forward_taskmgmt_command(enum task_mgmt_types tasktype,
-   struct scsi_cmnd *scsicmd)
+   struct scsi_device *scsidev)
 {
struct uiscmdrsp *cmdrsp;
-   struct scsi_device *scsidev = scsicmd->device;
struct visorhba_devdata *devdata =
(struct visorhba_devdata *)scsidev->host->hostdata;
int notifyresult = 0x;
@@ -347,12 +340,6 @@ static int forward_taskmgmt_command(enum task_mgmt_types 
tasktype,
dev_dbg(>sdev_gendev,
"visorhba: taskmgmt type=%d success; result=0x%x\n",
 tasktype, notifyresult);
-   if (tasktype == TASK_MGMT_ABORT_TASK)
-   scsicmd->result = DID_ABORT << 16;
-   else
-   scsicmd->result = DID_RESET << 16;
-
-   scsicmd->scsi_done(scsicmd);
cleanup_scsitaskmgmt_handles(>idr, cmdrsp);
return SUCCESS;
 
@@ -376,17 +363,20 @@ static int visorhba_abort_handler(struct scsi_cmnd 
*scsicmd)
/* issue TASK_MGMT_ABORT_TASK */
struct scsi_device *scsidev;
struct visordisk_info *vdisk;
-   struct visorhba_devdata *devdata;
+   int rtn;
 
scsidev = scsicmd->device;
-   devdata = (struct visorhba_devdata *)scsidev->host->hostdata;
-   for_each_vdisk_match(vdisk, devdata, scsidev) {
-   if (atomic_read(>error_count) < VISORHBA_ERROR_COUNT)
-   atomic_inc(>error_count);
-   else
-   atomic_set(>ios_threshold, IOS_ERROR_THRESHOLD);
+   vdisk = scsidev->hostdata;
+   if (atomic_read(>error_count) < VISORHBA_ERROR_COUNT)
+   atomic_inc(>error_count);
+   else
+   atomic_set(>ios_threshold, IOS_ERROR_THRESHOLD);
+   rtn = forward_taskmgmt_command(TASK_MGMT_ABORT_TASK, scsidev);
+   if (rtn == SUCCESS) {
+   scsicmd->result = DID_ABORT << 16;
+   scsicmd->scsi_done(scsicmd);
}
-   return forward_taskmgmt_command(TASK_MGMT_ABORT_TASK, scsicmd);
+   return rtn;
 }
 
 /*
@@ -400,17 +390,20 @@ static int visorhba_device_reset_handler(struct scsi_cmnd 
*scsicmd)
/* issue TASK_MGMT_LUN_RESET */
struct scsi_device *scsidev;
struct visordisk_info *vdisk;
-   struct visorhba_devdata *devdata;
+   int rtn;
 
scsidev = scsicmd->device;
-   devdata = (struct visorhba_devdata *)scsidev->host->hostdata;
-   for_each_vdisk_match(vdisk, devdata, scsidev) {
-   if (atomic_read(>error_count) < VISORHBA_ERROR_COUNT)
-   atomic_inc(>error_count);
-   else
-   atomic_set(>ios_threshold, IOS_ERROR_THRESHOLD);
+   vdisk = scsidev->hostdata;
+   if (atomic_read(>error_count) < VISORHBA_ERROR_COUNT)
+   atomic_inc(>error_count);
+   else
+   atomic_set(>ios_threshold, IOS_ERROR_THRESHOLD);
+   rtn = forward_taskmgmt_command(TASK_MGMT_LUN_RESET, scsidev);
+   if (rtn == SUCCESS) {
+   scsicmd->result = DID_RESET << 16;
+   scsicmd->scsi_done(scsicmd);
}
-   return forward_taskmgmt_command(TASK_MGMT_LUN_RESET, scsicmd);
+   return rtn;
 }
 
 /*
@@ -424,17 +417,22 @@ static int visorhba_bus_reset_handler(struct scsi_cmnd 
*scsicmd)
 {
struct scsi_device *scsidev;
struct visordisk_info