Fwd: Waking up from resume locks up on sr device

Fwd: Waking up from resume locks up on sr device

[Bagas Sanjaya]

Hi,

I notice a regression report on Bugzilla [1]. Quoting from it:

> I'm running LibreELEC.tv on an x86_64 machine that, following a (kernel) update, now locks up hard while trying to device_resume() => device_lock() on sr 2:0:0:0 (the only sr device in the system).
> 
> Through some digging of my own, I can pretty much isolate the fault in this device_lock() call:
> https://elixir.bootlin.com/linux/v6.3.4/source/drivers/base/power/main.c#L919
> 
> I put an additional debug line exactly before the device_lock(dev) call, like this:
> dev_info(dev, "device_lock() in device_resume()");
> 
> This is the last diagnostic I see, that device_lock() call never returns, ie line 920 in main.c is never reached (confirmed via TRACE_RESUME).
> The device, in my case, is printed as sr 2:0:0:0.
> 
> Knowing this, as a workaround, booting with libata.force=3:disable (libata port 3 corresponds to the SATA channel that sr 2:0:0:0 is attached to) allows suspend/resume to work correctly (but the optical drive is not accessible, obviously).
> 
> When resume hangs, the kernel is not _completely_ locked, interestingly the machine responds to pings and I see the e1000e 'link up' message a couple seconds after the hanging sr2 device_lock().
> Magic SysRq, however, does NOT work in that state; possibly because not enough of USB is resumed yet. Resuming devices seems to broadly follow a kind of breadth-first order; I see USB ports getting resumed closely before the lockup, but no USB (target) devices.
> 
> This is a regression, earlier kernels would work correctly on the exact same hardware. Since it's an 'embedded' type (LibreELEC.tv) install that overwrites its system parts completely on each update, I don't have a clear historical record of kernel versions. From the timeline and my memory, moving from 5.x to 6.x would make sense. Due to the nature of the system, it's somewhat inconvenient for me to try numerous kernel versions blindly for a bisection; I will try to test against some current 5.x soon, however.
> 
> I do have the hope that this information already might give someone with more background a strong idea about the issue.
> 
> Next, I will try to put debug_show_all_locks() before device_lock(), since I can't Alt+SysRq+d.

See Bugzilla for the full thread.

Anyway, I'm adding it to regzbot (with rough version range since the reporter
only knows major kernel version numbers):

#regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
#regzbot title: Waking up from resume locks up on SCSI CD/DVD drive

Thanks.

[1]: https://bugzilla.kernel.org/show_bug.cgi?id=217530

-- 
An old man doll... just what I always wanted! - Clara

Re: Fwd: Waking up from resume locks up on sr device

[Bagas Sanjaya]

On 6/9/23 18:04, Bagas Sanjaya wrote:
> Hi,
> 
> I notice a regression report on Bugzilla [1]. Quoting from it:
> 
>> I'm running LibreELEC.tv on an x86_64 machine that, following a (kernel) update, now locks up hard while trying to device_resume() => device_lock() on sr 2:0:0:0 (the only sr device in the system).
>>
>> Through some digging of my own, I can pretty much isolate the fault in this device_lock() call:
>> https://elixir.bootlin.com/linux/v6.3.4/source/drivers/base/power/main.c#L919
>>
>> I put an additional debug line exactly before the device_lock(dev) call, like this:
>> dev_info(dev, "device_lock() in device_resume()");
>>
>> This is the last diagnostic I see, that device_lock() call never returns, ie line 920 in main.c is never reached (confirmed via TRACE_RESUME).
>> The device, in my case, is printed as sr 2:0:0:0.
>>
>> Knowing this, as a workaround, booting with libata.force=3:disable (libata port 3 corresponds to the SATA channel that sr 2:0:0:0 is attached to) allows suspend/resume to work correctly (but the optical drive is not accessible, obviously).
>>
>> When resume hangs, the kernel is not _completely_ locked, interestingly the machine responds to pings and I see the e1000e 'link up' message a couple seconds after the hanging sr2 device_lock().
>> Magic SysRq, however, does NOT work in that state; possibly because not enough of USB is resumed yet. Resuming devices seems to broadly follow a kind of breadth-first order; I see USB ports getting resumed closely before the lockup, but no USB (target) devices.
>>
>> This is a regression, earlier kernels would work correctly on the exact same hardware. Since it's an 'embedded' type (LibreELEC.tv) install that overwrites its system parts completely on each update, I don't have a clear historical record of kernel versions. From the timeline and my memory, moving from 5.x to 6.x would make sense. Due to the nature of the system, it's somewhat inconvenient for me to try numerous kernel versions blindly for a bisection; I will try to test against some current 5.x soon, however.
>>
>> I do have the hope that this information already might give someone with more background a strong idea about the issue.
>>
>> Next, I will try to put debug_show_all_locks() before device_lock(), since I can't Alt+SysRq+d.
> 
> See Bugzilla for the full thread.
> 
> Anyway, I'm adding it to regzbot (with rough version range since the reporter
> only knows major kernel version numbers):
> 
> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
> 

The reporter had found the culprit (via bisection), so:

#regzbot introduced: a19a93e4c6a98c

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

Re: Fwd: Waking up from resume locks up on sr device

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

Hi!


> > #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
> > #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
> > 
> 
> The reporter had found the culprit (via bisection), so:
> 
> #regzbot introduced: a19a93e4c6a98c

Maybe cc the authors of that commit?

commit a19a93e4c6a98c9c0f2f5a6db76846f10d7d1f85
Author: Bart Van Assche <bvanassche@acm.org>
Date:   Wed Oct 6 14:54:51 2021 -0700

    scsi: core: pm: Rely on the device driver core for async power management
    
    Instead of implementing asynchronous resume support in the SCSI core, rely
    on the device driver core for resuming SCSI devices asynchronously.
    Instead of only supporting asynchronous resumes, also support asynchronous
    suspends.
    
    Link: https://lore.kernel.org/r/20211006215453.3318929-2-bvanassche@acm.org
    Cc: Alan Stern <stern@rowland.harvard.edu>
    Cc: Dan Williams <dan.j.williams@intel.com>
    Cc: Hannes Reinecke <hare@suse.com>
    Cc: Adrian Hunter <adrian.hunter@intel.com>
    Cc: Martin Kepplinger <martin.kepplinger@puri.sm>
    Signed-off-by: Bart Van Assche <bvanassche@acm.org>
    Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>

BR,
								Pavel
-- 
People of Russia, stop Putin before his war on Ukraine escalates.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: Fwd: Waking up from resume locks up on sr device

[Bagas Sanjaya]

On 6/10/23 15:55, Pavel Machek wrote:
> Hi!
> 
> 
>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>
>> The reporter had found the culprit (via bisection), so:
>>
>> #regzbot introduced: a19a93e4c6a98c
> Maybe cc the authors of that commit?
> 

Ah! I forgot to do that! Thanks anyway.

-- 
An old man doll... just what I always wanted! - Clara

Re: Fwd: Waking up from resume locks up on sr device

[Bart Van Assche]

On 6/10/23 06:27, Bagas Sanjaya wrote:
> On 6/10/23 15:55, Pavel Machek wrote:
>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>
>>> The reporter had found the culprit (via bisection), so:
>>>
>>> #regzbot introduced: a19a93e4c6a98c
>> Maybe cc the authors of that commit?
> 
> Ah! I forgot to do that! Thanks anyway.

Hi Damien,

Why does the ATA code call scsi_rescan_device() before system resume has
finished? Would ATA devices still work with the patch below applied?

Thanks,

Bart.


diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 6a959c993dd8..be3971b7fd27 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1629,6 +1629,20 @@ void scsi_rescan_device(struct device *dev)
  {
  	struct scsi_device *sdev = to_scsi_device(dev);

+#ifdef CONFIG_PM_SLEEP
+	/*
+	 * The ATA subsystem may call scsi_rescan_device() before resuming has
+	 * finished. If this happens, prevent a deadlock on the device_lock()
+	 * call by skipping rescanning.
+	 */
+	if (dev->power.is_suspended)
+		return;
+#endif
+
+	/*
+	 * Serialize scsi_driver.rescan() calls and scsi_driver.gendrv.remove()
+	 * calls.
+	 */
  	device_lock(dev);

  	scsi_attach_vpd(sdev);

Re: Fwd: Waking up from resume locks up on sr device

[Joe Breuer]

I'm the reporter of this issue.

I just tried this patch against 6.3.4, and it completely fixes my 
suspend/resume issue.

The optical drive stays usable after resume, even suspending/resuming 
during playback of CDDA content works flawlessly and playback resumes 
seamlessly after system resume.

So, from my perspective: Good one!


So long,
    Joe


On 10.06.23 17:03, Bart Van Assche wrote:
> On 6/10/23 06:27, Bagas Sanjaya wrote:
>> On 6/10/23 15:55, Pavel Machek wrote:
>>>>> #regzbot introduced: v5.0..v6.4-rc5 
>>>>> https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>>
>>>> The reporter had found the culprit (via bisection), so:
>>>>
>>>> #regzbot introduced: a19a93e4c6a98c
>>> Maybe cc the authors of that commit?
>>
>> Ah! I forgot to do that! Thanks anyway.
> 
> Hi Damien,
> 
> Why does the ATA code call scsi_rescan_device() before system resume has
> finished? Would ATA devices still work with the patch below applied?
> 
> Thanks,
> 
> Bart.
> 
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a959c993dd8..be3971b7fd27 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1629,6 +1629,20 @@ void scsi_rescan_device(struct device *dev)
>   {
>       struct scsi_device *sdev = to_scsi_device(dev);
> 
> +#ifdef CONFIG_PM_SLEEP
> +    /*
> +     * The ATA subsystem may call scsi_rescan_device() before resuming has
> +     * finished. If this happens, prevent a deadlock on the device_lock()
> +     * call by skipping rescanning.
> +     */
> +    if (dev->power.is_suspended)
> +        return;
> +#endif
> +
> +    /*
> +     * Serialize scsi_driver.rescan() calls and 
> scsi_driver.gendrv.remove()
> +     * calls.
> +     */
>       device_lock(dev);
> 
>       scsi_attach_vpd(sdev);
> 

Re: Fwd: Waking up from resume locks up on sr device

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1043 bytes --]

On Sun, Jun 11, 2023 at 11:05:27AM +0200, Joe Breuer wrote:
> I'm the reporter of this issue.
> 
> I just tried this patch against 6.3.4, and it completely fixes my
> suspend/resume issue.
> 
> The optical drive stays usable after resume, even suspending/resuming during
> playback of CDDA content works flawlessly and playback resumes seamlessly
> after system resume.
> 
> So, from my perspective: Good one!
> 

Thanks for trying the fix and telling the result. But tl;dr:

> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
> 
> A: No.
> Q: Should I include quotations after my reply?
> 
> http://daringfireball.net/2007/07/on_top

(while I'm removing the quoted context below your reply after the fact).

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/11/23 18:05, Joe Breuer wrote:
> I'm the reporter of this issue.
> 
> I just tried this patch against 6.3.4, and it completely fixes my
> suspend/resume issue.
> 
> The optical drive stays usable after resume, even suspending/resuming
> during playback of CDDA content works flawlessly and playback resumes
> seamlessly after system resume.
> 
> So, from my perspective: Good one!

In place of Bart's fix, could you please try this patch ?

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index b80e68000dd3..a81eb4f882ab 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
ata_port *ap)
        /* tell ACPI that we're resuming */
        ata_acpi_on_resume(ap);

-       /* update the flags */
        spin_lock_irqsave(ap->lock, flags);
+
+       /* Update the flags */
        ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
+
+       /*
+        * Resuming the port will trigger a rescan of the ATA device(s)
+        * connected to it. Before scheduling the rescan, make sure that
+        * the associated scsi device(s) are fully resumed as well.
+        */
+       ata_for_each_link(link, ap, HOST_FIRST) {
+               ata_for_each_dev(dev, link, ENABLED) {
+                       struct scsi_device *sdev = dev->sdev;
+
+                       if (!sdev)
+                               continue;
+                       if (scsi_device_get(sdev))
+                               continue;
+
+                       spin_unlock_irqrestore(ap->lock, flags);
+                       device_pm_wait_for_dev(&ap->tdev,
+                                              &sdev->sdev_gendev);
+                       scsi_device_put(sdev);
+                       spin_lock_irqsave(ap->lock, flags);
+               }
+       }
        spin_unlock_irqrestore(ap->lock, flags);
 }
 #endif /* CONFIG_PM */

Thanks !

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Kai-Heng Feng]

On Wed, Jun 14, 2023 at 12:49 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 6/11/23 18:05, Joe Breuer wrote:
> > I'm the reporter of this issue.
> >
> > I just tried this patch against 6.3.4, and it completely fixes my
> > suspend/resume issue.
> >
> > The optical drive stays usable after resume, even suspending/resuming
> > during playback of CDDA content works flawlessly and playback resumes
> > seamlessly after system resume.
> >
> > So, from my perspective: Good one!
>
> In place of Bart's fix, could you please try this patch ?

Issue still persists at my end, when /sys/power/pm_async is 0.
device_pm_wait_for_dev() in its current form is only usable for async case.

Kai-Heng

>
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index b80e68000dd3..a81eb4f882ab 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
> ata_port *ap)
>         /* tell ACPI that we're resuming */
>         ata_acpi_on_resume(ap);
>
> -       /* update the flags */
>         spin_lock_irqsave(ap->lock, flags);
> +
> +       /* Update the flags */
>         ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
> +
> +       /*
> +        * Resuming the port will trigger a rescan of the ATA device(s)
> +        * connected to it. Before scheduling the rescan, make sure that
> +        * the associated scsi device(s) are fully resumed as well.
> +        */
> +       ata_for_each_link(link, ap, HOST_FIRST) {
> +               ata_for_each_dev(dev, link, ENABLED) {
> +                       struct scsi_device *sdev = dev->sdev;
> +
> +                       if (!sdev)
> +                               continue;
> +                       if (scsi_device_get(sdev))
> +                               continue;
> +
> +                       spin_unlock_irqrestore(ap->lock, flags);
> +                       device_pm_wait_for_dev(&ap->tdev,
> +                                              &sdev->sdev_gendev);
> +                       scsi_device_put(sdev);
> +                       spin_lock_irqsave(ap->lock, flags);
> +               }
> +       }
>         spin_unlock_irqrestore(ap->lock, flags);
>  }
>  #endif /* CONFIG_PM */
>
> Thanks !
>
> --
> Damien Le Moal
> Western Digital Research
>

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/14/23 14:37, Kai-Heng Feng wrote:
> On Wed, Jun 14, 2023 at 12:49 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> On 6/11/23 18:05, Joe Breuer wrote:
>>> I'm the reporter of this issue.
>>>
>>> I just tried this patch against 6.3.4, and it completely fixes my
>>> suspend/resume issue.
>>>
>>> The optical drive stays usable after resume, even suspending/resuming
>>> during playback of CDDA content works flawlessly and playback resumes
>>> seamlessly after system resume.
>>>
>>> So, from my perspective: Good one!
>>
>> In place of Bart's fix, could you please try this patch ?
> 
> Issue still persists at my end, when /sys/power/pm_async is 0.
> device_pm_wait_for_dev() in its current form is only usable for async case.

OK. Thanks for checking. Let me dig further.

> 
> Kai-Heng
> 
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index b80e68000dd3..a81eb4f882ab 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
>> ata_port *ap)
>>         /* tell ACPI that we're resuming */
>>         ata_acpi_on_resume(ap);
>>
>> -       /* update the flags */
>>         spin_lock_irqsave(ap->lock, flags);
>> +
>> +       /* Update the flags */
>>         ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
>> +
>> +       /*
>> +        * Resuming the port will trigger a rescan of the ATA device(s)
>> +        * connected to it. Before scheduling the rescan, make sure that
>> +        * the associated scsi device(s) are fully resumed as well.
>> +        */
>> +       ata_for_each_link(link, ap, HOST_FIRST) {
>> +               ata_for_each_dev(dev, link, ENABLED) {
>> +                       struct scsi_device *sdev = dev->sdev;
>> +
>> +                       if (!sdev)
>> +                               continue;
>> +                       if (scsi_device_get(sdev))
>> +                               continue;
>> +
>> +                       spin_unlock_irqrestore(ap->lock, flags);
>> +                       device_pm_wait_for_dev(&ap->tdev,
>> +                                              &sdev->sdev_gendev);
>> +                       scsi_device_put(sdev);
>> +                       spin_lock_irqsave(ap->lock, flags);
>> +               }
>> +       }
>>         spin_unlock_irqrestore(ap->lock, flags);
>>  }
>>  #endif /* CONFIG_PM */
>>
>> Thanks !
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>>

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/14/23 14:37, Kai-Heng Feng wrote:
> On Wed, Jun 14, 2023 at 12:49 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> On 6/11/23 18:05, Joe Breuer wrote:
>>> I'm the reporter of this issue.
>>>
>>> I just tried this patch against 6.3.4, and it completely fixes my
>>> suspend/resume issue.
>>>
>>> The optical drive stays usable after resume, even suspending/resuming
>>> during playback of CDDA content works flawlessly and playback resumes
>>> seamlessly after system resume.
>>>
>>> So, from my perspective: Good one!
>>
>> In place of Bart's fix, could you please try this patch ?
> 
> Issue still persists at my end, when /sys/power/pm_async is 0.
> device_pm_wait_for_dev() in its current form is only usable for async case.

Can you try this horrible hack to see if it works:

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 9e79998e3958..87c093775a90 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4590,6 +4590,7 @@ void ata_scsi_dev_rescan(struct work_struct *work)
        ata_for_each_link(link, ap, EDGE) {
                ata_for_each_dev(dev, link, ENABLED) {
                        struct scsi_device *sdev = dev->sdev;
+                       struct device *sgdev;

                        if (!sdev)
                                continue;
@@ -4597,7 +4598,10 @@ void ata_scsi_dev_rescan(struct work_struct *work)
                                continue;

                        spin_unlock_irqrestore(ap->lock, flags);
-                       scsi_rescan_device(&(sdev->sdev_gendev));
+
+                       sgdev = &sdev->sdev_gendev;
+                       wait_for_completion(&sgdev->power.completion);
+                       scsi_rescan_device(sgdev);
                        scsi_device_put(sdev);
                        spin_lock_irqsave(ap->lock, flags);
                }



> 
> Kai-Heng
> 
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index b80e68000dd3..a81eb4f882ab 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
>> ata_port *ap)
>>         /* tell ACPI that we're resuming */
>>         ata_acpi_on_resume(ap);
>>
>> -       /* update the flags */
>>         spin_lock_irqsave(ap->lock, flags);
>> +
>> +       /* Update the flags */
>>         ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
>> +
>> +       /*
>> +        * Resuming the port will trigger a rescan of the ATA device(s)
>> +        * connected to it. Before scheduling the rescan, make sure that
>> +        * the associated scsi device(s) are fully resumed as well.
>> +        */
>> +       ata_for_each_link(link, ap, HOST_FIRST) {
>> +               ata_for_each_dev(dev, link, ENABLED) {
>> +                       struct scsi_device *sdev = dev->sdev;
>> +
>> +                       if (!sdev)
>> +                               continue;
>> +                       if (scsi_device_get(sdev))
>> +                               continue;
>> +
>> +                       spin_unlock_irqrestore(ap->lock, flags);
>> +                       device_pm_wait_for_dev(&ap->tdev,
>> +                                              &sdev->sdev_gendev);
>> +                       scsi_device_put(sdev);
>> +                       spin_lock_irqsave(ap->lock, flags);
>> +               }
>> +       }
>>         spin_unlock_irqrestore(ap->lock, flags);
>>  }
>>  #endif /* CONFIG_PM */
>>
>> Thanks !
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>>

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Hannes Reinecke]

On 6/14/23 06:49, Damien Le Moal wrote:
> On 6/11/23 18:05, Joe Breuer wrote:
>> I'm the reporter of this issue.
>>
>> I just tried this patch against 6.3.4, and it completely fixes my
>> suspend/resume issue.
>>
>> The optical drive stays usable after resume, even suspending/resuming
>> during playback of CDDA content works flawlessly and playback resumes
>> seamlessly after system resume.
>>
>> So, from my perspective: Good one!
> 
> In place of Bart's fix, could you please try this patch ?
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index b80e68000dd3..a81eb4f882ab 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
> ata_port *ap)
>          /* tell ACPI that we're resuming */
>          ata_acpi_on_resume(ap);
> 
> -       /* update the flags */
>          spin_lock_irqsave(ap->lock, flags);
> +
> +       /* Update the flags */
>          ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
> +
> +       /*
> +        * Resuming the port will trigger a rescan of the ATA device(s)
> +        * connected to it. Before scheduling the rescan, make sure that
> +        * the associated scsi device(s) are fully resumed as well.
> +        */
> +       ata_for_each_link(link, ap, HOST_FIRST) {
> +               ata_for_each_dev(dev, link, ENABLED) {
> +                       struct scsi_device *sdev = dev->sdev;
> +
> +                       if (!sdev)
> +                               continue;
> +                       if (scsi_device_get(sdev))
> +                               continue;
> +
> +                       spin_unlock_irqrestore(ap->lock, flags);
> +                       device_pm_wait_for_dev(&ap->tdev,
> +                                              &sdev->sdev_gendev);
> +                       scsi_device_put(sdev);
> +                       spin_lock_irqsave(ap->lock, flags);
> +               }
> +       }
>          spin_unlock_irqrestore(ap->lock, flags);
>   }
>   #endif /* CONFIG_PM */
> 
> Thanks !
> 
Well; not sure if that'll work out.
The whole reason why we initial a rescan is that we need to check if the 
ports are still connected, and whether the devices react.
So we can't iterate the ports here as this is the very thing which gets 
checked during EH.

We really should claim resume to be finished as soon as we can talk with 
the HBA, and kick off EH asynchronously to let it finish the job after 
resume has completed.

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

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/14/23 15:57, Hannes Reinecke wrote:
> On 6/14/23 06:49, Damien Le Moal wrote:
>> On 6/11/23 18:05, Joe Breuer wrote:
>>> I'm the reporter of this issue.
>>>
>>> I just tried this patch against 6.3.4, and it completely fixes my
>>> suspend/resume issue.
>>>
>>> The optical drive stays usable after resume, even suspending/resuming
>>> during playback of CDDA content works flawlessly and playback resumes
>>> seamlessly after system resume.
>>>
>>> So, from my perspective: Good one!
>>
>> In place of Bart's fix, could you please try this patch ?
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index b80e68000dd3..a81eb4f882ab 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
>> ata_port *ap)
>>          /* tell ACPI that we're resuming */
>>          ata_acpi_on_resume(ap);
>>
>> -       /* update the flags */
>>          spin_lock_irqsave(ap->lock, flags);
>> +
>> +       /* Update the flags */
>>          ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
>> +
>> +       /*
>> +        * Resuming the port will trigger a rescan of the ATA device(s)
>> +        * connected to it. Before scheduling the rescan, make sure that
>> +        * the associated scsi device(s) are fully resumed as well.
>> +        */
>> +       ata_for_each_link(link, ap, HOST_FIRST) {
>> +               ata_for_each_dev(dev, link, ENABLED) {
>> +                       struct scsi_device *sdev = dev->sdev;
>> +
>> +                       if (!sdev)
>> +                               continue;
>> +                       if (scsi_device_get(sdev))
>> +                               continue;
>> +
>> +                       spin_unlock_irqrestore(ap->lock, flags);
>> +                       device_pm_wait_for_dev(&ap->tdev,
>> +                                              &sdev->sdev_gendev);
>> +                       scsi_device_put(sdev);
>> +                       spin_lock_irqsave(ap->lock, flags);
>> +               }
>> +       }
>>          spin_unlock_irqrestore(ap->lock, flags);
>>   }
>>   #endif /* CONFIG_PM */
>>
>> Thanks !
>>
> Well; not sure if that'll work out.
> The whole reason why we initial a rescan is that we need to check if the
> ports are still connected, and whether the devices react.
> So we can't iterate the ports here as this is the very thing which gets
> checked during EH.

Hmmm... Right. So we need to move that loop into ata_scsi_dev_rescan(),
which itself already loops over the port devices anyway.

> We really should claim resume to be finished as soon as we can talk with
> the HBA, and kick off EH asynchronously to let it finish the job after
> resume has completed.

That is what's done already:

static int ata_port_pm_resume(struct device *dev)
{
	ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
	pm_runtime_disable(dev);
	pm_runtime_set_active(dev);
	pm_runtime_enable(dev);
	return 0;
}

EH is kicked by ata_port_resume_async() -> ata_port_request_pm() and it
is async. There is no synchronization in EH with the PM side though. We
probably should have EH check that the port resume is done first, which
can be done in ata_eh_handle_port_resume() since that is the first thing
done when entering EH.

The problem remains though that we *must* wait for the scsi device
resume to be done before calling scsi_rescan_device(), which is done
asynchronously from EH, as a different work. So that one needs to wait
for the scsi side resume to be done.

I also thought of trigerring the rescan from the scsi side, but since
the resume may be asynchronous, we could endup trigerring it with the
ata side not yet resumed... That would only turn the problem around
instead of solving it.

Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
is the only place in scsi code I can see that takes this lock. I suspect
this is to serialize either rescans, or serialize with resume, or both.
For serializing rescans, we can use another lock. For serializing with
PM, we should wait for PM transitions...
Something is not right here.

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Alan Stern]

On Wed, Jun 14, 2023 at 04:35:50PM +0900, Damien Le Moal wrote:
> On 6/14/23 15:57, Hannes Reinecke wrote:
> > On 6/14/23 06:49, Damien Le Moal wrote:
> >> On 6/11/23 18:05, Joe Breuer wrote:
> >>> I'm the reporter of this issue.
> >>>
> >>> I just tried this patch against 6.3.4, and it completely fixes my
> >>> suspend/resume issue.
> >>>
> >>> The optical drive stays usable after resume, even suspending/resuming
> >>> during playback of CDDA content works flawlessly and playback resumes
> >>> seamlessly after system resume.
> >>>
> >>> So, from my perspective: Good one!
> >>
> >> In place of Bart's fix, could you please try this patch ?
> >>
> >> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> >> index b80e68000dd3..a81eb4f882ab 100644
> >> --- a/drivers/ata/libata-eh.c
> >> +++ b/drivers/ata/libata-eh.c
> >> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
> >> ata_port *ap)
> >>          /* tell ACPI that we're resuming */
> >>          ata_acpi_on_resume(ap);
> >>
> >> -       /* update the flags */
> >>          spin_lock_irqsave(ap->lock, flags);
> >> +
> >> +       /* Update the flags */
> >>          ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
> >> +
> >> +       /*
> >> +        * Resuming the port will trigger a rescan of the ATA device(s)
> >> +        * connected to it. Before scheduling the rescan, make sure that
> >> +        * the associated scsi device(s) are fully resumed as well.
> >> +        */
> >> +       ata_for_each_link(link, ap, HOST_FIRST) {
> >> +               ata_for_each_dev(dev, link, ENABLED) {
> >> +                       struct scsi_device *sdev = dev->sdev;
> >> +
> >> +                       if (!sdev)
> >> +                               continue;
> >> +                       if (scsi_device_get(sdev))
> >> +                               continue;
> >> +
> >> +                       spin_unlock_irqrestore(ap->lock, flags);
> >> +                       device_pm_wait_for_dev(&ap->tdev,
> >> +                                              &sdev->sdev_gendev);
> >> +                       scsi_device_put(sdev);
> >> +                       spin_lock_irqsave(ap->lock, flags);
> >> +               }
> >> +       }
> >>          spin_unlock_irqrestore(ap->lock, flags);
> >>   }
> >>   #endif /* CONFIG_PM */
> >>
> >> Thanks !
> >>
> > Well; not sure if that'll work out.
> > The whole reason why we initial a rescan is that we need to check if the
> > ports are still connected, and whether the devices react.
> > So we can't iterate the ports here as this is the very thing which gets
> > checked during EH.
> 
> Hmmm... Right. So we need to move that loop into ata_scsi_dev_rescan(),
> which itself already loops over the port devices anyway.
> 
> > We really should claim resume to be finished as soon as we can talk with
> > the HBA, and kick off EH asynchronously to let it finish the job after
> > resume has completed.
> 
> That is what's done already:
> 
> static int ata_port_pm_resume(struct device *dev)
> {
> 	ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
> 	pm_runtime_disable(dev);
> 	pm_runtime_set_active(dev);
> 	pm_runtime_enable(dev);
> 	return 0;
> }
> 
> EH is kicked by ata_port_resume_async() -> ata_port_request_pm() and it
> is async. There is no synchronization in EH with the PM side though. We
> probably should have EH check that the port resume is done first, which
> can be done in ata_eh_handle_port_resume() since that is the first thing
> done when entering EH.
> 
> The problem remains though that we *must* wait for the scsi device
> resume to be done before calling scsi_rescan_device(), which is done
> asynchronously from EH, as a different work. So that one needs to wait
> for the scsi side resume to be done.
> 
> I also thought of trigerring the rescan from the scsi side, but since
> the resume may be asynchronous, we could endup trigerring it with the
> ata side not yet resumed... That would only turn the problem around
> instead of solving it.

The order in which devices get resumed isn't arbitrary.  If the system 
is set up not to use async suspends/resumes then the order is always the 
same as the order in which the devices were originally registered (for 
resume, that is -- suspend obviously takes place in the reverse order).

So if you're trying to perform an action that requires two devices to be 
active, you must not do it in the resume handler for the device that was 
registered first.  I don't know how the ATA and SCSI pieces interact 
here, but regardless, this is a pretty strict requirement.

It should be okay to perform the action in the resume handler for the 
device that was registered second.  But if the two devices aren't in an 
ancestor-descendant relationship then you also have to call 
device_pm_wait_for_dev() (or use device links as Rafael mentioned) to 
handle the async case properly.

> Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
> is the only place in scsi code I can see that takes this lock. I suspect
> this is to serialize either rescans, or serialize with resume, or both.
> For serializing rescans, we can use another lock. For serializing with
> PM, we should wait for PM transitions...
> Something is not right here.

Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against 
->remove", written by Christoph Hellwig) says:

    Lock the device embedded in the scsi_device to protect against
    concurrent calls to ->remove.

That's the commit which added the device_lock() call.

Alan Stern

Re: Fwd: Waking up from resume locks up on sr device

[Rafael J. Wysocki]

On Wed, Jun 14, 2023 at 4:26 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Wed, Jun 14, 2023 at 04:35:50PM +0900, Damien Le Moal wrote:
> > On 6/14/23 15:57, Hannes Reinecke wrote:
> > > On 6/14/23 06:49, Damien Le Moal wrote:
> > >> On 6/11/23 18:05, Joe Breuer wrote:
> > >>> I'm the reporter of this issue.
> > >>>
> > >>> I just tried this patch against 6.3.4, and it completely fixes my
> > >>> suspend/resume issue.
> > >>>
> > >>> The optical drive stays usable after resume, even suspending/resuming
> > >>> during playback of CDDA content works flawlessly and playback resumes
> > >>> seamlessly after system resume.
> > >>>
> > >>> So, from my perspective: Good one!
> > >>
> > >> In place of Bart's fix, could you please try this patch ?
> > >>
> > >> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > >> index b80e68000dd3..a81eb4f882ab 100644
> > >> --- a/drivers/ata/libata-eh.c
> > >> +++ b/drivers/ata/libata-eh.c
> > >> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
> > >> ata_port *ap)
> > >>          /* tell ACPI that we're resuming */
> > >>          ata_acpi_on_resume(ap);
> > >>
> > >> -       /* update the flags */
> > >>          spin_lock_irqsave(ap->lock, flags);
> > >> +
> > >> +       /* Update the flags */
> > >>          ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
> > >> +
> > >> +       /*
> > >> +        * Resuming the port will trigger a rescan of the ATA device(s)
> > >> +        * connected to it. Before scheduling the rescan, make sure that
> > >> +        * the associated scsi device(s) are fully resumed as well.
> > >> +        */
> > >> +       ata_for_each_link(link, ap, HOST_FIRST) {
> > >> +               ata_for_each_dev(dev, link, ENABLED) {
> > >> +                       struct scsi_device *sdev = dev->sdev;
> > >> +
> > >> +                       if (!sdev)
> > >> +                               continue;
> > >> +                       if (scsi_device_get(sdev))
> > >> +                               continue;
> > >> +
> > >> +                       spin_unlock_irqrestore(ap->lock, flags);
> > >> +                       device_pm_wait_for_dev(&ap->tdev,
> > >> +                                              &sdev->sdev_gendev);
> > >> +                       scsi_device_put(sdev);
> > >> +                       spin_lock_irqsave(ap->lock, flags);
> > >> +               }
> > >> +       }
> > >>          spin_unlock_irqrestore(ap->lock, flags);
> > >>   }
> > >>   #endif /* CONFIG_PM */
> > >>
> > >> Thanks !
> > >>
> > > Well; not sure if that'll work out.
> > > The whole reason why we initial a rescan is that we need to check if the
> > > ports are still connected, and whether the devices react.
> > > So we can't iterate the ports here as this is the very thing which gets
> > > checked during EH.
> >
> > Hmmm... Right. So we need to move that loop into ata_scsi_dev_rescan(),
> > which itself already loops over the port devices anyway.
> >
> > > We really should claim resume to be finished as soon as we can talk with
> > > the HBA, and kick off EH asynchronously to let it finish the job after
> > > resume has completed.
> >
> > That is what's done already:
> >
> > static int ata_port_pm_resume(struct device *dev)
> > {
> >       ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
> >       pm_runtime_disable(dev);
> >       pm_runtime_set_active(dev);
> >       pm_runtime_enable(dev);
> >       return 0;
> > }
> >
> > EH is kicked by ata_port_resume_async() -> ata_port_request_pm() and it
> > is async. There is no synchronization in EH with the PM side though. We
> > probably should have EH check that the port resume is done first, which
> > can be done in ata_eh_handle_port_resume() since that is the first thing
> > done when entering EH.
> >
> > The problem remains though that we *must* wait for the scsi device
> > resume to be done before calling scsi_rescan_device(), which is done
> > asynchronously from EH, as a different work. So that one needs to wait
> > for the scsi side resume to be done.
> >
> > I also thought of trigerring the rescan from the scsi side, but since
> > the resume may be asynchronous, we could endup trigerring it with the
> > ata side not yet resumed... That would only turn the problem around
> > instead of solving it.
>
> The order in which devices get resumed isn't arbitrary.  If the system
> is set up not to use async suspends/resumes then the order is always the
> same as the order in which the devices were originally registered (for
> resume, that is -- suspend obviously takes place in the reverse order).
>
> So if you're trying to perform an action that requires two devices to be
> active, you must not do it in the resume handler for the device that was
> registered first.  I don't know how the ATA and SCSI pieces interact
> here, but regardless, this is a pretty strict requirement.
>
> It should be okay to perform the action in the resume handler for the
> device that was registered second.  But if the two devices aren't in an
> ancestor-descendant relationship then you also have to call
> device_pm_wait_for_dev() (or use device links as Rafael mentioned) to
> handle the async case properly.

Note that the bare device_pm_wait_for_dev() is a bit risky though,
because in the sync case it will deadlock if dpm_list is not ordered
properly.

One of the things taken care of by device_link_add() is to ensure that
the ordering of dpm_list will reflect the dependency represented by
the given new device link.

> > Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
> > is the only place in scsi code I can see that takes this lock. I suspect
> > this is to serialize either rescans, or serialize with resume, or both.
> > For serializing rescans, we can use another lock. For serializing with
> > PM, we should wait for PM transitions...
> > Something is not right here.
>
> Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against
> ->remove", written by Christoph Hellwig) says:
>
>     Lock the device embedded in the scsi_device to protect against
>     concurrent calls to ->remove.
>
> That's the commit which added the device_lock() call.
>
> Alan Stern

Re: Fwd: Waking up from resume locks up on sr device

[Bart Van Assche]

On 6/14/23 07:26, Alan Stern wrote:
> On Wed, Jun 14, 2023 at 04:35:50PM +0900, Damien Le Moal wrote:
>> Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
>> is the only place in scsi code I can see that takes this lock. I suspect
>> this is to serialize either rescans, or serialize with resume, or both.
>> For serializing rescans, we can use another lock. For serializing with
>> PM, we should wait for PM transitions...
>> Something is not right here.
> 
> Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against
> ->remove", written by Christoph Hellwig) says:
> 
>      Lock the device embedded in the scsi_device to protect against
>      concurrent calls to ->remove.
> 
> That's the commit which added the device_lock() call.

Even if scsi_rescan_device() would use another mechanism for 
serialization against sd_remove() and sr_remove(), we still need to 
solve the issue that the ATA code calls scsi_rescan_device() before 
resuming has finished. scsi_rescan_device() issues I/O. Issuing I/O to a 
device is not allowed before that device has been resumed.

Thanks,

Bart.

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/15/23 03:04, Bart Van Assche wrote:
> On 6/14/23 07:26, Alan Stern wrote:
>> On Wed, Jun 14, 2023 at 04:35:50PM +0900, Damien Le Moal wrote:
>>> Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
>>> is the only place in scsi code I can see that takes this lock. I suspect
>>> this is to serialize either rescans, or serialize with resume, or both.
>>> For serializing rescans, we can use another lock. For serializing with
>>> PM, we should wait for PM transitions...
>>> Something is not right here.
>>
>> Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against
>> ->remove", written by Christoph Hellwig) says:
>>
>>      Lock the device embedded in the scsi_device to protect against
>>      concurrent calls to ->remove.
>>
>> That's the commit which added the device_lock() call.
> 
> Even if scsi_rescan_device() would use another mechanism for 
> serialization against sd_remove() and sr_remove(), we still need to 
> solve the issue that the ATA code calls scsi_rescan_device() before 
> resuming has finished. scsi_rescan_device() issues I/O. Issuing I/O to a 
> device is not allowed before that device has been resumed.

I am not convinced of that: scsi suspend quiecse the queue, thus preventing IOs
from the block layer, but not internale scsi ml commands, which is what
scsi_rescan_device() issues.

In any case, I am thinking that best (and quickest) fix for this issue for now
is to have libata define a device link to make the scsi device a "parent" of the
ata device (which is the ata link as of now). This way, PM operation ordering
will ensure that the scsi device resume will be done before the ata device. What
I really do not like about this though is that the suspend side would be done in
the reverse order: ata first and then scsi, but we really want the reverse here
to ensure that the request queue is quiesced before we suspend ata. That said,
there is no such synchronization right now and so this is probably happening
already without raising issues apparently.

So ideally:
1) Make the ata device the parent of the scsi device using a device link
2) For suspend, the scsi device suspend will be done first, followed by the ata
device, which is what we want.
3) For resume, ata device will be first, followed by scsi device. The call to
scsi_rescan_device() from libata being in a work task, asynchronous from the ata
resume context, we need to synchronize that work to wait for the scsi device
resume to complete. (but do we really given that we are going to issue internal
commands only ?)

Alan, Rafael,

For the synchronization of step (3), if I understand the pm code correctly,
using device_pm_wait_for_dev() would work only if async resume is on. This would
be ineffective for the sync case. How can we best deal with this ?


-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/14/23 23:26, Alan Stern wrote:
> On Wed, Jun 14, 2023 at 04:35:50PM +0900, Damien Le Moal wrote:
>> On 6/14/23 15:57, Hannes Reinecke wrote:
>>> On 6/14/23 06:49, Damien Le Moal wrote:
>>>> On 6/11/23 18:05, Joe Breuer wrote:
>>>>> I'm the reporter of this issue.
>>>>>
>>>>> I just tried this patch against 6.3.4, and it completely fixes my
>>>>> suspend/resume issue.
>>>>>
>>>>> The optical drive stays usable after resume, even suspending/resuming
>>>>> during playback of CDDA content works flawlessly and playback resumes
>>>>> seamlessly after system resume.
>>>>>
>>>>> So, from my perspective: Good one!
>>>>
>>>> In place of Bart's fix, could you please try this patch ?
>>>>
>>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>>> index b80e68000dd3..a81eb4f882ab 100644
>>>> --- a/drivers/ata/libata-eh.c
>>>> +++ b/drivers/ata/libata-eh.c
>>>> @@ -4006,9 +4006,32 @@ static void ata_eh_handle_port_resume(struct
>>>> ata_port *ap)
>>>>          /* tell ACPI that we're resuming */
>>>>          ata_acpi_on_resume(ap);
>>>>
>>>> -       /* update the flags */
>>>>          spin_lock_irqsave(ap->lock, flags);
>>>> +
>>>> +       /* Update the flags */
>>>>          ap->pflags &= ~(ATA_PFLAG_PM_PENDING | ATA_PFLAG_SUSPENDED);
>>>> +
>>>> +       /*
>>>> +        * Resuming the port will trigger a rescan of the ATA device(s)
>>>> +        * connected to it. Before scheduling the rescan, make sure that
>>>> +        * the associated scsi device(s) are fully resumed as well.
>>>> +        */
>>>> +       ata_for_each_link(link, ap, HOST_FIRST) {
>>>> +               ata_for_each_dev(dev, link, ENABLED) {
>>>> +                       struct scsi_device *sdev = dev->sdev;
>>>> +
>>>> +                       if (!sdev)
>>>> +                               continue;
>>>> +                       if (scsi_device_get(sdev))
>>>> +                               continue;
>>>> +
>>>> +                       spin_unlock_irqrestore(ap->lock, flags);
>>>> +                       device_pm_wait_for_dev(&ap->tdev,
>>>> +                                              &sdev->sdev_gendev);
>>>> +                       scsi_device_put(sdev);
>>>> +                       spin_lock_irqsave(ap->lock, flags);
>>>> +               }
>>>> +       }
>>>>          spin_unlock_irqrestore(ap->lock, flags);
>>>>   }
>>>>   #endif /* CONFIG_PM */
>>>>
>>>> Thanks !
>>>>
>>> Well; not sure if that'll work out.
>>> The whole reason why we initial a rescan is that we need to check if the
>>> ports are still connected, and whether the devices react.
>>> So we can't iterate the ports here as this is the very thing which gets
>>> checked during EH.
>>
>> Hmmm... Right. So we need to move that loop into ata_scsi_dev_rescan(),
>> which itself already loops over the port devices anyway.
>>
>>> We really should claim resume to be finished as soon as we can talk with
>>> the HBA, and kick off EH asynchronously to let it finish the job after
>>> resume has completed.
>>
>> That is what's done already:
>>
>> static int ata_port_pm_resume(struct device *dev)
>> {
>> 	ata_port_resume_async(to_ata_port(dev), PMSG_RESUME);
>> 	pm_runtime_disable(dev);
>> 	pm_runtime_set_active(dev);
>> 	pm_runtime_enable(dev);
>> 	return 0;
>> }
>>
>> EH is kicked by ata_port_resume_async() -> ata_port_request_pm() and it
>> is async. There is no synchronization in EH with the PM side though. We
>> probably should have EH check that the port resume is done first, which
>> can be done in ata_eh_handle_port_resume() since that is the first thing
>> done when entering EH.
>>
>> The problem remains though that we *must* wait for the scsi device
>> resume to be done before calling scsi_rescan_device(), which is done
>> asynchronously from EH, as a different work. So that one needs to wait
>> for the scsi side resume to be done.
>>
>> I also thought of trigerring the rescan from the scsi side, but since
>> the resume may be asynchronous, we could endup trigerring it with the
>> ata side not yet resumed... That would only turn the problem around
>> instead of solving it.
> 
> The order in which devices get resumed isn't arbitrary.  If the system 
> is set up not to use async suspends/resumes then the order is always the 
> same as the order in which the devices were originally registered (for 
> resume, that is -- suspend obviously takes place in the reverse order).
> 
> So if you're trying to perform an action that requires two devices to be 
> active, you must not do it in the resume handler for the device that was 
> registered first.  I don't know how the ATA and SCSI pieces interact 
> here, but regardless, this is a pretty strict requirement.
> 
> It should be okay to perform the action in the resume handler for the 
> device that was registered second.  But if the two devices aren't in an 
> ancestor-descendant relationship then you also have to call 
> device_pm_wait_for_dev() (or use device links as Rafael mentioned) to 
> handle the async case properly.
> 
>> Or... Why the heck scsi_rescan_device() is calling device_lock() ? This
>> is the only place in scsi code I can see that takes this lock. I suspect
>> this is to serialize either rescans, or serialize with resume, or both.
>> For serializing rescans, we can use another lock. For serializing with
>> PM, we should wait for PM transitions...
>> Something is not right here.
> 
> Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against 
> ->remove", written by Christoph Hellwig) says:
> 
>     Lock the device embedded in the scsi_device to protect against
>     concurrent calls to ->remove.
> 
> That's the commit which added the device_lock() call.

Thanks for the information.

+Christoph

Why is adding the device_lock() needed ? We could just do a
scsi_device_get()+scsi_device_put() to serialize against remove. No ?

> 
> Alan Stern

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Christoph Hellwig]

On Thu, Jun 15, 2023 at 09:10:28AM +0900, Damien Le Moal wrote:
> > Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against 
> > ->remove", written by Christoph Hellwig) says:
> > 
> >     Lock the device embedded in the scsi_device to protect against
> >     concurrent calls to ->remove.
> > 
> > That's the commit which added the device_lock() call.
> 
> Thanks for the information.
> 
> +Christoph
> 
> Why is adding the device_lock() needed ? We could just do a
> scsi_device_get()+scsi_device_put() to serialize against remove. No ?

No.  scsi_device_get just increments a reference count, and thus
prevents ->release from beeing called.  ->remove is not in any way
affected by the refcount.

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/15/23 13:40, Christoph Hellwig wrote:
> On Thu, Jun 15, 2023 at 09:10:28AM +0900, Damien Le Moal wrote:
>>> Here's what commit e27829dc92e5 ("scsi: serialize ->rescan against 
>>> ->remove", written by Christoph Hellwig) says:
>>>
>>>     Lock the device embedded in the scsi_device to protect against
>>>     concurrent calls to ->remove.
>>>
>>> That's the commit which added the device_lock() call.
>>
>> Thanks for the information.
>>
>> +Christoph
>>
>> Why is adding the device_lock() needed ? We could just do a
>> scsi_device_get()+scsi_device_put() to serialize against remove. No ?
> 
> No.  scsi_device_get just increments a reference count, and thus
> prevents ->release from beeing called.  ->remove is not in any way
> affected by the refcount.

What ->remove cb are you talking about ? The gendev one ?
I am trying to understand why the use of device_lock() helps in any way given
that this is not used by any other functions in scsi. And given that
scsi_rescan_device() should always be called with a ref on the scsi device (and
so on the gendev as well) held, why would this function be racy with device remove ?

Note that I did find a couple of places where scsi_rescan_device() seems to not
be called with a reference to the scsi dev held, e.g.  store_rescan_field() and
store_state_field().

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Christoph Hellwig]

On Thu, Jun 15, 2023 at 01:57:37PM +0900, Damien Le Moal wrote:
> > No.  scsi_device_get just increments a reference count, and thus
> > prevents ->release from beeing called.  ->remove is not in any way
> > affected by the refcount.
> 
> What ->remove cb are you talking about ? The gendev one ?

The one for the device locked.

> I am trying to understand why the use of device_lock() helps in any way given
> that this is not used by any other functions in scsi. And given that

The device model locks the device before calling ->remove.

> scsi_rescan_device() should always be called with a ref on the scsi device (and
> so on the gendev as well) held, why would this function be racy with device remove ?

Because ->remove ould otherwise be called at the same time as ->rescan.

> Note that I did find a couple of places where scsi_rescan_device() seems to not
> be called with a reference to the scsi dev held, e.g.  store_rescan_field() and
> store_state_field().

You need both a valid reference and ensure ->remove is not called at the
same time.

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/11/23 00:03, Bart Van Assche wrote:
> On 6/10/23 06:27, Bagas Sanjaya wrote:
>> On 6/10/23 15:55, Pavel Machek wrote:
>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>>
>>>> The reporter had found the culprit (via bisection), so:
>>>>
>>>> #regzbot introduced: a19a93e4c6a98c
>>> Maybe cc the authors of that commit?
>>
>> Ah! I forgot to do that! Thanks anyway.
> 
> Hi Damien,
> 
> Why does the ATA code call scsi_rescan_device() before system resume has
> finished? Would ATA devices still work with the patch below applied?

I do not know the PM code well at all, need to dig into it. But your patch
worries me as it seems it would prevent rescan of the device on a resume, which
can be an issue if the device has changed.

I am not yet 100% clear on the root cause for this, but I think it comes from
the fact that ata_port_pm_resume() runs before the sci device resume is done, so
with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
ata_port_resume_async() which triggers EH (which will do reset + rescan)
asynchronously. So it looks like we have scsi device resume and libata EH for
rescan fighting each others for the scan mutex and device lock, leading to deadlock.

Trying to recreate this issue now to confirm and debug further. But I suspect
the solution to this may be best implemented in libata, not in scsi.
This looks definitely related to this thread:

https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/

Similaraly to your comment on that thread, having to look at
dev->power.is_suspended is not ideal I think. What we need is to have ata and
scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.

> 
> Thanks,
> 
> Bart.
> 
> 
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 6a959c993dd8..be3971b7fd27 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1629,6 +1629,20 @@ void scsi_rescan_device(struct device *dev)
>   {
>   	struct scsi_device *sdev = to_scsi_device(dev);
> 
> +#ifdef CONFIG_PM_SLEEP
> +	/*
> +	 * The ATA subsystem may call scsi_rescan_device() before resuming has
> +	 * finished. If this happens, prevent a deadlock on the device_lock()
> +	 * call by skipping rescanning.
> +	 */
> +	if (dev->power.is_suspended)
> +		return;
> +#endif
> +
> +	/*
> +	 * Serialize scsi_driver.rescan() calls and scsi_driver.gendrv.remove()
> +	 * calls.
> +	 */
>   	device_lock(dev);
> 
>   	scsi_attach_vpd(sdev);
> 

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Hannes Reinecke]

On 6/12/23 05:09, Damien Le Moal wrote:
> On 6/11/23 00:03, Bart Van Assche wrote:
>> On 6/10/23 06:27, Bagas Sanjaya wrote:
>>> On 6/10/23 15:55, Pavel Machek wrote:
>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>>>
>>>>> The reporter had found the culprit (via bisection), so:
>>>>>
>>>>> #regzbot introduced: a19a93e4c6a98c
>>>> Maybe cc the authors of that commit?
>>>
>>> Ah! I forgot to do that! Thanks anyway.
>>
>> Hi Damien,
>>
>> Why does the ATA code call scsi_rescan_device() before system resume has
>> finished? Would ATA devices still work with the patch below applied?
> 
> I do not know the PM code well at all, need to dig into it. But your patch
> worries me as it seems it would prevent rescan of the device on a resume, which
> can be an issue if the device has changed.
> 
> I am not yet 100% clear on the root cause for this, but I think it comes from
> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
> ata_port_resume_async() which triggers EH (which will do reset + rescan)
> asynchronously. So it looks like we have scsi device resume and libata EH for
> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
> 
> Trying to recreate this issue now to confirm and debug further. But I suspect
> the solution to this may be best implemented in libata, not in scsi.
> This looks definitely related to this thread:
> 
> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
> 
> Similaraly to your comment on that thread, having to look at
> dev->power.is_suspended is not ideal I think. What we need is to have ata and
> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
> 
Which is my feeling, too.
libata runs rescan as part of the device discovery, so really it will 
run after resume. And consequently resume really cannot wait for rescan 
to finish.

What I would be looking at is to decouple resume from libata device 
rescan, and have resume to complete before libata EH runs.

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

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/12/23 15:09, Hannes Reinecke wrote:
> On 6/12/23 05:09, Damien Le Moal wrote:
>> On 6/11/23 00:03, Bart Van Assche wrote:
>>> On 6/10/23 06:27, Bagas Sanjaya wrote:
>>>> On 6/10/23 15:55, Pavel Machek wrote:
>>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>>>>
>>>>>> The reporter had found the culprit (via bisection), so:
>>>>>>
>>>>>> #regzbot introduced: a19a93e4c6a98c
>>>>> Maybe cc the authors of that commit?
>>>>
>>>> Ah! I forgot to do that! Thanks anyway.
>>>
>>> Hi Damien,
>>>
>>> Why does the ATA code call scsi_rescan_device() before system resume has
>>> finished? Would ATA devices still work with the patch below applied?
>>
>> I do not know the PM code well at all, need to dig into it. But your patch
>> worries me as it seems it would prevent rescan of the device on a resume, which
>> can be an issue if the device has changed.
>>
>> I am not yet 100% clear on the root cause for this, but I think it comes from
>> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
>> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
>> ata_port_resume_async() which triggers EH (which will do reset + rescan)
>> asynchronously. So it looks like we have scsi device resume and libata EH for
>> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
>>
>> Trying to recreate this issue now to confirm and debug further. But I suspect
>> the solution to this may be best implemented in libata, not in scsi.
>> This looks definitely related to this thread:
>>
>> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
>>
>> Similaraly to your comment on that thread, having to look at
>> dev->power.is_suspended is not ideal I think. What we need is to have ata and
>> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
>>
> Which is my feeling, too.
> libata runs rescan as part of the device discovery, so really it will 
> run after resume. And consequently resume really cannot wait for rescan 
> to finish.
> 
> What I would be looking at is to decouple resume from libata device 
> rescan, and have resume to complete before libata EH runs.

That is the case now, for the ata port at least, even though that is not super
explicit, and not reliable. See ata_port_pm_resume(): I think that the call to
EH in ata_port_pm_resume() -> ata_port_resume_async() -> ata_port_request_pm()
-> ata_port_schedule_eh() should instead use a sync resume, leading to a sync EH
call.

That EH execution essentially does ata_eh_handle_port_resume(), which calls into
the adapter resume operation. That in itself does not do much beside some
registers accesses to wakeup the port. There should be no issues doing that
synchronously.

The problem is that after that is done, ata EH calls ata_std_error_handler() ->
ata_do_eh() -> ata_eh_recover() -> ata_eh_revalidate_and_attach() ->
schedule_work(&(ap->scsi_rescan_task)). And the rescan work calls
scsi_rescan_device() (yet in another context than EH) which causes the problem
when the scsi disk device has not been resumed yet (dev->power_is_suspended
still true).

So it really looks like the solution should be to have ata_scsi_dev_rescan()
wait for the scsi device to resume first, but not sure how to do that with the
pm API. Digging...

> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Kai-Heng Feng]

On Mon, Jun 12, 2023 at 3:22 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>
> On 6/12/23 15:09, Hannes Reinecke wrote:
> > On 6/12/23 05:09, Damien Le Moal wrote:
> >> On 6/11/23 00:03, Bart Van Assche wrote:
> >>> On 6/10/23 06:27, Bagas Sanjaya wrote:
> >>>> On 6/10/23 15:55, Pavel Machek wrote:
> >>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
> >>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
> >>>>>>>
> >>>>>> The reporter had found the culprit (via bisection), so:
> >>>>>>
> >>>>>> #regzbot introduced: a19a93e4c6a98c
> >>>>> Maybe cc the authors of that commit?
> >>>>
> >>>> Ah! I forgot to do that! Thanks anyway.
> >>>
> >>> Hi Damien,
> >>>
> >>> Why does the ATA code call scsi_rescan_device() before system resume has
> >>> finished? Would ATA devices still work with the patch below applied?
> >>
> >> I do not know the PM code well at all, need to dig into it. But your patch
> >> worries me as it seems it would prevent rescan of the device on a resume, which
> >> can be an issue if the device has changed.
> >>
> >> I am not yet 100% clear on the root cause for this, but I think it comes from
> >> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
> >> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
> >> ata_port_resume_async() which triggers EH (which will do reset + rescan)
> >> asynchronously. So it looks like we have scsi device resume and libata EH for
> >> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
> >>
> >> Trying to recreate this issue now to confirm and debug further. But I suspect
> >> the solution to this may be best implemented in libata, not in scsi.
> >> This looks definitely related to this thread:
> >>
> >> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
> >>
> >> Similaraly to your comment on that thread, having to look at
> >> dev->power.is_suspended is not ideal I think. What we need is to have ata and
> >> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
> >>
> > Which is my feeling, too.
> > libata runs rescan as part of the device discovery, so really it will
> > run after resume. And consequently resume really cannot wait for rescan
> > to finish.
> >
> > What I would be looking at is to decouple resume from libata device
> > rescan, and have resume to complete before libata EH runs.
>
> That is the case now, for the ata port at least, even though that is not super
> explicit, and not reliable. See ata_port_pm_resume(): I think that the call to
> EH in ata_port_pm_resume() -> ata_port_resume_async() -> ata_port_request_pm()
> -> ata_port_schedule_eh() should instead use a sync resume, leading to a sync EH
> call.
>
> That EH execution essentially does ata_eh_handle_port_resume(), which calls into
> the adapter resume operation. That in itself does not do much beside some
> registers accesses to wakeup the port. There should be no issues doing that
> synchronously.
>
> The problem is that after that is done, ata EH calls ata_std_error_handler() ->
> ata_do_eh() -> ata_eh_recover() -> ata_eh_revalidate_and_attach() ->
> schedule_work(&(ap->scsi_rescan_task)). And the rescan work calls
> scsi_rescan_device() (yet in another context than EH) which causes the problem
> when the scsi disk device has not been resumed yet (dev->power_is_suspended
> still true).
>
> So it really looks like the solution should be to have ata_scsi_dev_rescan()
> wait for the scsi device to resume first, but not sure how to do that with the
> pm API. Digging...

Probably use dpm_wait_for_children()? Right now it's an internal PM API.

Rafael,
What do you think?

Kai-Heng

>
> >
> > Cheers,
> >
> > Hannes
>
> --
> Damien Le Moal
> Western Digital Research
>

Re: Fwd: Waking up from resume locks up on sr device

[Damien Le Moal]

On 6/12/23 16:36, Kai-Heng Feng wrote:
> On Mon, Jun 12, 2023 at 3:22 PM Damien Le Moal <dlemoal@kernel.org> wrote:
>>
>> On 6/12/23 15:09, Hannes Reinecke wrote:
>>> On 6/12/23 05:09, Damien Le Moal wrote:
>>>> On 6/11/23 00:03, Bart Van Assche wrote:
>>>>> On 6/10/23 06:27, Bagas Sanjaya wrote:
>>>>>> On 6/10/23 15:55, Pavel Machek wrote:
>>>>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
>>>>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
>>>>>>>>>
>>>>>>>> The reporter had found the culprit (via bisection), so:
>>>>>>>>
>>>>>>>> #regzbot introduced: a19a93e4c6a98c
>>>>>>> Maybe cc the authors of that commit?
>>>>>>
>>>>>> Ah! I forgot to do that! Thanks anyway.
>>>>>
>>>>> Hi Damien,
>>>>>
>>>>> Why does the ATA code call scsi_rescan_device() before system resume has
>>>>> finished? Would ATA devices still work with the patch below applied?
>>>>
>>>> I do not know the PM code well at all, need to dig into it. But your patch
>>>> worries me as it seems it would prevent rescan of the device on a resume, which
>>>> can be an issue if the device has changed.
>>>>
>>>> I am not yet 100% clear on the root cause for this, but I think it comes from
>>>> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
>>>> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
>>>> ata_port_resume_async() which triggers EH (which will do reset + rescan)
>>>> asynchronously. So it looks like we have scsi device resume and libata EH for
>>>> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
>>>>
>>>> Trying to recreate this issue now to confirm and debug further. But I suspect
>>>> the solution to this may be best implemented in libata, not in scsi.
>>>> This looks definitely related to this thread:
>>>>
>>>> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
>>>>
>>>> Similaraly to your comment on that thread, having to look at
>>>> dev->power.is_suspended is not ideal I think. What we need is to have ata and
>>>> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
>>>>
>>> Which is my feeling, too.
>>> libata runs rescan as part of the device discovery, so really it will
>>> run after resume. And consequently resume really cannot wait for rescan
>>> to finish.
>>>
>>> What I would be looking at is to decouple resume from libata device
>>> rescan, and have resume to complete before libata EH runs.
>>
>> That is the case now, for the ata port at least, even though that is not super
>> explicit, and not reliable. See ata_port_pm_resume(): I think that the call to
>> EH in ata_port_pm_resume() -> ata_port_resume_async() -> ata_port_request_pm()
>> -> ata_port_schedule_eh() should instead use a sync resume, leading to a sync EH
>> call.
>>
>> That EH execution essentially does ata_eh_handle_port_resume(), which calls into
>> the adapter resume operation. That in itself does not do much beside some
>> registers accesses to wakeup the port. There should be no issues doing that
>> synchronously.
>>
>> The problem is that after that is done, ata EH calls ata_std_error_handler() ->
>> ata_do_eh() -> ata_eh_recover() -> ata_eh_revalidate_and_attach() ->
>> schedule_work(&(ap->scsi_rescan_task)). And the rescan work calls
>> scsi_rescan_device() (yet in another context than EH) which causes the problem
>> when the scsi disk device has not been resumed yet (dev->power_is_suspended
>> still true).
>>
>> So it really looks like the solution should be to have ata_scsi_dev_rescan()
>> wait for the scsi device to resume first, but not sure how to do that with the
>> pm API. Digging...
> 
> Probably use dpm_wait_for_children()? Right now it's an internal PM API.

But I am not sure if there is a relationship between ata_device and its
scsi_device (dev->sdev)... Need to clarify that.
> 
> Rafael,
> What do you think?
> 
> Kai-Heng
> 
>>
>>>
>>> Cheers,
>>>
>>> Hannes
>>
>> --
>> Damien Le Moal
>> Western Digital Research
>>

-- 
Damien Le Moal
Western Digital Research

Re: Fwd: Waking up from resume locks up on sr device

[Alan Stern]

On Mon, Jun 12, 2023 at 04:47:09PM +0900, Damien Le Moal wrote:
> On 6/12/23 16:36, Kai-Heng Feng wrote:
> > On Mon, Jun 12, 2023 at 3:22 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> >>
> >> On 6/12/23 15:09, Hannes Reinecke wrote:
> >>> On 6/12/23 05:09, Damien Le Moal wrote:
> >>>> On 6/11/23 00:03, Bart Van Assche wrote:
> >>>>> On 6/10/23 06:27, Bagas Sanjaya wrote:
> >>>>>> On 6/10/23 15:55, Pavel Machek wrote:
> >>>>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
> >>>>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
> >>>>>>>>>
> >>>>>>>> The reporter had found the culprit (via bisection), so:
> >>>>>>>>
> >>>>>>>> #regzbot introduced: a19a93e4c6a98c
> >>>>>>> Maybe cc the authors of that commit?
> >>>>>>
> >>>>>> Ah! I forgot to do that! Thanks anyway.
> >>>>>
> >>>>> Hi Damien,
> >>>>>
> >>>>> Why does the ATA code call scsi_rescan_device() before system resume has
> >>>>> finished? Would ATA devices still work with the patch below applied?
> >>>>
> >>>> I do not know the PM code well at all, need to dig into it. But your patch
> >>>> worries me as it seems it would prevent rescan of the device on a resume, which
> >>>> can be an issue if the device has changed.
> >>>>
> >>>> I am not yet 100% clear on the root cause for this, but I think it comes from
> >>>> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
> >>>> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
> >>>> ata_port_resume_async() which triggers EH (which will do reset + rescan)
> >>>> asynchronously. So it looks like we have scsi device resume and libata EH for
> >>>> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
> >>>>
> >>>> Trying to recreate this issue now to confirm and debug further. But I suspect
> >>>> the solution to this may be best implemented in libata, not in scsi.
> >>>> This looks definitely related to this thread:
> >>>>
> >>>> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
> >>>>
> >>>> Similaraly to your comment on that thread, having to look at
> >>>> dev->power.is_suspended is not ideal I think. What we need is to have ata and
> >>>> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
> >>>>
> >>> Which is my feeling, too.
> >>> libata runs rescan as part of the device discovery, so really it will
> >>> run after resume. And consequently resume really cannot wait for rescan
> >>> to finish.
> >>>
> >>> What I would be looking at is to decouple resume from libata device
> >>> rescan, and have resume to complete before libata EH runs.
> >>
> >> That is the case now, for the ata port at least, even though that is not super
> >> explicit, and not reliable. See ata_port_pm_resume(): I think that the call to
> >> EH in ata_port_pm_resume() -> ata_port_resume_async() -> ata_port_request_pm()
> >> -> ata_port_schedule_eh() should instead use a sync resume, leading to a sync EH
> >> call.
> >>
> >> That EH execution essentially does ata_eh_handle_port_resume(), which calls into
> >> the adapter resume operation. That in itself does not do much beside some
> >> registers accesses to wakeup the port. There should be no issues doing that
> >> synchronously.
> >>
> >> The problem is that after that is done, ata EH calls ata_std_error_handler() ->
> >> ata_do_eh() -> ata_eh_recover() -> ata_eh_revalidate_and_attach() ->
> >> schedule_work(&(ap->scsi_rescan_task)). And the rescan work calls
> >> scsi_rescan_device() (yet in another context than EH) which causes the problem
> >> when the scsi disk device has not been resumed yet (dev->power_is_suspended
> >> still true).
> >>
> >> So it really looks like the solution should be to have ata_scsi_dev_rescan()
> >> wait for the scsi device to resume first, but not sure how to do that with the
> >> pm API. Digging...
> > 
> > Probably use dpm_wait_for_children()? Right now it's an internal PM API.
> 
> But I am not sure if there is a relationship between ata_device and its
> scsi_device (dev->sdev)... Need to clarify that.
> > 
> > Rafael,
> > What do you think?

Look into the device_pm_wait_for_dev() API.  It's the appropriate thing 
to use when you want to wait for another device to complete a system PM 
transition.  (However, it's not appropriate for runtime PM.)

Of course, if there is a parent-child relationship between two devices 
then waiting is never necessary.  The PM core guarantees that a parent 
will always be at full power when a child changes its power state, 
unless pm_suspend_ignore_children() has been called for the parent 
device.

Alan Stern

Re: Fwd: Waking up from resume locks up on sr device

[Rafael J. Wysocki]

On Mon, Jun 12, 2023 at 4:33 PM Alan Stern <stern@rowland.harvard.edu> wrote:
>
> On Mon, Jun 12, 2023 at 04:47:09PM +0900, Damien Le Moal wrote:
> > On 6/12/23 16:36, Kai-Heng Feng wrote:
> > > On Mon, Jun 12, 2023 at 3:22 PM Damien Le Moal <dlemoal@kernel.org> wrote:
> > >>
> > >> On 6/12/23 15:09, Hannes Reinecke wrote:
> > >>> On 6/12/23 05:09, Damien Le Moal wrote:
> > >>>> On 6/11/23 00:03, Bart Van Assche wrote:
> > >>>>> On 6/10/23 06:27, Bagas Sanjaya wrote:
> > >>>>>> On 6/10/23 15:55, Pavel Machek wrote:
> > >>>>>>>>> #regzbot introduced: v5.0..v6.4-rc5 https://bugzilla.kernel.org/show_bug.cgi?id=217530
> > >>>>>>>>> #regzbot title: Waking up from resume locks up on SCSI CD/DVD drive
> > >>>>>>>>>
> > >>>>>>>> The reporter had found the culprit (via bisection), so:
> > >>>>>>>>
> > >>>>>>>> #regzbot introduced: a19a93e4c6a98c
> > >>>>>>> Maybe cc the authors of that commit?
> > >>>>>>
> > >>>>>> Ah! I forgot to do that! Thanks anyway.
> > >>>>>
> > >>>>> Hi Damien,
> > >>>>>
> > >>>>> Why does the ATA code call scsi_rescan_device() before system resume has
> > >>>>> finished? Would ATA devices still work with the patch below applied?
> > >>>>
> > >>>> I do not know the PM code well at all, need to dig into it. But your patch
> > >>>> worries me as it seems it would prevent rescan of the device on a resume, which
> > >>>> can be an issue if the device has changed.
> > >>>>
> > >>>> I am not yet 100% clear on the root cause for this, but I think it comes from
> > >>>> the fact that ata_port_pm_resume() runs before the sci device resume is done, so
> > >>>> with scsi_dev->power.is_suspended still true. And ata_port_pm_resume() calls
> > >>>> ata_port_resume_async() which triggers EH (which will do reset + rescan)
> > >>>> asynchronously. So it looks like we have scsi device resume and libata EH for
> > >>>> rescan fighting each others for the scan mutex and device lock, leading to deadlock.
> > >>>>
> > >>>> Trying to recreate this issue now to confirm and debug further. But I suspect
> > >>>> the solution to this may be best implemented in libata, not in scsi.
> > >>>> This looks definitely related to this thread:
> > >>>>
> > >>>> https://lore.kernel.org/linux-scsi/7b553268-69d3-913a-f9de-28f8d45bdb1e@acm.org/
> > >>>>
> > >>>> Similaraly to your comment on that thread, having to look at
> > >>>> dev->power.is_suspended is not ideal I think. What we need is to have ata and
> > >>>> scsi pm resume be synchronized, but I am not yet 100% clear on the scsi layer side.
> > >>>>
> > >>> Which is my feeling, too.
> > >>> libata runs rescan as part of the device discovery, so really it will
> > >>> run after resume. And consequently resume really cannot wait for rescan
> > >>> to finish.
> > >>>
> > >>> What I would be looking at is to decouple resume from libata device
> > >>> rescan, and have resume to complete before libata EH runs.
> > >>
> > >> That is the case now, for the ata port at least, even though that is not super
> > >> explicit, and not reliable. See ata_port_pm_resume(): I think that the call to
> > >> EH in ata_port_pm_resume() -> ata_port_resume_async() -> ata_port_request_pm()
> > >> -> ata_port_schedule_eh() should instead use a sync resume, leading to a sync EH
> > >> call.
> > >>
> > >> That EH execution essentially does ata_eh_handle_port_resume(), which calls into
> > >> the adapter resume operation. That in itself does not do much beside some
> > >> registers accesses to wakeup the port. There should be no issues doing that
> > >> synchronously.
> > >>
> > >> The problem is that after that is done, ata EH calls ata_std_error_handler() ->
> > >> ata_do_eh() -> ata_eh_recover() -> ata_eh_revalidate_and_attach() ->
> > >> schedule_work(&(ap->scsi_rescan_task)). And the rescan work calls
> > >> scsi_rescan_device() (yet in another context than EH) which causes the problem
> > >> when the scsi disk device has not been resumed yet (dev->power_is_suspended
> > >> still true).
> > >>
> > >> So it really looks like the solution should be to have ata_scsi_dev_rescan()
> > >> wait for the scsi device to resume first, but not sure how to do that with the
> > >> pm API. Digging...
> > >
> > > Probably use dpm_wait_for_children()? Right now it's an internal PM API.
> >
> > But I am not sure if there is a relationship between ata_device and its
> > scsi_device (dev->sdev)... Need to clarify that.
> > >
> > > Rafael,
> > > What do you think?
>
> Look into the device_pm_wait_for_dev() API.  It's the appropriate thing
> to use when you want to wait for another device to complete a system PM
> transition.  (However, it's not appropriate for runtime PM.)
>
> Of course, if there is a parent-child relationship between two devices
> then waiting is never necessary.  The PM core guarantees that a parent
> will always be at full power when a child changes its power state,
> unless pm_suspend_ignore_children() has been called for the parent
> device.

There are also device links that go beyond the parent-child handling.
Please see Documentation/driver-api/device_link.rst and the kerneldoc
description of device_link_add() for more information.