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) {

Agree. That is safer. ATA has the device statistics feature set, which if
enabled, can also generates sense data for successful commands. But we do not
support this feature and so would likely be doing the wrong things with the
sense data.

Note that I would also add "qc->flags & ATA_QCFLAG_HAS_CDL &&":

        if (qc->result_tf.status & ATA_SENSE &&
            qc->flags & ATA_QCFLAG_HAS_CDL &&
            dev->flags & ATA_DFLAG_CDL_ENABLED) {

since for the case where the command did not use CDL we can still get the
ATA_SENSE flag set because of a CDL command having been aborted, but we do not
need to do anything for that non-CDL command.


-- 
Damien Le Moal
Western Digital Research