Re: [PATCH 3/6] ata: libata-scsi: Refactor ata_scsiop_maint_in()

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

On 3/20/26 11:13, Guenter Roeck wrote:
> Hi,
> 
> On Tue, Oct 22, 2024 at 11:45:34AM +0900, Damien Le Moal wrote:
>> Move the check for MI_REPORT_SUPPORTED_OPERATION_CODES from
>> ata_scsi_simulate() into ata_scsiop_maint_in() to simplify
>> ata_scsi_simulate() code.
>>
>> Furthermore, since an rbuff fill actor function returning a non-zero
>> value causes no data to be returned for the command, directly return
>> an error (return 1) for invalid command formt after setting the invalid
>> field in cdb error.
>>
>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>> ---
>>  drivers/ata/libata-scsi.c | 20 +++++++++++---------
>>  1 file changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index 8097cf318b04..f9c70f650cfc 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3388,12 +3388,16 @@ static unsigned int ata_scsiop_maint_in(struct ata_scsi_args *args, u8 *rbuf)
>>  	struct ata_device *dev = args->dev;
>>  	u8 *cdb = args->cmd->cmnd;
>>  	u8 supported = 0, cdlp = 0, rwcdlp = 0;
>> -	unsigned int err = 0;
>> +
>> +	if ((cdb[1] & 0x1f) != MI_REPORT_SUPPORTED_OPERATION_CODES) {
>> +		ata_scsi_set_invalid_field(dev, args->cmd, 1, 0xff);
>> +		return 1;
>> +	}
>>  
>>  	if (cdb[2] != 1 && cdb[2] != 3) {
>>  		ata_dev_warn(dev, "invalid command format %d\n", cdb[2]);
>> -		err = 2;
>> -		goto out;
>> +		ata_scsi_set_invalid_field(dev, args->cmd, 1, 0xff);
> 
> Nit, but should this possibly be
> 		ata_scsi_set_invalid_field(dev, args->cmd, 2, 0xff);
> 							   ^
> ?

Absolutely ! Good catch !
I will send a patch immediately.


-- 
Damien Le Moal
Western Digital Research