Re: [PATCH 1/5] scsi: sd: Have scsi-ml retry read_capacity_16 errors

[Bart Van Assche <bvanassche@acm.org>]

On 7/16/25 11:48 AM, Ewan D. Milne wrote:
> +		/*
> +		 * Do not retry Invalid Command Operation Code or Invalid
> +		 * Field in CDB.
> +		 */
> +		{
> +			.sense = ILLEGAL_REQUEST,
> +			.asc = 0x20,
> +			.result = SAM_STAT_CHECK_CONDITION,
> +		},
> +		{
> +			.sense = ILLEGAL_REQUEST,
> +			.asc = 0x24,
> +			.result = SAM_STAT_CHECK_CONDITION,
> +		},
> +		/* Do not retry Medium Not Present */
> +		{
> +			.sense = UNIT_ATTENTION,
> +			.asc = 0x3A,
> +			.result = SAM_STAT_CHECK_CONDITION,
> +		},
> +		{
> +			.sense = NOT_READY,
> +			.asc = 0x3A,
> +			.result = SAM_STAT_CHECK_CONDITION,
> +		},
> +		/* Device reset might occur several times so retry a lot */
> +		{
> +			.sense = UNIT_ATTENTION,
> +			.asc = 0x29,
> +			.allowed = READ_CAPACITY_RETRIES_ON_RESET,
> +			.result = SAM_STAT_CHECK_CONDITION,
> +		},

For the first, second and fifth array elements above: leaving out .ascq
is the same as requiring that the ASCQ value is zero. I prefer to make
this explicit by adding ".ascq = 0," in these array elements.

Additionally, media_not_present() doesn't check the ASCQ value while
the above array only accepts ASCQ == 0 if ASC == 0x3a. Please either
mention this behavior change in the patch description or add the
following in the third and fourth array elements:
".ascq = SCMD_FAILURE_ASCQ_ANY,".

> +	memset(buffer, 0, RC16_LEN);

Isn't the preferred style "memset(buffer, 0, ARRAY_SIZE(buffer))"? This
makes it easier for readers to verify that the third argument is
correct.

Otherwise this patch looks good to me.

Thanks,

Bart.