diff mbox series

[v10,11/33] scsi: hp_sw: Only access sshdr if res > 0

Message ID 20230714213419.95492-12-michael.christie@oracle.com
State New
Headers show
Series scsi: Allow scsi_execute users to control retries | expand

Commit Message

Mike Christie July 14, 2023, 9:33 p.m. UTC 
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>
---
 drivers/scsi/device_handler/scsi_dh_hp_sw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

John Garry July 18, 2023, 1:13 p.m. UTC | #1 
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 mbox series

Patch

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);