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

[Igor Pylypiv <ipylypiv@google.com>]

On Thu, Jun 20, 2024 at 04:24:11PM +0200, Niklas Cassel wrote:
> 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.
>

Yes! That's exactly what I was trying to do with the horrible empty if in
ata_gen_passthru_sense(). Factoring out ATA status and ATA error population
into a separate function makes the code more readable and less error-prone.

I'll make the change in v2.

Thank you!
Igor

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