ata: libata-core: Do not call ata_dev_power_set_standby() twice

PATCH Superseded Niklas Cassel 2 participants 4 messages Feb 16, 2024

Subsystems (2)

Stable branch libata

lore

Share & tools

lore

Niklas Cassel

author maintainer

2024-02-16 11:20 UTC #0 lore raw

PATCH 1/1 ata: libata-core: Do not call ata_dev_power_set_standby() twice

From: Damien Le Moal <[email protected]>

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 2da4c5e24e86 ("ata: libata-core: Improve ata_dev_power_set_active()")

Cc [email protected]

Signed-off-by Damien Le Moal <[email protected]>

Signed-off-by Niklas Cassel <[email protected]>

---
This fix was originally part of patch that contained both a fix and a revert in a single patch:
https://lore.kernel.org/linux-ide/[email protected]/

This patch contains the only the fix (as it is valid even without the revert), without the revert.

Updated the Fixes tag to point to a more appropriate commit, since we no longer revert any code.

 drivers/ata/libata-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/ata/libata-core.c +4 -2

index d9f80f4f70f5..af2334bc806d 100644
@@ -85,6 +85,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
+static bool ata_dev_power_is_active(struct ata_device *dev);
 
 atomic_t ata_print_id = ATOMIC_INIT(0);

@@ -2017,8 +2018,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;
 
 	/*

Signature · 1 line

2.43.1

Damien Le Moal

maintainer

2024-02-16 12:16 UTC #1 lore raw

Quoted 33 lines · Niklas Cassel

From: Damien Le Moal <[email protected]>
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 2da4c5e24e86 ("ata: libata-core: Improve ata_dev_power_set_active()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
This fix was originally part of patch that contained both a fix and a revert in a single patch:
https://lore.kernel.org/linux-ide/[email protected]/
This patch contains the only the fix (as it is valid even without the revert), without the revert.
Updated the Fixes tag to point to a more appropriate commit, since we no longer revert any code.
 drivers/ata/libata-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..af2334bc806d 100644

drivers/ata/libata-core.c static unsigned int ata_dev_init_params(struct ata_device *dev, · line 88

Quoted 4 lines

drivers/ata/libata-core.c +1 -0

@@ -85,6 +85,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
+static bool ata_dev_power_is_active(struct ata_device *dev);

I forgot what I did originally but didn't I move the code of ata_dev_power_is_active() before ata_dev_power_set_standby() to avoid this forward declaration ?

With that, the code is a little odd as ata_dev_power_is_active() is defined between ata_dev_power_set_standby() and ata_dev_power_set_active() but both functions use it...

Quoted 15 lines

drivers/ata/libata-core.c +3 -2

 atomic_t ata_print_id = ATOMIC_INIT(0);

@@ -2017,8 +2018,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;
 
 	/*

Signature · 2 lines

Damien Le Moal
Western Digital Research

Niklas Cassel

author maintainer

2024-02-16 12:33 UTC #2 lore raw

Quoted 43 lines · Damien Le Moal

Quoted 33 lines · Niklas Cassel
From: Damien Le Moal <[email protected]>
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 2da4c5e24e86 ("ata: libata-core: Improve ata_dev_power_set_active()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
This fix was originally part of patch that contained both a fix and a revert in a single patch:
https://lore.kernel.org/linux-ide/[email protected]/
This patch contains the only the fix (as it is valid even without the revert), without the revert.
Updated the Fixes tag to point to a more appropriate commit, since we no longer revert any code.
 drivers/ata/libata-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..af2334bc806d 100644
drivers/ata/libata-core.c static unsigned int ata_dev_init_params(struct ata_device *dev, · line 88
Quoted 4 lines
drivers/ata/libata-core.c +1 -0
@@ -85,6 +85,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
+static bool ata_dev_power_is_active(struct ata_device *dev);
I forgot what I did originally but didn't I move the code of ata_dev_power_is_active() before ata_dev_power_set_standby() to avoid this forward declaration ?

Quoted 3 lines

With that, the code is a little odd as ata_dev_power_is_active() is defined between ata_dev_power_set_standby() and ata_dev_power_set_active() but both functions use it...

Yes, you moved the function instead of forward declaring it.

But then there was a discussion of why ATA_TFLAG_ISADDR is set in ata_dev_power_is_active():
https://lore.kernel.org/linux-ide/[email protected]/

And you said that you were going to look in to it:
https://lore.kernel.org/linux-ide/[email protected]/

Since this fix does not strictly require any changes to ata_dev_power_is_active(), and since we already have a bunch of forward declared functions, I think that forward declaring it is a good way to avoid this actual fix from falling through the cracks.

Kind regards,
Niklas

Damien Le Moal

maintainer

2024-02-16 22:44 UTC #3 lore raw

Quoted 48 lines · Niklas Cassel

Quoted 43 lines · Damien Le Moal
Quoted 33 lines · Niklas Cassel
From: Damien Le Moal <[email protected]>
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 2da4c5e24e86 ("ata: libata-core: Improve ata_dev_power_set_active()")
Cc [email protected]
Signed-off-by Damien Le Moal <[email protected]>
Signed-off-by Niklas Cassel <[email protected]>
---
This fix was originally part of patch that contained both a fix and a revert in a single patch:
https://lore.kernel.org/linux-ide/[email protected]/
This patch contains the only the fix (as it is valid even without the revert), without the revert.
Updated the Fixes tag to point to a more appropriate commit, since we no longer revert any code.
 drivers/ata/libata-core.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
drivers/ata/libata-core.c +0 -0
index d9f80f4f70f5..af2334bc806d 100644
drivers/ata/libata-core.c static unsigned int ata_dev_init_params(struct ata_device *dev, · line 88
Quoted 4 lines
drivers/ata/libata-core.c +1 -0
@@ -85,6 +85,7 @@ static unsigned int ata_dev_init_params(struct ata_device *dev,
 static unsigned int ata_dev_set_xfermode(struct ata_device *dev);
 static void ata_dev_xfermask(struct ata_device *dev);
 static unsigned long ata_dev_blacklisted(const struct ata_device *dev);
+static bool ata_dev_power_is_active(struct ata_device *dev);
I forgot what I did originally but didn't I move the code of ata_dev_power_is_active() before ata_dev_power_set_standby() to avoid this forward declaration ?
Quoted 3 lines
With that, the code is a little odd as ata_dev_power_is_active() is defined between ata_dev_power_set_standby() and ata_dev_power_set_active() but both functions use it...

Quoted 9 lines

Yes, you moved the function instead of forward declaring it.
But then there was a discussion of why ATA_TFLAG_ISADDR is set in ata_dev_power_is_active():
https://lore.kernel.org/linux-ide/[email protected]/
And you said that you were going to look in to it:
https://lore.kernel.org/linux-ide/[email protected]/

Ah, yes, I remember now. Let me have a look at this and resend a proper patch, + another one for the ISADDR cleanup. I really don't want to fix this with that forward declaration if we can avoid it (and we clearly can here).

Quoted 8 lines

Since this fix does not strictly require any changes to ata_dev_power_is_active(), and since we already have a bunch of forward declared functions, I think that forward declaring it is a good way to avoid this actual fix from falling through the cracks.
Kind regards,
Niklas

Signature · 2 lines

Damien Le Moal
Western Digital Research