From: Damien Le Moal dlemoal@kernel.org
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: stable@vger.kernel.org Signed-off-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Niklas Cassel cassel@kernel.org --- 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/20240111115123.1258422-3-dlemoal@kernel.or...
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(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d9f80f4f70f5..af2334bc806d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -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;
/*
On 2/16/24 20:20, Niklas Cassel wrote:
From: Damien Le Moal dlemoal@kernel.org
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: stable@vger.kernel.org Signed-off-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Niklas Cassel cassel@kernel.org
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/20240111115123.1258422-3-dlemoal@kernel.or...
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(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d9f80f4f70f5..af2334bc806d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -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...
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))/*
On Fri, Feb 16, 2024 at 09:16:23PM +0900, Damien Le Moal wrote:
On 2/16/24 20:20, Niklas Cassel wrote:
From: Damien Le Moal dlemoal@kernel.org
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: stable@vger.kernel.org Signed-off-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Niklas Cassel cassel@kernel.org
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/20240111115123.1258422-3-dlemoal@kernel.or...
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(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d9f80f4f70f5..af2334bc806d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -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...
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/d63a7b93-d1a3-726e-355c-b4a4608626f4@gmail...
And you said that you were going to look in to it: https://lore.kernel.org/linux-ide/0563322c-4093-4e7d-bb48-61712238494e@kerne...
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
On 2/16/24 21:33, Niklas Cassel wrote:
On Fri, Feb 16, 2024 at 09:16:23PM +0900, Damien Le Moal wrote:
On 2/16/24 20:20, Niklas Cassel wrote:
From: Damien Le Moal dlemoal@kernel.org
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: stable@vger.kernel.org Signed-off-by: Damien Le Moal dlemoal@kernel.org Signed-off-by: Niklas Cassel cassel@kernel.org
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/20240111115123.1258422-3-dlemoal@kernel.or...
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(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index d9f80f4f70f5..af2334bc806d 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -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...
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/d63a7b93-d1a3-726e-355c-b4a4608626f4@gmail...
And you said that you were going to look in to it: https://lore.kernel.org/linux-ide/0563322c-4093-4e7d-bb48-61712238494e@kerne...
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).
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
linux-stable-mirror@lists.linaro.org
-
Damien Le Moal -
Niklas Cassel