Re: [PATCH v4 6/9] scsi: sd: Check for and retry in case of READ_CAPCITY(10)/(16) returning no data

[Damien Le Moal <dlemoal@kernel.org>]

On 10/6/25 16:10, Hannes Reinecke wrote:
>> -	memset(buffer, 0, 8);
>> +	for (count = 0; count < 3; ++count) {
>> +		memset(buffer, 0, RC10_LEN);
>> +
>> +		the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN,
>> +					      buffer, RC10_LEN, SD_TIMEOUT,
>> +					      sdkp->max_retries, &exec_args);
>> +
>> +		if (the_result || resid != RC10_LEN)
>> +			break;
>>   
> Hmm. What would happen if some data is returned, but less than the
> expected amount?
> We'd be having a hard time parsing that, wouldn't we?

All data received would normally mean success, so result == 0.
Bad devices cases would be success with resid != 0 but less than RC10_LEN. So I
think the break is correct here since the result is checked after the loop.

> 
> So I guess we should check if we had received _all_ data, no?
> 
>> -	the_result = scsi_execute_cmd(sdp, cmd, REQ_OP_DRV_IN, buffer,
>> -				      8, SD_TIMEOUT, sdkp->max_retries,
>> -				      &exec_args);
>> +		/*
>> +		 * If the status was good but nothing was transferred,
>> +		 * we retry. It is a workaround for some buggy devices
>> +		 * or SAT which sometimes do not return any data.
>> +		 */
>> +	}
>>   
>>   	if (the_result > 0) {
>>   		if (media_not_present(sdkp, &sshdr))
>> @@ -2821,6 +2858,12 @@ static int read_capacity_10(struct scsi_disk *sdkp, struct scsi_device *sdp,
>>   		return -EINVAL;
>>   	}
>>   
>> +	if (resid == RC10_LEN) {
>> +		sd_printk(KERN_ERR, sdkp,
>> +			  "Read Capacity(10) returned good status but no data");
>> +		return -EINVAL;
>> +	}
>> +

And here we should just check that resid is 0 to make the checks complete ? This
could be included in the above check with something like:

	if (resid) {
		sd_printk(KERN_ERR, sdkp,
			"Read Capacity(10) good status but incomplete data");
		return -EINVAL;
	}

>>   	sector_size = get_unaligned_be32(&buffer[4]);
>>   	lba = get_unaligned_be32(&buffer[0]);
>>   
> 
> Similar here. I would assume that _any_ resid value larger than zero 
> would render thecapacity value unreliable...
> 
> Cheers,
> 
> Hannes


-- 
Damien Le Moal
Western Digital Research