Message-ID: <ZnIBdj02yKFz4sK8@google.com>
Date: Tue, 18 Jun 2024 21:51:50 +0000
From: Igor Pylypiv <ipylypiv@...gle.com>
To: Niklas Cassel <cassel@...nel.org>
Cc: Damien Le Moal <dlemoal@...nel.org>, Tejun Heo <tj@...nel.org>,
	Hannes Reinecke <hare@...e.de>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when
 ATA ERR/DF are set

On Mon, Jun 17, 2024 at 01:29:25PM +0200, Niklas Cassel wrote:
> On Fri, Jun 14, 2024 at 07:18:33PM +0000, Igor Pylypiv wrote:
> > SCSI/ATA Translation-5 (SAT-5) Table 209 — "ATA command results"
> > specifies that SATL shall generate sense data for ATA PASS-THROUGH
> > commands when either CK_COND is set or when ATA_ERR or ATA_DF status
> > bits are set.
> > 
> > ata_eh_analyze_tf() sets AC_ERR_DEV bit in qc->err_mask when ATA_ERR
> > or ATA_DF bits are set. It looks like qc->err_mask can be used as
> > an error indicator but ata_eh_link_autopsy() clears AC_ERR_DEV bit
> > when ATA_QCFLAG_SENSE_VALID is set. This effectively clears the error
> > indication if no other bits were set in qc->err_mask.
> 
> The reason why libata clears the err_mask when having sense data,
> is because the upper layer, i.e. SCSI, should determine what to do
> with the command, if there is sense data.
> 
> For a non-passthrough command, this will be done by
> scsi_io_completion_action():
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1084-L1087
> 
> 
> However, if there is any bits set in cmd->result,
> scsi_io_completion_nz_result() will be called:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1052-L1053
> 
> which will do the following for a passthrough command:
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L969-L978
> which will set blk_stat.
> 
> After that, scsi_io_completion() which check blk_stat and if it is a
> scsi_noretry_cmd():
> https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/scsi/scsi_lib.c#L1073-L1078
> 
> A passthrough command will return true for scsi_noretry_cmd(), so
> scsi_io_completion_action() should NOT get called for a passthough command.
> 
> So IIUC, for a non-passthrough command, scsi_io_completion_action() will
> decide what to do depending on the sense data, but a passthrough command will
> get finished with the sense data, leaving the user to decide what to do.
>

Thank you for the detailed explanation, Niklas!
I was looking at a related logic in ata_eh_link_report():
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-eh.c#L2359-L2360
 
Is my understanding correct that if we have ATA_QCFLAG_SENSE_VALID set and 
qc->err_mask is zero then we don't want to report the error to user since
SCSI might decide that it is not an error based on the sense data?

> 
> > 
> > ata_scsi_qc_complete() should not use qc->err_mask for ATA PASS-THROUGH
> > commands because qc->err_mask can be zero (i.e. "no error") even when
> > the corresponding command has failed with ATA_ERR/ATA_DF bits set.
> > 
> > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@...gle.com>
> > ---
> >  drivers/ata/libata-scsi.c | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index 032cf11d0bcc..79e8103ef3a9 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1632,8 +1632,8 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  		!(qc->flags & ATA_QCFLAG_SENSE_VALID);
> >  
> >  	/* For ATA pass thru (SAT) commands, generate a sense block if
> > -	 * user mandated it or if there's an error.  Note that if we
> > -	 * generate because the user forced us to [CK_COND =1], a check
> > +	 * user mandated it or if ATA_ERR or ATA_DF bits are set. Note that
> > +	 * if we generate because the user forced us to [CK_COND=1], a check
> >  	 * condition is generated and the ATA register values are returned
> >  	 * whether the command completed successfully or not. If there
> >  	 * was no error, we use the following sense data:
> > @@ -1641,7 +1641,7 @@ 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))
> > +	    ((cdb[2] & 0x20) || (qc->result_tf.status & (ATA_ERR | ATA_DF))))
> 
> qc->result_tf can only be used if qc->flags has ATA_QCFLAG_RESULT_TF set,
> otherwise it can contain bogus data.
> You don't seem to check if ATA_QCFLAG_RESULT_TF is set.
>
> ata_scsi_pass_thru() does set ATA_QCFLAG_RESULT_TF.
> 

Thanks for pointing this out! Looks like ATA PASS-TRHOUGH case is fine
since the flag is always set by ata_scsi_pass_thru() as you pointed out.
Do we still want to add the check even though we know that it is always
set by ata_scsi_pass_thru()?

If the answer is "yes", I wonder if we should use the ATA_QCFLAG_RTF_FILLED
flag instead? Currently it is used for ahci only but looks like it can be
expanded to other drivers. inic_qc_fill_rtf() will benefit from this change
because it is not always setting the status/error values:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/sata_inic162x.c#L583-L586

For the non passthough case qc->result_tf in ata_gen_ata_sense() is also valid
because fill_result_tf() is being called for failed commands regardless of
the ATA_QCFLAG_RESULT_TF flag:
https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-core.c#L4856-L4873

In this case using ATA_QCFLAG_RTF_FILLED will be more accurate because
fill_result_tf() is being called even when ATA_QCFLAG_RESULT_TF is not set.

With that said I'm not sure if it makes sense to update all of the ATA
error handling to start checking for the ATA_QCFLAG_RTF_FILLED flag.

What are your thoughts on this?

> 
> >  		ata_gen_passthru_sense(qc);
> >  	else if (need_sense)
> >  		ata_gen_ata_sense(qc);
> > -- 
> > 2.45.2.627.g7a2c4fd464-goog
> > 

Thank you,
Igor

Powered by blists - more mailing lists