| Message ID | 20230714213419.95492-12-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: Allow scsi_execute users to control retries | expand |
On 14/07/2023 22:33, Mike Christie wrote: > If scsi_execute_cmd returns < 0, it doesn't initialize the sshdr, so we > shouldn't access the sshdr. If it returns 0, then the cmd executed > successfully, so there is no need to check the sshdr. This has us access > the sshdr when get a return value > 0. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > Reviewed-by: Christoph Hellwig <hch@lst.de> Regardless of a couple of coding style comments, below: Reviewed-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/device_handler/scsi_dh_hp_sw.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c > index 5f2f943d926c..785ab2c5391f 100644 > --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c > +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c > @@ -93,7 +93,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h) > res = scsi_execute_cmd(sdev, cmd, opf, NULL, 0, HP_SW_TIMEOUT, > HP_SW_RETRIES, &exec_args); > if (res) { > - if (scsi_sense_valid(&sshdr)) > + if (res > 0 && scsi_sense_valid(&sshdr)) > ret = tur_done(sdev, h, &sshdr); > else { > sdev_printk(KERN_WARNING, sdev, maybe this is better: if (res > 0 && scsi_sense_valid()) ret = tur_done(sdev, h, &sshdr); else if (res == 0) { h->path_state ... } else { sdev_printk(KERN_WARNING, sdev, ... ret = SCSI_DH_OK; } But I am not sure about the == 0 in the middle... > @@ -134,7 +134,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h) > res = scsi_execute_cmd(sdev, cmd, opf, NULL, 0, HP_SW_TIMEOUT, > HP_SW_RETRIES, &exec_args); > if (res) { > - if (!scsi_sense_valid(&sshdr)) { > + if (res < 0 || !scsi_sense_valid(&sshdr)) { > sdev_printk(KERN_WARNING, sdev, > "%s: sending start_stop_unit failed, " > "no sense available\n", HP_SW_NAME); I think that you could do something similar to above suggestion here. Thanks, John
diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c index 5f2f943d926c..785ab2c5391f 100644 --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c @@ -93,7 +93,7 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h) res = scsi_execute_cmd(sdev, cmd, opf, NULL, 0, HP_SW_TIMEOUT, HP_SW_RETRIES, &exec_args); if (res) { - if (scsi_sense_valid(&sshdr)) + if (res > 0 && scsi_sense_valid(&sshdr)) ret = tur_done(sdev, h, &sshdr); else { sdev_printk(KERN_WARNING, sdev, @@ -134,7 +134,7 @@ static int hp_sw_start_stop(struct hp_sw_dh_data *h) res = scsi_execute_cmd(sdev, cmd, opf, NULL, 0, HP_SW_TIMEOUT, HP_SW_RETRIES, &exec_args); if (res) { - if (!scsi_sense_valid(&sshdr)) { + if (res < 0 || !scsi_sense_valid(&sshdr)) { sdev_printk(KERN_WARNING, sdev, "%s: sending start_stop_unit failed, " "no sense available\n", HP_SW_NAME);