Re: [PATCH 1 17/25] hpsa: move scsi_add_device and scsi_remove_device calls to new function

[Hannes Reinecke]

On 10/28/2015 11:06 PM, Don Brace wrote:
> From: Kevin Barnett 
> 
> preparation for adding the sas transport class
> 
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
>  drivers/scsi/hpsa.c |   65 
> +++
>  1 file changed, 39 insertions(+), 26 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1 17/25] hpsa: move scsi_add_device and scsi_remove_device calls to new function

[Matthew R. Ochs]

> On Oct 29, 2015, at 3:30 PM, Don Brace  wrote:
> 
> On 10/29/2015 12:21 PM, Matthew R. Ochs wrote:
>>> On Oct 28, 2015, at 5:06 PM, Don Brace  wrote:
>>> 
>>> From: Kevin Barnett 
>>> 
>>> preparation for adding the sas transport class
>>> 
>>> Reviewed-by: Scott Teel 
>>> Reviewed-by: Justin Lindley 
>>> Reviewed-by: Kevin Barnett 
>>> Signed-off-by: Don Brace 
>>> ---
>>> drivers/scsi/hpsa.c |   65 
>>> +++
>>> 1 file changed, 39 insertions(+), 26 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
>>> index 24b3c8c..06207e2 100644
>>> --- a/drivers/scsi/hpsa.c
>>> +++ b/drivers/scsi/hpsa.c
>>> @@ -1660,6 +1660,37 @@ static void 
>>> hpsa_update_log_drive_phys_drive_ptrs(struct ctlr_info *h,
>>> }
>>> }
>>> 
>>> +static int hpsa_add_device(struct ctlr_info *h, struct hpsa_scsi_dev_t 
>>> *device)
>>> +{
>>> +   int rc = 0;
>>> +
>>> +   rc = scsi_add_device(h->scsi_host, device->bus,
>>> +   device->target, device->lun);
>>> +   return rc;
>>> +}
>>> +
>>> +static void hpsa_remove_device(struct ctlr_info *h,
>>> +   struct hpsa_scsi_dev_t *device)
>>> +{
>>> +   struct scsi_device *sdev = NULL;
>>> +
>>> +   sdev = scsi_device_lookup(h->scsi_host, device->bus,
>>> +   device->target, device->lun);
>>> +
>>> +   if (sdev) {
>>> +   scsi_remove_device(sdev);
>>> +   scsi_device_put(sdev);
>>> +   } else {
>>> +   /*
>>> +* We don't expect to get here.  Future commands
>>> +* to this device will get a selection timeout as
>>> +* if the device were gone.
>>> +*/
>>> +   hpsa_show_dev_msg(KERN_WARNING, h, device,
>>> +   "didn't find device for removal.");
>>> +   }
>>> +}
>>> +
>>> static void adjust_hpsa_scsi_table(struct ctlr_info *h, int hostno,
>>> struct hpsa_scsi_dev_t *sd[], int nsds)
>>> {
>>> @@ -1672,7 +1703,6 @@ static void adjust_hpsa_scsi_table(struct ctlr_info 
>>> *h, int hostno,
>>> unsigned long flags;
>>> struct hpsa_scsi_dev_t **added, **removed;
>>> int nadded, nremoved;
>>> -   struct Scsi_Host *sh = NULL;
>>> 
>>> /*
>>>  * A reset can cause a device status to change
>>> @@ -1792,46 +1822,29 @@ static void adjust_hpsa_scsi_table(struct ctlr_info 
>>> *h, int hostno,
>>> if (hostno == -1 || !changes)
>>> goto free_and_out;
>>> 
>>> -   sh = h->scsi_host;
>>> -   if (sh == NULL) {
>>> -   dev_warn(>pdev->dev, "%s: scsi_host is null\n", __func__);
>>> -   return;
>>> -   }
>> Are we guaranteed that h->scsi_host will never be NULL when running in here? 
>> Or when
>> the newly introduced hpsa_remove_device() is invoked elsewhere for that 
>> matter?
>> 
>> This commit loses this check and scsi_device_lookup() is not tolerant of a 
>> NULL
>> scsi_host * (first action is to grab the host_lock).
> Better to be safe. I'll add a check in both functions.

With these changes

Reviewed-by: Matthew R. Ochs 

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1 17/25] hpsa: move scsi_add_device and scsi_remove_device calls to new function

[Tomas Henzl]

On 28.10.2015 23:06, Don Brace wrote:
> From: Kevin Barnett 
>
> preparation for adding the sas transport class
>
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
>  drivers/scsi/hpsa.c |   65 
> +++
>  1 file changed, 39 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 24b3c8c..06207e2 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -1660,6 +1660,37 @@ static void 
> hpsa_update_log_drive_phys_drive_ptrs(struct ctlr_info *h,
>   }
>  }
>  
> +static int hpsa_add_device(struct ctlr_info *h, struct hpsa_scsi_dev_t 
> *device)
> +{
> + int rc = 0;

Just int rc; is better, but it's a nit.
(and struct scsi_device *sdev; few lines below)

Reviewed-by: Tomas Henzl 

Tomas

> +
> + rc = scsi_add_device(h->scsi_host, device->bus,
> + device->target, device->lun);
> + return rc;
> +}
> +
> +static void hpsa_remove_device(struct ctlr_info *h,
> + struct hpsa_scsi_dev_t *device)
> +{
> + struct scsi_device *sdev = NULL;
> +
> + sdev = scsi_device_lookup(h->scsi_host, device->bus,
> + device->target, device->lun);
> +
> + if (sdev) {
> + scsi_remove_device(sdev);
> + scsi_device_put(sdev);
> + } else {
> + /*
> +  * We don't expect to get here.  Future commands
> +  * to this device will get a selection timeout as
> +  * if the device were gone.
> +  */
> + hpsa_show_dev_msg(KERN_WARNING, h, device,
> + "didn't find device for removal.");
> + }
> +}
> +
>  static void adjust_hpsa_scsi_table(struct ctlr_info *h, int hostno,
>   struct hpsa_scsi_dev_t *sd[], int nsds)
>  {
> @@ -1672,7 +1703,6 @@ static void adjust_hpsa_scsi_table(struct ctlr_info *h, 
> int hostno,
>   unsigned long flags;
>   struct hpsa_scsi_dev_t **added, **removed;
>   int nadded, nremoved;
> - struct Scsi_Host *sh = NULL;
>  
>   /*
>* A reset can cause a device status to change
> @@ -1792,46 +1822,29 @@ static void adjust_hpsa_scsi_table(struct ctlr_info 
> *h, int hostno,
>   if (hostno == -1 || !changes)
>   goto free_and_out;
>  
> - sh = h->scsi_host;
> - if (sh == NULL) {
> - dev_warn(>pdev->dev, "%s: scsi_host is null\n", __func__);
> - return;
> - }
>   /* Notify scsi mid layer of any removed devices */
>   for (i = 0; i < nremoved; i++) {
>   if (removed[i] == NULL)
>   continue;
> - if (removed[i]->expose_device) {
> - struct scsi_device *sdev =
> - scsi_device_lookup(sh, removed[i]->bus,
> - removed[i]->target, removed[i]->lun);
> - if (sdev != NULL) {
> - scsi_remove_device(sdev);
> - scsi_device_put(sdev);
> - } else {
> - /*
> -  * We don't expect to get here.
> -  * future cmds to this device will get selection
> -  * timeout as if the device was gone.
> -  */
> - hpsa_show_dev_msg(KERN_WARNING, h, removed[i],
> - "didn't find device for removal.");
> - }
> - }
> + if (removed[i]->expose_device)
> + hpsa_remove_device(h, removed[i]);
>   kfree(removed[i]);
>   removed[i] = NULL;
>   }
>  
>   /* Notify scsi mid layer of any added devices */
>   for (i = 0; i < nadded; i++) {
> + int rc = 0;
> +
>   if (added[i] == NULL)
>   continue;
>   if (!(added[i]->expose_device))
>   continue;
> - if (scsi_add_device(sh, added[i]->bus,
> - added[i]->target, added[i]->lun) == 0)
> + rc = hpsa_add_device(h, added[i]);
> + if (!rc)
>   continue;
> - dev_warn(>pdev->dev, "addition failed, device not added.");
> + dev_warn(>pdev->dev,
> + "addition failed %d, device not added.", rc);
>   /* now we have to remove it from h->dev,
>* since it didn't get added to scsi mid layer
>*/
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majord...@vger.kernel.org
> More 

Re: [PATCH 1 17/25] hpsa: move scsi_add_device and scsi_remove_device calls to new function

[Matthew R. Ochs]

> On Oct 28, 2015, at 5:06 PM, Don Brace  wrote:
> 
> From: Kevin Barnett 
> 
> preparation for adding the sas transport class
> 
> Reviewed-by: Scott Teel 
> Reviewed-by: Justin Lindley 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
> drivers/scsi/hpsa.c |   65 +++
> 1 file changed, 39 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 24b3c8c..06207e2 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -1660,6 +1660,37 @@ static void 
> hpsa_update_log_drive_phys_drive_ptrs(struct ctlr_info *h,
>   }
> }
> 
> +static int hpsa_add_device(struct ctlr_info *h, struct hpsa_scsi_dev_t 
> *device)
> +{
> + int rc = 0;
> +
> + rc = scsi_add_device(h->scsi_host, device->bus,
> + device->target, device->lun);
> + return rc;
> +}
> +
> +static void hpsa_remove_device(struct ctlr_info *h,
> + struct hpsa_scsi_dev_t *device)
> +{
> + struct scsi_device *sdev = NULL;
> +
> + sdev = scsi_device_lookup(h->scsi_host, device->bus,
> + device->target, device->lun);
> +
> + if (sdev) {
> + scsi_remove_device(sdev);
> + scsi_device_put(sdev);
> + } else {
> + /*
> +  * We don't expect to get here.  Future commands
> +  * to this device will get a selection timeout as
> +  * if the device were gone.
> +  */
> + hpsa_show_dev_msg(KERN_WARNING, h, device,
> + "didn't find device for removal.");
> + }
> +}
> +
> static void adjust_hpsa_scsi_table(struct ctlr_info *h, int hostno,
>   struct hpsa_scsi_dev_t *sd[], int nsds)
> {
> @@ -1672,7 +1703,6 @@ static void adjust_hpsa_scsi_table(struct ctlr_info *h, 
> int hostno,
>   unsigned long flags;
>   struct hpsa_scsi_dev_t **added, **removed;
>   int nadded, nremoved;
> - struct Scsi_Host *sh = NULL;
> 
>   /*
>* A reset can cause a device status to change
> @@ -1792,46 +1822,29 @@ static void adjust_hpsa_scsi_table(struct ctlr_info 
> *h, int hostno,
>   if (hostno == -1 || !changes)
>   goto free_and_out;
> 
> - sh = h->scsi_host;
> - if (sh == NULL) {
> - dev_warn(>pdev->dev, "%s: scsi_host is null\n", __func__);
> - return;
> - }

Are we guaranteed that h->scsi_host will never be NULL when running in here? Or 
when
the newly introduced hpsa_remove_device() is invoked elsewhere for that matter?

This commit loses this check and scsi_device_lookup() is not tolerant of a NULL
scsi_host * (first action is to grab the host_lock).

>   /* Notify scsi mid layer of any removed devices */
>   for (i = 0; i < nremoved; i++) {
>   if (removed[i] == NULL)
>   continue;
> - if (removed[i]->expose_device) {
> - struct scsi_device *sdev =
> - scsi_device_lookup(sh, removed[i]->bus,
> - removed[i]->target, removed[i]->lun);
> - if (sdev != NULL) {
> - scsi_remove_device(sdev);
> - scsi_device_put(sdev);
> - } else {
> - /*
> -  * We don't expect to get here.
> -  * future cmds to this device will get selection
> -  * timeout as if the device was gone.
> -  */
> - hpsa_show_dev_msg(KERN_WARNING, h, removed[i],
> - "didn't find device for removal.");
> - }
> - }
> + if (removed[i]->expose_device)
> + hpsa_remove_device(h, removed[i]);
>   kfree(removed[i]);
>   removed[i] = NULL;
>   }
> 
>   /* Notify scsi mid layer of any added devices */
>   for (i = 0; i < nadded; i++) {
> + int rc = 0;
> +
>   if (added[i] == NULL)
>   continue;
>   if (!(added[i]->expose_device))
>   continue;
> - if (scsi_add_device(sh, added[i]->bus,
> - added[i]->target, added[i]->lun) == 0)
> + rc = hpsa_add_device(h, added[i]);
> + if (!rc)
>   continue;
> - dev_warn(>pdev->dev, "addition failed, device not added.");
> + dev_warn(>pdev->dev,
> + "addition failed %d, device not added.", rc);
>   /* now we have to remove it from h->dev,
>* since it didn't get added to