diff mbox series

[v1,3/4] ata: libata-scsi: Report valid sense data for ATA PT if present

Message ID 20240614191835.3056153-4-ipylypiv@google.com
State New
Headers show
Series ATA PASS-THROUGH sense data fixes | expand

Commit Message

Igor Pylypiv June 14, 2024, 7:18 p.m. UTC
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(-)

Comments

Damien Le Moal June 16, 2024, 11:25 p.m. UTC | #1
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);
Niklas Cassel June 17, 2024, 10:49 a.m. UTC | #2
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
>
Igor Pylypiv June 17, 2024, 11:31 p.m. UTC | #3
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
> >
Igor Pylypiv June 18, 2024, 12:02 a.m. UTC | #4
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
Damien Le Moal June 18, 2024, 2:20 a.m. UTC | #5
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 
>
Niklas Cassel June 20, 2024, 2:24 p.m. UTC | #6
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
Niklas Cassel June 20, 2024, 2:39 p.m. UTC | #7
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
Igor Pylypiv June 20, 2024, 11:34 p.m. UTC | #8
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 mbox series

Patch

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);