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@xxxxxxxxxxxxxxxxxx>
>> ---
>> 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
