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.
"""
>
> 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
