On Fri, Sep 15, 2023 at 03:32:50PM +0900, Damien Le Moal wrote:
> On 9/14/23 00:04, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@xxxxxxx>
> >
> > Currently, we fetch sense data for a _successful_ command if either:
> > 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
> > ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
> > sense data supported bit is set); or
> > 2) Command was non-NCQ and regular sense data reporting is enabled.
> >
> > This means that case 2) will trigger for a non-NCQ command which has
> > ATA_SENSE bit set, regardless if CDL is enabled or not.
> >
> > This decision was by design. If the device reports that it has sense data
> > available, it makes sense to fetch that sense data, since the sk/asc/ascq
> > could be important information regardless if CDL is enabled or not.
> >
> > However, the fetching of sense data for a successful command is done via
> > ATA EH. Considering how intricate the ATA EH is, we really do not want to
> > invoke ATA EH unless absolutely needed.
> >
> > Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
> > commands using policy 0xD") we never fetched sense data for successful
> > non-NCQ commands.
> >
> > In order to not invoke the ATA EH unless absolutely necessary, even if the
> > device claims support for sense data reporting, only fetch sense data for
> > successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
> >
> > Fixes: 3ac873c76d79 ("ata: libata-core: fix when to fetch sense data for successful commands")
> > Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> > ---
> > drivers/ata/libata-core.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 74314311295f..2f7f72994cd7 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -4784,10 +4784,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> > * 0xD. For these commands, invoke EH to get the command sense data.
> > */
> > if (qc->result_tf.status & ATA_SENSE &&
> > - ((ata_is_ncq(qc->tf.protocol) &&
> > - dev->flags & ATA_DFLAG_CDL_ENABLED) ||
> > - (!ata_is_ncq(qc->tf.protocol) &&
> > - ata_id_sense_reporting_enabled(dev->id)))) {
> > + dev->flags & ATA_DFLAG_CDL_ENABLED) {
>
> Applied to for-6.6-fixes with a tweak:
>
> if (qc->flags & ATA_QCFLAG_HAS_CDL &&
> qc->result_tf.status & ATA_SENSE) {
>
> is the test I tweaked. This allows ignoring command that do not use CDL. And
> seeing ATA_QCFLAG_HAS_CDL set implies that dev->flags & ATA_DFLAG_CDL_ENABLED is
> true. So this is better I think.
I agree.
Kind regards,
Niklas
