| 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: <20200918114021.5c67610e@coco.lan>
Date: Fri, 18 Sep 2020 11:40:21 +0200
From: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To: Rob Herring <robh@...nel.org>
Cc: Felipe Balbi <balbi@...nel.org>, Linuxarm <linuxarm@...wei.com>,
mauro.chehab@...wei.com, John Stultz <john.stultz@...aro.org>,
Manivannan Sadhasivam <mani@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Linux USB List <linux-usb@...r.kernel.org>,
devicetree@...r.kernel.org,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] dt-bindings: document a new quirk for dwc3
Em Thu, 17 Sep 2020 08:47:48 -0600
Rob Herring <robh@...nel.org> escreveu:
> On Thu, Sep 17, 2020 at 1:18 AM Mauro Carvalho Chehab
> <mchehab+huawei@...nel.org> wrote:
> >
> > Em Tue, 15 Sep 2020 10:38:14 -0600
> > Rob Herring <robh@...nel.org> escreveu:
> >
> > > On Tue, Sep 08, 2020 at 09:20:57AM +0200, Mauro Carvalho Chehab wrote:
> > > > At Hikey 970, setting the SPLIT disable at the General
> > > > User Register 3 is required.
> > > >
> > > > Without that, the URBs generated by the usbhid driver
> > > > return -EPROTO errors. That causes the code at
> > > > hid-core.c to call hid_io_error(), which schedules
> > > > a reset_work, causing a call to hid_reset().
> > > >
> > > > In turn, the code there will call:
> > > >
> > > > usb_queue_reset_device(usbhid->intf);
> > > >
> > > > The net result is that the input devices won't work, and
> > > > will be reset on every 0.5 seconds:
> > > >
> > > > [ 33.122384] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > > [ 33.378220] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > > [ 33.698394] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > > [ 34.882365] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > > [ 35.138217] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > > [ 35.458617] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > > [ 36.642392] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > > [ 36.898207] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > > [ 37.218598] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > > [ 38.402368] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > > [ 38.658174] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > > [ 38.978594] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0000
> > > > [ 40.162361] hub 1-1:1.0: state 7 ports 4 chg 0000 evt 0002
> > > > [ 40.418148] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > > ...
> > > > [ 397.698132] usb 1-1.1: reset low-speed USB device number 3 using xhci-hcd
> > > >
> > > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> > > > ---
> > > > Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > index d03edf9d3935..1aae2b6160c1 100644
> > > > --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> > > > @@ -78,6 +78,9 @@ Optional properties:
> > > > park mode are disabled.
> > > > - snps,dis_metastability_quirk: when set, disable metastability workaround.
> > > > CAUTION: use only if you are absolutely sure of it.
> > > > + - snps,dis-split-quirk: when set, change the way URBs are handled by the
> > > > + driver. Needed to avoid -EPROTO errors with usbhid
> > > > + on some devices (Hikey 970).
> > >
> > > Can't this be implied by the compatible string? Yes we have quirk
> > > properties already, but the problem with them is you can't address them
> > > without a DT change.
> >
> > Short answer:
> >
> > While technically doable, I don't think that this would be a good idea.
> >
> > -
> >
> > Long answer:
> >
> > The first thing is related to the compatible namespace: should such
> > quirk be added at SoC level or at board level?
> >
> > I don't know if the need to disable split mode came from a different
> > dwc3 variant used by the Kirin 970 SoC, or if this is due to the way
> > USB is wired at the Hikey 970 board.
>
> If board specific, then I agree that a separate property makes sense.
> This doesn't really sound board specific though.
Yeah, I agree that the higher chance is that this would be SoC specific,
but I can't discard being board-specific either, as, on this board,
all the output ports are all connected via an USB GPIO hub provided
on a different chipset. Funny enough, setting this is only required
for HID. The USB ports work fine either with split mode enabled or
disabled for USB sticks. So, I guess this affects only INT (and maybe
ISOC) URB transfers. So I wouldn't doubt that such quirk would be
required due to some limitation at the USB GPIO hub.
> > 2) Because this specific device uses the dwc3-of-simple driver,
> > the actual DT binding is:
> >
> > usb3: hisi_dwc3 {
> > compatible = "hisilicon,hi3670-dwc3";
> > ...
> > dwc3: dwc3@...00000 {
> > compatible = "snps,dwc3";
> > ...
> > };
> > };
>
> This parent child split should never have happened for the cases that
> don't have 'wrapper registers'. We should have had on node here with
> just:
>
> compatible = "hisilicon,hi3670-dwc3", "snps,dwc3";
I tried to do that, but the ARM CPU panics:
https://lore.kernel.org/linux-usb/731e13f9fbba3a81bedb39f1c1deaf41200acd0c.1599559004.git.mchehab+huawei@kernel.org/
From my tests, it sounds that this is due to some (minor) differences on
how the power clocks are enabled/suspended when using dwc3-of-simple as
the parent node.
It sounds that, if the PM clocks are not stable yet by the time
dwc3 tries to initialize, the CPU crashes.
> > For boards that use dwc3-of-simple drivers, we could add a hack
> > at the dwc3 core that would seek for the parent's device's
> > compatible string with something like (not tested):
> >
> > if (of_device_is_compatible(pdev->parent->dev.of_node,
> > "hisilicon,hi3670-dwc3"))
> > dwc->dis_split_quirk = true;
>
> This is what I'd do. You could have a match table instead as that
> would scale better.
Not sure if a match table would work here, because we need to
enforce that the parent node will be called before dwc3, as
otherwise the panic() will occur.
> > It should be noticed that both platform_data and pdev->parent
> > alternatives will only work for boards using dwc3-of-simple driver.
> > This could limit this quirk usage on future devices.
> >
> > -
> >
> > IMO, adding a new quirk is cleaner, and adopts the same solution
> > that it is currently used by other drivers with Designware IP.
>
> We already have a bunch of quirk properties. What's one more, sigh. So
> if that's what you want, fine.
Ok, thanks!
Thanks,
Mauro
Powered by blists - more mailing lists