| 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: <CAATdQgCV+M5J1z6n9HcouOvoX34bofrb6E2e-BBimJTqmHs=gQ@mail.gmail.com>
Date: Tue, 29 Dec 2020 15:38:37 +0800
From: Ikjoon Jang <ikjn@...omium.org>
To: Chunfeng Yun <chunfeng.yun@...iatek.com>
Cc: Tianping Fang <tianping.fang@...iatek.com>,
Zhanyong Wang <zhangyong.wang@...iatek.com>,
"moderated list:ARM/Mediatek SoC support"
<linux-mediatek@...ts.infradead.org>, linux-usb@...r.kernel.org,
open list <linux-kernel@...r.kernel.org>,
CK Hu <ck.hu@...iatek.com>,
Zhanyong Wang <zhanyong.wang@...iatek.com>
Subject: Re: [RFC PATCH v3 4/5] usb: xhci-mtk: add support runtime pm
On Tue, Dec 22, 2020 at 5:35 PM Chunfeng Yun <chunfeng.yun@...iatek.com> wrote:
>
> From: CK Hu <ck.hu@...iatek.com>
>
> add support runtime pm feature
>
> Signed-off-by: Zhanyong Wang <zhanyong.wang@...iatek.com>
> Signed-off-by: Chunfeng Yun <chunfeng.yun@...iatek.com>
> ---
> v3:
> 1. fix some issues
> 2. remove attribute files
>
> v2: fix error caused by request irq suggested by CK
> ---
> drivers/usb/host/xhci-mtk.c | 285 +++++++++++++++++++++++++++++++++++-
> drivers/usb/host/xhci-mtk.h | 14 ++
> 2 files changed, 294 insertions(+), 5 deletions(-)
> mode change 100755 => 100644 drivers/usb/host/xhci-mtk.c
>
> diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
> old mode 100755
> new mode 100644
> index 34bd3de090b1..c07d54acbcb7
> --- a/drivers/usb/host/xhci-mtk.c
> +++ b/drivers/usb/host/xhci-mtk.c
> @@ -14,6 +14,7 @@
> #include <linux/mfd/syscon.h>
> #include <linux/module.h>
> #include <linux/of.h>
> +#include <linux/of_irq.h>
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/regmap.h>
> @@ -77,6 +78,72 @@ enum ssusb_uwk_vers {
> SSUSB_UWK_V3,
> };
>
> +int xhci_mtk_runtime_ready;
> +
> +static int xhci_mtk_runtime_resume(struct device *dev);
> +
> +static void xhci_mtk_seal_work(struct work_struct *work)
> +{
> + struct xhci_hcd_mtk *mtk =
> + container_of(work, struct xhci_hcd_mtk, seal.work);
> + struct usb_hcd *hcd = mtk->hcd;
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> + xhci_dbg(xhci, "spm unseals xHCI controller %i\n", mtk->seal_status);
> + if (mtk->seal_status == SEAL_SUSPENDED) {
> + mtk->seal_status = SEAL_RESUMING;
> + pm_runtime_mark_last_busy(mtk->dev);
> + pm_runtime_put_sync_autosuspend(mtk->dev);
If I understand correctly, this function is for waking up the device
but this function calls put() only without get().
> + } else {
> + /*
> + * FIXME: Sometimes seal_status will keep it on SEAL_RESUMING staus not to expect
> + * since pm_runtime_put_sync_autosuspend doesn't invoke runtime_resume of callback.
> + * and to survey why not to invoke runtime_resume callback named
> + * xhci_mtk_runtime_resume in time in short feature, Herely provide one solution
> + * that make sure seal_status to match h/w state machine,and MTK xHCI clocks
> + * on as soon as unseal event received.
I guess actual resuming should be happened only from the 1st interrupt
(when in SEAL_SUSPENDED)
and following spurious interrupts can be just ignored.
> + */
> + if (mtk->seal_status == SEAL_RESUMING)
> + xhci_mtk_runtime_resume(mtk->dev);
xhci_mtk_runtime_resume() is defined as a runtime pm callback,
pm core will call that callback when pm usage counter reaches to zero.
> + else
> + xhci_warn(xhci,
> + "Ignore seal wakeup source disordered in xHCI controller\n");
> + }
> +}
> +
> +static irqreturn_t xhci_mtk_seal_irq(int irq, void *data)
> +{
> + struct xhci_hcd_mtk *mtk = data;
> + struct usb_hcd *hcd = mtk->hcd;
> + struct xhci_hcd *xhci = hcd_to_xhci(hcd);
> +
> + xhci_dbg(xhci, "seal irq ISR invoked\n");
> +
> + schedule_delayed_work(&mtk->seal, 0);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static void xhci_mtk_seal_wakeup_enable(struct xhci_hcd_mtk *mtk, bool enable)
> +{
> + struct irq_desc *desc;
> + struct device *dev = mtk->dev;
> +
> + if (mtk && mtk->seal_irq) {
> + desc = irq_to_desc(mtk->seal_irq);
> + if (enable) {
> + desc->irq_data.chip->irq_ack(&desc->irq_data);
> + enable_irq(mtk->seal_irq);
> + dev_dbg(dev, "%s: enable_irq %i\n",
> + __func__, mtk->seal_irq);
> + } else {
> + disable_irq(mtk->seal_irq);
> + dev_dbg(dev, "%s: disable_irq %i\n",
> + __func__, mtk->seal_irq);
> + }
> + }
> +}
> +
I think this is unnecessary if this driver can check the current state
and ignore the spurious irqs if spm sometimes triggers the wake-up irqs.
> static int xhci_mtk_host_enable(struct xhci_hcd_mtk *mtk)
> {
> struct mu3c_ippc_regs __iomem *ippc = mtk->ippc_regs;
> @@ -347,7 +414,6 @@ static int usb_wakeup_of_property_parse(struct xhci_hcd_mtk *mtk,
> mtk->uwk_reg_base, mtk->uwk_vers);
>
> return PTR_ERR_OR_ZERO(mtk->uwk);
> -
> }
>
> static void usb_wakeup_set(struct xhci_hcd_mtk *mtk, bool enable)
> @@ -482,9 +548,11 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> return ret;
> }
>
> + pm_runtime_set_active(dev);
> + pm_runtime_use_autosuspend(dev);
> + pm_runtime_set_autosuspend_delay(dev,
> + CONFIG_USB_AUTOSUSPEND_DELAY * 1000);
> pm_runtime_enable(dev);
> - pm_runtime_get_sync(dev);
The only one left pm_runtime_get() is removed by here,
now this driver only calls pm_runtime_put().
> - device_enable_async_suspend(dev);
>
> ret = xhci_mtk_ldos_enable(mtk);
> if (ret)
> @@ -499,6 +567,14 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> ret = irq;
> goto disable_clk;
> }
> + dev_dbg(dev, "irq %i\n", irq);
> +
> + mtk->seal_irq = platform_get_irq_optional(pdev, 1);
> + if (mtk->seal_irq < 0) {
> + ret = mtk->seal_irq;
> + goto disable_clk;
> + }
> + dev_dbg(dev, "seal_irq %i\n", mtk->seal_irq);
>
> hcd = usb_create_hcd(driver, dev, dev_name(dev));
> if (!hcd) {
> @@ -565,6 +641,27 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> if (ret)
> goto dealloc_usb2_hcd;
>
> + INIT_DELAYED_WORK(&mtk->seal, xhci_mtk_seal_work);
> + snprintf(mtk->seal_descr, sizeof(mtk->seal_descr), "seal%s:usb%d",
> + hcd->driver->description, hcd->self.busnum);
> + ret = devm_request_irq(dev, mtk->seal_irq, &xhci_mtk_seal_irq,
> + IRQF_TRIGGER_FALLING, mtk->seal_descr, mtk);
> + if (ret != 0) {
> + dev_err(dev, "seal request interrupt %d failed\n",
> + mtk->seal_irq);
> + goto dealloc_usb2_hcd;
> + }
> + xhci_mtk_seal_wakeup_enable(mtk, false);
> +
> + device_enable_async_suspend(dev);
> + xhci_mtk_runtime_ready = 1;
> +
> + pm_runtime_mark_last_busy(dev);
> + pm_runtime_put_autosuspend(dev);
I expect the usage count will be -1 by here in probe.
> +
> + dev_dbg(dev, "%s: xhci_mtk_runtime_ready %i",
> + __func__, xhci_mtk_runtime_ready);
> +
> return 0;
>
> dealloc_usb2_hcd:
> @@ -587,7 +684,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
> xhci_mtk_ldos_disable(mtk);
>
> disable_pm:
> - pm_runtime_put_sync(dev);
> + pm_runtime_put_sync_autosuspend(dev);
> pm_runtime_disable(dev);
> return ret;
> }
> @@ -602,6 +699,7 @@ static int xhci_mtk_remove(struct platform_device *dev)
> pm_runtime_put_noidle(&dev->dev);
> pm_runtime_disable(&dev->dev);
>
> + xhci_mtk_runtime_ready = 0;
> usb_remove_hcd(shared_hcd);
> xhci->shared_hcd = NULL;
> device_init_wakeup(&dev->dev, false);
> @@ -638,6 +736,7 @@ static int __maybe_unused xhci_mtk_suspend(struct device *dev)
> xhci_mtk_host_disable(mtk);
> xhci_mtk_clks_disable(mtk);
> usb_wakeup_set(mtk, true);
> +
> return 0;
> }
>
> @@ -659,10 +758,185 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
> return 0;
> }
>
> +static int __maybe_unused xhci_mtk_bus_status(struct device *dev)
> +{
> + struct xhci_hcd_mtk *mtk = dev_get_drvdata(dev);
> + struct usb_hcd *hcd;
> + struct xhci_hcd *xhci;
> + struct xhci_hub *usb2_rhub;
> + struct xhci_hub *usb3_rhub;
> + struct xhci_bus_state *bus_state;
> + struct xhci_port *port;
> + u32 usb2_suspended_ports = -1;
> + u32 usb3_suspended_ports = -1;
> + u16 status;
> + int num_ports;
> + int ret = 0;
> + int i;
> +
> + if (!mtk->hcd)
> + return -ESHUTDOWN;
> +
> + hcd = mtk->hcd;
> + xhci = hcd_to_xhci(hcd);
> + if ((xhci->xhc_state & XHCI_STATE_REMOVING) ||
> + (xhci->xhc_state & XHCI_STATE_HALTED)) {
> + return -ESHUTDOWN;
> + }
This is duplicated from xhci_mtk_runtime_suspend()
> +
> + usb2_rhub = &xhci->usb2_rhub;
> + if (usb2_rhub) {
> + bus_state = &usb2_rhub->bus_state;
> + num_ports = usb2_rhub->num_ports;
> + usb2_suspended_ports = bus_state->suspended_ports;
> + usb2_suspended_ports ^= (BIT(num_ports) - 1);
> + usb2_suspended_ports &= (BIT(num_ports) - 1);
> + for (i = 0; i < num_ports; i++) {
> + if (usb2_suspended_ports & (1UL << i)) {
> + port = usb2_rhub->ports[i];
> + status = readl(port->addr);
> +
> + xhci_dbg(xhci,
> + "USB20: portsc[%i]: 0x%04X\n",
> + i, status);
> +
> + if (!(status & PORT_CONNECT))
> + usb2_suspended_ports &= ~(1UL << i);
> + }
> + }
> +
> + if (usb2_suspended_ports) {
> + ret = -EBUSY;
> + goto ebusy;
> + }
> + }
> +
> + usb3_rhub = &xhci->usb3_rhub;
> + if (usb3_rhub) {
> + bus_state = &usb3_rhub->bus_state;
> + num_ports = usb3_rhub->num_ports;
> + usb3_suspended_ports = bus_state->suspended_ports;
> + usb3_suspended_ports ^= (BIT(num_ports) - 1);
> + usb3_suspended_ports &= (BIT(num_ports) - 1);
> + for (i = 0; i < num_ports; i++) {
> + if (usb3_suspended_ports & BIT(i)) {
> + port = usb3_rhub->ports[i];
> + status = readl(port->addr);
> +
> + xhci_dbg(xhci, "USB3: portsc[%i]: 0x%04X\n",
> + i, status);
> +
> + if (!(status & PORT_CONNECT))
> + usb3_suspended_ports &= ~BIT(i);
> + }
> + }
> +
> + if (usb3_suspended_ports) {
> + ret = -EBUSY;
> + goto ebusy;
> + }
> + }
> +
> +ebusy:
> + xhci_dbg(xhci, "%s: USB2: 0x%08X, USB3: 0x%08X ret: %i\n",
> + __func__, usb2_suspended_ports,
> + usb3_suspended_ports, ret);
> +
> + return ret;
> +}
> +
This is basically counting active ports by directly reading portsc register?
I expect this function never return -EBUSY if you balance pm usage
counter well.
Are there any specific reasons of doing this manually?
> +static int __maybe_unused xhci_mtk_runtime_suspend(struct device *dev)
> +{
> + bool wakeup = device_may_wakeup(dev);
> + struct xhci_hcd_mtk *mtk = dev_get_drvdata(dev);
> + struct usb_hcd *hcd;
> + struct xhci_hcd *xhci;
> + int ret = 0;
> +
> + if (!mtk->hcd)
> + return -ESHUTDOWN;
> +
> + hcd = mtk->hcd;
> + xhci = hcd_to_xhci(hcd);
> + if ((xhci->xhc_state & XHCI_STATE_REMOVING) ||
> + (xhci->xhc_state & XHCI_STATE_HALTED)) {
> + return -ESHUTDOWN;
> + }
> +
> + mtk->seal_status = SEAL_BUSY;
> + ret = xhci_mtk_bus_status(dev);
> + if (wakeup && !ret) {
> + mtk->seal_status = SEAL_SUSPENDING;
> + xhci_mtk_suspend(dev);
> + xhci_mtk_seal_wakeup_enable(mtk, true);
> + mtk->seal_status = SEAL_SUSPENDED;
> + xhci_dbg(xhci, "%s: seals xHCI controller\n", __func__);
> + }
> +
> + xhci_dbg(xhci, "%s: seals wakeup = %i, ret = %i!\n",
> + __func__, wakeup, ret);
> +
> + return ret;
> +}
> +
> +static int __maybe_unused xhci_mtk_runtime_resume(struct device *dev)
> +{
> + bool wakeup = device_may_wakeup(dev);
> + struct xhci_hcd_mtk *mtk = dev_get_drvdata(dev);
> + struct usb_hcd *hcd;
> + struct xhci_hcd *xhci;
> +
> + if (!mtk->hcd)
> + return -ESHUTDOWN;
> +
> + hcd = mtk->hcd;
> + xhci = hcd_to_xhci(hcd);
> + if ((xhci->xhc_state & XHCI_STATE_REMOVING) ||
> + (xhci->xhc_state & XHCI_STATE_HALTED)) {
> + return -ESHUTDOWN;
> + }
> +
> + /*
> + * list cases by one extra interrupt named seal to process!!!
> + * Who to process these module reinitilization after SPM wakeup
> + * case 1: usb remote wakeup, therefore xHCI need reinitilizate also.
> + * case 2: other-wakeup-source wakeup, therefore, xHCI need reinit
> + * case 3: usb client driver can invoke it in runtime mechanism
> + * case 4: user active
> + */
> + if (wakeup) {
> + xhci_mtk_seal_wakeup_enable(mtk, false);
> + xhci_mtk_resume(dev);
> + xhci_dbg(xhci, "%s: unseals xHCI controller\n", __func__);
> + }
> + mtk->seal_status = SEAL_RESUMED;
> +
> + xhci_dbg(xhci, "%s: unseals wakeup = %i\n", __func__, wakeup);
> +
> + return 0;
> +}
> +
> +static int __maybe_unused xhci_mtk_runtime_idle(struct device *dev)
> +{
> + int ret = 0;
> +
> + dev_dbg(dev, "%s: xhci_mtk_runtime_ready %i",
> + __func__, xhci_mtk_runtime_ready);
> +
> + if (!xhci_mtk_runtime_ready)
> + ret = -EAGAIN;
> +
> + return ret;
> +}
> +
> static const struct dev_pm_ops xhci_mtk_pm_ops = {
> SET_SYSTEM_SLEEP_PM_OPS(xhci_mtk_suspend, xhci_mtk_resume)
> + SET_RUNTIME_PM_OPS(xhci_mtk_runtime_suspend,
> + xhci_mtk_runtime_resume,
> + xhci_mtk_runtime_idle)
> };
> -#define DEV_PM_OPS IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL
> +
> +#define DEV_PM_OPS (IS_ENABLED(CONFIG_PM) ? &xhci_mtk_pm_ops : NULL)
>
> #ifdef CONFIG_OF
> static const struct of_device_id mtk_xhci_of_match[] = {
> @@ -686,6 +960,7 @@ MODULE_ALIAS("platform:xhci-mtk");
>
> static int __init xhci_mtk_init(void)
> {
> + xhci_mtk_runtime_ready = 0;
> xhci_init_driver(&xhci_mtk_hc_driver, &xhci_mtk_overrides);
> return platform_driver_register(&mtk_xhci_driver);
> }
> diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-mtk.h
> index 323b281933b9..103d83ce6a3e 100644
> --- a/drivers/usb/host/xhci-mtk.h
> +++ b/drivers/usb/host/xhci-mtk.h
> @@ -133,6 +133,14 @@ struct mu3c_ippc_regs {
> __le32 reserved3[33]; /* 0x80 ~ 0xff */
> };
>
> +enum xhci_mtk_seal {
> + SEAL_BUSY = 0,
> + SEAL_SUSPENDING,
> + SEAL_SUSPENDED,
> + SEAL_RESUMING,
> + SEAL_RESUMED
> +};
> +
> struct xhci_hcd_mtk {
> struct device *dev;
> struct usb_hcd *hcd;
> @@ -158,6 +166,12 @@ struct xhci_hcd_mtk {
> struct regmap *uwk;
> u32 uwk_reg_base;
> u32 uwk_vers;
> +
> + /* usb eint wakeup source */
> + int seal_irq;
> + enum xhci_mtk_seal seal_status;
> + struct delayed_work seal;
> + char seal_descr[32]; /* "seal" + driver + bus # */
> };
>
> static inline struct xhci_hcd_mtk *hcd_to_mtk(struct usb_hcd *hcd)
> --
> 2.18.0
>
Powered by blists - more mailing lists