| Message ID | 20240626230411.3471543-2-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | ATA PASS-THROUGH sense data fixes | expand |
Hello Igor, Hannes,
The changes in this patch looks good, however, there is still one thing that
bothers me:
https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877
Specifically the code in the else statement below:
if (qc->err_mask ||
tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) {
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);
} else {
/*
* ATA PASS-THROUGH INFORMATION AVAILABLE
* Always in descriptor format sense.
*/
scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
}
Looking at sat6r01, I see that this is table:
Table 217 — ATA command results
And this text:
No error, successful completion or command in progress. The SATL
shall terminate the command with CHECK CONDITION status with
the sense key set to RECOVERED ERROR with the additional
sense code set to ATA PASS-THROUGH INFORMATION
AVAILABLE (see SPC-5). Descriptor format sense data shall include
the ATA Status Return sense data descriptor (see 12.2.2.7).
However, I don't see anything in this text that says that the
sense key should always be in descriptor format sense.
In fact, what will happen if the user has not set the D_SENSE bit
(libata will default not set it), is that:
The else statement above will be executed, filling in sense key in
descriptor format, after this if/else, we will continue checking
if the sense buffer is in descriptor format, or fixed format.
Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D);
is called with (..., 1, ..., ..., ...) it will always generate
the sense data in descriptor format, regardless of
dev->flags ATA_DFLAG_D_SENSE being set or not.
Should perhaps the code in the else statement be:
} else {
ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D);
}
So that we actually respect the D_SENSE bit?
(We currently respect if when filling the sense data buffer with
sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't
respect it for successful ATA PASS-THROUGH commands.)
Kind regards,
Niklas
On Thu, Jun 27, 2024 at 02:08:50PM +0200, Niklas Cassel wrote: > Hello Igor, Hannes, > > The changes in this patch looks good, however, there is still one thing that > bothers me: > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877 > > Specifically the code in the else statement below: > > if (qc->err_mask || > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > 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); > } else { > /* > * ATA PASS-THROUGH INFORMATION AVAILABLE > * Always in descriptor format sense. > */ > scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > } > > Looking at sat6r01, I see that this is table: > Table 217 — ATA command results > > And this text: > No error, successful completion or command in progress. The SATL > shall terminate the command with CHECK CONDITION status with > the sense key set to RECOVERED ERROR with the additional > sense code set to ATA PASS-THROUGH INFORMATION > AVAILABLE (see SPC-5). Descriptor format sense data shall include > the ATA Status Return sense data descriptor (see 12.2.2.7). > > However, I don't see anything in this text that says that the > sense key should always be in descriptor format sense. > > In fact, what will happen if the user has not set the D_SENSE bit > (libata will default not set it), is that: > > The else statement above will be executed, filling in sense key in > descriptor format, after this if/else, we will continue checking > if the sense buffer is in descriptor format, or fixed format. > > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > is called with (..., 1, ..., ..., ...) it will always generate > the sense data in descriptor format, regardless of > dev->flags ATA_DFLAG_D_SENSE being set or not. > > Should perhaps the code in the else statement be: > > } else { > ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D); > } > > So that we actually respect the D_SENSE bit? > > (We currently respect if when filling the sense data buffer with > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't > respect it for successful ATA PASS-THROUGH commands.) > Thanks for pointing this out, Niklas! I agree, it seems like there is no reason to ignore the D_SENSE bit. Interestingly, the code was using ata_scsi_set_sense() before. Commit 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense)" changed it to always be in the descriptor format. > > Kind regards, > Niklas Thanks, Igor
On 6/27/24 14:08, Niklas Cassel wrote: > Hello Igor, Hannes, > > The changes in this patch looks good, however, there is still one thing that > bothers me: > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877 > > Specifically the code in the else statement below: > > if (qc->err_mask || > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > 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); > } else { > /* > * ATA PASS-THROUGH INFORMATION AVAILABLE > * Always in descriptor format sense. > */ > scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > } > > Looking at sat6r01, I see that this is table: > Table 217 — ATA command results > > And this text: > No error, successful completion or command in progress. The SATL > shall terminate the command with CHECK CONDITION status with > the sense key set to RECOVERED ERROR with the additional > sense code set to ATA PASS-THROUGH INFORMATION > AVAILABLE (see SPC-5). Descriptor format sense data shall include > the ATA Status Return sense data descriptor (see 12.2.2.7). > > However, I don't see anything in this text that says that the > sense key should always be in descriptor format sense. > > In fact, what will happen if the user has not set the D_SENSE bit > (libata will default not set it), is that: > > The else statement above will be executed, filling in sense key in > descriptor format, after this if/else, we will continue checking > if the sense buffer is in descriptor format, or fixed format. > > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > is called with (..., 1, ..., ..., ...) it will always generate > the sense data in descriptor format, regardless of > dev->flags ATA_DFLAG_D_SENSE being set or not. > > Should perhaps the code in the else statement be: > > } else { > ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D); > } > > So that we actually respect the D_SENSE bit? > > (We currently respect if when filling the sense data buffer with > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't > respect it for successful ATA PASS-THROUGH commands.) > I guess that we've misread the spec. The sentence: "Descriptor format sense data shall include the ATA Status Return Descriptor" should be interpreted as: _If_ the sense code is formatted in descriptor format _then_ the ATA Status Return Descriptor should be included. IE if the sense code is not in descriptor format then the ATA Status Return Descriptor shouldn't be included (kinda obvious, really). But of course the D_SENSE bit should be honoured. Cheers, Hannes
On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote: > On 6/27/24 14:08, Niklas Cassel wrote: > > Hello Igor, Hannes, > > > > The changes in this patch looks good, however, there is still one thing that > > bothers me: > > https://github.com/torvalds/linux/blob/v6.10-rc5/drivers/ata/libata-scsi.c#L873-L877 > > > > Specifically the code in the else statement below: > > > > if (qc->err_mask || > > tf->status & (ATA_BUSY | ATA_DF | ATA_ERR | ATA_DRQ)) { > > 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); > > } else { > > /* > > * ATA PASS-THROUGH INFORMATION AVAILABLE > > * Always in descriptor format sense. > > */ > > scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > > } > > > > Looking at sat6r01, I see that this is table: > > Table 217 — ATA command results > > > > And this text: > > No error, successful completion or command in progress. The SATL > > shall terminate the command with CHECK CONDITION status with > > the sense key set to RECOVERED ERROR with the additional > > sense code set to ATA PASS-THROUGH INFORMATION > > AVAILABLE (see SPC-5). Descriptor format sense data shall include > > the ATA Status Return sense data descriptor (see 12.2.2.7). > > > > However, I don't see anything in this text that says that the > > sense key should always be in descriptor format sense. > > > > In fact, what will happen if the user has not set the D_SENSE bit > > (libata will default not set it), is that: > > > > The else statement above will be executed, filling in sense key in > > descriptor format, after this if/else, we will continue checking > > if the sense buffer is in descriptor format, or fixed format. > > > > Since the scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > > is called with (..., 1, ..., ..., ...) it will always generate > > the sense data in descriptor format, regardless of > > dev->flags ATA_DFLAG_D_SENSE being set or not. > > > > Should perhaps the code in the else statement be: > > > > } else { > > ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D); > > } > > > > So that we actually respect the D_SENSE bit? > > > > (We currently respect if when filling the sense data buffer with > > sense data from REQUEST SENSE EXT, so I'm not sure why we shouldn't > > respect it for successful ATA PASS-THROUGH commands.) > > > I guess that we've misread the spec. I think I might have an idea where you got this from: In sat5r06.pdf """ 12.2.2.8 Fixed format sense data Table 212 shows the fields returned in the fixed format sense data (see SPC-5) for ATA PASS-THROUGH commands. SATLs compliant with ANSI INCITS 431-2007, SCSI/ATA Translation (SAT) return descriptor format sense data for the ATA PASS-THROUGH commands regardless of the setting of the D_SENSE bit. """ In sat6r01.pdf: """ 12.2.2.8 Fixed format sense data Table 219 shows the fields returned in the fixed format sense data (see SPC-5) for ATA PASS-THROUGH commands. """ In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should ignore D_SENSE bit and unconditionally return sense data in descriptor format. Anyway, considering that: 1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS 431-2007. 2) This text has been removed from SAT-6. 3) We currently honour the D_SENSE bit when creating the sense buffer with the SK/ASC/ASCQ that we get from the device. I think that it makes sense to honour the D_SENSE bit also when generating sense data for successful ATA PASS-THROUGH commands (from ATA registers). Kind regards, Niklas
On Fri, Jun 28, 2024 at 05:49:22PM +0200, Niklas Cassel wrote: > On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote: > > On 6/27/24 14:08, Niklas Cassel wrote: > > In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should > ignore D_SENSE bit and unconditionally return sense data in descriptor format. > > Anyway, considering that: > 1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS > 431-2007. > 2) This text has been removed from SAT-6. > 3) We currently honour the D_SENSE bit when creating the sense buffer with the > SK/ASC/ASCQ that we get from the device. > > I think that it makes sense to honour the D_SENSE bit also when generating > sense data for successful ATA PASS-THROUGH commands (from ATA registers). Igor, I think you should add a new patch in your series that does: 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 @@ -949,11 +949,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) &sense_key, &asc, &ascq); ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); } else { - /* - * ATA PASS-THROUGH INFORMATION AVAILABLE - * Always in descriptor format sense. - */ - scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); + /* ATA PASS-THROUGH INFORMATION AVAILABLE */ + ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D); } } Feel free to copy my arguments above. I also checked VPD page 89h (ATA Information VPD page), and there are no bits there either to claim certain SAT version compliance. And since this text is not in SAT-6, I can only imagine that they decided that is was not a good idea to not always honor D_SENSE... (It does seem simpler to just always honor it...) Kind regards, Niklas
On Fri, Jun 28, 2024 at 08:25:40PM +0200, Niklas Cassel wrote: > On Fri, Jun 28, 2024 at 05:49:22PM +0200, Niklas Cassel wrote: > > On Fri, Jun 28, 2024 at 08:47:03AM +0200, Hannes Reinecke wrote: > > > On 6/27/24 14:08, Niklas Cassel wrote: > > > > In SAT-6 there is no mention of compliance with ANSI INCITS 431-2007 should > > ignore D_SENSE bit and unconditionally return sense data in descriptor format. > > > > Anyway, considering that: > > 1) I'm not sure how a SAT would expose that it is compliant with ANSI INCITS > > 431-2007. > > 2) This text has been removed from SAT-6. > > 3) We currently honour the D_SENSE bit when creating the sense buffer with the > > SK/ASC/ASCQ that we get from the device. > > > > I think that it makes sense to honour the D_SENSE bit also when generating > > sense data for successful ATA PASS-THROUGH commands (from ATA registers). > > Igor, I think you should add a new patch in your series that does: Thanks Niklas, I'll add the patch in v4. > > 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 > @@ -949,11 +949,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) > &sense_key, &asc, &ascq); > ata_scsi_set_sense(qc->dev, cmd, sense_key, asc, ascq); > } else { > - /* > - * ATA PASS-THROUGH INFORMATION AVAILABLE > - * Always in descriptor format sense. > - */ > - scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); > + /* ATA PASS-THROUGH INFORMATION AVAILABLE */ > + ata_scsi_set_sense(qc->dev, cmd, RECOVERED_ERROR, 0, 0x1D); > } > } > > > Feel free to copy my arguments above. > > I also checked VPD page 89h (ATA Information VPD page), and there are > no bits there either to claim certain SAT version compliance. > > And since this text is not in SAT-6, I can only imagine that they decided > that is was not a good idea to not always honor D_SENSE... > > (It does seem simpler to just always honor it...) > > > Kind regards, > Niklas
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c index bb4d30d377ae..a9e44ad4c2de 100644 --- a/drivers/ata/libata-scsi.c +++ b/drivers/ata/libata-scsi.c @@ -855,7 +855,6 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) struct scsi_cmnd *cmd = qc->scsicmd; struct ata_taskfile *tf = &qc->result_tf; unsigned char *sb = cmd->sense_buffer; - unsigned char *desc = sb + 8; u8 sense_key, asc, ascq; memset(sb, 0, SCSI_SENSE_BUFFERSIZE); @@ -877,7 +876,8 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) scsi_build_sense(cmd, 1, RECOVERED_ERROR, 0, 0x1D); } - if ((cmd->sense_buffer[0] & 0x7f) >= 0x72) { + if ((sb[0] & 0x7f) >= 0x72) { + unsigned char *desc; u8 len; /* descriptor format */ @@ -916,21 +916,21 @@ static void ata_gen_passthru_sense(struct ata_queued_cmd *qc) } } else { /* Fixed sense format */ - desc[0] = tf->error; - desc[1] = tf->status; - desc[2] = tf->device; - desc[3] = tf->nsect; - desc[7] = 0; + sb[0] |= 0x80; + sb[3] = tf->error; + sb[4] = tf->status; + sb[5] = tf->device; + sb[6] = tf->nsect; if (tf->flags & ATA_TFLAG_LBA48) { - desc[8] |= 0x80; + sb[8] |= 0x80; if (tf->hob_nsect) - desc[8] |= 0x40; + sb[8] |= 0x40; if (tf->hob_lbal || tf->hob_lbam || tf->hob_lbah) - desc[8] |= 0x20; + sb[8] |= 0x20; } - desc[9] = tf->lbal; - desc[10] = tf->lbam; - desc[11] = tf->lbah; + sb[9] = tf->lbal; + sb[10] = tf->lbam; + sb[11] = tf->lbah; } }
Correct the ATA PASS-THROUGH fixed format sense data offsets to conform to SPC-6 and SAT-5 specifications. Additionally, set the VALID bit to indicate that the INFORMATION field contains valid information. INFORMATION =========== SAT-5 Table 212 — "Fixed format sense data INFORMATION field for the ATA PASS-THROUGH commands" defines the following format: +------+------------+ | Byte | Field | +------+------------+ | 0 | ERROR | | 1 | STATUS | | 2 | DEVICE | | 3 | COUNT(7:0) | +------+------------+ SPC-6 Table 48 - "Fixed format sense data" specifies that the INFORMATION field starts at byte 3 in sense buffer resulting in the following offsets for the ATA PASS-THROUGH commands: +------------+-------------------------+ | Field | Offset in sense buffer | +------------+-------------------------+ | ERROR | 3 | | STATUS | 4 | | DEVICE | 5 | | COUNT(7:0) | 6 | +------------+-------------------------+ COMMAND-SPECIFIC INFORMATION ============================ SAT-5 Table 213 - "Fixed format sense data COMMAND-SPECIFIC INFORMATION field for ATA PASS-THROUGH" defines the following format: +------+-------------------+ | Byte | Field | +------+-------------------+ | 0 | FLAGS | LOG INDEX | | 1 | LBA (7:0) | | 2 | LBA (15:8) | | 3 | LBA (23:16) | +------+-------------------+ SPC-6 Table 48 - "Fixed format sense data" specifies that the COMMAND-SPECIFIC-INFORMATION field starts at byte 8 in sense buffer resulting in the following offsets for the ATA PASS-THROUGH commands: Offsets of these fields in the fixed sense format are as follows: +-------------------+-------------------------+ | Field | Offset in sense buffer | +-------------------+-------------------------+ | FLAGS | LOG INDEX | 8 | | LBA (7:0) | 9 | | LBA (15:8) | 10 | | LBA (23:16) | 11 | +-------------------+-------------------------+ Reported-by: Akshat Jain <akshatzen@google.com> Fixes: 11093cb1ef56 ("libata-scsi: generate correct ATA pass-through sense") Cc: stable@vger.kernel.org Reviewed-by: Hannes Reinecke <hare@suse.de> Signed-off-by: Igor Pylypiv <ipylypiv@google.com> --- drivers/ata/libata-scsi.c | 26 +++++++++++++------------- 1 file changed, 13 insertions(+), 13 deletions(-)