| 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: <20190412235633.GY112750@google.com>
Date: Fri, 12 Apr 2019 16:56:33 -0700
From: Matthias Kaehlcke <mka@...omium.org>
To: Douglas Anderson <dianders@...omium.org>
Cc: Rob Herring <robh+dt@...nel.org>,
Minas Harutyunyan <hminas@...opsys.com>,
Heiko Stuebner <heiko@...ech.de>,
Felipe Balbi <felipe.balbi@...ux.intel.com>,
amstan@...omium.org, linux-rockchip@...ts.infradead.org,
linux-usb@...r.kernel.org, Randy Li <ayaka@...lik.info>,
ryandcase@...omium.org, jwerner@...omium.org,
Elaine Zhang <zhangqing@...k-chips.com>,
Yunzhi Li <lyz@...k-chips.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/4] usb: dwc2: optionally assert phy reset when waking up
On Fri, Apr 12, 2019 at 03:41:47PM -0700, Douglas Anderson wrote:
> On the rk3288 USB host-only port (the one that's not the OTG-enabled
> port) the PHY can get into a bad state when a wakeup is asserted (not
> just a wakeup from full system suspend but also a wakeup from
> autosuspend).
>
> We can get the PHY out of its bad state by asserting its "port reset",
> but unfortunately that seems to assert a reset onto the USB bus so it
> could confuse things if we don't actually deenumerate / reenumerate the
> device.
>
> We can also get the PHY out of its bad state by fully resetting it using
> the reset from the CRU (clock reset unit), which does a more full
> reset. The CRU-based reset appears to actually cause devices on the bus
> to be removed and reinserted, which fixes the problem (albeit in a hacky
> way).
>
> It's unfortunate that we need to do a full re-enumeration of devices at
> wakeup time, but this is better than alternative of letting the bus get
> wedged.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> Signed-off-by: Yunzhi Li <lyz@...k-chips.com>
> ---
>
> drivers/usb/dwc2/core.h | 5 +++++
> drivers/usb/dwc2/core_intr.c | 12 ++++++++++++
> drivers/usb/dwc2/hcd.c | 16 +++++++++++++---
> drivers/usb/dwc2/platform.c | 9 +++++++++
> 4 files changed, 39 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index 30bab8463c96..f30748f4fe7e 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -859,6 +859,8 @@ struct dwc2_hregs_backup {
> * @gadget_enabled: Peripheral mode sub-driver initialization indicator.
> * @ll_hw_enabled: Status of low-level hardware resources.
> * @hibernated: True if core is hibernated
> + * @reset_phy_on_wake: Quirk saying that we should assert PHY reset on a
> + * remote wakeup.
> * @frame_number: Frame number read from the core. For both device
> * and host modes. The value ranges are from 0
> * to HFNUM_MAX_FRNUM.
> @@ -972,6 +974,7 @@ struct dwc2_hregs_backup {
> * @status_buf_dma: DMA address for status_buf
> * @start_work: Delayed work for handling host A-cable connection
> * @reset_work: Delayed work for handling a port reset
> + * @phy_reset_work: Work structure for doing a PHY reset
> * @otg_port: OTG port number
> * @frame_list: Frame list
> * @frame_list_dma: Frame list DMA address
> @@ -1045,6 +1048,7 @@ struct dwc2_hsotg {
> unsigned int gadget_enabled:1;
> unsigned int ll_hw_enabled:1;
> unsigned int hibernated:1;
> + unsigned int reset_phy_on_wake:1;
> u16 frame_number;
>
> struct phy *phy;
> @@ -1147,6 +1151,7 @@ struct dwc2_hsotg {
>
> struct delayed_work start_work;
> struct delayed_work reset_work;
> + struct work_struct phy_reset_work;
> u8 otg_port;
> u32 *frame_list;
> dma_addr_t frame_list_dma;
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 19ae2595f1c3..16ff33574116 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -435,6 +435,18 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
> /* Restart the Phy Clock */
> pcgcctl &= ~PCGCTL_STOPPCLK;
> dwc2_writel(hsotg, pcgcctl, PCGCTL);
> +
> + /*
> + * If we've got this quirk then the PHY is stuck upon
> + * wakeup. Assert reset. This will propagate out and
> + * eventually we'll re-enumerate the device. Not great
> + * but the best we can do. We can't call phy_reset()
> + * at interrupt time but there's no hurry, so we'll
> + * schedule it for later.
> + */
> + if (hsotg->reset_phy_on_wake)
> + schedule_work(&hsotg->phy_reset_work);
> +
> mod_timer(&hsotg->wkp_timer,
> jiffies + msecs_to_jiffies(71));
> } else {
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 3f087962f498..332349148d3f 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -4376,6 +4376,17 @@ static void dwc2_hcd_reset_func(struct work_struct *work)
> spin_unlock_irqrestore(&hsotg->lock, flags);
> }
>
> +static void dwc2_hcd_phy_reset_func(struct work_struct *work)
> +{
> + struct dwc2_hsotg *hsotg = container_of(work, struct dwc2_hsotg,
> + phy_reset_work);
> + int ret;
> +
> + ret = phy_reset(hsotg->phy);
> + if (ret)
> + dev_warn(hsotg->dev, "PHY reset failed\n");
> +}
> +
> /*
> * =========================================================================
> * Linux HC Driver Functions
> @@ -5271,11 +5282,10 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> hsotg->hc_ptr_array[i] = channel;
> }
>
> - /* Initialize hsotg start work */
> + /* Initialize work */
> INIT_DELAYED_WORK(&hsotg->start_work, dwc2_hcd_start_func);
> -
> - /* Initialize port reset work */
> INIT_DELAYED_WORK(&hsotg->reset_work, dwc2_hcd_reset_func);
> + INIT_WORK(&hsotg->phy_reset_work, dwc2_hcd_phy_reset_func);
You also want to make sure that the work is cancelled when the
controller is stopped/removed. It seems dwc2_hcd_stop() would be a
suitable place for that.
Powered by blists - more mailing lists