On 7/23/23 22:03, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@xxxxxxx>
>
> The condition to fetch sense data was supposed to be:
> ATA_SENSE set AND 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.
>
> However the check in 2) accidentally had the negation at the wrong place,
> causing it to try to fetch sense data if it was a non-NCQ command _or_
> if regular sense data reporting was _not_ enabled.
>
> Fix this by removing the extra parentheses that should not be there,
> such that only the correct return (ata_is_ncq()) is negated.
>
> Fixes: 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL commands using policy 0xD")
> Reported-by: Borislav Petkov <bp@xxxxxxxxx>
> Closes: https://lore.kernel.org/linux-ide/20230722155621.GIZLv8JbURKzHtKvQE@fat_crate.local/
> Signed-off-by: Niklas Cassel <niklas.cassel@xxxxxxx>
> ---
> drivers/ata/libata-core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d37ab6087f2f..04db0f2c683a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4938,8 +4938,8 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> 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))))) {
> + (!ata_is_ncq(qc->tf.protocol) &&
Even though it is not necessary, to be extra clear, it would be nice to have
parenthesis around "!ata_is_ncq(qc->tf.protocol)". I will add that when applying.
> + ata_id_sense_reporting_enabled(dev->id)))) {
> /*
> * 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
