Re: [PATCH v1 2/4] ata: libata-scsi: Generate ATA PT sense data when ATA ERR/DF are set

[Igor Pylypiv <ipylypiv@google.com>]

On Mon, Jun 17, 2024 at 08:22:07AM +0900, Damien Le Moal wrote:
> On 6/15/24 04:18, 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.
> > 
> > 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.
> 
> If there was a failed command, qc->err_mask will be non-zero since the command
> completion will be signaled by an error interrupt. So I am confused here. Are
> you seeing this happening in practice ? If yes, doing what with which  adapter ?
>

Yes, this is happening in practice with the PM8006 adapter. ata_eh_analyze_tf()
sets AC_ERR_DEV and later ata_eh_link_autopsy() clears AC_ERR_DEV making
qc->err_mask == 0.

ata_eh_link_autopsy()
  \
   `ata_eh_analyze_tf()
            if (stat & (ATA_ERR | ATA_DF)) {                                        
                    qc->err_mask |= AC_ERR_DEV;     <<< set AC_ERR_DEV                                   
                    /*                                                              
                     * Sense data reporting does not work if the                    
                     * device fault bit is set.                                     
                     */                                                             
                     if (stat & ATA_DF)                                              
                         stat &= ~ATA_SENSE;                                     
    ...

<cont. ata_eh_link_autopsy()>
        /*                                                              
         * SENSE_VALID trumps dev/unknown error and revalidation. Upper 
         * layers will determine whether the command is worth retrying  
         * based on the sense data and device class/type. Otherwise,    
         * determine directly if the command is worth retrying using its
         * error mask and flags.                                        
         */                                                             
        if (qc->flags & ATA_QCFLAG_SENSE_VALID)                         
                qc->err_mask &= ~(AC_ERR_DEV | AC_ERR_OTHER); <<< clear AC_ERR_DEV



> > Additionally, the presence of valid sense data (ATA_QCFLAG_SENSE_VALID)
> > should not prevent SATL from generating sense data for ATA PASS-THROUGH.
> 
> Same here, this is strange: ATA_QCFLAG_SENSE_VALID indicates that we have sense
> data for the command, regardless if the command is passthrough or not. So if
> that flag is set, we should not overwrite the already valid sense data with
> sense data generated from the error and status bits.

Sorry about the confusion. I meant that the "ATA Status Return sense data
descriptor" is not getting populated when ATA_QCFLAG_SENSE_VALID is set
and CK_COND is 0.

What you described is true when CK_COND is 0, however, when CK_COND is 1,
existing code will call ata_gen_passthru_sense() without checking
the "need_sense" value. This will generate fake sk/asc/ascq regardless of
the ATA_QCFLAG_SENSE_VALID flag.

> Do you see an issue without this change ? If yes, what are the adapter and
> operations you are running ?
> 
Yes, we see the issue on PM8006 adapter.

Any failed ATA PASS-THROUGH command with CK_COND=1 has this issue. For example,
Sending READ SECTOR(S) EXT via ATA PASS-THROUGH with CK_COND=1 to an lba
that was previously corrupted by WRITE UNCORRECTABLE EXT is supposed to
return "READ ERROR - LBA MARKED BAD BY APPLICATION CLIENT" but instead it
returns generated "UNRECOVERED READ ERROR - AUTO REALLOCATE FAILED".

> > 
> > Signed-off-by: Igor Pylypiv <ipylypiv@google.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))))
> >  		ata_gen_passthru_sense(qc);
> >  	else if (need_sense)
> >  		ata_gen_ata_sense(qc);
> 
> -- 
> Damien Le Moal
> Western Digital Research
>

Thank you,
Igor