linux-kernel - Re: [PATCH v6 04/13] usb: dwc3: Add splitdisable quirk for Hisilicon Kirin Soc

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20200907165000.7c42a6da@coco.lan>
Date:   Mon, 7 Sep 2020 16:50:00 +0200
From:   Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
To:     Felipe Balbi <balbi@...nel.org>, Yu Chen <chenyu56@...wei.com>
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-usb@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, john.stultz@...aro.org,
        suzhuangluan@...ilicon.com, kongfei@...ilicon.com,
        liuyu712@...ilicon.com, wanghu17@...ilicon.com,
        butao@...ilicon.com, chenyao11@...wei.com,
        fangshengzhou@...ilicon.com, lipengcheng8@...wei.com,
        songxiaowei@...ilicon.com, xuyiping@...ilicon.com,
        xuyoujun4@...wei.com, yudongbin@...ilicon.com,
        zangleigang@...ilicon.com,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Binghui Wang <wangbinghui@...ilicon.com>
Subject: Re: [PATCH v6 04/13] usb: dwc3: Add splitdisable quirk for
 Hisilicon Kirin Soc

Em Mon, 07 Sep 2020 17:04:31 +0300
Felipe Balbi <balbi@...nel.org> escreveu:

> Hi Mauro,
> 
> Mauro Carvalho Chehab <mchehab+huawei@...nel.org> writes:
> 
> > Hi Felipe/Greg,
> >
> > What's the status of this patch?   
> 
> to be frank, I don't think I have this in my inbox anymore.
> 
> > I tested here, together with the Hikey 970 phy RFC patches I sent
> > last week.
> >
> > Without this patch, the USB HID driver receives -EPROTO from
> > submitted URBs, causing it to enter into an endless reset cycle
> > on every 500 ms, at the hid_io_error() logic.  
> 
> > Tested-by: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
> >
> > If you prefer, I can re-submit this one with my SOB.  
> 
> Please do, but since you're changing device tree, I need Rob's acked-by.

Ok, I'll do that.

> > Thanks,
> > Mauro
> >
> > Em Sat, 20 Apr 2019 14:40:10 +0800
> > Yu Chen <chenyu56@...wei.com> escreveu:
> >  
> >> SPLIT_BOUNDARY_DISABLE should be set for DesignWare USB3 DRD Core
> >> of Hisilicon Kirin Soc when dwc3 core act as host.  
> 
> is this Kirin-specific or is this something that we should do a revision
> check? 

I've no idea. I don't have any datasheets from this device.

> Why does it affect only Hikey kirin? 

As John Stultz didn't re-submit this one (and looking at the DT
between Kirin 960 and 970 from the original Kernel 4.9 official
drivers), I suspect that only Kirin 970 requires this quirk.

It could well be due to some Dwc3 revision, but it could also be due
to some differences at the USB part of the SoC, as there are a
few other things different between hikey 960 and 970: it has a
different PHY driver, and there are also some differences at the
USB HUB which is connected into it.

On both devices, the USB physical ports are actually connected
into a HUB. In the case of Hikey 970, the hub seems to be a
TI TUSB8041 4-Port Hub:
	
	$ lsusb
	Bus 002 Device 002: ID 0451:8140 Texas Instruments, Inc. TUSB8041 4-Port Hub
	Bus 002 Device 001: ID 1d6b:0003 Linux Foundation 3.0 root hub
	Bus 001 Device 004: ID 090c:1000 Silicon Motion, Inc. - Taiwan (formerly Feiya Technology Corp.) Flash Drive
	Bus 001 Device 003: ID 413c:301a Dell Computer Corp. Dell MS116 Optical Mouse
	Bus 001 Device 002: ID 0451:8142 Texas Instruments, Inc. TUSB8041 4-Port Hub
	Bus 001 Device 001: ID 1d6b:0002 Linux Foundation 2.0 root hub

> What's the dwc3 revision on
> that SoC (grep SNPSID /sys/kernel/debugfs/*dwc3/regdump)?

	GSNPSID = 0x33313130

> 
> >> @@ -1825,10 +1834,27 @@ static int dwc3_resume(struct device *dev)
> >>  
> >>  	return 0;
> >>  }
> >> +
> >> +static void dwc3_complete(struct device *dev)
> >> +{
> >> +	struct dwc3	*dwc = dev_get_drvdata(dev);
> >> +	u32		reg;
> >> +
> >> +	if (dwc->current_dr_role == DWC3_GCTL_PRTCAP_HOST &&
> >> +			dwc->dis_split_quirk) {
> >> +		dev_dbg(dwc->dev, "set DWC3_GUCTL3_SPLITDISABLE\n");  
> 
> no more dev_dbg() should be added. This driver relies exclusively on
> tracepoints for debugging.

Ok. 

> 
> >> +		reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
> >> +		reg |= DWC3_GUCTL3_SPLITDISABLE;
> >> +		dwc3_writel(dwc->regs, DWC3_GUCTL3, reg);
> >> +	}
> >> +}
> >> +#else
> >> +#define dwc3_complete NULL
> >>  #endif /* CONFIG_PM_SLEEP */
> >>  
> >>  static const struct dev_pm_ops dwc3_dev_pm_ops = {
> >>  	SET_SYSTEM_SLEEP_PM_OPS(dwc3_suspend, dwc3_resume)
> >> +	.complete = dwc3_complete,  
> 
> why is this done on complete? Why can't it be done at the end of
> dwc3_resume()?

Again, no idea. I didn't actually tried to suspend/resume.

Maybe the original author can shed a light on it.

Thanks,
Mauro