Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230502150435.423770-1-kai.heng.feng@canonical.com>
 <20230502150435.423770-2-kai.heng.feng@canonical.com> <8eea2352-1d4f-aa3a-7c86-9306adb0aaae@kernel.org>
In-Reply-To: <8eea2352-1d4f-aa3a-7c86-9306adb0aaae@kernel.org>
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
Date:   Thu, 1 Jun 2023 23:55:21 +0800
Message-ID: <CAAd53p7H2C+LenmzynRHqnxd-qgc2+p9ZLKidVUOAtXMyU2rmQ@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] ata: libata: Defer rescan on suspended device
To:     Damien Le Moal <dlemoal@kernel.org>
Cc:     jejb@linux.ibm.com, martin.petersen@oracle.com,
        bblock@linux.ibm.com, acelan.kao@canonical.com,
        linux-ide@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, May 7, 2023 at 5:23=E2=80=AFPM Damien Le Moal <dlemoal@kernel.org> =
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@canonical.com>
> > ---
> > 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 *d=
ev)
> >       return 0;
> >  }
> >
> > +static void ata_port_pm_complete(struct device *dev)
> > +{
> > +     struct ata_port *ap =3D to_ata_port(dev);
> > +
> > +     if (ap->pflags & ATA_PFLAG_DEFER_RESCAN)
> > +             schedule_work(&(ap->scsi_rescan_task));
> > +
> > +     ap->pflags &=3D ~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 revalidat=
ing 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 =3D ATA_EHI_NO_AUTOPSY
> >                                               | ATA_EHI_QUIET;
> >
> > @@ -5158,6 +5168,7 @@ static const struct dev_pm_ops ata_port_pm_ops =
=3D {
> >       .thaw =3D ata_port_pm_resume,
> >       .poweroff =3D ata_port_pm_poweroff,
> >       .restore =3D ata_port_pm_resume,
> > +     .complete =3D ata_port_pm_complete,
> >
> >       .runtime_suspend =3D ata_port_runtime_suspend,
> >       .runtime_resume =3D 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 a=
ta_link *link,
> >                        */
> >                       ehc->i.flags |=3D 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 !=3D PM_SUSPEND_ON)
>
> Why ? Shouldn't this be "pm_suspend_target_state =3D=3D PM_SUSPEND_ON" ? =
Because if
> the device is already resumed, why would we need to defer the rescan ?

"pm_suspend_target_state !=3D 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 |=3D ATA_PFLAG_DEFER_RESCAN;
> > +                     else
> > +                             schedule_work(&(ap->scsi_rescan_task));
> >               } else if (dev->class =3D=3D 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     =3D (1 << 9), /* driver is being unloaded=
 */
> >       ATA_PFLAG_UNLOADED      =3D (1 << 10), /* driver is unloaded */
> >
> > +     ATA_PFLAG_DEFER_RESCAN  =3D (1 << 16), /* peform deferred rescan =
on system resume */
>
> Do we really need a new flag ? Can't we use ATA_PFLAG_PM_PENDING correctl=
y ?
> From the rather sparse commit message description, it sounds like this fl=
ag 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     =3D (1 << 17), /* port is suspended (powe=
r) */
> >       ATA_PFLAG_PM_PENDING    =3D (1 << 18), /* PM operation pending */
> >       ATA_PFLAG_INIT_GTM_VALID =3D (1 << 19), /* initial gtm data valid=
 */
>
> --
> Damien Le Moal
> Western Digital Research
>