RE: [PATCH 07/12] hpsa: cleanup reset handler

[Don Brace]

> -Original Message-
> From: Martin Wilck [mailto:mwi...@suse.com]
> Sent: Tuesday, April 11, 2017 7:36 AM
> To: Don Brace <don.br...@microsemi.com>; joseph.szczy...@hpe.com;
> Gerry Morong <gerry.mor...@microsemi.com>; John Hall
> <john.h...@microsemi.com>; j...@linux.vnet.ibm.com; Kevin Barnett
> <kevin.barn...@microsemi.com>; Mahesh Rajashekhara
> <mahesh.rajashekh...@microsemi.com>; Bader Ali - Saleh
> <bader.alisa...@microsemi.com>; h...@infradead.org; Scott Teel
> <scott.t...@microsemi.com>; Viswas G <viswa...@microsemi.com>; Justin
> Lindley <justin.lind...@microsemi.com>; Scott Benesh
> <scott.ben...@microsemi.com>; posw...@suse.com
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 07/12] hpsa: cleanup reset handler
> 
> EXTERNAL EMAIL
> 
> 
> On Fri, 2017-04-07 at 15:06 -0500, Don Brace wrote:
> >  - mark device state sooner.
> >
> > Reviewed-by: Scott Benesh <scott.ben...@microsemi.com>
> > Reviewed-by: Scott Teel <scott.t...@microsemi.com>
> > Reviewed-by: Kevin Barnett <kevin.barn...@microsemi.com>
> > Signed-off-by: Don Brace <don.br...@microsemi.com>
> > ---
> >  drivers/scsi/hpsa.c |   44 ++---
> > ---
> >  1 file changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index a2852da..a6a37e0 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -5834,7 +5834,7 @@ static int
> > wait_for_device_to_become_ready(struct ctlr_info *h,
> >   */
> >  static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
> >  {
> > - int rc;
> > + int rc = SUCCESS;
> >   struct ctlr_info *h;
> >   struct hpsa_scsi_dev_t *dev;
> >   u8 reset_type;
> > @@ -5845,17 +5845,24 @@ static int
> > hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
> >   if (h == NULL) /* paranoia */
> >   return FAILED;
> >
> > - if (lockup_detected(h))
> > - return FAILED;
> > + h->reset_in_progress = 1;
> > +
> > + if (lockup_detected(h)) {
> > + rc = FAILED;
> > + goto return_reset_status;
> > + }
> 
> if this is meant to communicate host state to other threads, maybe you
> should use an atomic type for h->reset_in_progress, or locking of some
> sort?
> 
> Regards,
> Martin
> 
Agreed.
Thanks for your review. It has actually helped out...a lot.
Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation


> --
> Dr. Martin Wilck <mwi...@suse.com>, Tel. +49 (0)911 74053 2107
> SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
> HRB 21284 (AG Nürnberg)

Re: [PATCH 07/12] hpsa: cleanup reset handler

[Martin Wilck]

On Fri, 2017-04-07 at 15:06 -0500, Don Brace wrote:
>  - mark device state sooner.
>
> Reviewed-by: Scott Benesh 
> Reviewed-by: Scott Teel 
> Reviewed-by: Kevin Barnett 
> Signed-off-by: Don Brace 
> ---
>  drivers/scsi/hpsa.c |   44 ++---
> ---
>  1 file changed, 30 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index a2852da..a6a37e0 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -5834,7 +5834,7 @@ static int
> wait_for_device_to_become_ready(struct ctlr_info *h,
>   */
>  static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
>  {
> - int rc;
> + int rc = SUCCESS;
>   struct ctlr_info *h;
>   struct hpsa_scsi_dev_t *dev;
>   u8 reset_type;
> @@ -5845,17 +5845,24 @@ static int
> hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
>   if (h == NULL) /* paranoia */
>   return FAILED;
>  
> - if (lockup_detected(h))
> - return FAILED;
> + h->reset_in_progress = 1;
> +
> + if (lockup_detected(h)) {
> + rc = FAILED;
> + goto return_reset_status;
> + }

if this is meant to communicate host state to other threads, maybe you
should use an atomic type for h->reset_in_progress, or locking of some
sort?

Regards,
Martin

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

[PATCH 07/12] hpsa: cleanup reset handler

[Don Brace]

 - mark device state sooner.

Reviewed-by: Scott Benesh 
Reviewed-by: Scott Teel 
Reviewed-by: Kevin Barnett 
Signed-off-by: Don Brace 
---
 drivers/scsi/hpsa.c |   44 ++--
 1 file changed, 30 insertions(+), 14 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index a2852da..a6a37e0 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5834,7 +5834,7 @@ static int wait_for_device_to_become_ready(struct 
ctlr_info *h,
  */
 static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 {
-   int rc;
+   int rc = SUCCESS;
struct ctlr_info *h;
struct hpsa_scsi_dev_t *dev;
u8 reset_type;
@@ -5845,17 +5845,24 @@ static int hpsa_eh_device_reset_handler(struct 
scsi_cmnd *scsicmd)
if (h == NULL) /* paranoia */
return FAILED;
 
-   if (lockup_detected(h))
-   return FAILED;
+   h->reset_in_progress = 1;
+
+   if (lockup_detected(h)) {
+   rc = FAILED;
+   goto return_reset_status;
+   }
 
dev = scsicmd->device->hostdata;
if (!dev) {
dev_err(>pdev->dev, "%s: device lookup failed\n", __func__);
-   return FAILED;
+   rc = FAILED;
+   goto return_reset_status;
}
 
-   if (dev->devtype == TYPE_ENCLOSURE)
-   return SUCCESS;
+   if (dev->devtype == TYPE_ENCLOSURE) {
+   rc = SUCCESS;
+   goto return_reset_status;
+   }
 
/* if controller locked up, we can guarantee command won't complete */
if (lockup_detected(h)) {
@@ -5863,7 +5870,8 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd 
*scsicmd)
 "cmd %d RESET FAILED, lockup detected",
 hpsa_get_cmd_index(scsicmd));
hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
-   return FAILED;
+   rc = FAILED;
+   goto return_reset_status;
}
 
/* this reset request might be the result of a lockup; check */
@@ -5872,12 +5880,15 @@ static int hpsa_eh_device_reset_handler(struct 
scsi_cmnd *scsicmd)
 "cmd %d RESET FAILED, new lockup detected",
 hpsa_get_cmd_index(scsicmd));
hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
-   return FAILED;
+   rc = FAILED;
+   goto return_reset_status;
}
 
/* Do not attempt on controller */
-   if (is_hba_lunid(dev->scsi3addr))
-   return SUCCESS;
+   if (is_hba_lunid(dev->scsi3addr)) {
+   rc = SUCCESS;
+   goto return_reset_status;
+   }
 
if (is_logical_dev_addr_mode(dev->scsi3addr))
reset_type = HPSA_DEVICE_RESET_MSG;
@@ -5888,17 +5899,22 @@ static int hpsa_eh_device_reset_handler(struct 
scsi_cmnd *scsicmd)
reset_type == HPSA_DEVICE_RESET_MSG ? "logical " : "physical ");
hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 
-   h->reset_in_progress = 1;
-
/* send a reset to the SCSI LUN which the command was sent to */
rc = hpsa_do_reset(h, dev, dev->scsi3addr, reset_type,
   DEFAULT_REPLY_QUEUE);
+   if (rc == 0)
+   rc = SUCCESS;
+   else
+   rc = FAILED;
+
sprintf(msg, "reset %s %s",
reset_type == HPSA_DEVICE_RESET_MSG ? "logical " : "physical ",
-   rc == 0 ? "completed successfully" : "failed");
+   rc == SUCCESS ? "completed successfully" : "failed");
hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
+
+return_reset_status:
h->reset_in_progress = 0;
-   return rc == 0 ? SUCCESS : FAILED;
+   return rc;
 }
 
 static void swizzle_abort_tag(u8 *tag)