Date: Thu, 20 Jun 2024 14:55:59 +0200
From: Niklas Cassel <cassel@...nel.org>
To: Igor Pylypiv <ipylypiv@...gle.com>
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 Tue, Jun 18, 2024 at 09:51:50PM +0000, Igor Pylypiv wrote:
> 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?

I'm assuming that that was the reasoning.

However, IIUC, passthrough commands should never be retried by SCSI,
it should always be reported back to the user.


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

I see your point, we will fill the result if there is an error,
even if ATA_QCFLAG_RESULT_TF wasn't set.

Perhaps we should modify fill_result_tf() to set ATA_QCFLAG_RTF_FILLED,
after it has called ap->ops->qc_fill_rtf(qc);

Then this code can check if ATA_QCFLAG_RTF_FILLED is set, like you suggested.


Kind regards,
Niklas

Powered by blists - more mailing lists