Re: [PATCH v2 1/2] usb: ohci: Default to per-port over-current protection

[Hamish Martin <Hamish.Martin@alliedtelesis.co.nz>]

On Sat, 2021-01-09 at 16:26 -0500, Alan Stern wrote:
> On Sun, Dec 27, 2020 at 12:22:34PM +0100, Paul Kocialkowski wrote:
> > Hi,
> 
> Sorry it has taken so long to respond to this.  The holidays
> intervened, 
> but that's no excuse.
I'm sorry too, same reason/non-excuse. Thanks for your thorough report
on the issue my changes caused and pass on my apologies to your Mom!

> 
> > On Fri 11 Sep 20, 09:25, 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 specific patch lead to breaking OHCI on my mom's laptop (whom
> > was about
> > to buy a new one thinking the hardware had failed). I get no OHCI
> > interrupt at
> > all and no USB 1 device is ever detected.
> > 
> > I haven't really found a reasonable explanation about why that is,
> > but here
> > are some notes I was able to collect:
> > - The issue showed up on 5.8,18 and 5.9.15, which don't include the
> > patch
> >   from this series that sets distrust_firmware = false; This
> > results in the NPS
> >   bit being set via OHCI_QUIRK_HUB_POWER.
> > - Adding val &= ~RH_A_PSM; (as was done before this change) solves
> > the issue
> >   which is weird because the bit is supposed to be inactive when
> > NPS is set;
> > - Setting ohci_hcd.distrust_firmware=0 in the cmdline results in
> > not setting
> >   the NPS bit and also solves the issue;
> > - The initial value of the register at function entry is 0x1001104
> > (PSM bit
> >   is set, NPS is unset);
> > - The OHCI controller is the following:
> > 00:03.0 USB controller: Silicon Integrated Systems [SiS] USB 1.1
> > Controller (rev 0f) (prog-if 10 [OHCI])
> > 	Subsystem: ASUSTeK Computer Inc. Device 1aa7
> 
> Great reporting -- thanks.
> 
> > Does that make any sense to you?
> > 
> > I really wonder what a proper fix could be and here are some
> > suggestions:
> > - Adding a specific quirk to clear the PSM bit for this hardware
> > which seems to
> >   consider the bit regardless of NPS;
> 
> We don't need a quirk for this.  There shouldn't be anything wrong
> with 
> _always_ clearing PSM whenever NPS is set, since the controller is 
> supposed to ignore PSM under that condition.
> 
> Would you like to submit a patch for this?
Yes, I think that looks reasonable too.

> 
> > - Adding the patch that sets distrust_firmware = false to stable
> > branches;
> 
> That's certainly reasonable.  Nobody has reported any problems caused
> by 
> that patch, so adding it to the stable branches should be safe
> enough.
> 
Yes, that is probably a good idea. I've carried both patches locally
for my systems.

> > What do you think?
> 
> We could even do both.  That would help if, for example, somebody 
> decided to set ohci_hcd.distrust_firmware=true explicitly.
I think both might be best.

> 
> Greg, in the meantime can we have commit c4005a8f65ed ("usb: ohci:
> Make 
> distrust_firmware param default to false") added to all the stable 
> kernels which have back-ported versions of commit b77d2a0a223b?
> 
> Alan Stern
I second that.

Thanks,
Hamish Martin