Re: [PATCH v1 3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

[Niklas Cassel <cassel@kernel.org>]

On Mon, Jun 17, 2024 at 11:31:49PM +0000, Igor Pylypiv wrote:
> On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote:
> > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote:
> > > Do not generate sense data from ATA status/error registers
> > > if valid sense data is already present.
> > > 
> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> > > ---
> > >  drivers/ata/libata-scsi.c | 17 +++++++++++------
> > >  1 file changed, 11 insertions(+), 6 deletions(-)
> > > 
> > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > > index 79e8103ef3a9..4bfe47e7d266 100644
> > > --- a/drivers/ata/libata-scsi.c
> > > +++ b/drivers/ata/libata-scsi.c
> > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> > >  	unsigned char *desc = sb + 8;
> > >  	u8 sense_key, asc, ascq;
> > 
> > Like I suggested in the earlier patch,
> > 
> > can't you do a:
> > 
> > if (qc->flags & ATA_QCFLAG_SENSE_VALID)
> > 	return;
> > 
> > here instead?
> > 
> 
> We need to populate the "ATA Status Return sense data descriptor" as per SAT-5
> "Table 209 — ATA command results". By returning early we are skipping the code
> that copies ATA output fields into descriptor/fixed sense data buffer.

We might get sense data from the drive.
If we use REQUEST SENSE DATA EXT, we will get SK/ASC/ASCQ,
we will then call scsi_build_sense_buffer() which will generate
sense data in either the descriptor format or the fixed format,
based on the D_SENSE bit, which is set in the control mode page.

The user can toggle this bit, see:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L3691-L3694

But by default it is 0:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L86
which means fixed format.

This all seems to be in accordance with
"Table 209 — Returned sense data with the CK_COND bit set to one"
in sat6r1.



When calling scsi_build_sense_buffer(), we supply:
scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE,
                        cmd->sense_buffer, tf.lbah,
                        tf.lbam, tf.lbal);

so we do not supply the STATUS and ERROR fields when building the sense data.

This seems fine, since SCSI has no knowledge of ATA status or ATA error.


However, for ATA-passthrough commands, ATA status and ATA error fields
are part of either the COMMAND-SPECIFIC information in the fixed format
case, or part for the descriptor format, in case of the descriptor type
being ATA Status Return sense data descriptor.


So what I think we should do:
1) Keep the sense data if it exists, and fill in
   the ATA status and ATA error at the correct offset (depending on if
   the existing sense data is in fixed format or descriptor format).
2) If there doesn't exist any sense data, generate it, including the
   ATA passthru command specific fields (ATA status and ATA error).
   This is basically what ata_gen_passthru_sense() does today.


So something like this:
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index bb4d30d377ae..a0687eb28ff7 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
         * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
         */
        if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
-           ((cdb[2] & 0x20) || need_sense))
-               ata_gen_passthru_sense(qc);
-       else if (need_sense)
+           ((cdb[2] & 0x20) || need_sense)) {
+               if ((qc->flags & ATA_QCFLAG_SENSE_VALID)) {
+                       /*
+                        * ATA PASS-THROUGH commands also need to fill in the
+                        * command specific ATA status and ATA error fields.
+                        */
+                       ata_fill_passthru_specific_sense_fields(qc);
+               } else {
+                       ata_gen_passthru_sense(qc);
+               }
+       } else if (need_sense)
                ata_gen_ata_sense(qc);
        else
                /* Keep the SCSI ML and status byte, clear host byte. */


Kind regards,
Niklas