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@xxxxxxxxxx>
Reviewed-by: Christoph Hellwig <hch@xxxxxx>
Regardless of a couple of coding style comments, below:
Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
---
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
