mbox series

[v4,00/23] Fix libata suspend/resume handling and code cleanup

Message ID 20230920135439.929695-1-dlemoal@kernel.org
Headers show
Series Fix libata suspend/resume handling and code cleanup | expand

Message

Damien Le Moal Sept. 20, 2023, 1:54 p.m. UTC
The first 9 patches of this series fix several issues with suspend/resume
power management operations in scsi and libata. The most significant
changes introduced are in patch 4 and 5, where the manage_start_stop
flag of scsi devices is split into the manage_system_start_stop and
manage_runtime_start_stop flags to allow keeping scsi runtime power
operations for spining up/down ATA devices but have libata do its own
system suspend/resume device power state management using EH.

The remaining patches are code cleanup that do not introduce any
significant functional change.

This series was tested on qemu and on various PCs and servers. I am
CC-ing people who recently reported issues with suspend/resume.
Additional testing would be much appreciated.

Changes from v3:
 * Corrected pathc 1 (typo in commit message and WARN_ON() removal)
 * Changed path 3 as suggested by Niklas (moved definition of
   ->slave_alloc)
 * Rebased on rc2
 * Added review tags

Changes from v2:
 * Added patch 4 as simply disabling manage_start_stop from libata was
   breaking individual disk runtime suspend/autosuspend. Patch 5 was
   reworked accordingly to the changes in patch 4.
 * Fixed patch 3: applying the link creation was missing and the link
   creation itself was also incorrect, preventing sd probe to execute
   correctly. Thanks to Geert for testing and reporting this issue.
 * Split the "Fix delayed scsi_rescan_device() execution" patch into
   patch 6 (scsi part) and patch 7 (ata part).
 * Modified patch 9 to not call sd_shutdown() from sd_remove() for
   devices that are not running.
 * Added Chia-Lin Tested tag to unchanged patches

Changes from v1:
 * Added patch 8 and 9 to fix compilation warnings with W=1
 * Addressed John comment in patch 19
 * Fixed patch 20 commit message (Sergei)
 * Added Hannes Review tag

Damien Le Moal (23):
  ata: libata-core: Fix ata_port_request_pm() locking
  ata: libata-core: Fix port and device removal
  ata: libata-scsi: link ata port and scsi device
  scsi: sd: Differentiate system and runtime start/stop management
  ata: libata-scsi: Disable scsi device manage_system_start_stop
  scsi: Do not attempt to rescan suspended devices
  ata: libata-scsi: Fix delayed scsi_rescan_device() execution
  ata: libata-core: Do not register PM operations for SAS ports
  scsi: sd: Do not issue commands to suspended disks on shutdown
  ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
  ata: libata-eh: Fix compilation warning in ata_eh_link_report()
  scsi: Remove scsi device no_start_on_resume flag
  ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
  ata: libata-core: Synchronize ata_port_detach() with hotplug
  ata: libata-core: Detach a port devices on shutdown
  ata: libata-core: Remove ata_port_suspend_async()
  ata: libata-core: Remove ata_port_resume_async()
  ata: libata-core: Do not poweroff runtime suspended ports
  ata: libata-core: Do not resume runtime suspended ports
  ata: libata-sata: Improve ata_sas_slave_configure()
  ata: libata-eh: Improve reset error messages
  ata: libata-eh: Reduce "disable device" message verbosity
  ata: libata: Cleanup inline DMA helper functions

 drivers/ata/libata-core.c      | 242 +++++++++++++++++++++++++--------
 drivers/ata/libata-eh.c        |  76 +++++++++--
 drivers/ata/libata-sata.c      |   5 +-
 drivers/ata/libata-scsi.c      | 146 ++++++++++----------
 drivers/ata/libata-transport.c |   9 +-
 drivers/ata/libata.h           |   7 +
 drivers/firewire/sbp2.c        |   9 +-
 drivers/scsi/scsi_scan.c       |  18 ++-
 drivers/scsi/sd.c              |  88 ++++++++----
 include/linux/libata.h         |  26 ++--
 include/scsi/scsi_device.h     |   4 +-
 include/scsi/scsi_host.h       |   2 +-
 12 files changed, 444 insertions(+), 188 deletions(-)

Comments

Geert Uytterhoeven Sept. 21, 2023, 9:21 a.m. UTC | #1
Hi Damien,

On Wed, Sep 20, 2023 at 3:54 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> The first 9 patches of this series fix several issues with suspend/resume
> power management operations in scsi and libata. The most significant
> changes introduced are in patch 4 and 5, where the manage_start_stop
> flag of scsi devices is split into the manage_system_start_stop and
> manage_runtime_start_stop flags to allow keeping scsi runtime power
> operations for spining up/down ATA devices but have libata do its own
> system suspend/resume device power state management using EH.
>
> The remaining patches are code cleanup that do not introduce any
> significant functional change.
>
> This series was tested on qemu and on various PCs and servers. I am
> CC-ing people who recently reported issues with suspend/resume.
> Additional testing would be much appreciated.
>
> Changes from v3:
>  * Corrected pathc 1 (typo in commit message and WARN_ON() removal)
>  * Changed path 3 as suggested by Niklas (moved definition of
>    ->slave_alloc)
>  * Rebased on rc2
>  * Added review tags

Thanks for the update!

I gave this a try on Renesas Salvator-XS with R-Car H3 ES2.0 and
a SATA hard drive:
  - The drive is spun up during system resume,
  - Accessing the drive after the system was resumed is instantaneous.


Gr{oetje,eeting}s,

                        Geert
Geert Uytterhoeven Sept. 21, 2023, 9:22 a.m. UTC | #2
On Thu, Sep 21, 2023 at 11:21 AM Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Wed, Sep 20, 2023 at 3:54 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> > The first 9 patches of this series fix several issues with suspend/resume
> > power management operations in scsi and libata. The most significant
> > changes introduced are in patch 4 and 5, where the manage_start_stop
> > flag of scsi devices is split into the manage_system_start_stop and
> > manage_runtime_start_stop flags to allow keeping scsi runtime power
> > operations for spining up/down ATA devices but have libata do its own
> > system suspend/resume device power state management using EH.
> >
> > The remaining patches are code cleanup that do not introduce any
> > significant functional change.
> >
> > This series was tested on qemu and on various PCs and servers. I am
> > CC-ing people who recently reported issues with suspend/resume.
> > Additional testing would be much appreciated.
> >
> > Changes from v3:
> >  * Corrected pathc 1 (typo in commit message and WARN_ON() removal)
> >  * Changed path 3 as suggested by Niklas (moved definition of
> >    ->slave_alloc)
> >  * Rebased on rc2
> >  * Added review tags
>
> Thanks for the update!
>
> I gave this a try on Renesas Salvator-XS with R-Car H3 ES2.0 and
> a SATA hard drive:
>   - The drive is spun up during system resume,
>   - Accessing the drive after the system was resumed is instantaneous.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Gr{oetje,eeting}s,

                        Geert
Damien Le Moal Sept. 21, 2023, 5:30 p.m. UTC | #3
On 2023/09/21 2:22, Geert Uytterhoeven wrote:
> On Thu, Sep 21, 2023 at 11:21 AM Geert Uytterhoeven
> <geert@linux-m68k.org> wrote:
>> On Wed, Sep 20, 2023 at 3:54 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>> The first 9 patches of this series fix several issues with suspend/resume
>>> power management operations in scsi and libata. The most significant
>>> changes introduced are in patch 4 and 5, where the manage_start_stop
>>> flag of scsi devices is split into the manage_system_start_stop and
>>> manage_runtime_start_stop flags to allow keeping scsi runtime power
>>> operations for spining up/down ATA devices but have libata do its own
>>> system suspend/resume device power state management using EH.
>>>
>>> The remaining patches are code cleanup that do not introduce any
>>> significant functional change.
>>>
>>> This series was tested on qemu and on various PCs and servers. I am
>>> CC-ing people who recently reported issues with suspend/resume.
>>> Additional testing would be much appreciated.
>>>
>>> Changes from v3:
>>>  * Corrected pathc 1 (typo in commit message and WARN_ON() removal)
>>>  * Changed path 3 as suggested by Niklas (moved definition of
>>>    ->slave_alloc)
>>>  * Rebased on rc2
>>>  * Added review tags
>>
>> Thanks for the update!
>>
>> I gave this a try on Renesas Salvator-XS with R-Car H3 ES2.0 and
>> a SATA hard drive:
>>   - The drive is spun up during system resume,
>>   - Accessing the drive after the system was resumed is instantaneous.
> 
> Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks Geert !

> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Damien Le Moal Sept. 21, 2023, 7:53 p.m. UTC | #4
On 2023/09/20 6:54, Damien Le Moal wrote:
> The first 9 patches of this series fix several issues with suspend/resume
> power management operations in scsi and libata. The most significant
> changes introduced are in patch 4 and 5, where the manage_start_stop
> flag of scsi devices is split into the manage_system_start_stop and
> manage_runtime_start_stop flags to allow keeping scsi runtime power
> operations for spining up/down ATA devices but have libata do its own
> system suspend/resume device power state management using EH.
> 
> The remaining patches are code cleanup that do not introduce any
> significant functional change.

Martin,

Could you please review and Ack (if appropriate) the scsi bits of this series ?
I would like to send out the first 9 patches ASAP to address regressions.
Thanks !

> 
> This series was tested on qemu and on various PCs and servers. I am
> CC-ing people who recently reported issues with suspend/resume.
> Additional testing would be much appreciated.
> 
> Changes from v3:
>  * Corrected pathc 1 (typo in commit message and WARN_ON() removal)
>  * Changed path 3 as suggested by Niklas (moved definition of
>    ->slave_alloc)
>  * Rebased on rc2
>  * Added review tags
> 
> Changes from v2:
>  * Added patch 4 as simply disabling manage_start_stop from libata was
>    breaking individual disk runtime suspend/autosuspend. Patch 5 was
>    reworked accordingly to the changes in patch 4.
>  * Fixed patch 3: applying the link creation was missing and the link
>    creation itself was also incorrect, preventing sd probe to execute
>    correctly. Thanks to Geert for testing and reporting this issue.
>  * Split the "Fix delayed scsi_rescan_device() execution" patch into
>    patch 6 (scsi part) and patch 7 (ata part).
>  * Modified patch 9 to not call sd_shutdown() from sd_remove() for
>    devices that are not running.
>  * Added Chia-Lin Tested tag to unchanged patches
> 
> Changes from v1:
>  * Added patch 8 and 9 to fix compilation warnings with W=1
>  * Addressed John comment in patch 19
>  * Fixed patch 20 commit message (Sergei)
>  * Added Hannes Review tag
> 
> Damien Le Moal (23):
>   ata: libata-core: Fix ata_port_request_pm() locking
>   ata: libata-core: Fix port and device removal
>   ata: libata-scsi: link ata port and scsi device
>   scsi: sd: Differentiate system and runtime start/stop management
>   ata: libata-scsi: Disable scsi device manage_system_start_stop
>   scsi: Do not attempt to rescan suspended devices
>   ata: libata-scsi: Fix delayed scsi_rescan_device() execution
>   ata: libata-core: Do not register PM operations for SAS ports
>   scsi: sd: Do not issue commands to suspended disks on shutdown
>   ata: libata-core: Fix compilation warning in ata_dev_config_ncq()
>   ata: libata-eh: Fix compilation warning in ata_eh_link_report()
>   scsi: Remove scsi device no_start_on_resume flag
>   ata: libata-scsi: Cleanup ata_scsi_start_stop_xlat()
>   ata: libata-core: Synchronize ata_port_detach() with hotplug
>   ata: libata-core: Detach a port devices on shutdown
>   ata: libata-core: Remove ata_port_suspend_async()
>   ata: libata-core: Remove ata_port_resume_async()
>   ata: libata-core: Do not poweroff runtime suspended ports
>   ata: libata-core: Do not resume runtime suspended ports
>   ata: libata-sata: Improve ata_sas_slave_configure()
>   ata: libata-eh: Improve reset error messages
>   ata: libata-eh: Reduce "disable device" message verbosity
>   ata: libata: Cleanup inline DMA helper functions
> 
>  drivers/ata/libata-core.c      | 242 +++++++++++++++++++++++++--------
>  drivers/ata/libata-eh.c        |  76 +++++++++--
>  drivers/ata/libata-sata.c      |   5 +-
>  drivers/ata/libata-scsi.c      | 146 ++++++++++----------
>  drivers/ata/libata-transport.c |   9 +-
>  drivers/ata/libata.h           |   7 +
>  drivers/firewire/sbp2.c        |   9 +-
>  drivers/scsi/scsi_scan.c       |  18 ++-
>  drivers/scsi/sd.c              |  88 ++++++++----
>  include/linux/libata.h         |  26 ++--
>  include/scsi/scsi_device.h     |   4 +-
>  include/scsi/scsi_host.h       |   2 +-
>  12 files changed, 444 insertions(+), 188 deletions(-)
>