| Message ID | 20230614071719.6372-16-michael.christie@oracle.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: Allow scsi_execute users to control retries | expand |
On 6/14/23 00:17, Mike Christie wrote: > If scsi_execute_cmd returns < 0 it will not have set the sshdr, so we > can't access it. > > Signed-off-by: Mike Christie <michael.christie@oracle.com> > --- > drivers/scsi/scsi_transport_spi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c > index 2442d4d2e3f3..2100c3adb456 100644 > --- a/drivers/scsi/scsi_transport_spi.c > +++ b/drivers/scsi/scsi_transport_spi.c > @@ -126,7 +126,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd, > */ > result = scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, > DV_TIMEOUT, 1, &exec_args); > - if (result < 0 || !scsi_sense_valid(sshdr) || > + if (result <= 0 || !scsi_sense_valid(sshdr) || > sshdr->sense_key != UNIT_ATTENTION) > break; > } Hmm ... why is this change necessary? If result == 0 and args->sshdr != 0 then scsi_execute() has called scsi_normalize_sense(). The first function call in scsi_normalize_sense() is memset(sshdr, 0, sizeof(struct scsi_sense_hdr)). Does this mean that it is safe to access the contents of sshdr if scsi_execute_cmd() returns 0? Thanks, Bart.
On 6/14/23 4:46 PM, Bart Van Assche wrote: > On 6/14/23 00:17, Mike Christie wrote: >> If scsi_execute_cmd returns < 0 it will not have set the sshdr, so we >> can't access it. >> >> Signed-off-by: Mike Christie <michael.christie@oracle.com> >> --- >> drivers/scsi/scsi_transport_spi.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c >> index 2442d4d2e3f3..2100c3adb456 100644 >> --- a/drivers/scsi/scsi_transport_spi.c >> +++ b/drivers/scsi/scsi_transport_spi.c >> @@ -126,7 +126,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd, >> */ >> result = scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, >> DV_TIMEOUT, 1, &exec_args); >> - if (result < 0 || !scsi_sense_valid(sshdr) || >> + if (result <= 0 || !scsi_sense_valid(sshdr) || >> sshdr->sense_key != UNIT_ATTENTION) >> break; >> } > > Hmm ... why is this change necessary? It's not needed. Will drop. I think when reviewing sshdr code I thought it was a waste to check for sense when result was zero. When I broke up the set, it got caught in the sshdr fixes. Will drop since not related.
On Wed, Jun 14, 2023 at 02:17:01AM -0500, Mike Christie wrote: > If scsi_execute_cmd returns < 0 it will not have set the sshdr, so we > can't access it. Looks good: Reviewed-by: Christoph Hellwig <hch@lst.de>
diff --git a/drivers/scsi/scsi_transport_spi.c b/drivers/scsi/scsi_transport_spi.c index 2442d4d2e3f3..2100c3adb456 100644 --- a/drivers/scsi/scsi_transport_spi.c +++ b/drivers/scsi/scsi_transport_spi.c @@ -126,7 +126,7 @@ static int spi_execute(struct scsi_device *sdev, const void *cmd, */ result = scsi_execute_cmd(sdev, cmd, opf, buffer, bufflen, DV_TIMEOUT, 1, &exec_args); - if (result < 0 || !scsi_sense_valid(sshdr) || + if (result <= 0 || !scsi_sense_valid(sshdr) || sshdr->sense_key != UNIT_ATTENTION) break; } @@ -676,10 +676,10 @@ spi_dv_device_echo_buffer(struct scsi_device *sdev, u8 *buffer, for (r = 0; r < retries; r++) { result = spi_execute(sdev, spi_write_buffer, REQ_OP_DRV_OUT, buffer, len, &sshdr); - if(result || !scsi_device_online(sdev)) { + if (result || !scsi_device_online(sdev)) { scsi_device_set_state(sdev, SDEV_QUIESCE); - if (scsi_sense_valid(&sshdr) + if (result > 0 && scsi_sense_valid(&sshdr) && sshdr.sense_key == ILLEGAL_REQUEST /* INVALID FIELD IN CDB */ && sshdr.asc == 0x24 && sshdr.ascq == 0x00)
If scsi_execute_cmd returns < 0 it will not have set the sshdr, so we can't access it. Signed-off-by: Mike Christie <michael.christie@oracle.com> --- drivers/scsi/scsi_transport_spi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)