Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled

[Damien Le Moal <dlemoal@kernel.org>]

On 9/14/23 00:04, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
> 
> 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@wdc.com>
> ---
>  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