RE: [PATCH 25/41] hpsa: teach hpsa_device_reset to do either target or lun reset

Gates, Matt
2014-01-16T13:34:09Z

Hi Hannes,

Just want to chime in to clarify that in the case of a 'device reset', I think the driver's use of LUN Reset to the Smart Array controller is the correct choice here. This is because the 'device' is actually a single logical unit on the controller (logical RAID volume). Sending a Target Reset to the controller would cause it to reset *all* its logical units (i.e., abort *all* pending commands on the entire controller, not just this single device), which isn't what we want here, I don't think.

So, I like the code as written in the sense that it's doing a LUN Reset to the controller. I wasn't sure if your reply below implied you thought a Target Reset was correct instead, or if you were just suggesting the correct use of eh_X callbacks... so again, just wanted to clarify this one detail.

Thanks,

Matt

> -----Original Message-----
> From: Hannes Reinecke [mailto:hare@suse.de]
> Sent: Thursday, January 16, 2014 2:37 AM
> To: Stephen M. Cameron; jbottomley@parallels.com
> Cc: stephenmcameron@gmail.com; mikem@beardog.cce.hp.com; Gates, Matt;
> linux-scsi@vger.kernel.org; Teel, Scott Stacy
> Subject: Re: [PATCH 25/41] hpsa: teach hpsa_device_reset to do either
> target or lun reset
>
> On 01/15/2014 11:38 PM, Stephen M. Cameron wrote:
> > From: Scott Teel <scott.teel@hp.com>
> >
> > Signed-off-by: Scott Teel <scott.teel@hp.com>
> > Acked-by: Stephen M. Cameron <scameron@beardog.cce.hp.com>
> > ---
> > drivers/scsi/hpsa.c | 9 +++++----
> > 1 files changed, 5 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 9628e12..7cab95f 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -1858,7 +1858,8 @@ out:
> > return rc;
> > }
> >
> > -static int hpsa_send_reset(struct ctlr_info *h, unsigned char
> *scsi3addr)
> > +static int hpsa_send_reset(struct ctlr_info *h, unsigned char
> *scsi3addr,
> > + u8 reset_type)
> > {
> > int rc = IO_OK;
> > struct CommandList *c;
> > @@ -1872,8 +1873,8 @@ static int hpsa_send_reset(struct ctlr_info *h,
> unsigned char *scsi3addr)
> > }
> >
> > /* fill_cmd can't fail here, no data buffer to map. */
> > - (void) fill_cmd(c, HPSA_DEVICE_RESET_MSG, h,
> > - NULL, 0, 0, scsi3addr, TYPE_MSG);
> > + (void) fill_cmd(c, HPSA_DEVICE_RESET_MSG, h, NULL, 0, 0, scsi3addr,
> TYPE_MSG);
> > + c->Request.CDB[1] = reset_type; /* fill_cmd defaults to LUN reset
> type */
> > hpsa_scsi_do_simple_cmd_core(h, c);
> > /* no unmap needed here because no data xfer. */
> >
> > @@ -3397,7 +3398,7 @@ static int hpsa_eh_device_reset_handler(struct
> scsi_cmnd *scsicmd)
> > dev_warn(&h->pdev->dev, "resetting device %d:%d:%d:%d\n",
> > h->scsi_host->host_no, dev->bus, dev->target, dev->lun);
> > /* send a reset to the SCSI LUN which the command was sent to */
> > - rc = hpsa_send_reset(h, dev->scsi3addr);
> > + rc = hpsa_send_reset(h, dev->scsi3addr, HPSA_RESET_TYPE_LUN);
> > if (rc == 0 && wait_for_device_to_become_ready(h, dev->scsi3addr) ==
> 0)
> > return SUCCESS;
> >
> >
> Would you mind using the correct eh_X callbacks here?
> LUN Reset is _not_ identical to TARGET Reset (conceptually, at
> least). And I'm currently trying to disentangle those;
> idea is to have the actual device as the argument, not the command.
>
> Having yet another driver which mangles those only serves to
> increase the patch here ...
>
> Thanks.
>
> Cheers,
>
> Hannes
> --
> Dr. Hannes Reinecke zSeries & Storage
> hare@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)