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
