Re: [PATCH 2/2] ata: libata-scsi: Return aborted command when missing sense and result TF

[Igor Pylypiv <ipylypiv@google.com>]

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@vger.kernel.org
> >> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >> ---
> >>  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