On Sun, May 7, 2023 at 5:23 PM Damien Le Moal <dlemoal@xxxxxxxxxx> wrote:
>
> On 2023/05/03 0:04, Kai-Heng Feng wrote:
> > During system resume, if an EH is schduled after ATA host is resumed
> > (i.e. ATA_PFLAG_PM_PENDING cleared), but before the disk device is
> > fully resumed, the device_lock hold by scsi_rescan_device() is never
> > released so the dpm_resume() of the disk is blocked forerver.
> >
> > That's because scsi_attach_vpd() is expecting the disk device is in
> > operational state, as it doesn't work on suspended device.
> >
> > To avoid such deadlock, defer rescan if the disk is still suspended so
> > the resume process of the disk device can proceed. At the end of the
> > resume process, use the complete() callback to schedule the rescan task.
> >
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> > v4:
> > - No change.
> >
> > v3:
> > - New patch to resolve undefined pm_suspend_target_state.
> >
> > v2:
> > - Schedule rescan task at the end of system resume phase.
> > - Wording.
> >
> > drivers/ata/libata-core.c | 11 +++++++++++
> > drivers/ata/libata-eh.c | 11 +++++++++--
> > include/linux/libata.h | 1 +
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> > index 8bf612bdd61a..bdd244bdb8a2 100644
> > --- a/drivers/ata/libata-core.c
> > +++ b/drivers/ata/libata-core.c
> > @@ -5093,6 +5093,16 @@ static int ata_port_pm_poweroff(struct device *dev)
> > return 0;
> > }
> >
> > +static void ata_port_pm_complete(struct device *dev)
> > +{
> > + struct ata_port *ap = to_ata_port(dev);
> > +
> > + if (ap->pflags & ATA_PFLAG_DEFER_RESCAN)
> > + schedule_work(&(ap->scsi_rescan_task));
> > +
> > + ap->pflags &= ~ATA_PFLAG_DEFER_RESCAN;
>
> Is this called with the port lock held ? Otherwise, there is a race with
> ata_eh_revalidate_and_attach() and we may end up never actually revalidating the
> drive. At the very least, I think that ATA_PFLAG_DEFER_RESCAN needs to be
> cleared before calling schedule_work().
OK. Maybe all of these are unnecessary if the rescanning can wait for
disk device to be resumed.
>
> > +}
> > +
> > static const unsigned int ata_port_resume_ehi = ATA_EHI_NO_AUTOPSY
> > | ATA_EHI_QUIET;
> >
> > @@ -5158,6 +5168,7 @@ static const struct dev_pm_ops ata_port_pm_ops = {
> > .thaw = ata_port_pm_resume,
> > .poweroff = ata_port_pm_poweroff,
> > .restore = ata_port_pm_resume,
> > + .complete = ata_port_pm_complete,
> >
> > .runtime_suspend = ata_port_runtime_suspend,
> > .runtime_resume = ata_port_runtime_resume,
> > diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> > index a6c901811802..0881b590fb7e 100644
> > --- a/drivers/ata/libata-eh.c
> > +++ b/drivers/ata/libata-eh.c
> > @@ -15,6 +15,7 @@
> > #include <linux/blkdev.h>
> > #include <linux/export.h>
> > #include <linux/pci.h>
> > +#include <linux/suspend.h>
> > #include <scsi/scsi.h>
> > #include <scsi/scsi_host.h>
> > #include <scsi/scsi_eh.h>
> > @@ -2983,8 +2984,14 @@ static int ata_eh_revalidate_and_attach(struct ata_link *link,
> > */
> > ehc->i.flags |= ATA_EHI_SETMODE;
> >
> > - /* schedule the scsi_rescan_device() here */
> > - schedule_work(&(ap->scsi_rescan_task));
> > + /* Schedule the scsi_rescan_device() here.
>
> Code style: please start multi-line comment with a line starting with "/*"
> without text after it.
OK. Will do.
>
> > + * Defer the rescan if it's in process of
> > + * suspending or resuming.
> > + */
> > + if (pm_suspend_target_state != PM_SUSPEND_ON)
>
> Why ? Shouldn't this be "pm_suspend_target_state == PM_SUSPEND_ON" ? Because if
> the device is already resumed, why would we need to defer the rescan ?
"pm_suspend_target_state != PM_SUSPEND_ON" means the system is not
"ON" state. For this particular issue, it means the system is still in
resume process.
>
> > + ap->pflags |= ATA_PFLAG_DEFER_RESCAN;
> > + else
> > + schedule_work(&(ap->scsi_rescan_task));
> > } else if (dev->class == ATA_DEV_UNKNOWN &&
> > ehc->tries[dev->devno] &&
> > ata_class_enabled(ehc->classes[dev->devno])) {
> > diff --git a/include/linux/libata.h b/include/linux/libata.h
> > index 311cd93377c7..1696c9ebd168 100644
> > --- a/include/linux/libata.h
> > +++ b/include/linux/libata.h
> > @@ -189,6 +189,7 @@ enum {
> > ATA_PFLAG_UNLOADING = (1 << 9), /* driver is being unloaded */
> > ATA_PFLAG_UNLOADED = (1 << 10), /* driver is unloaded */
> >
> > + ATA_PFLAG_DEFER_RESCAN = (1 << 16), /* peform deferred rescan on system resume */
>
> Do we really need a new flag ? Can't we use ATA_PFLAG_PM_PENDING correctly ?
> From the rather sparse commit message description, it sounds like this flag is
> being cleared too early. Not sure though. Need to dig further into this.
Defer clearing ATA_PFLAG_PM_PENDING doesn't seem to be trivial.
I'll send a new version which IMO is a lot simpler.
Kai-Heng
>
> > ATA_PFLAG_SUSPENDED = (1 << 17), /* port is suspended (power) */
> > ATA_PFLAG_PM_PENDING = (1 << 18), /* PM operation pending */
> > ATA_PFLAG_INIT_GTM_VALID = (1 << 19), /* initial gtm data valid */
>
> --
> Damien Le Moal
> Western Digital Research
>
