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

[Kai-Heng Feng]

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

So maybe add runtime PM support to memstick core is the only way to support  
it properly?

> >        }
> >
> >  }
> > @@ -572,7 +571,7 @@ static int rtsx_usb_ms_set_param(struct  
> > memstick_host *msh,
> >         dev_dbg(ms_dev(host), "%s: param = %d, value = %d\n",
> >                         __func__, param, value);
> >
> > -       pm_runtime_get_sync(ms_dev(host));
> > +       pm_runtime_get_noresume(ms_dev(host));
>
> Ditto.
>
> >        mutex_lock(&ucr->dev_mutex);
> >
> >         err = rtsx_usb_card_exclusive_check(ucr, RTSX_USB_MS_CARD);
> > @@ -585,14 +584,14 @@ static int rtsx_usb_ms_set_param(struct  
> > memstick_host *msh,
> >                         break;
> >
> >                 if (value == MEMSTICK_POWER_ON) {
> > -                       pm_runtime_get_sync(ms_dev(host));
> > +                       pm_runtime_get_noresume(ms_dev(host));
>
> Ditto.
>
> >                        err = ms_power_on(host);
> > +                       if (err)
> > +                               pm_runtime_put_noidle(ms_dev(host));
> >                 } else if (value == MEMSTICK_POWER_OFF) {
> >                         err = ms_power_off(host);
> > -                       if (host->msh->card)
> > +                       if (!err)
> >                                 pm_runtime_put_noidle(ms_dev(host));
> > -                       else
> > -                               pm_runtime_put(ms_dev(host));
>
> Ditto.
>
> >                } else
> >                         err = -EINVAL;
> >                 if (!err)
> > @@ -638,7 +637,7 @@ static int rtsx_usb_ms_set_param(struct  
> > memstick_host *msh,
> >         }
> >  out:
> >         mutex_unlock(&ucr->dev_mutex);
> > -       pm_runtime_put(ms_dev(host));
> > +       pm_runtime_put_noidle(ms_dev(host));
>
> Ditto.
>
> >         /* power-on delay */
> >         if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
> > @@ -648,75 +647,86 @@ static int rtsx_usb_ms_set_param(struct  
> > memstick_host *msh,
> >         return err;
> >  }
> >
> > -#ifdef CONFIG_PM_SLEEP
> > -static int rtsx_usb_ms_suspend(struct device *dev)
> > +#ifdef CONFIG_PM
> > +static int rtsx_usb_ms_runtime_suspend(struct device *dev)
> >  {
> >         struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> >         struct memstick_host *msh = host->msh;
> >
> > -       dev_dbg(ms_dev(host), "--> %s\n", __func__);
> > -
> > +       host->suspend = true;
> >         memstick_suspend_host(msh);
> > +
> >         return 0;
> >  }
> >
> > -static int rtsx_usb_ms_resume(struct device *dev)
> > +static int rtsx_usb_ms_runtime_resume(struct device *dev)
> >  {
> >         struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> >         struct memstick_host *msh = host->msh;
> >
> > -       dev_dbg(ms_dev(host), "--> %s\n", __func__);
> > -
> >         memstick_resume_host(msh);
> > +       host->suspend = false;
> > +       schedule_delayed_work(&host->poll_card, 100);
> > +
> >         return 0;
> >  }
> > -#endif /* CONFIG_PM_SLEEP */
> >
> > -/*
> > - * Thread function of ms card slot detection. The thread starts right  
> > after
> > - * successful host addition. It stops while the driver removal function  
> > sets
> > - * host->eject true.
> > - */
> > -static int rtsx_usb_detect_ms_card(void *__host)
> > +static int rtsx_usb_ms_runtime_idle(struct device *dev)
> > +{
> > +       struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> > +
> > +       if (!host->msh->card && host->power_mode == MEMSTICK_POWER_OFF) {
> > +               pm_schedule_suspend(dev, 0);
> > +               return 0;
> > +       }
> > +
> > +       return -EAGAIN;
> > +}
> > +
> > +static UNIVERSAL_DEV_PM_OPS(rtsx_usb_ms_pm_ops,
> > +               rtsx_usb_ms_runtime_suspend, rtsx_usb_ms_runtime_resume,
> > +               rtsx_usb_ms_runtime_idle);
> > +#endif /* CONFIG_PM */
> > +
> > +static void rtsx_usb_ms_poll_card(struct work_struct *work)
> >  {
> > -       struct rtsx_usb_ms *host = (struct rtsx_usb_ms *)__host;
> > +       struct rtsx_usb_ms *host = container_of(work, struct rtsx_usb_ms,
> > +                       poll_card.work);
> >         struct rtsx_ucr *ucr = host->ucr;
> > -       u8 val = 0;
> >         int err;
> > +       u8 val;
> >
> > -       for (;;) {
> > -               pm_runtime_get_sync(ms_dev(host));
> > -               mutex_lock(&ucr->dev_mutex);
> > +       if (host->eject || host->suspend)
>
> The runtime PM state of the device could potentially be changed by
> user space, via sysfs, as well.
>
> It seems like what you really should be checking is whether
> host->power_mode is MEMSTICK_POWER_OFF and then act accordingly.

Makes sense.

> I also think you be able to implement this without a ->runtime_idle()
> callback, as it just makes this a bit unnecessary complicated.

You are right. Will update it with runtime_suspend() version.

> > +               return;
> >
> > -               /* Check pending MS card changes */
> > -               err = rtsx_usb_read_register(ucr, CARD_INT_PEND, &val);
> > -               if (err) {
> > -                       mutex_unlock(&ucr->dev_mutex);
> > -                       goto poll_again;
> > -               }
> > -
> > -               /* Clear the pending */
> > -               rtsx_usb_write_register(ucr, CARD_INT_PEND,
> > -                               XD_INT | MS_INT | SD_INT,
> > -                               XD_INT | MS_INT | SD_INT);
> > +       pm_runtime_get_noresume(ms_dev(host));
> > +       mutex_lock(&ucr->dev_mutex);
> >
> > +       /* Check pending MS card changes */
> > +       err = rtsx_usb_read_register(ucr, CARD_INT_PEND, &val);
> > +       if (err) {
> >                 mutex_unlock(&ucr->dev_mutex);
> > +               goto poll_again;
> > +       }
> >
> > -               if (val & MS_INT) {
> > -                       dev_dbg(ms_dev(host), "MS slot change  
> > detected\n");
> > -                       memstick_detect_change(host->msh);
> > -               }
> > +       /* Clear the pending */
> > +       rtsx_usb_write_register(ucr, CARD_INT_PEND,
> > +                       XD_INT | MS_INT | SD_INT,
> > +                       XD_INT | MS_INT | SD_INT);
> >
> > -poll_again:
> > -               pm_runtime_put(ms_dev(host));
> > -               if (host->eject)
> > -                       break;
> > +       mutex_unlock(&ucr->dev_mutex);
> > +       pm_runtime_put_noidle(ms_dev(host));
> >
> > -               schedule_timeout_idle(HZ);
> > +       if (val & MS_INT) {
> > +               dev_dbg(ms_dev(host), "MS slot change detected\n");
> > +               memstick_detect_change(host->msh);
> >         }
> >
> > -       complete(&host->detect_ms_exit);
> > -       return 0;
> > +       pm_runtime_idle(ms_dev(host));
> > +
> > +poll_again:
> > +       if (!host->eject && !host->suspend)
> > +               schedule_delayed_work(&host->poll_card, 100);
> >  }
> >
> >  static int rtsx_usb_ms_drv_probe(struct platform_device *pdev)
> > @@ -747,26 +757,21 @@ static int rtsx_usb_ms_drv_probe(struct  
> > platform_device *pdev)
> >         mutex_init(&host->host_mutex);
> >         INIT_WORK(&host->handle_req, rtsx_usb_ms_handle_req);
> >
> > -       init_completion(&host->detect_ms_exit);
> > -       host->detect_ms = kthread_create(rtsx_usb_detect_ms_card, host,
> > -                       "rtsx_usb_ms_%d", pdev->id);
> > -       if (IS_ERR(host->detect_ms)) {
> > -               dev_dbg(&(pdev->dev),
> > -                               "Unable to create polling thread.\n");
> > -               err = PTR_ERR(host->detect_ms);
> > -               goto err_out;
> > -       }
> > +       INIT_DELAYED_WORK(&host->poll_card, rtsx_usb_ms_poll_card);
> >
> >         msh->request = rtsx_usb_ms_request;
> >         msh->set_param = rtsx_usb_ms_set_param;
> >         msh->caps = MEMSTICK_CAP_PAR4;
> >
> > -       pm_runtime_enable(&pdev->dev);
> > +       pm_runtime_set_active(ms_dev(host));
> > +       pm_runtime_enable(ms_dev(host));
> > +
> >         err = memstick_add_host(msh);
> >         if (err)
> >                 goto err_out;
> >
> > -       wake_up_process(host->detect_ms);
> > +       schedule_delayed_work(&host->poll_card, 100);
> > +
> >         return 0;
> >  err_out:
> >         memstick_free_host(msh);
> > @@ -782,6 +787,7 @@ static int rtsx_usb_ms_drv_remove(struct  
> > platform_device *pdev)
> >         msh = host->msh;
> >         host->eject = true;
> >         cancel_work_sync(&host->handle_req);
> > +       cancel_delayed_work_sync(&host->poll_card);
> >
> >         mutex_lock(&host->host_mutex);
> >         if (host->req) {
> > @@ -797,7 +803,6 @@ static int rtsx_usb_ms_drv_remove(struct  
> > platform_device *pdev)
> >         }
> >         mutex_unlock(&host->host_mutex);
> >
> > -       wait_for_completion(&host->detect_ms_exit);
> >         memstick_remove_host(msh);
> >         memstick_free_host(msh);
> >
> > @@ -816,9 +821,6 @@ static int rtsx_usb_ms_drv_remove(struct  
> > platform_device *pdev)
> >         return 0;
> >  }
> >
> > -static SIMPLE_DEV_PM_OPS(rtsx_usb_ms_pm_ops,
> > -               rtsx_usb_ms_suspend, rtsx_usb_ms_resume);
> > -
> >  static struct platform_device_id rtsx_usb_ms_ids[] = {
> >         {
> >                 .name = "rtsx_usb_ms",
> > @@ -834,7 +836,9 @@ static struct platform_driver rtsx_usb_ms_driver = {
> >         .id_table       = rtsx_usb_ms_ids,
> >         .driver         = {
> >                 .name   = "rtsx_usb_ms",
> > +#ifdef CONFIG_PM
> >                 .pm     = &rtsx_usb_ms_pm_ops,
> > +#endif
> >         },
> >  };
> >  module_platform_driver(rtsx_usb_ms_driver);
> > --
> > 2.17.1
>
> Kind regards
> Uffe