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
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <87wpthnc2e.fsf@saruman.tx.rr.com>
Date:	Mon, 16 Nov 2015 10:28:25 -0600
From:	Felipe Balbi <balbi@...com>
To:	Douglas Anderson <dianders@...omium.org>,
	John Youn <John.Youn@...opsys.com>
CC:	Yunzhi Li <lyz@...k-chips.com>,
	Heiko Stübner <heiko@...ech.de>,
	<linux-rockchip@...ts.infradead.org>,
	Julius Werner <jwerner@...omium.org>,
	<gregory.herrero@...el.com>, <yousaf.kaukab@...el.com>,
	<dinguyen@...nsource.altera.com>,
	Douglas Anderson <dianders@...omium.org>,
	<johnyoun@...opsys.com>, <gregkh@...uxfoundation.org>,
	<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them


Hi,

Douglas Anderson <dianders@...omium.org> writes:
> In general it is wise to clear interrupts before processing them.  If
> you don't do that, you can get:
>  1. Interrupt happens
>  2. You look at system state and process interrupt
>  3. A new interrupt happens
>  4. You clear interrupt without processing it.
>
> This patch was actually a first attempt to fix missing device insertions
> as described in (usb: dwc2: host: Fix missing device insertions) and it
> did solve some of the signal bouncing problems but not all of
> them (which is why I submitted the other patch).  Specifically, this
> patch itself would sometimes change:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. hardware sees connect
>  4. dwc2_port_intr() - clears connect interrupt
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...to:
>  1. hardware sees connect
>  2. hardware sees disconnect
>  3. dwc2_port_intr() - clears connect interrupt
>  4. hardware sees connect
>  5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>
> ...but with different timing then sometimes we'd still miss cable
> insertions.
>
> In any case, though this patch doesn't fix any (known) problems, it
> still seems wise as a general policy to clear interrupt before handling
> them.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> Changes in v2: None
>
>  drivers/usb/dwc2/core_intr.c | 55 ++++++++++++++++++++++----------------------
>  drivers/usb/dwc2/hcd_intr.c  | 16 ++++++-------
>  2 files changed, 35 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 61601d16e233..2a166b7eec41 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg *hsotg)
>   */
>  static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>  {
> -	u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
> +	u32 hprt0;
>  
> +	/* Clear interrupt */
> +	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
> +
> +	hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>  	if (hprt0 & HPRT0_ENACHG) {
>  		hprt0 &= ~HPRT0_ENA;
>  		dwc2_writel(hprt0, hsotg->regs + HPRT0);
>  	}
> -
> -	/* Clear interrupt */
> -	dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);

isn't this a regression ? You're first clearing the interrupts and only
then reading to check what's pending, however, what's pending has just
been cleared. Seems like this should be:

hprt0 = dwc2_readl(HPRT0);
dwc2_writeal(PRTINT, GINTSTS);


Also, these two patches look very large for the -rc. I'll move them to
v4.5 merge window unless you can convince me that they are, indeed, the
smallest change possible to fix the problem you're facing and that they
are, indeed, regressions.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (819 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ