| Message ID | 1430743343-47174-4-git-send-email-hare@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/04/15 14:42, Hannes Reinecke wrote: > -/* > * submit_rtpg - Issue a REPORT TARGET GROUP STATES command > * @sdev: sdev the command should be sent to > */ > @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h) > sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n", > ALUA_DH_NAME); > break; > - default: > - h->tpgs = TPGS_MODE_NONE; > + case TPGS_MODE_NONE: > sdev_printk(KERN_INFO, sdev, "%s: not supported\n", > ALUA_DH_NAME); > err = SCSI_DH_DEV_UNSUPP; > break; > + default: > + sdev_printk(KERN_INFO, sdev, > + "%s: unsupported TPGS setting %d\n", > + ALUA_DH_NAME, h->tpgs); > + h->tpgs = TPGS_MODE_NONE; > + err = SCSI_DH_DEV_UNSUPP; > + break; > } > > return err; > } The function scsi_device_tpgs() returns a value between 0 and 3. So why to add a fifth case in this switch statement ? Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/07/2015 01:41 PM, Bart Van Assche wrote: > On 05/04/15 14:42, Hannes Reinecke wrote: >> -/* >> * submit_rtpg - Issue a REPORT TARGET GROUP STATES command >> * @sdev: sdev the command should be sent to >> */ >> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct >> scsi_device *sdev, struct alua_dh_data *h) >> sdev_printk(KERN_INFO, sdev, "%s: supports implicit >> TPGS\n", >> ALUA_DH_NAME); >> break; >> - default: >> - h->tpgs = TPGS_MODE_NONE; >> + case TPGS_MODE_NONE: >> sdev_printk(KERN_INFO, sdev, "%s: not supported\n", >> ALUA_DH_NAME); >> err = SCSI_DH_DEV_UNSUPP; >> break; >> + default: >> + sdev_printk(KERN_INFO, sdev, >> + "%s: unsupported TPGS setting %d\n", >> + ALUA_DH_NAME, h->tpgs); >> + h->tpgs = TPGS_MODE_NONE; >> + err = SCSI_DH_DEV_UNSUPP; >> + break; >> } >> >> return err; >> } > > The function scsi_device_tpgs() returns a value between 0 and 3. So > why to add a fifth case in this switch statement ? > Because I'm paranoid? 'h->tpgs' is an integer, so _in principle_ it could take any value. We can only safely restrict this by turning 'h->tpgs' into an enum. _And_ 'h->tpgs' is being set to '-1' initially, so this is to catch any logic / initialisation issues. Cheers, Hannes
> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h) > sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n", > ALUA_DH_NAME); > break; > - default: > - h->tpgs = TPGS_MODE_NONE; > + case TPGS_MODE_NONE: > sdev_printk(KERN_INFO, sdev, "%s: not supported\n", > ALUA_DH_NAME); > err = SCSI_DH_DEV_UNSUPP; > break; > + default: > + sdev_printk(KERN_INFO, sdev, > + "%s: unsupported TPGS setting %d\n", > + ALUA_DH_NAME, h->tpgs); > + h->tpgs = TPGS_MODE_NONE; > + err = SCSI_DH_DEV_UNSUPP; > + break; Can you split this into a separate patch, please? Otherwise looks fine, so: Reviewed-by: Christoph Hellwig <hch@lst.de> for both resulting patches. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 05/11/2015 08:48 AM, Christoph Hellwig wrote: >> @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h) >> sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n", >> ALUA_DH_NAME); >> break; >> - default: >> - h->tpgs = TPGS_MODE_NONE; >> + case TPGS_MODE_NONE: >> sdev_printk(KERN_INFO, sdev, "%s: not supported\n", >> ALUA_DH_NAME); >> err = SCSI_DH_DEV_UNSUPP; >> break; >> + default: >> + sdev_printk(KERN_INFO, sdev, >> + "%s: unsupported TPGS setting %d\n", >> + ALUA_DH_NAME, h->tpgs); >> + h->tpgs = TPGS_MODE_NONE; >> + err = SCSI_DH_DEV_UNSUPP; >> + break; > > Can you split this into a separate patch, please? > > Otherwise looks fine, so: > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > for both resulting patches. > Okay, will do. Cheers, Hannes
diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c index 834e80f..b5903eb 100644 --- a/drivers/scsi/device_handler/scsi_dh_alua.c +++ b/drivers/scsi/device_handler/scsi_dh_alua.c @@ -131,43 +131,6 @@ static struct request *get_alua_req(struct scsi_device *sdev, } /* - * submit_vpd_inquiry - Issue an INQUIRY VPD page 0x83 command - * @sdev: sdev the command should be sent to - */ -static int submit_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h) -{ - struct request *rq; - int err = SCSI_DH_RES_TEMP_UNAVAIL; - - rq = get_alua_req(sdev, h->buff, h->bufflen, READ); - if (!rq) - goto done; - - /* Prepare the command. */ - rq->cmd[0] = INQUIRY; - rq->cmd[1] = 1; - rq->cmd[2] = 0x83; - rq->cmd[4] = h->bufflen; - rq->cmd_len = COMMAND_SIZE(INQUIRY); - - rq->sense = h->sense; - memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE); - rq->sense_len = h->senselen = 0; - - err = blk_execute_rq(rq->q, NULL, rq, 1); - if (err == -EIO) { - sdev_printk(KERN_INFO, sdev, - "%s: evpd inquiry failed with %x\n", - ALUA_DH_NAME, rq->errors); - h->senselen = rq->sense_len; - err = SCSI_DH_IO; - } - blk_put_request(rq); -done: - return err; -} - -/* * submit_rtpg - Issue a REPORT TARGET GROUP STATES command * @sdev: sdev the command should be sent to */ @@ -352,55 +315,42 @@ static int alua_check_tpgs(struct scsi_device *sdev, struct alua_dh_data *h) sdev_printk(KERN_INFO, sdev, "%s: supports implicit TPGS\n", ALUA_DH_NAME); break; - default: - h->tpgs = TPGS_MODE_NONE; + case TPGS_MODE_NONE: sdev_printk(KERN_INFO, sdev, "%s: not supported\n", ALUA_DH_NAME); err = SCSI_DH_DEV_UNSUPP; break; + default: + sdev_printk(KERN_INFO, sdev, + "%s: unsupported TPGS setting %d\n", + ALUA_DH_NAME, h->tpgs); + h->tpgs = TPGS_MODE_NONE; + err = SCSI_DH_DEV_UNSUPP; + break; } return err; } /* - * alua_vpd_inquiry - Evaluate INQUIRY vpd page 0x83 + * alua_check_vpd - Evaluate INQUIRY vpd page 0x83 * @sdev: device to be checked * * Extract the relative target port and the target port group * descriptor from the list of identificators. */ -static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h) +static int alua_check_vpd(struct scsi_device *sdev, struct alua_dh_data *h) { - int len; - unsigned err; unsigned char *d; - retry: - err = submit_vpd_inquiry(sdev, h); - - if (err != SCSI_DH_OK) - return err; - - /* Check if vpd page exceeds initial buffer */ - len = (h->buff[2] << 8) + h->buff[3] + 4; - if (len > h->bufflen) { - /* Resubmit with the correct length */ - if (realloc_buffer(h, len)) { - sdev_printk(KERN_WARNING, sdev, - "%s: kmalloc buffer failed\n", - ALUA_DH_NAME); - /* Temporary failure, bypass */ - return SCSI_DH_DEV_TEMP_BUSY; - } - goto retry; - } + if (!sdev->vpd_pg83) + return SCSI_DH_DEV_UNSUPP; /* - * Now look for the correct descriptor. + * Look for the correct descriptor. */ - d = h->buff + 4; - while (d < h->buff + len) { + d = sdev->vpd_pg83 + 4; + while (d < sdev->vpd_pg83 + sdev->vpd_pg83_len) { switch (d[1] & 0xf) { case 0x4: /* Relative target port */ @@ -427,14 +377,13 @@ static int alua_vpd_inquiry(struct scsi_device *sdev, struct alua_dh_data *h) ALUA_DH_NAME); h->state = TPGS_STATE_OPTIMIZED; h->tpgs = TPGS_MODE_NONE; - err = SCSI_DH_DEV_UNSUPP; - } else { - sdev_printk(KERN_INFO, sdev, - "%s: port group %02x rel port %02x\n", - ALUA_DH_NAME, h->group_id, h->rel_port); + return SCSI_DH_DEV_UNSUPP; } + sdev_printk(KERN_INFO, sdev, + "%s: port group %02x rel port %02x\n", + ALUA_DH_NAME, h->group_id, h->rel_port); - return err; + return 0; } static char print_alua_state(int state) @@ -697,7 +646,7 @@ static int alua_initialize(struct scsi_device *sdev, struct alua_dh_data *h) if (err != SCSI_DH_OK) goto out; - err = alua_vpd_inquiry(sdev, h); + err = alua_check_vpd(sdev, h); if (err != SCSI_DH_OK) goto out;
The SCSI device now has the VPD page 0x83 information attached, so there is no need to query it again. Signed-off-by: Hannes Reinecke <hare@suse.de> --- drivers/scsi/device_handler/scsi_dh_alua.c | 93 +++++++----------------------- 1 file changed, 21 insertions(+), 72 deletions(-)