| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <E133AFB3-27F6-4DD3-BB85-A85EDB51EE57@canonical.com>
Date: Thu, 23 Aug 2018 16:03:51 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Ulf Hansson <ulf.hansson@...aro.org>
Cc: Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alan Stern <stern@...land.harvard.edu>,
"Bauer.Chen" <bauer.chen@...ltek.com>, ricky_wu@...ltek.com,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux USB List <linux-usb@...r.kernel.org>
Subject: Re: [PATCH 4/5] memstick: rtsx_usb_ms: Support runtime power
management
Hi Ulf,
Sorry for the late reply.
at 21:14, Ulf Hansson <ulf.hansson@...aro.org> wrote:
> On 31 July 2018 at 08:17, Kai-Heng Feng <kai.heng.feng@...onical.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@...onical.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
Powered by blists - more mailing lists