| 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: <159191771455.242598.12826819292508436205@swboyd.mtv.corp.google.com>
Date: Thu, 11 Jun 2020 16:21:54 -0700
From: Stephen Boyd <swboyd@...omium.org>
To: Andy Gross <agross@...nel.org>,
Bjorn Andersson <bjorn.andersson@...aro.org>,
Doug Anderson <dianders@...omium.org>,
Felipe Balbi <balbi@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Mark Rutland <mark.rutland@....com>,
Matthias Kaehlcke <mka@...omium.org>,
Michael Turquette <mturquette@...libre.com>,
Rob Herring <robh+dt@...nel.org>,
Sandeep Maheswaram <sanm@...eaurora.org>
Cc: linux-arm-msm@...r.kernel.org, linux-usb@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Manu Gautam <mgautam@...eaurora.org>,
linux-clk@...r.kernel.org, Taniya Das <tdas@...eaurora.org>,
Sandeep Maheswaram <sanm@...eaurora.org>
Subject: Re: [PATCH 2/2] usb: dwc3: Host wake up support from system suspend
Quoting Sandeep Maheswaram (2020-06-11 07:28:03)
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 25c686a7..8370350 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -31,15 +31,19 @@
> #include <linux/usb/gadget.h>
> #include <linux/usb/of.h>
> #include <linux/usb/otg.h>
> +#include <linux/usb/hcd.h>
>
> #include "core.h"
> #include "gadget.h"
> #include "io.h"
>
> #include "debug.h"
> +#include "../host/xhci.h"
>
> #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
>
> +bool need_phy_for_wakeup;
static? But why isn't it part of 'struct dwc3'? There could be multiple
dwc3 instances that may or may not be wakeup capable.
> +
> /**
> * dwc3_get_dr_mode - Validates and sets dr_mode
> * @dwc: pointer to our context structure
> @@ -1627,10 +1631,36 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> return ret;
> }
>
> +static void dwc3_set_phy_speed_flags(struct dwc3 *dwc)
> +{
> +
> + int i, num_ports;
> + u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
> + struct xhci_hcd *xhci_hcd = hcd_to_xhci(hcd);
> +
> + dwc->hs_phy_flags &= ~(PHY_MODE_USB_HOST_HS | PHY_MODE_USB_HOST_LS);
> +
> + reg = readl(&xhci_hcd->cap_regs->hcs_params1);
> +
> + num_ports = HCS_MAX_PORTS(reg);
> + for (i = 0; i < num_ports; i++) {
> + reg = readl(&xhci_hcd->op_regs->port_status_base + i*0x10);
Please format this as 'port_status_base + i * 0x10'
> + if (reg & PORT_PE) {
> + if (DEV_HIGHSPEED(reg) || DEV_FULLSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_HS;
> + else if (DEV_LOWSPEED(reg))
> + dwc->hs_phy_flags |= PHY_MODE_USB_HOST_LS;
> + }
> + }
> + phy_set_mode(dwc->usb2_generic_phy, dwc->hs_phy_flags);
> +}
> +
> static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> {
> unsigned long flags;
> u32 reg;
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
>
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 013f42a..ff02d41 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -1094,6 +1094,7 @@ struct dwc3 {
> struct phy *usb3_generic_phy;
>
> bool phys_ready;
> + int hs_phy_flags;
Does it need to be signed? Why not unsigned int or unsigned long?
>
> struct ulpi *ulpi;
> bool ulpi_ready;
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 1dfd024..ec183646 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -19,6 +19,7 @@
> #include <linux/usb/of.h>
> #include <linux/reset.h>
> #include <linux/iopoll.h>
> +#include <linux/usb/hcd.h>
>
> #include "core.h"
>
> @@ -192,21 +193,34 @@ static int dwc3_qcom_register_extcon(struct dwc3_qcom *qcom)
>
> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> {
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +
> if (qcom->hs_phy_irq) {
> disable_irq_wake(qcom->hs_phy_irq);
> disable_irq_nosync(qcom->hs_phy_irq);
> }
> + if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
> + if (qcom->dp_hs_phy_irq) {
> + disable_irq_wake(qcom->dp_hs_phy_irq);
> + disable_irq_nosync(qcom->dp_hs_phy_irq);
> + }
> + } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
> + if (qcom->dm_hs_phy_irq) {
> + disable_irq_wake(qcom->dm_hs_phy_irq);
> + disable_irq_nosync(qcom->dm_hs_phy_irq);
> + }
> + } else {
>
> - if (qcom->dp_hs_phy_irq) {
> - disable_irq_wake(qcom->dp_hs_phy_irq);
> - disable_irq_nosync(qcom->dp_hs_phy_irq);
> - }
> + if (qcom->dp_hs_phy_irq) {
> + disable_irq_wake(qcom->dp_hs_phy_irq);
> + disable_irq_nosync(qcom->dp_hs_phy_irq);
> + }
>
> - if (qcom->dm_hs_phy_irq) {
> - disable_irq_wake(qcom->dm_hs_phy_irq);
> - disable_irq_nosync(qcom->dm_hs_phy_irq);
> + if (qcom->dm_hs_phy_irq) {
> + disable_irq_wake(qcom->dm_hs_phy_irq);
> + disable_irq_nosync(qcom->dm_hs_phy_irq);
> + }
> }
> -
> if (qcom->ss_phy_irq) {
> disable_irq_wake(qcom->ss_phy_irq);
> disable_irq_nosync(qcom->ss_phy_irq);
> @@ -215,21 +229,34 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>
> static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> {
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> +
> if (qcom->hs_phy_irq) {
> enable_irq(qcom->hs_phy_irq);
> enable_irq_wake(qcom->hs_phy_irq);
> }
> + if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_LS) {
> + if (qcom->dp_hs_phy_irq) {
> + enable_irq(qcom->dp_hs_phy_irq);
> + enable_irq_wake(qcom->dp_hs_phy_irq);
> + }
> + } else if (dwc->hs_phy_flags & PHY_MODE_USB_HOST_HS) {
> + if (qcom->dm_hs_phy_irq) {
> + enable_irq(qcom->dm_hs_phy_irq);
> + enable_irq_wake(qcom->dm_hs_phy_irq);
> + }
> + } else {
>
> - if (qcom->dp_hs_phy_irq) {
> - enable_irq(qcom->dp_hs_phy_irq);
> - enable_irq_wake(qcom->dp_hs_phy_irq);
> - }
> + if (qcom->dp_hs_phy_irq) {
> + enable_irq(qcom->dp_hs_phy_irq);
> + enable_irq_wake(qcom->dp_hs_phy_irq);
> + }
>
> - if (qcom->dm_hs_phy_irq) {
> - enable_irq(qcom->dm_hs_phy_irq);
> - enable_irq_wake(qcom->dm_hs_phy_irq);
> + if (qcom->dm_hs_phy_irq) {
> + enable_irq(qcom->dm_hs_phy_irq);
> + enable_irq_wake(qcom->dm_hs_phy_irq);
> + }
> }
> -
> if (qcom->ss_phy_irq) {
> enable_irq(qcom->ss_phy_irq);
> enable_irq_wake(qcom->ss_phy_irq);
Is it possible to move this code to use the wakeirq library? I believe
only one irq can be the "wakeup" irq in that case but maybe that is
possible if we know what mode that phy is in? Or does the superspeed and
some sort of high speed irq need to be enabled for wakeup in case a usb2
or usb3 device wants to wakeup?
> @@ -240,6 +267,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom)
> {
> u32 val;
> int i;
> + struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> + struct usb_hcd *hcd = platform_get_drvdata(dwc->xhci);
Weird spacing here ---^
> +
> + if (usb_wakeup_enabled_descendants(hcd->self.root_hub))
> + device_init_wakeup(qcom->dev, 1);
>
> if (qcom->is_suspended)
> return 0;
> @@ -262,6 +294,8 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom)
> int ret;
> int i;
>
> + device_init_wakeup(qcom->dev, 0);
Usually device_init_wakeup() is called once during probe and then the
wakeup enable state for a device is controlled from userspace. Calling
this here will be semi-disastrous in the sense that we're going to be
creating and destroying a wakeup sysfs object each time we suspend. I
see that dwc3 core code has this pattern of calling device_init_wakeup()
from the suspend/resume path too, which looks wrong.
Shouldn't we be forwarding the wakeup request from the root hub to the
controller? I'm not super clear on how USB PM is supposed to work but
this doesn't look right.
Powered by blists - more mailing lists