linux-kernel - Re: [PATCH] usb: ohci: Default to per-port over-current protection

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] [day] [month] [year] [list]

Message-ID: <26e5ca85dc566b1fa9b11d07c0d70cac1573525e.camel@alliedtelesis.co.nz>
Date:   Thu, 10 Sep 2020 21:10:39 +0000
From:   Hamish Martin <Hamish.Martin@...iedtelesis.co.nz>
To:     "stern@...land.harvard.edu" <stern@...land.harvard.edu>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: Re: [PATCH] usb: ohci: Default to per-port over-current protection

On Wed, 2020-09-09 at 10:05 -0400, Alan Stern wrote:
> On Wed, Sep 09, 2020 at 03:57:34PM +1200, Hamish Martin wrote:
> > Some integrated OHCI controller hubs do not expose all ports of the
> > hub
> > to pins on the SoC. In some cases the unconnected ports generate
> > spurious over-current events. For example the Broadcom 56060/Ranger
> > 2 SoC
> > contains a nominally 3 port hub but only the first port is wired.
> > 
> > Default behaviour for ohci-platform driver is to use global over-
> > current
> > protection mode (AKA "ganged"). This leads to the spurious over-
> > current
> > events affecting all ports in the hub.
> > 
> > We now alter the default to use per-port over-current protection.
> > 
> > This patch results in the following configuration changes depending
> > on quirks:
> > - For quirk OHCI_QUIRK_SUPERIO no changes. These systems remain set
> > up
> >   for ganged power switching and no over-current protection.
> 
> How about changing the quirk name to something more meaningful, such
> as 
> OHCI_QUIRK_GANGED_POWER_NO_OVERCURRENT?
I'll look at that in a separate patch later. The code I'm modifying
seems to be cloned in various drivers so perhaps that needs to be
centralised as the next step? See refs to OHCI_QUIRK_SUPERIO in u132-
hcd.c and ftdi-elan.c.

> 
> > - For quirk OHCI_QUIRK_AMD756 or OHCI_QUIRK_HUB_POWER power
> > switching
> >   remains at none, while over-current protection is now guaranteed
> > to be
> >   set to per-port rather than the previous behaviour where it was
> > either
> >   none or global over-current protection depending on the value at
> >   function entry.
> 
> Also consider renaming OHCI_QUIRK_HUB_POWER to something like 
> OHCI_QUIRK_PORT_POWER_ALWAYS_ON.
Ditto the above comment.

> 
> > Suggested-by: Alan Stern <stern@...land.harvard.edu>
> > Signed-off-by: Hamish Martin <hamish.martin@...iedtelesis.co.nz>
> > ---
> >  drivers/usb/host/ohci-hcd.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-
> > hcd.c
> > index dd37e77dae00..8ab81f6ab150 100644
> > --- a/drivers/usb/host/ohci-hcd.c
> > +++ b/drivers/usb/host/ohci-hcd.c
> > @@ -673,20 +673,25 @@ static int ohci_run (struct ohci_hcd *ohci)
> >  
> >  	/* handle root hub init quirks ... */
> >  	val = roothub_a (ohci);
> > -	val &= ~(RH_A_PSM | RH_A_OCPM);
> > +	/* Configure for per-port over-current protection by default */
> > +	val &= ~RH_A_NOCP;
> > +	val |= RH_A_OCPM;
> >  	if (ohci->flags & OHCI_QUIRK_SUPERIO) {
> > -		/* NSC 87560 and maybe others */
> > +		/* NSC 87560 and maybe others.
> > +		 * Ganged power switching, no over-current protection.
> > +		 */
> >  		val |= RH_A_NOCP;
> > -		val &= ~(RH_A_POTPGT | RH_A_NPS);
> > -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
> > +		val &= ~(RH_A_POTPGT | RH_A_NPS | RH_A_PSM |
> > RH_A_OCPM);
> >  	} else if ((ohci->flags & OHCI_QUIRK_AMD756) ||
> >  			(ohci->flags & OHCI_QUIRK_HUB_POWER)) {
> >  		/* hub power always on; required for AMD-756 and some
> > -		 * Mac platforms.  ganged overcurrent reporting, if
> > any.
> > +		 * Mac platforms.
> >  		 */
> > +		val &= ~RH_A_PSM;
> >  		val |= RH_A_NPS;
> 
> PSM is ignored when NPS is on.  You needn't bother to set it.
OK, I'll remove that.

> 
> > -		ohci_writel (ohci, val, &ohci->regs->roothub.a);
> >  	}
> > +	ohci_writel(ohci, val, &ohci->regs->roothub.a);
> > +
> >  	ohci_writel (ohci, RH_HS_LPSC, &ohci->regs->roothub.status);
> >  	ohci_writel (ohci, (val & RH_A_NPS) ? 0 : RH_B_PPCM,
> >  						&ohci->regs-
> > >roothub.b);
> 
> You didn't actually change the default: distrust_firmware is still 
> initialized to true.
Yes. I didn't need to for my system but I understand you want to do the
change. I'll make that a second patch in this series to keep the
logical changes quite separate in case of regression.

v2 (3?) series coming soon.

Thanks,
Hamish M.

> 
> Alan Stern