linux-kernel - Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux

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]

Date:	Tue, 07 Jun 2016 18:05:25 +0300
From:	Felipe Balbi <felipe.balbi@...ux.intel.com>
To:	Roger Quadros <rogerq@...com>, Lu Baolu <baolu.lu@...ux.intel.com>,
	Jun Li <jun.li@....com>, Peter Chen <hzpeterchen@...il.com>
Cc:	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lee Jones <lee.jones@...aro.org>,
	Heikki Krogerus <heikki.krogerus@...ux.intel.com>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	"linux-usb\@vger.kernel.org" <linux-usb@...r.kernel.org>,
	"linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v10 2/7] usb: mux: add generic code for dual role port mux


Hi,

Roger Quadros <rogerq@...com> writes:
>> I might be able to find some time to implement a proof of concept which
>> would allow your platforms to get dual-role with code we already have,
>> but I need DWC3's OTG support which, I'm assuming, you already have :-)
>> 
>> If you wanna try something offline, just ping me ;-) I'll be happy to
>> help.
>
> What you are proposing is a dwc3 only solution. With the otg/dual-role
> series we are trying to be generic as much as possible.

Well, if there is a need for that, sure. Take MUSB for instance. It
makes use of nothing of the sorts, because it doesn't have to.

> Whether controller drivers want to use it or not is upto the driver
> maintainers but we should at least ensure that user space ABI if any,
> is consistent across different implementations.

Role decisions should not be exposed to userspace unless as debug
feature (using e.g. DebugFS). That should be done either by the HW or
within the kernel.

If we're discussing userspace ABI here, there's something very wrong
with OTG/DRD layer design.

>>> How are you switching the port mux between host and peripheral? Only
>>> by sysfs or do you have a GPIO for ID pin as well?
>> 
>> depends. Some SoCs have GPIO-controller muxes while some just have mux's
>> select signals (one for ID, one for VBUS) mapped on xHCI's address
>> space.
>> 
>>> What happens to the gadget controller when the port is muxed to the
>>> host controller?  Is it stopped or it continues to run?
>> 
>> it continues running, but that's pretty irrelevant for Intel's dual-role
>
> Isn't that unnecessary waste of power? Or you have firmware assisted
> low power mode?

that's an implementation detail which brings nothing to this discussion,
right? :-)

We can, certainly, put the other side to D3.

>> setup. We have an actual physical (inside the die, though) mux which
>> muxes USB signals to XHCI (not DWC3's XHCI) or to a peripheral-only
>> DWC3.
>> 
>
> Probably irrelevant for Intel's dual-role but many platforms that
> share the port can't have device controller running when port is in
> host mode and vice versa.

but that doesn't mean we need an entire new layer added to the kernel
;-)

DWC3 already gives us all the information necessary to make a decision
on which role we should assume. Just consider your options. Here's how
things would look like without any OTG/DRD layer:

-> DWC3 OTG IRQ
 -> readl(OSTS);
  -> if (OSTS & BIT(4))
   -> dwc3_host_exit(); __dwc3_gadget_start();
  -> else
   -> __dwc3_gadget_stop(); dwc3_host_init();

Can you draw something similar for your proposed OTG/DRD layer?

I remember there were at least two schedule_work(). IIRC it looked
something like below:

-> DWC3 OTG IRQ
 -> readl(OSTS);
  -> if (OSTS & BIT(4))
   -> otg_set_mode(PERIPHERAL);
    -> schedule_work();
     -> otg_ops->stop_host();
      -> usb_del_hcd();
     -> otg_ops->start_peripheral();
      -> usb_gadget_add_udc();
  -> else
   -> otg_set_mode(HOST);
    -> schedule_work();
     -> otg_ops->stop_peripheral();
      -> usb_gadget_del_udc();
     -> otg_ops->start_host();
      -> usb_add_hcd();

I'm probably missing some steps there.    

> So there has to be a central point of control where the respective
> controllers are started/stopped.

some implementations might need this, yes. DWC3 and MUSB don't seem to
be this type of system.

> That is the other point we are trying to address with the common
> otg/dual-role code.
>
> Even in the TI dwc3 implementation we use dwc3's XHCI so I guess we need
> to stop the host controller for device mode, right?

yes, see above. We already have that code.

> If so then who will deal with start/stop of the controllers then?

dwc3 itself.

-- 
balbi

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