Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
In-Reply-To: <20180731061721.15831-5-kai.heng.feng@canonical.com>
References: <20180731061721.15831-1-kai.heng.feng@canonical.com> <20180731061721.15831-5-kai.heng.feng@canonical.com>
From:   Ulf Hansson <ulf.hansson@linaro.org>
Date:   Wed, 1 Aug 2018 15:14:18 +0200
Message-ID: <CAPDyKFqv5apJp=zbyoZi-UE8+omu7JauJ5_7+ymY9B2BSS48bA@mail.gmail.com>
Subject: Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power management
To:     Kai-Heng Feng <kai.heng.feng@canonical.com>
Cc:     Arnd Bergmann <arnd@arndb.de>,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        Alan Stern <stern@rowland.harvard.edu>,
        "Bauer.Chen" <bauer.chen@realtek.com>, ricky_wu@realtek.com,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linux USB List <linux-usb@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@canonical.com> 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@canonical.com>
> ---
>  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?

>                 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.

>         }
>
>  }
> @@ -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.

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

> +               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