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