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

From: Ulf Hansson
Date: Wed Oct 31 2018 - 06:40:55 EST

Next message: Vovo Yang: "Re: [PATCH v3] mm, drm/i915: mark pinned shmemfs pages as unevictable"
Previous message: Xiaowei Bao: "RE: [PATCH 5/6] pci: layerscape: Add the EP mode support."
In reply to: Kai-Heng Feng: "[PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 31 October 2018 at 07:59, 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.
>
> Put the device to suspend when there's no card and the power mode is
> MEMSTICK_POWER_OFF.
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> ---
> drivers/memstick/host/rtsx_usb_ms.c | 167 +++++++++++++++++-----------
> 1 file changed, 102 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/memstick/host/rtsx_usb_ms.c b/drivers/memstick/host/rtsx_usb_ms.c
> index cd12f3d1c088..3800c24b084e 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 system_suspending;
> };
>
> static inline struct device *ms_dev(struct rtsx_usb_ms *host)
> @@ -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_sync(ms_dev(host));
> }
>
> }
> @@ -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));
> 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));
> } else
> err = -EINVAL;
> if (!err)
> @@ -638,25 +637,44 @@ 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_sync(ms_dev(host));
>
> /* power-on delay */
> - if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON)
> + if (param == MEMSTICK_POWER && value == MEMSTICK_POWER_ON) {
> usleep_range(10000, 12000);
>
> + if (!host->eject)
> + schedule_delayed_work(&host->poll_card, 100);
> + }
> +
> dev_dbg(ms_dev(host), "%s: return = %d\n", __func__, err);
> return err;
> }
>
> -#ifdef CONFIG_PM_SLEEP
> +#ifdef CONFIG_PM

I think you should keep CONFIG_PM_SLEEP, else this triggers a warning
about unused functions, when CONFIG_PM is set but CONFIG_PM_SLEEP is
unset.

> static int rtsx_usb_ms_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__);
> + /* Since we use rtsx_usb's resume callback to runtime resume its
> + * children to implement remote wakeup signaling, this causes
> + * rtsx_usb_ms' runtime resume callback runs after its suspend
> + * callback:
> + * rtsx_usb_ms_suspend()
> + * rtsx_usb_resume()
> + * -> rtsx_usb_ms_runtime_resume()
> + * -> memstick_detect_change()
> + *
> + * rtsx_usb_suspend()
> + *
> + * To avoid this, skip runtime resume/suspend if system suspend is
> + * underway.
> + */
>
> + host->system_suspending = true;
> memstick_suspend_host(msh);
> +
> return 0;
> }
>
> @@ -665,58 +683,83 @@ static int rtsx_usb_ms_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->system_suspending = false;
> +
> 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)

Because of the above comment, then add:
#ifdef CONFIG_PM

> +static int rtsx_usb_ms_runtime_suspend(struct device *dev)
> +{
> + struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> +
> + if (host->system_suspending)
> + return 0;
> +
> + if (host->msh->card || host->power_mode != MEMSTICK_POWER_OFF)
> + return -EAGAIN;
> +
> + return 0;
> +}
> +
> +static int rtsx_usb_ms_runtime_resume(struct device *dev)
> +{
> + struct rtsx_usb_ms *host = dev_get_drvdata(dev);
> +
> +
> + if (host->system_suspending)
> + return 0;
> +
> + memstick_detect_change(host->msh);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops rtsx_usb_ms_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(rtsx_usb_ms_suspend, rtsx_usb_ms_resume)
> + SET_RUNTIME_PM_OPS(rtsx_usb_ms_runtime_suspend, rtsx_usb_ms_runtime_resume, NULL)
> +};
> +
> +#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);
> -
> - /* 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 (host->eject || host->power_mode != MEMSTICK_POWER_ON)
> + return;
>
> - /* 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_sync(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);
>
> - 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;
> +poll_again:
> + pm_runtime_put_sync(ms_dev(host));
> +
> + if (!host->eject && host->power_mode == MEMSTICK_POWER_ON)
> + schedule_delayed_work(&host->poll_card, 100);
> }
>
> static int rtsx_usb_ms_drv_probe(struct platform_device *pdev)
> @@ -747,39 +790,35 @@ 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));
> +
> + pm_runtime_get_sync(ms_dev(host));

I would rather re-place/re-order this with:

pm_runtime_get_noresume()
pm_runtime_set_active()
pm_runtime_enable()

> err = memstick_add_host(msh);
> if (err)
> goto err_out;
>
> - wake_up_process(host->detect_ms);
> + pm_runtime_put(ms_dev(host));
> +
> return 0;
> err_out:
> memstick_free_host(msh);

Looks like you need a pm_runtime_disable(); here as well. However,
that seems like an existing bug, perhaps it deserves it own change.

> + pm_runtime_put_noidle(ms_dev(host));
> return err;
> }
>

[...]

Kind regards
Uffe

Next message: Vovo Yang: "Re: [PATCH v3] mm, drm/i915: mark pinned shmemfs pages as unevictable"
Previous message: Xiaowei Bao: "RE: [PATCH 5/6] pci: layerscape: Add the EP mode support."
In reply to: Kai-Heng Feng: "[PATCH 4/4 v6] memstick: rtsx_usb_ms: Support runtime power management"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]