[PATCH 0/2] Power management fixes

[PATCH 0/2] Power management fixes

[Damien Le Moal]

The first patch is a fairly obvious fix. The second one needs testing.

Dieter, Zhihao,

Can you please try these patches to see if they resolve your issues
with system low power state transitions ?

Damien Le Moal (2):
  ata: libata-core: Do not try to set sleeping devices to standby
  ata: libata-core: Revert "ata: libata-core: Fix
    ata_pci_shutdown_one()"

 drivers/ata/libata-core.c | 75 ++++++++++++++++++++++++---------------
 1 file changed, 47 insertions(+), 28 deletions(-)

-- 
2.43.0

[PATCH 1/2] ata: libata-core: Do not try to set sleeping devices to standby

[Damien Le Moal]

In ata ata_dev_power_set_standby(), check that the target device is not
sleeping. If it is, there is no need to do anything.

Fixes: aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 09ed67772fae..d9f80f4f70f5 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2017,6 +2017,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
+	/* If the device is already sleeping, do nothing. */
+	if (dev->flags & ATA_DFLAG_SLEEPING)
+		return;
+
 	/*
 	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
 	 * causing some drives to spin up and down again. For these, do nothing
-- 
2.43.0

[PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

[Damien Le Moal]

This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.

Several users have signaled issues with commit fd3a6837d8e1 ("ata:
libata-core: Fix ata_pci_shutdown_one()") which causes failure of the
system SoC to go to a low power state. The reason for this problem
is not well understood but given that this patch is harmless with the
improvements to ata_dev_power_set_standby(), restore it to allow system
lower power state transitions.

For regular system shutdown, ata_dev_power_set_standby() will be
executed twice: once the scsi device is removed and another when
ata_pci_shutdown_one() executes and EH completes unloading the devices.
Make the second call to ata_dev_power_set_standby() do nothing by using
ata_dev_power_is_active() and return if the device is already in
standby.

Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
Cc: stable@vger.kernel.org
Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
---
 drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
 1 file changed, 45 insertions(+), 30 deletions(-)

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index d9f80f4f70f5..20a366942626 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
 	return true;
 }
 
+static bool ata_dev_power_is_active(struct ata_device *dev)
+{
+	struct ata_taskfile tf;
+	unsigned int err_mask;
+
+	ata_tf_init(dev, &tf);
+	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
+	tf.protocol = ATA_PROT_NODATA;
+	tf.command = ATA_CMD_CHK_POWER;
+
+	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
+	if (err_mask) {
+		ata_dev_err(dev, "Check power mode failed (err_mask=0x%x)\n",
+			    err_mask);
+		/*
+		 * Assume we are in standby mode so that we always force a
+		 * spinup in ata_dev_power_set_active().
+		 */
+		return false;
+	}
+
+	ata_dev_dbg(dev, "Power mode: 0x%02x\n", tf.nsect);
+
+	/* Active or idle */
+	return tf.nsect == 0xff;
+}
+
 /**
  *	ata_dev_power_set_standby - Set a device power mode to standby
  *	@dev: target device
@@ -2017,8 +2044,9 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 	struct ata_taskfile tf;
 	unsigned int err_mask;
 
-	/* If the device is already sleeping, do nothing. */
-	if (dev->flags & ATA_DFLAG_SLEEPING)
+	/* If the device is already sleeping or in standby, do nothing. */
+	if ((dev->flags & ATA_DFLAG_SLEEPING) ||
+	    !ata_dev_power_is_active(dev))
 		return;
 
 	/*
@@ -2046,33 +2074,6 @@ void ata_dev_power_set_standby(struct ata_device *dev)
 			    err_mask);
 }
 
-static bool ata_dev_power_is_active(struct ata_device *dev)
-{
-	struct ata_taskfile tf;
-	unsigned int err_mask;
-
-	ata_tf_init(dev, &tf);
-	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
-	tf.protocol = ATA_PROT_NODATA;
-	tf.command = ATA_CMD_CHK_POWER;
-
-	err_mask = ata_exec_internal(dev, &tf, NULL, DMA_NONE, NULL, 0, 0);
-	if (err_mask) {
-		ata_dev_err(dev, "Check power mode failed (err_mask=0x%x)\n",
-			    err_mask);
-		/*
-		 * Assume we are in standby mode so that we always force a
-		 * spinup in ata_dev_power_set_active().
-		 */
-		return false;
-	}
-
-	ata_dev_dbg(dev, "Power mode: 0x%02x\n", tf.nsect);
-
-	/* Active or idle */
-	return tf.nsect == 0xff;
-}
-
 /**
  *	ata_dev_power_set_active -  Set a device power mode to active
  *	@dev: target device
@@ -6184,10 +6185,24 @@ EXPORT_SYMBOL_GPL(ata_pci_remove_one);
 void ata_pci_shutdown_one(struct pci_dev *pdev)
 {
 	struct ata_host *host = pci_get_drvdata(pdev);
+	struct ata_port *ap;
+	unsigned long flags;
 	int i;
 
+	/* Tell EH to disable all devices */
 	for (i = 0; i < host->n_ports; i++) {
-		struct ata_port *ap = host->ports[i];
+		ap = host->ports[i];
+		spin_lock_irqsave(ap->lock, flags);
+		ap->pflags |= ATA_PFLAG_UNLOADING;
+		ata_port_schedule_eh(ap);
+		spin_unlock_irqrestore(ap->lock, flags);
+	}
+
+	for (i = 0; i < host->n_ports; i++) {
+		ap = host->ports[i];
+
+		/* Wait for EH to complete before freezing the port */
+		ata_port_wait_eh(ap);
 
 		ap->pflags |= ATA_PFLAG_FROZEN;
 
-- 
2.43.0

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

[Sergei Shtylyov]

On 1/11/24 2:51 PM, Damien Le Moal wrote:

> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.
> 
> Several users have signaled issues with commit fd3a6837d8e1 ("ata:
> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the
> system SoC to go to a low power state. The reason for this problem
> is not well understood but given that this patch is harmless with the
> improvements to ata_dev_power_set_standby(), restore it to allow system
> lower power state transitions.
> 
> For regular system shutdown, ata_dev_power_set_standby() will be
> executed twice: once the scsi device is removed and another when
> ata_pci_shutdown_one() executes and EH completes unloading the devices.
> Make the second call to ata_dev_power_set_standby() do nothing by using
> ata_dev_power_is_active() and return if the device is already in
> standby.
> 
> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
>  1 file changed, 45 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index d9f80f4f70f5..20a366942626 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
>  	return true;
>  }
>  
> +static bool ata_dev_power_is_active(struct ata_device *dev)
> +{
> +	struct ata_taskfile tf;
> +	unsigned int err_mask;
> +
> +	ata_tf_init(dev, &tf);
> +	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;

   Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
regs but the device/head reg. Material for a fix, I guess... :-)

> +	tf.protocol = ATA_PROT_NODATA;
> +	tf.command = ATA_CMD_CHK_POWER;
> +
[...]

MBR, Sergey

Re: [PATCH 0/2] Power management fixes

[Damien Le Moal]

On 1/11/24 21:09, 王 志豪 wrote:
> thanks a lot！I will take a try and reply you soon

Did you try the patches ?

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] ata: libata-core: Do not try to set sleeping devices to standby

[Niklas Cassel]

On Thu, Jan 11, 2024 at 08:51:22PM +0900, Damien Le Moal wrote:
> In ata ata_dev_power_set_standby(), check that the target device is not
> sleeping. If it is, there is no need to do anything.
> 
> Fixes: aa3998dbeb3a ("ata: libata-scsi: Disable scsi device manage_system_start_stop")
> Cc: stable@vger.kernel.org
> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> ---
>  drivers/ata/libata-core.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index 09ed67772fae..d9f80f4f70f5 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -2017,6 +2017,10 @@ void ata_dev_power_set_standby(struct ata_device *dev)
>  	struct ata_taskfile tf;
>  	unsigned int err_mask;
>  
> +	/* If the device is already sleeping, do nothing. */
> +	if (dev->flags & ATA_DFLAG_SLEEPING)
> +		return;
> +
>  	/*
>  	 * Some odd clown BIOSes issue spindown on power off (ACPI S4 or S5)
>  	 * causing some drives to spin up and down again. For these, do nothing
> -- 
> 2.43.0
> 

Holding off [PATCH 2/2] until we get a better understanding of things,
thus for [PATCH 1/2] only:

Applied:
https://git.kernel.org/pub/scm/linux/kernel/git/libata/linux.git/log/?h=for-6.8-fixes

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

[Niklas Cassel]

Hello Sergei,

On Thu, Jan 11, 2024 at 09:10:43PM +0300, Sergei Shtylyov wrote:
> On 1/11/24 2:51 PM, Damien Le Moal wrote:
> 
> > This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.
> > 
> > Several users have signaled issues with commit fd3a6837d8e1 ("ata:
> > libata-core: Fix ata_pci_shutdown_one()") which causes failure of the
> > system SoC to go to a low power state. The reason for this problem
> > is not well understood but given that this patch is harmless with the
> > improvements to ata_dev_power_set_standby(), restore it to allow system
> > lower power state transitions.
> > 
> > For regular system shutdown, ata_dev_power_set_standby() will be
> > executed twice: once the scsi device is removed and another when
> > ata_pci_shutdown_one() executes and EH completes unloading the devices.
> > Make the second call to ata_dev_power_set_standby() do nothing by using
> > ata_dev_power_is_active() and return if the device is already in
> > standby.
> > 
> > Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> > ---
> >  drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
> >  1 file changed, 45 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index d9f80f4f70f5..20a366942626 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
> >  	return true;
> >  }
> >  
> > +static bool ata_dev_power_is_active(struct ata_device *dev)
> > +{
> > +	struct ata_taskfile tf;
> > +	unsigned int err_mask;
> > +
> > +	ata_tf_init(dev, &tf);
> > +	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> 
>    Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
> regs but the device/head reg. Material for a fix, I guess... :-)
> 
> > +	tf.protocol = ATA_PROT_NODATA;
> > +	tf.command = ATA_CMD_CHK_POWER;
> > +
> [...]

Looking at the definition of the flag:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76

"enable r/w to nsect/lba regs"

This function does read from the nsect reg:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069

So I would prefer to keep it as it.


Basically the whole motto for libata right now:
"If it ain't broke, don't fix it."


Sure, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343

it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE
is used to "guard" if these regs should be written to the TF.


However, if we look at:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392

is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should
be read from the TF.

So it looks like the intention was that these flags should be used
to guard if certain TF regs should be written or read, but in reality,
only some of the flags are actually for guarding reads.
(While all of the flags are used for guarding writes.)


Personally, I don't really see the point of using flags to guard writes
to the host controller. Why would we want to skip writing certain TF regs?
The struct ata_taskfile should be zero-initialized before filling it with
a command, so why not always write all TF regs and remove these flags?

Anyway, why touch it now and risk introducing regressions on some old PATA
hardware?


Kind regards,
Niklas

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

[Sergey Shtylyov]

On 2/19/24 6:29 PM, Niklas Cassel wrote:
[...]

>>> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.
>>>
>>> Several users have signaled issues with commit fd3a6837d8e1 ("ata:
>>> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the
>>> system SoC to go to a low power state. The reason for this problem
>>> is not well understood but given that this patch is harmless with the
>>> improvements to ata_dev_power_set_standby(), restore it to allow system
>>> lower power state transitions.
>>>
>>> For regular system shutdown, ata_dev_power_set_standby() will be
>>> executed twice: once the scsi device is removed and another when
>>> ata_pci_shutdown_one() executes and EH completes unloading the devices.
>>> Make the second call to ata_dev_power_set_standby() do nothing by using
>>> ata_dev_power_is_active() and return if the device is already in
>>> standby.
>>>
>>> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
>>> ---
>>>  drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
>>>  1 file changed, 45 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
>>> index d9f80f4f70f5..20a366942626 100644
>>> --- a/drivers/ata/libata-core.c
>>> +++ b/drivers/ata/libata-core.c
>>> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
>>>  	return true;
>>>  }
>>>  
>>> +static bool ata_dev_power_is_active(struct ata_device *dev)
>>> +{
>>> +	struct ata_taskfile tf;
>>> +	unsigned int err_mask;
>>> +
>>> +	ata_tf_init(dev, &tf);
>>> +	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
>>
>>    Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
>> regs but the device/head reg. Material for a fix, I guess... :-)
>>
>>> +	tf.protocol = ATA_PROT_NODATA;
>>> +	tf.command = ATA_CMD_CHK_POWER;
>>> +
>> [...]
> 
> Looking at the definition of the flag:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76
> 
> "enable r/w to nsect/lba regs"

   I'm afraid this comment doesn't reflect the reality in its r/w part --
if you look at e.g. ata_sff_tf_read(), you'll see that it always reads 
all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to
determine whether it should read the HOBs as well...

> This function does read from the nsect reg:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-core.c?h=v6.8-rc5#n2069
> 
> So I would prefer to keep it as it.

   IMO, it doesn't make much sense -- unless you assume that the device
could leave that reg unset as a result of this command...

> Basically the whole motto for libata right now:
> "If it ain't broke, don't fix it."

   Do you realize that each taskfile reg access takes e.g. 900-990 ns on
the Intel PIIX/ICH (the part # was 82371/82801) IDE controllers (with 33
MHz PCI bus)? Luckily, we just have to write (almost?) whole taskfile on
the read/write commands anyway...

> Sure, if we look at:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n343
> 
> it seems that flags ATA_TFLAG_ISADDR, ATA_TFLAG_LBA48, and ATA_TFLAG_DEVICE
> is used to "guard" if these regs should be written to the TF.
> 
> 
> However, if we look at:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/ata/libata-sff.c?h=v6.8-rc5#n392
> 
> is seems that only flag ATA_TFLAG_LBA48 is used to "guard" if the regs should
> be read from the TF.

   Luckily, we have to read back the whole taskfile only on the read/write
errors...

> So it looks like the intention was that these flags should be used
> to guard if certain TF regs should be written or read, but in reality,
> only some of the flags are actually for guarding reads.
> (While all of the flags are used for guarding writes.)

   So you're seeing that inconsistency (I mentioned) yourself... :-)

> Personally, I don't really see the point of using flags to guard writes
> to the host controller. Why would we want to skip writing certain TF regs?
> 
> The struct ata_taskfile should be zero-initialized before filling it with

   TBH, I generally hate how libata implemented the taskfile accessors
and I hate how *struct* ata_taskfile looks too... :-)

> a command, so why not always write all TF regs and remove these flags?

   To stop wasting a good microsecond per a reg R/W cycle, perhaps? :-)
   Anyway, the ATA standards clearly describe what the regs are used by
each command and what to expect on a normal/erratic command completion...

   In drivers/ide/ we finanally ended up with 8-bit reg validity flags,
each bit corresponding to an individual taskfile reg:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=60f85019c6c8c1aebf3485a313e0da094bc95d07

> Anyway, why touch it now and risk introducing regressions on some old PATA
> hardware?

   Do you realize that drivers/ide/ wasn't writing out the whole taskfile
when issuing this particular command since:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=d364c7f50b3bb6dc77259974038567b821e2cf0a

   If there were regressions, we would have seen them a long time ago,
no? :-)

> Kind regards,
> Niklas

MBR, Sergey

Re: [PATCH 2/2] ata: libata-core: Revert "ata: libata-core: Fix ata_pci_shutdown_one()"

[Niklas Cassel]

Hello Sergey,

On Sat, Feb 24, 2024 at 12:04:46AM +0300, Sergey Shtylyov wrote:
> On 2/19/24 6:29 PM, Niklas Cassel wrote:
> [...]
> 
> >>> This reverts commit fd3a6837d8e18cb7be80dcca1283276290336a7a.
> >>>
> >>> Several users have signaled issues with commit fd3a6837d8e1 ("ata:
> >>> libata-core: Fix ata_pci_shutdown_one()") which causes failure of the
> >>> system SoC to go to a low power state. The reason for this problem
> >>> is not well understood but given that this patch is harmless with the
> >>> improvements to ata_dev_power_set_standby(), restore it to allow system
> >>> lower power state transitions.
> >>>
> >>> For regular system shutdown, ata_dev_power_set_standby() will be
> >>> executed twice: once the scsi device is removed and another when
> >>> ata_pci_shutdown_one() executes and EH completes unloading the devices.
> >>> Make the second call to ata_dev_power_set_standby() do nothing by using
> >>> ata_dev_power_is_active() and return if the device is already in
> >>> standby.
> >>>
> >>> Fixes: fd3a6837d8e1 ("ata: libata-core: Fix ata_pci_shutdown_one()")
> >>> Cc: stable@vger.kernel.org
> >>> Signed-off-by: Damien Le Moal <dlemoal@kernel.org>
> >>> ---
> >>>  drivers/ata/libata-core.c | 75 +++++++++++++++++++++++----------------
> >>>  1 file changed, 45 insertions(+), 30 deletions(-)
> >>>
> >>> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> >>> index d9f80f4f70f5..20a366942626 100644
> >>> --- a/drivers/ata/libata-core.c
> >>> +++ b/drivers/ata/libata-core.c
> >>> @@ -2001,6 +2001,33 @@ bool ata_dev_power_init_tf(struct ata_device *dev, struct ata_taskfile *tf,
> >>>  	return true;
> >>>  }
> >>>  
> >>> +static bool ata_dev_power_is_active(struct ata_device *dev)
> >>> +{
> >>> +	struct ata_taskfile tf;
> >>> +	unsigned int err_mask;
> >>> +
> >>> +	ata_tf_init(dev, &tf);
> >>> +	tf.flags |= ATA_TFLAG_DEVICE | ATA_TFLAG_ISADDR;
> >>
> >>    Why set ATA_TFLAG_ISADDR, BTW? This command doesn't use any taskfile
> >> regs but the device/head reg. Material for a fix, I guess... :-)
> >>
> >>> +	tf.protocol = ATA_PROT_NODATA;
> >>> +	tf.command = ATA_CMD_CHK_POWER;
> >>> +
> >> [...]
> > 
> > Looking at the definition of the flag:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/libata.h?h=v6.8-rc5#n76
> > 
> > "enable r/w to nsect/lba regs"
> 
>    I'm afraid this comment doesn't reflect the reality in its r/w part --
> if you look at e.g. ata_sff_tf_read(), you'll see that it always reads 
> all the legacy taskfile and only checks ATA_TFLAG_LBA48 in order to
> determine whether it should read the HOBs as well...

Considering that you have experience with cleaning up a similar mess in
drivers/ide, patches for drivers/ata which fixes the comment and the
functions needlessly setting the ATA_TFLAG_DEVICE flag is more than
welcome.

From a quick look, there appears to be quite a few functions (in
addition to ata_dev_power_is_active()) which needlessly sets this flag.


Kind regards,
Niklas