diff mbox series

[v3,32/35] scsi: sd: Have scsi-ml retry read_capacity_10 errors

Message ID 20221003175321.8040-33-michael.christie@oracle.com
State Superseded
Headers show
Series Allow scsi_execute users to control retries | expand

Commit Message

Mike Christie Oct. 3, 2022, 5:53 p.m. UTC 
This has read_capacity_10 have scsi-ml retry errors instead of driving
them itself.

Signed-off-by: Mike Christie <michael.christie@oracle.com>
Reviewed-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/sd.c | 60 +++++++++++++++++++++++------------------------
 1 file changed, 30 insertions(+), 30 deletions(-)

Comments

Bart Van Assche Oct. 4, 2022, 11:42 p.m. UTC | #1 
On 10/3/22 10:53, Mike Christie wrote:
> +	cmd[0] = READ_CAPACITY;
> +	memset(&cmd[1], 0, 9);

Please remove the above code and change the cmd[] declaration into something like

static const u8 cmd[10] = { READ_CAPACITY };

Thanks,

Bart.
Mike Christie Oct. 5, 2022, 5 p.m. UTC | #2 
On 10/4/22 6:42 PM, Bart Van Assche wrote:
> On 10/3/22 10:53, Mike Christie wrote:
>> +    cmd[0] = READ_CAPACITY;
>> +    memset(&cmd[1], 0, 9);
> 
> Please remove the above code and change the cmd[] declaration into something like
> 
> static const u8 cmd[10] = { READ_CAPACITY };

Would you be ok if I made these types of changes in a separate patchset?
This patcheset tried to only remove the loop/goto logic since it was
focused on retries.

Besides the ones you pointed out, I think I can make maybe 5-10 more
scsi_execute* users use static const or at least do:

unsigned char cmd[6] = { CMD } or { }

and remove some extra memsets so we end up doing it more or less the same
in all the users.

It seemed more like a cleanup patchset so I didn't want to mix it up as
this one was getting big.
Bart Van Assche Oct. 5, 2022, 5:40 p.m. UTC | #3 
On 10/5/22 10:00, Mike Christie wrote:
> On 10/4/22 6:42 PM, Bart Van Assche wrote:
>> On 10/3/22 10:53, Mike Christie wrote:
>>> +    cmd[0] = READ_CAPACITY;
>>> +    memset(&cmd[1], 0, 9);
>>
>> Please remove the above code and change the cmd[] declaration into something like
>>
>> static const u8 cmd[10] = { READ_CAPACITY };
> 
> Would you be ok if I made these types of changes in a separate patchset?
> This patcheset tried to only remove the loop/goto logic since it was
> focused on retries.
> 
> Besides the ones you pointed out, I think I can make maybe 5-10 more
> scsi_execute* users use static const or at least do:
> 
> unsigned char cmd[6] = { CMD } or { }
> 
> and remove some extra memsets so we end up doing it more or less the same
> in all the users.
> 
> It seemed more like a cleanup patchset so I didn't want to mix it up as
> this one was getting big.

Hi Mike,

Patch 32/35 already changes the code that initializes cmd[] so I think 
it is fine to make the change I suggested and to rework error handling 
in a single patch. However, I agree that if reworking the initialization 
of cmd[] would be done as separate patches that it would make this 
series too big.

Thanks,

Bart.
diff mbox series

Patch

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index cb07a887b40c..dacd54af40c3 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -2401,41 +2401,41 @@  static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int the_result;
-	int retries = 3, reset_retries = READ_CAPACITY_RETRIES_ON_RESET;
 	sector_t lba;
 	unsigned sector_size;
+	struct scsi_failure failures[] = {
+		{
+			.sense = UNIT_ATTENTION,
+			.asc = 0x29,
+			.ascq = 0,
+			/* Device reset might occur several times */
+			.allowed = READ_CAPACITY_RETRIES_ON_RESET,
+			.result = SAM_STAT_CHECK_CONDITION,
+		},
+		{
+			.result = SCMD_FAILURE_ANY,
+			.allowed = 3,
+		},
+		{},
+	};
 
-	do {
-		cmd[0] = READ_CAPACITY;
-		memset(&cmd[1], 0, 9);
-		memset(buffer, 0, 8);
-
-		the_result = scsi_exec_req(((struct scsi_exec_args) {
-						.sdev = sdp,
-						.cmd = cmd,
-						.data_dir = DMA_FROM_DEVICE,
-						.buf = buffer,
-						.buf_len = 8,
-						.sshdr = &sshdr,
-						.timeout = SD_TIMEOUT,
-						.retries = sdkp->max_retries }));
-
-		if (media_not_present(sdkp, &sshdr))
-			return -ENODEV;
+	cmd[0] = READ_CAPACITY;
+	memset(&cmd[1], 0, 9);
+	memset(buffer, 0, 8);
 
-		if (the_result > 0) {
-			sense_valid = scsi_sense_valid(&sshdr);
-			if (sense_valid &&
-			    sshdr.sense_key == UNIT_ATTENTION &&
-			    sshdr.asc == 0x29 && sshdr.ascq == 0x00)
-				/* Device reset might occur several times,
-				 * give it one more chance */
-				if (--reset_retries > 0)
-					continue;
-		}
-		retries--;
+	the_result = scsi_exec_req(((struct scsi_exec_args) {
+					.sdev = sdp,
+					.cmd = cmd,
+					.data_dir = DMA_FROM_DEVICE,
+					.buf = buffer,
+					.buf_len = 8,
+					.sshdr = &sshdr,
+					.timeout = SD_TIMEOUT,
+					.retries = sdkp->max_retries,
+					.failures = failures }));
 
-	} while (the_result && retries);
+	if (media_not_present(sdkp, &sshdr))
+		return -ENODEV;
 
 	if (the_result) {
 		sd_print_result(sdkp, "Read Capacity(10) failed", the_result);