RE: [PATCH 1/3] hpsa: update check for logical volume status
> -Original Message-
> From: Tomas Henzl [mailto:the...@redhat.com]
> Sent: Friday, March 10, 2017 7:24 AM
> To: Don Brace ; joseph.szczy...@hpe.com;
> Gerry Morong ; John Hall
> ; j...@linux.vnet.ibm.com; Kevin Barnett
> ; Mahesh Rajashekhara
> ; Bader Ali - Saleh
> ; h...@infradead.org; Scott Teel
> ; Viswas G ; Justin
> Lindley ; Scott Benesh
> ; posw...@suse.com
> Cc: linux-scsi@vger.kernel.org
> Subject: Re: [PATCH 1/3] hpsa: update check for logical volume status
>
> EXTERNAL EMAIL
>
>
> On 6.3.2017 22:24, Don Brace wrote:
> > - Add in a new case for volume offline. Resolves internal
> >testing bug for multilun array management.
> > - Return correct status for failed TURs.
> >
> > Reviewed-by: Scott Benesh
> > Reviewed-by: Scott Teel
> > Signed-off-by: Don Brace
> > ---
> > drivers/scsi/hpsa.c | 26 +-
> > drivers/scsi/hpsa_cmd.h |2 ++
> > 2 files changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 524a0c7..1adc4ec 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -3735,7 +3735,7 @@ static int hpsa_volume_offline(struct ctlr_info
> *h,
> > DEFAULT_TIMEOUT);
> > if (rc) {
> > cmd_free(h, c);
>
> Hi Don,
> patch is ok, but this function returns a mix of either HPSA_LV_*
> values or some magic numbers.
> Could you replace the 0xff with HPSA_VPD_LV_STATUS_UNSUPPORTED
> (like it is in hpsa_get_volume_status)
> and in case of success return HPSA_LV_OK ?
>
> Also it would make sense to change the return value
> from 'int' to 'unsigned char' and to drop
> this test
> volume_offline = hpsa_volume_offline(h, scsi3addr);
> if (volume_offline < 0 || volume_offline > 0xff)
> volume_offline = HPSA_VPD_LV_STATUS_UNSUPPORTED;
> this_device->volume_offline = volume_offline & 0xff;
>
> from hpsa_update_device_info since the condition is never met.
> Switching volume_offline to an unsigned char everywhere might be also
> possible.
>
> tomash
Agreed, be up in a V2.
And thank-you for your reviews.
Thanks,
Don Brace
ESC - Smart Storage
Microsemi Corporation
>
> > - return 0;
> > + return 0xff;
> > }
> > sense = c->err_info->SenseInfo;
> > if (c->err_info->SenseLen > sizeof(c->err_info->SenseInfo))
> > @@ -3746,19 +3746,13 @@ static int hpsa_volume_offline(struct
> ctlr_info *h,
> > cmd_status = c->err_info->CommandStatus;
> > scsi_status = c->err_info->ScsiStatus;
> > cmd_free(h, c);
> > - /* Is the volume 'not ready'? */
> > - if (cmd_status != CMD_TARGET_STATUS ||
> > - scsi_status != SAM_STAT_CHECK_CONDITION ||
> > - sense_key != NOT_READY ||
> > - asc != ASC_LUN_NOT_READY) {
> > - return 0;
> > - }
> >
> > /* Determine the reason for not ready state */
> > ldstat = hpsa_get_volume_status(h, scsi3addr);
> >
> > /* Keep volume offline in certain cases: */
> > switch (ldstat) {
> > + case HPSA_LV_FAILED:
> > case HPSA_LV_UNDERGOING_ERASE:
> > case HPSA_LV_NOT_AVAILABLE:
> > case HPSA_LV_UNDERGOING_RPI:
> > @@ -3853,10 +3847,10 @@ static int hpsa_update_device_info(struct
> ctlr_info *h,
> > /* Do an inquiry to the device to see what it is. */
> > if (hpsa_scsi_do_inquiry(h, scsi3addr, 0, inq_buff,
> > (unsigned char) OBDR_TAPE_INQ_SIZE) != 0) {
> > - /* Inquiry failed (msg printed already) */
> > dev_err(&h->pdev->dev,
> > - "hpsa_update_device_info: inquiry failed\n");
> > - rc = -EIO;
> > + "%s: inquiry failed, device will be skipped.\n",
> > + __func__);
> > + rc = HPSA_INQUIRY_FAILED;
> > goto bail_out;
> > }
> >
> > @@ -3894,6 +3888,13 @@ static int hpsa_update_device_info(struct
> ctlr_info *h,
> > if (volume_offline < 0 || volume_offline > 0xff)
> > volume_offline = HPSA_VPD_LV_STATUS_UNSUPPORTED;
> > this_device->volume_offline = volume_offline & 0xff;
> > + if (volume_offline == HPSA_LV_FAILED) {
> > +
Re: [PATCH 1/3] hpsa: update check for logical volume status
On 6.3.2017 22:24, Don Brace wrote:
> - Add in a new case for volume offline. Resolves internal
>testing bug for multilun array management.
> - Return correct status for failed TURs.
>
> Reviewed-by: Scott Benesh
> Reviewed-by: Scott Teel
> Signed-off-by: Don Brace
> ---
> drivers/scsi/hpsa.c | 26 +-
> drivers/scsi/hpsa_cmd.h |2 ++
> 2 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> index 524a0c7..1adc4ec 100644
> --- a/drivers/scsi/hpsa.c
> +++ b/drivers/scsi/hpsa.c
> @@ -3735,7 +3735,7 @@ static int hpsa_volume_offline(struct ctlr_info *h,
> DEFAULT_TIMEOUT);
> if (rc) {
> cmd_free(h, c);
Hi Don,
patch is ok, but this function returns a mix of either HPSA_LV_*
values or some magic numbers.
Could you replace the 0xff with HPSA_VPD_LV_STATUS_UNSUPPORTED
(like it is in hpsa_get_volume_status)
and in case of success return HPSA_LV_OK ?
Also it would make sense to change the return value
from 'int' to 'unsigned char' and to drop
this test
volume_offline = hpsa_volume_offline(h, scsi3addr);
if (volume_offline < 0 || volume_offline > 0xff)
volume_offline = HPSA_VPD_LV_STATUS_UNSUPPORTED;
this_device->volume_offline = volume_offline & 0xff;
from hpsa_update_device_info since the condition is never met.
Switching volume_offline to an unsigned char everywhere might be also possible.
tomash
> - return 0;
> + return 0xff;
> }
> sense = c->err_info->SenseInfo;
> if (c->err_info->SenseLen > sizeof(c->err_info->SenseInfo))
> @@ -3746,19 +3746,13 @@ static int hpsa_volume_offline(struct ctlr_info *h,
> cmd_status = c->err_info->CommandStatus;
> scsi_status = c->err_info->ScsiStatus;
> cmd_free(h, c);
> - /* Is the volume 'not ready'? */
> - if (cmd_status != CMD_TARGET_STATUS ||
> - scsi_status != SAM_STAT_CHECK_CONDITION ||
> - sense_key != NOT_READY ||
> - asc != ASC_LUN_NOT_READY) {
> - return 0;
> - }
>
> /* Determine the reason for not ready state */
> ldstat = hpsa_get_volume_status(h, scsi3addr);
>
> /* Keep volume offline in certain cases: */
> switch (ldstat) {
> + case HPSA_LV_FAILED:
> case HPSA_LV_UNDERGOING_ERASE:
> case HPSA_LV_NOT_AVAILABLE:
> case HPSA_LV_UNDERGOING_RPI:
> @@ -3853,10 +3847,10 @@ static int hpsa_update_device_info(struct ctlr_info
> *h,
> /* Do an inquiry to the device to see what it is. */
> if (hpsa_scsi_do_inquiry(h, scsi3addr, 0, inq_buff,
> (unsigned char) OBDR_TAPE_INQ_SIZE) != 0) {
> - /* Inquiry failed (msg printed already) */
> dev_err(&h->pdev->dev,
> - "hpsa_update_device_info: inquiry failed\n");
> - rc = -EIO;
> + "%s: inquiry failed, device will be skipped.\n",
> + __func__);
> + rc = HPSA_INQUIRY_FAILED;
> goto bail_out;
> }
>
> @@ -3894,6 +3888,13 @@ static int hpsa_update_device_info(struct ctlr_info *h,
> if (volume_offline < 0 || volume_offline > 0xff)
> volume_offline = HPSA_VPD_LV_STATUS_UNSUPPORTED;
> this_device->volume_offline = volume_offline & 0xff;
> + if (volume_offline == HPSA_LV_FAILED) {
> + rc = HPSA_LV_FAILED;
> + dev_err(&h->pdev->dev,
> + "%s: LV failed, device will be skipped.\n",
> + __func__);
> + goto bail_out;
> + }
> } else {
> this_device->raid_level = RAID_UNKNOWN;
> this_device->offload_config = 0;
> @@ -4379,8 +4380,7 @@ static void hpsa_update_scsi_devices(struct ctlr_info
> *h)
> goto out;
> }
> if (rc) {
> - dev_warn(&h->pdev->dev,
> - "Inquiry failed, skipping device.\n");
> + h->drv_req_rescan = 1;
> continue;
> }
>
> diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
> index a584cdf..5961705 100644
> --- a/drivers/scsi/hpsa_cmd.h
> +++ b/drivers/scsi/hpsa_cmd.h
> @@ -156,6 +156,7 @@
> #define CFGTBL_BusType_Fibre2G 0x0200l
>
> /* VPD Inquiry types */
> +#define HPSA_INQUIRY_FAILED 0x02
> #define HPSA_VPD_SUPPORTED_PAGES0x00
> #define HPSA_VPD_LV_DEVICE_ID 0x83
> #define HPSA_VPD_LV_DEVICE_GEOMETRY 0xC1
> @@ -166,6 +167,7 @@
> /* Logical volume states */
> #define HPSA_VPD_LV_STATUS_UNSUPPORTED 0xff
> #define HPSA_LV_OK 0x0
> +#define HPSA_LV_FAILED
