| Message ID | 20240614191835.3056153-4-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ATA PASS-THROUGH sense data fixes | expand |
On 6/15/24 04:18, Igor Pylypiv wrote: > Do not generate sense data from ATA status/error registers > if valid sense data is already present. This kind of contradicts what you said in patch 2... So I am really confused now. Though this patch actually looks good to me, modulo the comment below. But shouldn't this be squashed with patch 2 ? > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 79e8103ef3a9..4bfe47e7d266 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > unsigned char *desc = sb + 8; > u8 sense_key, asc, ascq; > > - /* > - * Use ata_to_sense_error() to map status register bits > - * onto sense key, asc & ascq. > - */ > - if (qc->err_mask || > - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { > + /* > + * Do not generate sense data from ATA status/error > + * registers if valid sense data is already present. > + */ The empty "if" here is really horrible. Please revert the condition and add it as a "&&" in the below if. > + } else if (qc->err_mask || > + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > + /* > + * Use ata_to_sense_error() to map status register bits > + * onto sense key, asc & ascq. > + */ > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > &sense_key, &asc, &ascq); > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote: > Do not generate sense data from ATA status/error registers > if valid sense data is already present. > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/ata/libata-scsi.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index 79e8103ef3a9..4bfe47e7d266 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > unsigned char *desc = sb + 8; > u8 sense_key, asc, ascq; Like I suggested in the earlier patch, can't you do a: if (qc->flags & ATA_QCFLAG_SENSE_VALID) return; here instead? > > - /* > - * Use ata_to_sense_error() to map status register bits > - * onto sense key, asc & ascq. > - */ > - if (qc->err_mask || > - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { > + /* > + * Do not generate sense data from ATA status/error > + * registers if valid sense data is already present. > + */ > + } else if (qc->err_mask || > + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > + /* > + * Use ata_to_sense_error() to map status register bits > + * onto sense key, asc & ascq. > + */ > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > &sense_key, &asc, &ascq); > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > -- > 2.45.2.627.g7a2c4fd464-goog >
On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote: > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote: > > Do not generate sense data from ATA status/error registers > > if valid sense data is already present. > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-scsi.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index 79e8103ef3a9..4bfe47e7d266 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > unsigned char *desc = sb + 8; > > u8 sense_key, asc, ascq; > > Like I suggested in the earlier patch, > > can't you do a: > > if (qc->flags & ATA_QCFLAG_SENSE_VALID) > return; > > here instead? > We need to populate the "ATA Status Return sense data descriptor" as per SAT-5 "Table 209 — ATA command results". By returning early we are skipping the code that copies ATA output fields into descriptor/fixed sense data buffer. > > > > > - /* > > - * Use ata_to_sense_error() to map status register bits > > - * onto sense key, asc & ascq. > > - */ > > - if (qc->err_mask || > > - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { > > + /* > > + * Do not generate sense data from ATA status/error > > + * registers if valid sense data is already present. > > + */ > > + } else if (qc->err_mask || > > + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > + /* > > + * Use ata_to_sense_error() to map status register bits > > + * onto sense key, asc & ascq. > > + */ > > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > > &sense_key, &asc, &ascq); > > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > > -- > > 2.45.2.627.g7a2c4fd464-goog > >
On Mon, Jun 17, 2024 at 08:25:54AM +0900, Damien Le Moal wrote: > On 6/15/24 04:18, Igor Pylypiv wrote: > > Do not generate sense data from ATA status/error registers > > if valid sense data is already present. > > This kind of contradicts what you said in patch 2... So I am really confused now. Sorry about the confustion. I think the problem is that I was using "sense data" to describe two different things: #1. SK/ASC/ASCQ #2. ATA Status Return sense data descriptor Both #1 and #2 need to be populated into sense buffer. The problem with the current code is that we can only have either valid #1 or valid #2 but not both at the same time. > Though this patch actually looks good to me, modulo the comment below. > But shouldn't this be squashed with patch 2 ? Yes, that's a good point. Let me factor out the sense data descriptor population code into a separate function and then squash this patch with the patch 2. > > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/ata/libata-scsi.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > index 79e8103ef3a9..4bfe47e7d266 100644 > > --- a/drivers/ata/libata-scsi.c > > +++ b/drivers/ata/libata-scsi.c > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > unsigned char *desc = sb + 8; > > u8 sense_key, asc, ascq; > > > > - /* > > - * Use ata_to_sense_error() to map status register bits > > - * onto sense key, asc & ascq. > > - */ > > - if (qc->err_mask || > > - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { > > + /* > > + * Do not generate sense data from ATA status/error > > + * registers if valid sense data is already present. > > + */ > > The empty "if" here is really horrible. Please revert the condition and add it > as a "&&" in the below if. > Adding the condition to the below if will change the code flow and we'll end up executing scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D) when ATA_QCFLAG_SENSE_VALID is set, which is not what we want. I agree about horrible :) Perhaps I should have factored out the descriptor population code into a separate function to make the code correct and not so horrible. Let me do that in v2. > > + } else if (qc->err_mask || > > + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > + /* > > + * Use ata_to_sense_error() to map status register bits > > + * onto sense key, asc & ascq. > > + */ > > ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, > > &sense_key, &asc, &ascq); > > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > > -- > Damien Le Moal > Western Digital Research > Thank you, Igor
On 6/18/24 09:02, Igor Pylypiv wrote: > On Mon, Jun 17, 2024 at 08:25:54AM +0900, Damien Le Moal wrote: >> On 6/15/24 04:18, Igor Pylypiv wrote: >>> Do not generate sense data from ATA status/error registers >>> if valid sense data is already present. >> >> This kind of contradicts what you said in patch 2... So I am really confused now. > > Sorry about the confustion. I think the problem is that I was using "sense data" > to describe two different things: > #1. SK/ASC/ASCQ > #2. ATA Status Return sense data descriptor > > Both #1 and #2 need to be populated into sense buffer. The problem with > the current code is that we can only have either valid #1 or valid #2 but > not both at the same time. > >> Though this patch actually looks good to me, modulo the comment below. >> But shouldn't this be squashed with patch 2 ? > > Yes, that's a good point. Let me factor out the sense data descriptor > population code into a separate function and then squash this patch with > the patch 2. > >> >>> >>> Signed-off-by: Igor Pylypiv <ipylypiv@google.com> >>> --- >>> drivers/ata/libata-scsi.c | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c >>> index 79e8103ef3a9..4bfe47e7d266 100644 >>> --- a/drivers/ata/libata-scsi.c >>> +++ b/drivers/ata/libata-scsi.c >>> @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) >>> unsigned char *desc = sb + 8; >>> u8 sense_key, asc, ascq; >>> >>> - /* >>> - * Use ata_to_sense_error() to map status register bits >>> - * onto sense key, asc & ascq. >>> - */ >>> - if (qc->err_mask || >>> - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { >>> + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { >>> + /* >>> + * Do not generate sense data from ATA status/error >>> + * registers if valid sense data is already present. >>> + */ >> >> The empty "if" here is really horrible. Please revert the condition and add it >> as a "&&" in the below if. >> > Adding the condition to the below if will change the code flow and we'll end > up executing scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D) when > ATA_QCFLAG_SENSE_VALID is set, which is not what we want. I did say "reverse the condition" :) So that if would be done only if ATA_QCFLAG_SENSE_VALID is *not* set. > > I agree about horrible :) > > Perhaps I should have factored out the descriptor population code into > a separate function to make the code correct and not so horrible. Let me > do that in v2. > >>> + } else if (qc->err_mask || >>> + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { >>> + /* >>> + * Use ata_to_sense_error() to map status register bits >>> + * onto sense key, asc & ascq. >>> + */ >>> ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, >>> &sense_key, &asc, &ascq); >>> ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); >> >> -- >> Damien Le Moal >> Western Digital Research >> > Thank you, > Igor >
On Mon, Jun 17, 2024 at 11:31:49PM +0000, Igor Pylypiv wrote: > On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote: > > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote: > > > Do not generate sense data from ATA status/error registers > > > if valid sense data is already present. > > > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > > --- > > > drivers/ata/libata-scsi.c | 17 +++++++++++------ > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > > index 79e8103ef3a9..4bfe47e7d266 100644 > > > --- a/drivers/ata/libata-scsi.c > > > +++ b/drivers/ata/libata-scsi.c > > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > > unsigned char *desc = sb + 8; > > > u8 sense_key, asc, ascq; > > > > Like I suggested in the earlier patch, > > > > can't you do a: > > > > if (qc->flags & ATA_QCFLAG_SENSE_VALID) > > return; > > > > here instead? > > > > We need to populate the "ATA Status Return sense data descriptor" as per SAT-5 > "Table 209 — ATA command results". By returning early we are skipping the code > that copies ATA output fields into descriptor/fixed sense data buffer. We might get sense data from the drive. If we use REQUEST SENSE DATA EXT, we will get SK/ASC/ASCQ, we will then call scsi_build_sense_buffer() which will generate sense data in either the descriptor format or the fixed format, based on the D_SENSE bit, which is set in the control mode page. The user can toggle this bit, see: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L3691-L3694 But by default it is 0: https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L86 which means fixed format. This all seems to be in accordance with "Table 209 — Returned sense data with the CK_COND bit set to one" in sat6r1. When calling scsi_build_sense_buffer(), we supply: scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE, cmd->sense_buffer, tf.lbah, tf.lbam, tf.lbal); so we do not supply the STATUS and ERROR fields when building the sense data. This seems fine, since SCSI has no knowledge of ATA status or ATA error. However, for ATA-passthrough commands, ATA status and ATA error fields are part of either the COMMAND-SPECIFIC information in the fixed format case, or part for the descriptor format, in case of the descriptor type being ATA Status Return sense data descriptor. So what I think we should do: 1) Keep the sense data if it exists, and fill in the ATA status and ATA error at the correct offset (depending on if the existing sense data is in fixed format or descriptor format). 2) If there doesn't exist any sense data, generate it, including the ATA passthru command specific fields (ATA status and ATA error). This is basically what ata_gen_passthru_sense() does today. So something like this: diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index bb4d30d377ae..a0687eb28ff7 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE */ if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && - ((cdb[2] & 0x20) || need_sense)) - ata_gen_passthru_sense(qc); - else if (need_sense) + ((cdb[2] & 0x20) || need_sense)) { + if ((qc->flags & ATA_QCFLAG_SENSE_VALID)) { + /* + * ATA PASS-THROUGH commands also need to fill in the + * command specific ATA status and ATA error fields. + */ + ata_fill_passthru_specific_sense_fields(qc); + } else { + ata_gen_passthru_sense(qc); + } + } else if (need_sense) ata_gen_ata_sense(qc); else /* Keep the SCSI ML and status byte, clear host byte. */ Kind regards, Niklas
On Thu, Jun 20, 2024 at 04:24:11PM +0200, Niklas Cassel wrote: > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index bb4d30d377ae..a0687eb28ff7 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > */ > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > - ((cdb[2] & 0x20) || need_sense)) Hmm... I just noticed that we don't seem to have support for ATA_32 commands. We should probably add support for that sometime in the future. (I don't think that it makes sense to do it as part of this series.) Kind regards, Niklas
On Thu, Jun 20, 2024 at 04:24:11PM +0200, Niklas Cassel wrote: > On Mon, Jun 17, 2024 at 11:31:49PM +0000, Igor Pylypiv wrote: > > On Mon, Jun 17, 2024 at 12:49:47PM +0200, Niklas Cassel wrote: > > > On Fri, Jun 14, 2024 at 07:18:34PM +0000, Igor Pylypiv wrote: > > > > Do not generate sense data from ATA status/error registers > > > > if valid sense data is already present. > > > > > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > > > --- > > > > drivers/ata/libata-scsi.c | 17 +++++++++++------ > > > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > > > > index 79e8103ef3a9..4bfe47e7d266 100644 > > > > --- a/drivers/ata/libata-scsi.c > > > > +++ b/drivers/ata/libata-scsi.c > > > > @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > > > > unsigned char *desc = sb + 8; > > > > u8 sense_key, asc, ascq; > > > > > > Like I suggested in the earlier patch, > > > > > > can't you do a: > > > > > > if (qc->flags & ATA_QCFLAG_SENSE_VALID) > > > return; > > > > > > here instead? > > > > > > > We need to populate the "ATA Status Return sense data descriptor" as per SAT-5 > > "Table 209 — ATA command results". By returning early we are skipping the code > > that copies ATA output fields into descriptor/fixed sense data buffer. > > We might get sense data from the drive. > If we use REQUEST SENSE DATA EXT, we will get SK/ASC/ASCQ, > we will then call scsi_build_sense_buffer() which will generate > sense data in either the descriptor format or the fixed format, > based on the D_SENSE bit, which is set in the control mode page. > > The user can toggle this bit, see: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L3691-L3694 > > But by default it is 0: > https://github.com/torvalds/linux/blob/v6.10-rc4/drivers/ata/libata-scsi.c#L86 > which means fixed format. > > This all seems to be in accordance with > "Table 209 — Returned sense data with the CK_COND bit set to one" > in sat6r1. > > > > When calling scsi_build_sense_buffer(), we supply: > scsi_build_sense_buffer(dev->flags & ATA_DFLAG_D_SENSE, > cmd->sense_buffer, tf.lbah, > tf.lbam, tf.lbal); > > so we do not supply the STATUS and ERROR fields when building the sense data. > > This seems fine, since SCSI has no knowledge of ATA status or ATA error. > > > However, for ATA-passthrough commands, ATA status and ATA error fields > are part of either the COMMAND-SPECIFIC information in the fixed format > case, or part for the descriptor format, in case of the descriptor type > being ATA Status Return sense data descriptor. > > > So what I think we should do: > 1) Keep the sense data if it exists, and fill in > the ATA status and ATA error at the correct offset (depending on if > the existing sense data is in fixed format or descriptor format). > 2) If there doesn't exist any sense data, generate it, including the > ATA passthru command specific fields (ATA status and ATA error). > This is basically what ata_gen_passthru_sense() does today. > Yes! That's exactly what I was trying to do with the horrible empty if in ata_gen_passthru_sense(). Factoring out ATA status and ATA error population into a separate function makes the code more readable and less error-prone. I'll make the change in v2. Thank you! Igor > > So something like this: > diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c > index bb4d30d377ae..a0687eb28ff7 100644 > --- a/drivers/ata/libata-scsi.c > +++ b/drivers/ata/libata-scsi.c > @@ -1645,9 +1645,17 @@ static void ata_scsi_qc_complete(struct ata_queued_cmd *qc) > * asc,ascq = ATA PASS-THROUGH INFORMATION AVAILABLE > */ > if (((cdb[0] == ATA_16) || (cdb[0] == ATA_12)) && > - ((cdb[2] & 0x20) || need_sense)) > - ata_gen_passthru_sense(qc); > - else if (need_sense) > + ((cdb[2] & 0x20) || need_sense)) { > + if ((qc->flags & ATA_QCFLAG_SENSE_VALID)) { > + /* > + * ATA PASS-THROUGH commands also need to fill in the > + * command specific ATA status and ATA error fields. > + */ > + ata_fill_passthru_specific_sense_fields(qc); > + } else { > + ata_gen_passthru_sense(qc); > + } > + } else if (need_sense) > ata_gen_ata_sense(qc); > else > /* Keep the SCSI ML and status byte, clear host byte. */ > > > Kind regards, > Niklas > > > >
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index 79e8103ef3a9..4bfe47e7d266 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -858,12 +858,17 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) unsigned char *desc = sb + 8; u8 sense_key, asc, ascq; - /* - * Use ata_to_sense_error() to map status register bits - * onto sense key, asc & ascq. - */ - if (qc->err_mask || - tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { + if (qc->flags & ATA_QCFLAG_SENSE_VALID) { + /* + * Do not generate sense data from ATA status/error + * registers if valid sense data is already present. + */ + } else if (qc->err_mask || + tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { + /* + * Use ata_to_sense_error() to map status register bits + * onto sense key, asc & ascq. + */ ata_to_sense_error(qc->ap->print_id, tf->status, tf->error, &sense_key, &asc, &ascq); ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq);
Do not generate sense data from ATA status/error registers if valid sense data is already present. Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/ata/libata-scsi.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)