| 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: <20160525024427.GA31142@shlinux2>
Date: Wed, 25 May 2016 10:44:27 +0800
From: Peter Chen <hzpeterchen@...il.com>
To: Roger Quadros <rogerq@...com>
Cc: peter.chen@...escale.com, balbi@...nel.org, tony@...mide.com,
gregkh@...uxfoundation.org, dan.j.williams@...el.com,
mathias.nyman@...ux.intel.com, Joao.Pinto@...opsys.com,
sergei.shtylyov@...entembedded.com, jun.li@...escale.com,
grygorii.strashko@...com, yoshihiro.shimoda.uh@...esas.com,
robh@...nel.org, nsekhar@...com, b-liu@...com,
linux-usb@...r.kernel.org, linux-omap@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core
On Tue, May 24, 2016 at 12:45:46PM +0300, Roger Quadros wrote:
> Hi Peter,
>
> I have one question here. Please see below.
>
> On 13/05/16 13:03, Roger Quadros wrote:
> > It provides APIs for the following tasks
> >
> > - Registering an OTG/dual-role capable controller
> > - Registering Host and Gadget controllers to OTG core
> > - Providing inputs to and kicking the OTG state machine
> >
> > Provide a dual-role device (DRD) state machine.
> > DRD mode is a reduced functionality OTG mode. In this mode
> > we don't support SRP, HNP and dynamic role-swap.
> >
> > In DRD operation, the controller mode (Host or Peripheral)
> > is decided based on the ID pin status. Once a cable plug (Type-A
> > or Type-B) is attached the controller selects the state
> > and doesn't change till the cable in unplugged and a different
> > cable type is inserted.
> >
> > As we don't need most of the complex OTG states and OTG timers
> > we implement a lean DRD state machine in usb-otg.c.
> > The DRD state machine is only interested in 2 hardware inputs
> > 'id' and 'b_sess_vld'.
> >
> > Signed-off-by: Roger Quadros <rogerq@...com>
> > ---
> > drivers/usb/common/Makefile | 2 +-
> > drivers/usb/common/usb-otg.c | 1042 ++++++++++++++++++++++++++++++++++++++++++
> > drivers/usb/core/Kconfig | 4 +-
> > include/linux/usb/gadget.h | 2 +
> > include/linux/usb/hcd.h | 1 +
> > include/linux/usb/otg-fsm.h | 7 +
> > include/linux/usb/otg.h | 154 ++++++-
> > 7 files changed, 1206 insertions(+), 6 deletions(-)
> > create mode 100644 drivers/usb/common/usb-otg.c
> >
>
> <snip>
>
> > +
> > +/**
> > + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> > + * @dev: OTG/dual-role controller device.
> > + * @config: OTG configuration.
> > + *
> > + * Registers the OTG/dual-role controller device with the USB OTG core.
> > + *
> > + * Return: struct usb_otg * if success, ERR_PTR() if error.
> > + */
> > +struct usb_otg *usb_otg_register(struct device *dev,
> > + struct usb_otg_config *config)
> > +{
> > + struct usb_otg *otg;
> > + struct otg_wait_data *wait;
> > + int ret = 0;
> > +
> > + if (!dev || !config || !config->fsm_ops)
> > + return ERR_PTR(-EINVAL);
> > +
> > + /* already in list? */
> > + mutex_lock(&otg_list_mutex);
> > + if (usb_otg_get_data(dev)) {
> > + dev_err(dev, "otg: %s: device already in otg list\n",
> > + __func__);
> > + ret = -EINVAL;
> > + goto unlock;
> > + }
> > +
> > + /* allocate and add to list */
> > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > + if (!otg) {
> > + ret = -ENOMEM;
> > + goto unlock;
> > + }
> > +
> > + otg->dev = dev;
> > + otg->caps = config->otg_caps;
>
> Here, we should be checking if user needs to disable any OTG features. So,
>
> if (dev->of_node)
> of_usb_update_otg_caps(dev->of_node, &otg->caps);
>
> Do you agree?
> This means we need to change otg->caps from 'struct usb_otg_caps *caps;'
> to 'struct usb_otg_caps caps;' so that we can modify the local copy instead
> of the one passed by the OTG controller.
Why can't modify the one from OTG controller directly?
>
> We can also move of_usb_update_otg_caps() to otg.h.
>
> We will also need to modify the udc-core code so that it sets gadget->otg_caps
> to the modified otg_caps from the OTG core. This will ensure that the right
> OTG descriptors are sent.
>
> So we will have to introduce an API.
>
> struct usb_otg_caps *usb_otg_get_otg_caps(struct device *otg_dev)
>
> And in udc-core.c,
>
> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *driver)
> {
> ..
> ret = driver->bind(udc->gadget, driver);
> if (ret)
> goto err1;
>
> /* If OTG, the otg core starts the UDC when needed */
> if (udc->gadget->otg_dev) {
> + udc->gadget->is_otg = true;
gadget->is_otg is only set to true if fully OTG is supported and it
needs to send OTG descriptors at this case. DRD devices should not send OTG
descriptors.
> + udc->gadget->otg_caps = usb_otg_get_otg_caps(udc->gadget->otg_dev);
Getting otg capabilities should be prior to driver->bind since
usb_otg_descriptor_init is called at that. Besides, Gadget driver
may be probed before otg driver is registered
I am wonder if we can implement defer probe for gadget/udc/host driver
if otg driver is not probed, in that case, some designs can be simpler
like wait list in otg driver.
> mutex_unlock(&udc_lock);
> usb_otg_register_gadget(udc->gadget, &otg_gadget_intf);
> mutex_lock(&udc_lock);
> } else {
> ret = usb_gadget_udc_start(udc);
> if (ret) {
> driver->unbind(udc->gadget);
> goto err1;
> }
> usb_udc_connect_control(udc);
> }
> ..
> }
>
> What do you say?
>
> regards,
> -roger
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
Powered by blists - more mailing lists