| 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
| ||
|
Date: Wed, 1 Aug 2018 15:14:18 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Kai-Heng Feng <kai.heng.feng@...onical.com>
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
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?
> 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
Powered by blists - more mailing lists