Message-ID: <b0fdf454-b2f7-c273-66f5-efe42fbc2807@kernel.org>
Date:   Wed, 14 Jun 2023 16:35:50 +0900
From:   Damien Le Moal <dlemoal@...nel.org>
To:     Hannes Reinecke <hare@...e.de>,
        Joe Breuer <linux-kernel@...reuer.net>,
        Bart Van Assche <bvanassche@....org>,
        Bagas Sanjaya <bagasdotme@...il.com>,
        Pavel Machek <pavel@....cz>
Cc:     "Rafael J. Wysocki" <rafael@...nel.org>,
        Len Brown <len.brown@...el.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Kees Cook <keescook@...omium.org>,
        Tony Luck <tony.luck@...el.com>,
        "Guilherme G. Piccoli" <gpiccoli@...lia.com>,
        Thorsten Leemhuis <linux@...mhuis.info>,
        "James E.J. Bottomley" <jejb@...ux.ibm.com>,
        "Martin K. Petersen" <martin.petersen@...cle.com>,
        Phillip Potter <phil@...lpotter.co.uk>,
        Linux Power Management <linux-pm@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linux Hardening <linux-hardening@...r.kernel.org>,
        Linux Regressions <regressions@...ts.linux.dev>,
        Linux SCSI <linux-scsi@...r.kernel.org>,
        Alan Stern <stern@...land.harvard.edu>,
        Dan Williams <dan.j.williams@...el.com>,
        Hannes Reinecke <hare@...e.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Martin Kepplinger <martin.kepplinger@...i.sm>,
        Kai-Heng Feng <kai.heng.feng@...onical.com>
Subject: Re: Fwd: Waking up from resume locks up on sr device

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

Powered by blists - more mailing lists