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;
Content-Type: text/plain;
        charset=us-ascii;
        delsp=yes;
        format=flowed
Mime-Version: 1.0 (Mac OS X Mail 11.5 \(3445.9.1\))
Subject: Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power
 management
From:   Kai-Heng Feng <kai.heng.feng@canonical.com>
In-Reply-To: <CAPDyKFqv5apJp=zbyoZi-UE8+omu7JauJ5_7+ymY9B2BSS48bA@mail.gmail.com>
Date:   Thu, 23 Aug 2018 16:03:51 +0800
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-Transfer-Encoding: 7bit
Message-Id: <E133AFB3-27F6-4DD3-BB85-A85EDB51EE57@canonical.com>
References: <20180731061721.15831-1-kai.heng.feng@canonical.com>
 <20180731061721.15831-5-kai.heng.feng@canonical.com>
 <CAPDyKFqv5apJp=zbyoZi-UE8+omu7JauJ5_7+ymY9B2BSS48bA@mail.gmail.com>
To:     Ulf Hansson <ulf.hansson@linaro.org>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Ulf,

Sorry for the late reply.

at 21:14, Ulf Hansson <ulf.hansson@linaro.org> wrote:

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

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