linux-kernel - Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

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: <20160909015233.GC15637@b29397-desktop>
Date:   Fri, 9 Sep 2016 09:52:33 +0800
From:   Peter Chen <hzpeterchen@...il.com>
To:     Grygorii Strashko <grygorii.strashko@...com>
Cc:     Arnd Bergmann <arnd@...db.de>, Felipe Balbi <balbi@...nel.org>,
        Leo Li <pku.leo@...il.com>,
        Russell King - ARM Linux <linux@....linux.org.uk>,
        Catalin Marinas <catalin.marinas@....com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        Sekhar Nori <nsekhar@...com>,
        lkml <linux-kernel@...r.kernel.org>,
        Stuart Yoder <stuart.yoder@....com>,
        Scott Wood <oss@...error.net>,
        David Fisher <david.fisher1@...opsys.com>,
        "Thang Q. Nguyen" <tqnguyen@....com>,
        Alan Stern <stern@...land.harvard.edu>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent
 dev

On Thu, Sep 08, 2016 at 03:59:19PM +0300, Grygorii Strashko wrote:
> On 09/08/2016 03:28 PM, Peter Chen wrote:
> > On Thu, Sep 08, 2016 at 12:17:21PM +0200, Arnd Bergmann wrote:
> >> On Thursday, September 8, 2016 12:43:06 PM CEST Felipe Balbi wrote:
> >>> Arnd Bergmann <arnd@...db.de> writes:
> >>>> On Thursday, September 8, 2016 11:29:04 AM CEST Felipe Balbi wrote:
> >>>>>> If we do that, we have to put child devices of the dwc3 devices into
> >>>>>> the platform glue, and it also breaks those dwc3 devices that don't
> >>>>>> have a parent driver.
> >>>>>
> >>>>> Well, this is easy to fix:
> >>>>>
> >>>>>         if (dwc->dev->parent) {
> >>>>>                 dwc->sysdev = dwc->dev->parent;
> >>>>>         } else {
> >>>>>                 dev_info(dwc->dev, "Please provide a glue layer!\n");
> >>>>>                 dwc->sysdev = dwc->dev;
> >>>>>         }
> >>>>
> >>>> I don't understand. Do you mean we should have an extra level of
> >>>> stacking and splitting "static struct platform_driver dwc3_driver"
> >>>> in two so instead of
> >>>>
> >>>>       "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "xhci" (usb_bus.dev)
> >>>>
> >>>> we do this?
> >>>>
> >>>>       "qcom,dwc3" -> "snps,dwc3" (usb_bus.sysdev) -> "dwc3-glue" -> "xhci" (usb_bus.dev)
> >>>
> >>> no 
> >>>
> >>> If we have a parent device, use that as sysdev, otherwise use self as
> >>> sysdev.
> >>
> >> But there is often a parent device in DT, as the xhci device is
> >> attached to some internal bus that gets turned into a platform_device
> >> as well, so checking whether there is a parent will get the wrong
> >> device node.
> > 
> > From my point, all platform and firmware information at dwc3 are
> > correct, so we don't need to change dwc3/core.c, only changing for
> > xhci-plat.c is ok.
> > 
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index ed56bf9..fd57c0d 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -145,6 +145,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	struct clk              *clk;
> >  	int			ret;
> >  	int			irq;
> > +	struct device *dev = &pdev->dev, *sysdev;
> >  
> >  	if (usb_disabled())
> >  		return -ENODEV;
> > @@ -155,6 +156,12 @@ static int xhci_plat_probe(struct platform_device *pdev)
> >  	if (irq < 0)
> >  		return -ENODEV;
> >  
> > +	if (dev->parent) {
> > +		sysdev = dev->parent;
> > +	} else {
> > +        	sysdev = dev;
> > +	}
> > +
> 
> Shouldn't we be more careful with that?
> 

Above code does not consider pci device case, Arnd's patch covers
all cases.

> armada-375.dtsi
> 
> 	soc {
> 		compatible = "marvell,armada375-mbus", "simple-bus";
> 
> 		internal-regs {
> 			compatible = "simple-bus";
> 
> 			usb3@...00 {
> 				compatible = "marvell,armada-375-xhci";
> 				reg = <0x58000 0x20000>,<0x5b880 0x80>;
> 				interrupts = <GIC_SPI 16 IRQ_TYPE_LEVEL_HIGH>;
> 				clocks = <&gateclk 16>;
> 				phys = <&usbcluster PHY_TYPE_USB3>;
> 				phy-names = "usb";
> 				status = "disabled";
> 			};
> 
> 
> What will be the parent dev in above case?
> 

In this case, no parent dev for above case, it will use itself as sysdev
since it has of_node at dts.

-- 

Best Regards,
Peter Chen