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 ? > 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. Do you see an issue without this change ? If yes, what are the adapter and operations you are running ? > > Signed-off-by: Igor Pylypiv <ipylypiv@xxxxxxxxxx> > --- > 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
