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.
> /*
> * Tell SCSI EH to not overwrite scmd->result even if this
> * command is finished with result SAM_STAT_GOOD.
--
Damien Le Moal
Western Digital Research
