Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management

[Kai-Heng Feng]

at 14:28, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:

> On 23 August 2018 at 10:03, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>  
> wrote:
> > Hi Ulf,
> >
> > Sorry for the late reply.
> >
> >
> > at 21:14, Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
> >
> > > On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > > wrote:
> > > > In order to let host's parent device, rtsx_usb, to use USB remote wake
> > > > up signaling to do card detection, it needs to be suspended. Hence it's
> > > > necessary to add runtime PM support for the memstick host.
> > > >
> > > > To keep memstick host stays suspended when it's not in use, convert the
> > > > card detection function from kthread to delayed_work, which can be
> > > > scheduled when the host is resumed and can be canceled when the host is
> > > > suspended.
> > > >
> > > > Use an idle function check if there's no card and the power mode is
> > > > MEMSTICK_POWER_OFF. If both criteria are met, put the device to suspend.
> > > >
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/memstick/host/rtsx_usb_ms.c | 138 ++++++++++++++--------------
> > > >  1 file changed, 71 insertions(+), 67 deletions(-)
> > > >
> > > > diff --git a/drivers/memstick/host/rtsx_usb_ms.c
> > > > b/drivers/memstick/host/rtsx_usb_ms.c
> > > > index cd12f3d1c088..126eb310980a 100644
> > > > --- a/drivers/memstick/host/rtsx_usb_ms.c
> > > > +++ b/drivers/memstick/host/rtsx_usb_ms.c
> > > > @@ -40,15 +40,14 @@ struct rtsx_usb_ms {
> > > >
> > > >         struct mutex            host_mutex;
> > > >         struct work_struct      handle_req;
> > > > -
> > > > -       struct task_struct      *detect_ms;
> > > > -       struct completion       detect_ms_exit;
> > > > +       struct delayed_work     poll_card;
> > > >
> > > >         u8                      ssc_depth;
> > > >         unsigned int            clock;
> > > >         int                     power_mode;
> > > >         unsigned char           ifmode;
> > > >         bool                    eject;
> > > > +       bool                    suspend;
> > > >  };
> > > >
> > > >  static inline struct device *ms_dev(struct rtsx_usb_ms *host)
> > > > @@ -524,7 +523,7 @@ static void rtsx_usb_ms_handle_req(struct  
> > > > work_struct
> > > > *work)
> > > >         int rc;
> > > >
> > > >         if (!host->req) {
> > > > -               pm_runtime_get_sync(ms_dev(host));
> > > > +               pm_runtime_get_noresume(ms_dev(host));
> > >
> > >
> > > I don't think this is safe.
> > >
> > > The memstick core doesn't manage runtime PM, hence there are no
> > > guarantee that the device is runtime resumed at this point, or is
> > > there?
> >
> >
> > No guarantees there.
> > Right now this only gets call when the host is powered on.
> >
> > > >               do {
> > > >                         rc = memstick_next_req(msh, &host->req);
> > > >                         dev_dbg(ms_dev(host), "next req %d\n", rc);
> > > > @@ -545,7 +544,7 @@ static void rtsx_usb_ms_handle_req(struct  
> > > > work_struct
> > > > *work)
> > > >                                                 host->req->error);
> > > >                         }
> > > >                 } while (!rc);
> > > > -               pm_runtime_put(ms_dev(host));
> > > > +               pm_runtime_put_noidle(ms_dev(host));
> > >
> > >
> > > According to the above, I think this should stay as is. Or perhaps you
> > > want to use pm_runtime_put_sync() instead, as to avoid the device from
> > > being unnecessary resumed and hence also its parent.
> >
> >
> > The tricky part is that pm_runtime_put_sync() calls  
> > rtsx_usb_ms_set_param()
> > which calls pm_runtime_* helpers again
>
> Why does a pm_runtime_put_sync() triggers a call to
> rtsx_usb_ms_set_param()? It should not.
>
> Ahh, that's because you have implemented the ->runtime_suspend()
> callback to wrongly call memstick_suspend_host(). Drop that change,
> then you should be able to call pm_runtime_put_sync() here and at
> other places correctly.

Thanks for the catch! I'll address that.

> > So maybe add runtime PM support to memstick core is the only way to  
> > support
> > it properly?
>
> It shouldn't be needed, and I wonder about if the effort is worth it,
> especially since it seems that the only memstick driver that using
> runtime PM is rtsx_usb_ms.
>
> BTW, are you testing this change by using a memstick card, since I
> haven't seen them for ages. :-)

Yes, Realtek borrowed me a MMC/MS combo device to let me work on this.

Kai-Heng

> [...]
>
> Kind regards
> Uffe