Re: [PATCH] ata: libata-scsi: Improve ata_scsiop_maint_in()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 2022/08/31 15:27, Niklas Cassel wrote:
> On Fri, Aug 26, 2022 at 07:39:12AM +0900, Damien Le Moal wrote:
>> Allow translation of REPORT_SUPPORTED_OPERATION_CODES commands using
>> the command format 0x3, that is, checking support for commands that are
>> identified using an opcode and a service action.
> 
> So, while the function ata_scsiop_maint_in() has the kdoc:
> *      ata_scsiop_maint_in - Simulate a subset of MAINTENANCE_IN
> 
> It actually only handles a MAINTENANCE_IN service action (command).
> 
> The name is thus quite misleading.
> Perhaps you could do a patch 1/2 that renames the function so that
> it is more clear that it only handles a single service action.
> 
> (If we ever add translation support for more than one action,
> we could reintroduce a ata_scsiop_maint_in() which calls the
> correct function to translate the correct service action.
> 
> 
> 
>> Allow translation of REPORT_SUPPORTED_OPERATION_CODES commands using
>> the command format 0x3, that is, checking support for commands that are
>> identified using an opcode and a service action.
> 
> If you rename the function, the commit message will make more sense,
> but could be further clarified to something like:
> 
> """
> The ata_scsi_report_supported_opcodes_xlat() currently only handles
> currently only handles a command specifying "REPORTING OPTIONS" field
> set to 0x1.
> 0x1 means:
> return data in one_command format if:
> -The opcode in REQUESTED OPERATION CODE is supported,
> REQUESTED SERVICE ACTION field is ignored.
> If opcode implements service actions, then terminate the command
> with CHECK CONDITION and sense key set to ILLEGAL REQUEST and
> additional sense code set to INVALID FIELD IN CDB.
> 
> Add support for translating a "REPORTING OPTIONS" field set to 0x3.
> 0x3 means:
> return data in one_command format if:
> -The opcode in REQUESTED OPERATION CODE does not have service actions
> and the service action in REQUESTED SERVICE ACTION is set to 0x0; or
> -The opcode in REQUESTED OPERATION CODE has service actions and the service action in REQUESTED SERVICE ACTION is supported.
> else:
> the command support data shall indicate that the command is not supported,
> i.e. the support field is set to 0x1 rather than 0x3 or 0x5.
> """

I have dropped this patch for now. Will rework it.

> 
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
>> ---
>>  drivers/ata/libata-scsi.c | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index f3c64e796423..99ebd7bf3a9c 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -3252,11 +3252,12 @@ static unsigned int ata_scsiop_maint_in(struct ata_scsi_args *args, u8 *rbuf)
>>  	u8 supported = 0;
> 
> A supported value of 0x0 means "Data about the requested SCSI command
> is not currently available".
> 
> However, considering the differences between 0x3 and 0x1, if we want to
> follow the spec strictly, we should initialize the variable "supported"
> to 0x1 rather than 0x0, when the supplied REPORTING OPTIONS is 0x3.
> 
> REPORTING OPTIONS 0x3 mentions that "supported" should be 0x5, 0x3 or 0x1.
> So (according to the spec) 0x0 does not appear to be a valid value when
> REPORTING OPTIONS is 0x3.
> 
>>  	unsigned int err = 0;
>>  
>> -	if (cdb[2] != 1) {
>> +	if (cdb[2] != 1 && cdb[2] != 3) {
> 
> Considering that this function never terminated a command with
> sense key and additional sense code set before, none of the commands
> support a service action.
> 
> Therefore, we could change this to only allow commands where:
> cdb[2] == 1; or
> cdb[2] == 3 && REQUESTED SERVICE ACTION (cdb[4] && cdb[5]) == 0x0
> 
> 
> Kind regards,
> Niklas

-- 
Damien Le Moal
Western Digital Research