Re: [PATCH v3 2/6] ata: libata-scsi: Do not overwrite valid sense data when CK_COND=1

[Niklas Cassel <cassel@kernel.org>]

On Fri, Jun 28, 2024 at 11:31:35PM +0000, Igor Pylypiv wrote:
> On Fri, Jun 28, 2024 at 08:44:41PM +0200, Niklas Cassel wrote:
> > On Thu, Jun 27, 2024 at 09:54:06PM +0000, Igor Pylypiv wrote:
> > > 
> > > Thank you, Niklas! I agree that this code is too complicated and should be
> > > simplified. I don't think we should change the code too much in this patch
> > > since it is going to be backported to stable releases.
> > > 
> > > Would you mind sending a patch for the proposed simplifications following
> > > this patch series?
> > > 
> > 
> > I would prefer if we changed it as part of this commit to be honest.
> > 
> > 
> > I also re-read the SAT spec, and found that it says that:
> > """
> > If the CK_COND bit is set to:
> > a) one, then the SATL shall return a status of CHECK CONDITION upon ATA command completion,
> > without interpreting the contents of the STATUS field and returning the ATA fields from the request
> > completion in the sense data as specified in table 209; and
> > b) zero, then the SATL shall terminate the command with CHECK CONDITION status only if an error
> > occurs in processing the command. See clause 11 for a description of ATA error conditions.
> > """
> > 
> > So it seems quite clear that if CK_COND == 1, we should set CHECK CONDITION,
> > so that answers the question/uncertainty I asked/expressed in earlier emails.
> > 
> > 
> > I think this patch (which should be applied on top of your v3 series),
> > makes the code way easier to read/understand:
> > 
> 
> Agree, having self-explanatory variable names makes the code much more
> readable. I'll add the patch in v4.
> 
> Do you mind if I set you as the author of the patch with the corresponding
> Signed-off-by tag?

I still think that you are the author.

But if you want, feel free to add me as: Co-developed-by
(which would also require you to add my Signed-off-by), see:
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by


> 
> > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> > index d5874d4b9253..5b211551ac10 100644
> > --- a/drivers/ata/libata-scsi.c
> > +++ b/drivers/ata/libata-scsi.c
> > @@ -1659,26 +1656,27 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc)
> >  {
> >         struct scsi_cmnd *cmd = qc->scsicmd;
> >         u8 *cdb = cmd->cmnd;
> > -       int need_sense = (qc->err_mask != 0) &&
> > -               !(qc->flags & ATA_QCFLAG_SENSE_VALID);
> > -       int need_passthru_sense = (qc->err_mask != 0) ||
> > -               (qc->flags & ATA_QCFLAG_SENSE_VALID);
> > +       bool have_sense = qc->flags & ATA_QCFLAG_SENSE_VALID;
> > +       bool is_ata_passthru = cdb[0] == ATA_16 || cdb[0] == ATA_12;
> > +       bool is_ck_cond_request = cdb[2] & 0x20;
> > +       bool is_error = qc->err_mask != 0;
> >  
> >         /* For ATA pass thru (SAT) commands, generate a sense block if
> >          * user mandated it or if there's an error.  Note that if we
> > -        * generate because the user forced us to [CK_COND =1], a check
> > +        * generate because the user forced us to [CK_COND=1], a check
> >          * condition is generated and the ATA register values are returned
> >          * whether the command completed successfully or not. If there
> > -        * was no error, we use the following sense data:
> > +        * was no error, and CK_COND=1, we use the following sense data:
> >          * sk = RECOVERED ERROR
> >          * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE
> >          */
> > -       if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) &&
> > -           ((cdb[2] & 0x20) || need_passthru_sense)) {
> > -               if (!(qc->flags & ATA_QCFLAG_SENSE_VALID))
> > +       if (is_ata_passthru && (is_ck_cond_request || is_error || have_sense)) {
> > +               if (!have_sense)
> >                         ata_gen_passthru_sense(qc);
> >                 ata_scsi_set_passthru_sense_fields(qc);
> > -       } else if (need_sense) {
> > +               if (is_ck_cond_request)
> > +                       set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);
> 
> SAM_STAT_CHECK_CONDITION will be set by ata_gen_passthru_sense(). Perhaps we
> can move the SAM_STAT_CHECK_CONDITION setting into else if?

I think it is fine that:
if (is_ck_cond_request)
	set_status_byte(qc->scsicmd, SAM_STAT_CHECK_CONDITION);

might set SAM_STAT_CHECK_CONDITION even if it is already set.

Personally, I think that my suggestion is slightly clearer when it comes
to highlight the behavior of CK_COND. (CK_COND will set CHECK_CONDITION,
regardless if successful command or error command, and regardless if
we already had sense or not.)

And considering that we finally make this hard to read code slightly more
readable than it was to start off with, I would prefer my alternative.


Kind regards,
Niklas