Re: [PATCH] ata: libata-eh: Clear scsicmd->result when setting SAM_STAT_CHECK_CONDITION

[Niklas Cassel <cassel@kernel.org>]

On Thu, Sep 05, 2024 at 11:25:55AM +0200, Niklas Cassel wrote:
> On Thu, Sep 05, 2024 at 10:33:38AM +0200, Niklas Cassel wrote:
> > There are many different paths a QC can take via EH, e.g. policy 0xD NCQ
> > commands will not fetch sense data via ata_eh_request_sense(), so clearing
> > the host byte in ata_scsi_qc_complete() should be fine, otherwise we need
> > to do a similar change to yours in all the different code paths that sets
> > sense data ...which might actually be something that makes sense, but then
> > I would expect a patch series that changes all the locations where we set
> > sense data, not just in ata_eh_analyze_tf(), and then drops the clearing in
> > ata_scsi_qc_complete() (which was introduced in commit 7574a8377c7a ("ata:
> > libata-scsi: do not overwrite SCSI ML and status bytes")).
> 
> I tried to implement the suggestion above, it looks like this:
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index e4023fc288ac..ff4945a8f647 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4824,6 +4824,14 @@ void ata_qc_free(struct ata_queued_cmd *qc)
>  		qc->tag = ATA_TAG_POISON;
>  }
>  
> +void ata_qc_set_sense_valid_flag(struct ata_queued_cmd *qc)
> +{
> +	qc->flags |= ATA_QCFLAG_SENSE_VALID;
> +
> +	/* Keep the SCSI ML and status byte, clear host byte. */
> +	qc->scsicmd->result &= 0x0000ffff;

This would have to be:
	/* Keep the SCSI ML and status byte, clear host byte. */
	if (qc->scsicmd)
		qc->scsicmd->result &= 0x0000ffff;

Since for ata_qc_complete_internal(), qc->scsicmd will be NULL.


> +}
> +

(snip)

> I guess the obvious advantage that I can see is that we would do the
> right thing regardless of qc->complete_fn.
> 
> qc->complete_fn can be any of:
> ata_qc_complete_internal()
> ata_scsi_qc_complete()
> atapi_qc_complete()
> ata_scsi_report_zones_complete()
> 
> Instead of only doing the right thing if:
> qc->complete_fn == ata_scsi_qc_complete().
> 
> Thoughts?

ata_scsi_report_zones_complete() calls ata_scsi_qc_complete().
And like I said above, qc->scsicmd is NULL for ata_qc_complete_internal(),
so I guess the one qc->complete_fn that is currently not doing the right
thing is atapi_qc_complete().

However, looking at atapi_qc_complete(), it actually does:

	if (unlikely(err_mask || qc->flags & ATA_QCFLAG_SENSE_VALID)) {
		...
		qc->scsicmd->result = SAM_STAT_CHECK_CONDITION;
		ata_qc_done(qc);
		return;
	}

	...
	cmd->result = SAM_STAT_GOOD;
	ata_qc_done(qc);

So atapi_qc_complete() will always overwrite the host byte.

So, AFAICT, there is no problematic qc->complete_fn function in the existing
libata code, so I don't think there is any urgency to change the code.

Anyway, I think I came up with an even nicer patch to clear the driver byte.

Change ata_scsi_set_sense():
-to set SENSE_DATA_VALID
-to clear driver byte (if scsicmd)

For the cases that calls:
-scsi_build_sense_buffer()
(because they don't want to set the SAM_STAT_CHECK_CONDITION)
or
-scsi_build_sense()
without using ata_scsi_set_sense():
create a new function/functions (e.g. ata_build_sense_keep_status())
-sets SENSE_DATA_VALID
-clears driver byte (if scsicmd)

Will send a PATCH/RFC series today or tomorrow.


Kind regards,
Niklas