| Message ID | 20220825223912.355011-1-damien.lemoal@opensource.wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | ata: libata-scsi: Improve ata_scsiop_maint_in() | expand |
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@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
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
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; unsigned int err = 0; - if (cdb[2] != 1) { + if (cdb[2] != 1 && cdb[2] != 3) { ata_dev_warn(dev, "invalid command format %d\n", cdb[2]); err = 2; goto out; } + switch (cdb[3]) { case INQUIRY: case MODE_SENSE:
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. Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com> --- drivers/ata/libata-scsi.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)