linux-kernel - Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

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: <20150903154444.GA4031@saruman.tx.rr.com>
Date:	Thu, 3 Sep 2015 10:44:44 -0500
From:	Felipe Balbi <balbi@...com>
To:	Roger Quadros <rogerq@...com>
CC:	<balbi@...com>, <tony@...mide.com>, <Joao.Pinto@...opsys.com>,
	<sergei.shtylyov@...entembedded.com>, <peter.chen@...escale.com>,
	<jun.li@...escale.com>, <linux-usb@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>, <linux-omap@...r.kernel.org>
Subject: Re: [PATCH v4 1/9] usb: dwc3: add dual-role support

Hi,

On Thu, Sep 03, 2015 at 03:21:48PM +0300, Roger Quadros wrote:
> >> +	dwc->fsm->id = id;
> >> +	dwc->fsm->b_sess_vld = vbus;
> >> +	usb_otg_sync_inputs(dwc->fsm);
> >> +}
> >> +
> >> +static int dwc3_drd_start_host(struct otg_fsm *fsm, int on)
> >> +{
> >> +	struct device *dev = usb_otg_fsm_to_dev(fsm);
> >> +	struct dwc3 *dwc = dev_get_drvdata(dev);
> > 
> > how about adding a usb_otg_get_drvdata(fsm) ?
> 
> You meant for otg core? That can be done.

yeah. BTW, I think otg core needs quite a few changes to become actually
useful. Currently it's just too much pointer ping-pong going back and
forth between phy, otg core, udc and hcd.

Also, I caught a ton of issues with it and suspend/resume. You might
want to fix them before adding more users to it.

It's also rather racy and that needs fixing too. On top of all that, I
think there's too much being added to UDC just to get Dual-Role, let's
see if we can improve that too.

> >> @@ -843,6 +998,16 @@ static int dwc3_probe(struct platform_device *pdev)
> >>  	hird_threshold = 12;
> >>  
> >>  	if (node) {
> >> +		if (of_property_read_bool(node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev, 0);
> >> +		else if (of_property_read_bool(dev->parent->of_node, "extcon"))
> >> +			dwc->edev = extcon_get_edev_by_phandle(dev->parent, 0);
> > 
> > why do you need to check the parent ? Why isn't that done on the glue
> > layer ?
> 
> On DRA7-evm, the extcon device is defined in the glue layer node. But
> we need the device both at the glue layer and at the core layer.

why do you need extcon here ? Glue updates core via UTMI about the
states, right ? So you should get proper VBUS and ID status via OTG IRQ.
Is that not working ?

> We do get the extcon device in dwc3-omap.c
> 
> Any suggestion how to pass the extcon device from glue layer to core.c?
> Or should I add the extcon property to dwc3 USB node as well in the DT?

GPIO toggles
  dwc3-omap extcon event
    update status via UTMI STATUS register
      OTG IRQ on core
        Horray!

:-)

> >> +
> >> +		if (IS_ERR(dwc->edev)) {
> >> +			dev_vdbg(dev, "couldn't get extcon device\n");
> > 
> > dev_err() ??
> 
> Is it ok to print it even in EPROBE_DEFER case?

hmm, probably pointless, indeed.

-- 
balbi

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)