On Sat, Aug 02, 2025 at 12:33:05PM +0900, Damien Le Moal wrote:
> On 8/2/25 05:28, Igor Pylypiv wrote:
> > On Wed, Jul 30, 2025 at 09:24:41AM +0900, Damien Le Moal wrote:
> >> ata_gen_ata_sense() is always called for a failed qc missing sense data
> >> so that a sense key, code and code qualifier can be generated using
> >> ata_to_sense_error() from the qc status and error fields of its result
> >> task file. However, if the qc does not have its result task file filled,
> >> ata_gen_ata_sense() returns early without setting a sense key.
> >>
> >> Improve this by defaulting to returning ABORTED COMMAND without any
> >> additional sense code, since we do not know the reason for the failure.
> >> The same fix is also applied in ata_gen_passthru_sense() with the
> >> additional check that the qc failed (qc->err_mask is set).
> >>
> >> Fixes: 816be86c7993 ("ata: libata-scsi: Check ATA_QCFLAG_RTF_FILLED before using result_tf")
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> Signed-off-by: Damien Le Moal <dlemoal@xxxxxxxxxx>
> >> ---
> >> drivers/ata/libata-scsi.c | 18 +++++++++++-------
> >> 1 file changed, 11 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> >> index 9b16c0f553e0..57f674f51b0c 100644
> >> --- a/drivers/ata/libata-scsi.c
> >> +++ b/drivers/ata/libata-scsi.c
> >> @@ -938,6 +938,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc)
> >> if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> >> ata_dev_dbg(dev,
> >> "missing result TF: can't generate ATA PT sense data\n");
> >> + if (qc->err_mask)
> >> + ata_scsi_set_sense(dev, cmd, ABORTED_COMMAND, 0, 0);
> >> return;
> >> }
> >>
> >> @@ -992,8 +994,8 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >>
> >> if (!(qc->flags & ATA_QCFLAG_RTF_FILLED)) {
> >> ata_dev_dbg(dev,
> >> - "missing result TF: can't generate sense data\n");
> >> - return;
> >> + "Missing result TF: reporting aborted command\n");
> >> + goto aborted;
> >> }
> >>
> >> /* Use ata_to_sense_error() to map status register bits
> >> @@ -1004,13 +1006,15 @@ static void ata_gen_ata_sense(struct ata_queued_cmd *qc)
> >
> > There is a redundant check in ata_gen_ata_sense(). qc->err_mask (is_error) is
> > already checked in ata_scsi_qc_complete() before it calls ata_gen_ata_sense().
> >
> > if (qc->err_mask ||
> > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
> >
> > The function will be much cleaner once we remove this check.
>
> Yep, we can remove the err_mask check.
>
To clarify, I mean that both conditions can be removed, not just the err_mask
check. In the current code the err_mask check always evaluates to true so
the right part of the OR expression is skipped due to lazy evaluation.
Thanks,
Igor
>
> --
> Damien Le Moal
> Western Digital Research
