* Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled
2023-09-13 15:04 [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled Niklas Cassel
@ 2023-09-14 0:14 ` Damien Le Moal
2023-09-14 6:01 ` Hannes Reinecke
2023-09-15 6:32 ` Damien Le Moal
2 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2023-09-14 0:14 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Bagas Sanjaya, patenteng, linux-ide, Niklas Cassel
On 9/14/23 00:04, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> Currently, we fetch sense data for a _successful_ command if either:
> 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
> ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
> sense data supported bit is set); or
> 2) Command was non-NCQ and regular sense data reporting is enabled.
>
> This means that case 2) will trigger for a non-NCQ command which has
> ATA_SENSE bit set, regardless if CDL is enabled or not.
>
> This decision was by design. If the device reports that it has sense data
> available, it makes sense to fetch that sense data, since the sk/asc/ascq
> could be important information regardless if CDL is enabled or not.
>
> However, the fetching of sense data for a successful command is done via
> ATA EH. Considering how intricate the ATA EH is, we really do not want to
> invoke ATA EH unless absolutely needed.
>
> Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
> commands using policy 0xD") we never fetched sense data for successful
> non-NCQ commands.
>
> In order to not invoke the ATA EH unless absolutely necessary, even if the
> device claims support for sense data reporting, only fetch sense data for
> successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
>
> Fixes: 3ac873c76d79 ("ata: libata-core: fix when to fetch sense data for successful commands")
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
> drivers/ata/libata-core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 74314311295f..2f7f72994cd7 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4784,10 +4784,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> * 0xD. For these commands, invoke EH to get the command sense data.
> */
> if (qc->result_tf.status & ATA_SENSE &&
> - ((ata_is_ncq(qc->tf.protocol) &&
> - dev->flags & ATA_DFLAG_CDL_ENABLED) ||
> - (!ata_is_ncq(qc->tf.protocol) &&
> - ata_id_sense_reporting_enabled(dev->id)))) {
> + dev->flags & ATA_DFLAG_CDL_ENABLED) {
Agree. That is safer. ATA has the device statistics feature set, which if
enabled, can also generates sense data for successful commands. But we do not
support this feature and so would likely be doing the wrong things with the
sense data.
Note that I would also add "qc->flags & ATA_QCFLAG_HAS_CDL &&":
if (qc->result_tf.status & ATA_SENSE &&
qc->flags & ATA_QCFLAG_HAS_CDL &&
dev->flags & ATA_DFLAG_CDL_ENABLED) {
since for the case where the command did not use CDL we can still get the
ATA_SENSE flag set because of a CDL command having been aborted, but we do not
need to do anything for that non-CDL command.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled
2023-09-13 15:04 [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled Niklas Cassel
2023-09-14 0:14 ` Damien Le Moal
@ 2023-09-14 6:01 ` Hannes Reinecke
2023-09-14 6:07 ` Damien Le Moal
2023-09-15 6:32 ` Damien Le Moal
2 siblings, 1 reply; 6+ messages in thread
From: Hannes Reinecke @ 2023-09-14 6:01 UTC (permalink / raw)
To: Niklas Cassel, Damien Le Moal
Cc: Bagas Sanjaya, patenteng, linux-ide, Niklas Cassel
On 9/13/23 17:04, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> Currently, we fetch sense data for a _successful_ command if either:
> 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
> ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
> sense data supported bit is set); or
> 2) Command was non-NCQ and regular sense data reporting is enabled.
>
> This means that case 2) will trigger for a non-NCQ command which has
> ATA_SENSE bit set, regardless if CDL is enabled or not.
>
> This decision was by design. If the device reports that it has sense data
> available, it makes sense to fetch that sense data, since the sk/asc/ascq
> could be important information regardless if CDL is enabled or not.
>
> However, the fetching of sense data for a successful command is done via
> ATA EH. Considering how intricate the ATA EH is, we really do not want to
> invoke ATA EH unless absolutely needed.
>
> Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
> commands using policy 0xD") we never fetched sense data for successful
> non-NCQ commands.
>
> In order to not invoke the ATA EH unless absolutely necessary, even if the
> device claims support for sense data reporting, only fetch sense data for
> successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
>
Errm. We need sense data for zoned devices, do we not?
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@suse.de +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled
2023-09-14 6:01 ` Hannes Reinecke
@ 2023-09-14 6:07 ` Damien Le Moal
0 siblings, 0 replies; 6+ messages in thread
From: Damien Le Moal @ 2023-09-14 6:07 UTC (permalink / raw)
To: Hannes Reinecke, Niklas Cassel
Cc: Bagas Sanjaya, patenteng, linux-ide, Niklas Cassel
On 9/14/23 15:01, Hannes Reinecke wrote:
> On 9/13/23 17:04, Niklas Cassel wrote:
>> From: Niklas Cassel <niklas.cassel@wdc.com>
>>
>> Currently, we fetch sense data for a _successful_ command if either:
>> 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
>> ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
>> sense data supported bit is set); or
>> 2) Command was non-NCQ and regular sense data reporting is enabled.
>>
>> This means that case 2) will trigger for a non-NCQ command which has
>> ATA_SENSE bit set, regardless if CDL is enabled or not.
>>
>> This decision was by design. If the device reports that it has sense data
>> available, it makes sense to fetch that sense data, since the sk/asc/ascq
>> could be important information regardless if CDL is enabled or not.
>>
>> However, the fetching of sense data for a successful command is done via
>> ATA EH. Considering how intricate the ATA EH is, we really do not want to
>> invoke ATA EH unless absolutely needed.
>>
>> Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
>> commands using policy 0xD") we never fetched sense data for successful
>> non-NCQ commands.
>>
>> In order to not invoke the ATA EH unless absolutely necessary, even if the
>> device claims support for sense data reporting, only fetch sense data for
>> successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
>>
> Errm. We need sense data for zoned devices, do we not?
Any SATA device that supports sense reporting feature, including ZAC devices,
can still get sense data for failed commands.
This case is for the weird CDL aborts with policy 0xD which will fail a command
that exceeds its limits with a success status and sense data available bit set
in the SDB (which translate to ATA_SENSE flag). So for commands using CDL,
getting a success with ATA_SENSE set most likely means that the command was
aborted by the drive.
So this is not the regular error case. We still use EH to get that sense data
from the "sense data for successful NCQ command log" though, because otherwise
we can run into problems if we do not have a free tag.
>
> Cheers,
>
> Hannes
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled
2023-09-13 15:04 [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled Niklas Cassel
2023-09-14 0:14 ` Damien Le Moal
2023-09-14 6:01 ` Hannes Reinecke
@ 2023-09-15 6:32 ` Damien Le Moal
2023-09-15 12:05 ` Niklas Cassel
2 siblings, 1 reply; 6+ messages in thread
From: Damien Le Moal @ 2023-09-15 6:32 UTC (permalink / raw)
To: Niklas Cassel; +Cc: Bagas Sanjaya, patenteng, linux-ide, Niklas Cassel
On 9/14/23 00:04, Niklas Cassel wrote:
> From: Niklas Cassel <niklas.cassel@wdc.com>
>
> Currently, we fetch sense data for a _successful_ command if either:
> 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
> ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
> sense data supported bit is set); or
> 2) Command was non-NCQ and regular sense data reporting is enabled.
>
> This means that case 2) will trigger for a non-NCQ command which has
> ATA_SENSE bit set, regardless if CDL is enabled or not.
>
> This decision was by design. If the device reports that it has sense data
> available, it makes sense to fetch that sense data, since the sk/asc/ascq
> could be important information regardless if CDL is enabled or not.
>
> However, the fetching of sense data for a successful command is done via
> ATA EH. Considering how intricate the ATA EH is, we really do not want to
> invoke ATA EH unless absolutely needed.
>
> Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
> commands using policy 0xD") we never fetched sense data for successful
> non-NCQ commands.
>
> In order to not invoke the ATA EH unless absolutely necessary, even if the
> device claims support for sense data reporting, only fetch sense data for
> successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
>
> Fixes: 3ac873c76d79 ("ata: libata-core: fix when to fetch sense data for successful commands")
> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> ---
> drivers/ata/libata-core.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 74314311295f..2f7f72994cd7 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -4784,10 +4784,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> * 0xD. For these commands, invoke EH to get the command sense data.
> */
> if (qc->result_tf.status & ATA_SENSE &&
> - ((ata_is_ncq(qc->tf.protocol) &&
> - dev->flags & ATA_DFLAG_CDL_ENABLED) ||
> - (!ata_is_ncq(qc->tf.protocol) &&
> - ata_id_sense_reporting_enabled(dev->id)))) {
> + dev->flags & ATA_DFLAG_CDL_ENABLED) {
Applied to for-6.6-fixes with a tweak:
if (qc->flags & ATA_QCFLAG_HAS_CDL &&
qc->result_tf.status & ATA_SENSE) {
is the test I tweaked. This allows ignoring command that do not use CDL. And
seeing ATA_QCFLAG_HAS_CDL set implies that dev->flags & ATA_DFLAG_CDL_ENABLED is
true. So this is better I think.
> /*
> * Tell SCSI EH to not overwrite scmd->result even if this
> * command is finished with result SAM_STAT_GOOD.
--
Damien Le Moal
Western Digital Research
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] ata: libata-core: fetch sense data for successful commands iff CDL enabled
2023-09-15 6:32 ` Damien Le Moal
@ 2023-09-15 12:05 ` Niklas Cassel
0 siblings, 0 replies; 6+ messages in thread
From: Niklas Cassel @ 2023-09-15 12:05 UTC (permalink / raw)
To: Damien Le Moal
Cc: Niklas Cassel, Bagas Sanjaya, patenteng,
linux-ide@vger.kernel.org
On Fri, Sep 15, 2023 at 03:32:50PM +0900, Damien Le Moal wrote:
> On 9/14/23 00:04, Niklas Cassel wrote:
> > From: Niklas Cassel <niklas.cassel@wdc.com>
> >
> > Currently, we fetch sense data for a _successful_ command if either:
> > 1) Command was NCQ and ATA_DFLAG_CDL_ENABLED flag set (flag
> > ATA_DFLAG_CDL_ENABLED will only be set if the Successful NCQ command
> > sense data supported bit is set); or
> > 2) Command was non-NCQ and regular sense data reporting is enabled.
> >
> > This means that case 2) will trigger for a non-NCQ command which has
> > ATA_SENSE bit set, regardless if CDL is enabled or not.
> >
> > This decision was by design. If the device reports that it has sense data
> > available, it makes sense to fetch that sense data, since the sk/asc/ascq
> > could be important information regardless if CDL is enabled or not.
> >
> > However, the fetching of sense data for a successful command is done via
> > ATA EH. Considering how intricate the ATA EH is, we really do not want to
> > invoke ATA EH unless absolutely needed.
> >
> > Before commit 18bd7718b5c4 ("scsi: ata: libata: Handle completion of CDL
> > commands using policy 0xD") we never fetched sense data for successful
> > non-NCQ commands.
> >
> > In order to not invoke the ATA EH unless absolutely necessary, even if the
> > device claims support for sense data reporting, only fetch sense data for
> > successful (NCQ and non-NCQ commands) if CDL is supported and enabled.
> >
> > Fixes: 3ac873c76d79 ("ata: libata-core: fix when to fetch sense data for successful commands")
> > Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
> > ---
> > drivers/ata/libata-core.c | 5 +----
> > 1 file changed, 1 insertion(+), 4 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 74314311295f..2f7f72994cd7 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -4784,10 +4784,7 @@ void ata_qc_complete(struct ata_queued_cmd *qc)
> > * 0xD. For these commands, invoke EH to get the command sense data.
> > */
> > if (qc->result_tf.status & ATA_SENSE &&
> > - ((ata_is_ncq(qc->tf.protocol) &&
> > - dev->flags & ATA_DFLAG_CDL_ENABLED) ||
> > - (!ata_is_ncq(qc->tf.protocol) &&
> > - ata_id_sense_reporting_enabled(dev->id)))) {
> > + dev->flags & ATA_DFLAG_CDL_ENABLED) {
>
> Applied to for-6.6-fixes with a tweak:
>
> if (qc->flags & ATA_QCFLAG_HAS_CDL &&
> qc->result_tf.status & ATA_SENSE) {
>
> is the test I tweaked. This allows ignoring command that do not use CDL. And
> seeing ATA_QCFLAG_HAS_CDL set implies that dev->flags & ATA_DFLAG_CDL_ENABLED is
> true. So this is better I think.
I agree.
Kind regards,
Niklas
^ permalink raw reply [flat|nested] 6+ messages in thread