| 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
| ||
|
Date: Wed, 9 Sep 2015 14:20:07 +0800
From: Li Jun <b47624@...escale.com>
To: Roger Quadros <rogerq@...com>
CC: <stern@...land.harvard.edu>, <balbi@...com>,
<gregkh@...uxfoundation.org>, <peter.chen@...escale.com>,
<dan.j.williams@...el.com>, <jun.li@...escale.com>,
<mathias.nyman@...ux.intel.com>, <tony@...mide.com>,
<Joao.Pinto@...opsys.com>, <abrestic@...omium.org>,
<linux-usb@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-omap@...r.kernel.org>
Subject: Re: [PATCH v4 07/13] usb: otg: add OTG core
On Mon, Sep 07, 2015 at 01:53:19PM +0300, Roger Quadros wrote:
> On 07/09/15 10:40, Li Jun wrote:
> > On Mon, Aug 24, 2015 at 04:21:18PM +0300, Roger Quadros wrote:
> >> The OTG core instantiates the OTG Finite State Machine
> >> per OTG controller and manages starting/stopping the
> >> host and gadget controllers based on the bus state.
> >>
> >> It provides APIs for the following tasks
> >>
> >> - Registering an OTG capable controller
> >> - Registering Host and Gadget controllers to OTG core
> >> - Providing inputs to and kicking the OTG state machine
> >>
> >> Signed-off-by: Roger Quadros <rogerq@...com>
> >> ---
> >> MAINTAINERS | 4 +-
> >> drivers/usb/Kconfig | 2 +-
> >> drivers/usb/Makefile | 1 +
> >> drivers/usb/common/Makefile | 3 +-
> >> drivers/usb/common/usb-otg.c | 1061 ++++++++++++++++++++++++++++++++++++++++++
> >> drivers/usb/common/usb-otg.h | 71 +++
> >> drivers/usb/core/Kconfig | 11 +-
> >> include/linux/usb/otg.h | 189 +++++++-
> >> 8 files changed, 1321 insertions(+), 21 deletions(-)
> >> create mode 100644 drivers/usb/common/usb-otg.c
> >> create mode 100644 drivers/usb/common/usb-otg.h
> >>
> >
> > ... ...
> >
> >> +
> >> +/**
> >> + * Get OTG device from host or gadget device.
> >> + *
> >> + * For non device tree boot, the OTG controller is assumed to be
> >> + * the parent of the host/gadget device.
> >
> > This assumption/restriction maybe a problem, as I pointed in your previous
> > version, usb_create_hcd() use the passed dev as its dev, but,
> > usb_add_gadget_udc() use the passed dev as its parent dev, so often the
> > host and gadget don't share the same parent device, at least it doesn't
> > apply to chipidea case.
>
> Let's provide a way for OTG driver to provide the OTG core exactly which is
> the related host/gadget device.
>
> >
> >> + * For device tree boot, the OTG controller is derived from the
> >> + * "otg-controller" property.
> >> + */
> >> +static struct device *usb_otg_get_device(struct device *hcd_gcd_dev)
> >> +{
> >> + struct device *otg_dev;
> >> +
> >> + if (!hcd_gcd_dev)
> >> + return NULL;
> >> +
> >> + if (hcd_gcd_dev->of_node) {
> >> + struct device_node *np;
> >> + struct platform_device *pdev;
> >> +
> >> + np = of_parse_phandle(hcd_gcd_dev->of_node, "otg-controller",
> >> + 0);
> >> + if (!np)
> >> + goto legacy; /* continue legacy way */
> >> +
> >> + pdev = of_find_device_by_node(np);
> >> + of_node_put(np);
> >> + if (!pdev) {
> >> + dev_err(&pdev->dev, "couldn't get otg-controller device\n");
> >> + return NULL;
> >> + }
> >> +
> >> + otg_dev = &pdev->dev;
> >> + return otg_dev;
> >> + }
> >> +
> >> +legacy:
> >> + /* otg device is parent and must be registered */
> >> + otg_dev = hcd_gcd_dev->parent;
> >> + if (!usb_otg_get_data(otg_dev))
> >> + return NULL;
> >> +
> >> + return otg_dev;
> >> +}
> >> +
> >
> > ... ...
> >
> >> +static void usb_otg_init_timers(struct usb_otg *otgd, unsigned *timeouts)
> >> +{
> >> + struct otg_fsm *fsm = &otgd->fsm;
> >> + unsigned long tmouts[NUM_OTG_FSM_TIMERS];
> >> + int i;
> >> +
> >> + /* set default timeouts */
> >> + tmouts[A_WAIT_VRISE] = TA_WAIT_VRISE;
> >> + tmouts[A_WAIT_VFALL] = TA_WAIT_VFALL;
> >> + tmouts[A_WAIT_BCON] = TA_WAIT_BCON;
> >> + tmouts[A_AIDL_BDIS] = TA_AIDL_BDIS;
> >> + tmouts[A_BIDL_ADIS] = TA_BIDL_ADIS;
> >> + tmouts[B_ASE0_BRST] = TB_ASE0_BRST;
> >> + tmouts[B_SE0_SRP] = TB_SE0_SRP;
> >> + tmouts[B_SRP_FAIL] = TB_SRP_FAIL;
> >> +
> >> + /* set controller provided timeouts */
> >> + if (timeouts) {
> >> + for (i = 0; i < NUM_OTG_FSM_TIMERS; i++) {
> >> + if (timeouts[i])
> >> + tmouts[i] = timeouts[i];
> >> + }
> >> + }
> >> +
> >> + otg_timer_init(A_WAIT_VRISE, otgd, set_tmout, TA_WAIT_VRISE,
> >> + &fsm->a_wait_vrise_tmout);
> >> + otg_timer_init(A_WAIT_VFALL, otgd, set_tmout, TA_WAIT_VFALL,
> >> + &fsm->a_wait_vfall_tmout);
> >> + otg_timer_init(A_WAIT_BCON, otgd, set_tmout, TA_WAIT_BCON,
> >> + &fsm->a_wait_bcon_tmout);
> >> + otg_timer_init(A_AIDL_BDIS, otgd, set_tmout, TA_AIDL_BDIS,
> >> + &fsm->a_aidl_bdis_tmout);
> >> + otg_timer_init(A_BIDL_ADIS, otgd, set_tmout, TA_BIDL_ADIS,
> >> + &fsm->a_bidl_adis_tmout);
> >> + otg_timer_init(B_ASE0_BRST, otgd, set_tmout, TB_ASE0_BRST,
> >> + &fsm->b_ase0_brst_tmout);
> >> +
> >> + otg_timer_init(B_SE0_SRP, otgd, set_tmout, TB_SE0_SRP,
> >> + &fsm->b_se0_srp);
> >> + otg_timer_init(B_SRP_FAIL, otgd, set_tmout, TB_SRP_FAIL,
> >> + &fsm->b_srp_done);
> >> +
> >> + /* FIXME: what about A_WAIT_ENUM? */
> >
> > Either you init it as other timers, or you remove all of it, otherwise
> > there will be NULL pointer crash.
>
> I want to initialize it but was not sure about the timeout value.
> What timeout value I must use?
>
It's not defined in OTG spec, I don't know either.
or you filter it out when add/del timer in below 2 functions.
if (id == A_WAIT_ENUM)
return;
> >
> >> +}
> >> +
> >> +/**
> >> + * OTG FSM ops function to add timer
> >> + */
> >> +static void usb_otg_add_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
> >> +{
> >> + struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm);
> >> + struct otg_timer *otgtimer = &otgd->timers[id];
> >> + struct hrtimer *timer = &otgtimer->timer;
> >> +
> >> + if (!otgd->fsm_running)
> >> + return;
> >> +
> >> + /* if timer is already active, exit */
> >> + if (hrtimer_active(timer)) {
> >> + dev_err(otgd->dev, "otg: timer %d is already running\n", id);
> >> + return;
> >> + }
> >> +
> >> + hrtimer_start(timer, otgtimer->timeout, HRTIMER_MODE_REL);
> >> +}
> >> +
> >> +/**
> >> + * OTG FSM ops function to delete timer
> >> + */
> >> +static void usb_otg_del_timer(struct otg_fsm *fsm, enum otg_fsm_timer id)
> >> +{
> >> + struct usb_otg *otgd = container_of(fsm, struct usb_otg, fsm);
> >> + struct hrtimer *timer = &otgd->timers[id].timer;
> >> +
> >> + hrtimer_cancel(timer);
> >> +}
> >> +
... ...
> >> + }
> >> +
> >> + otgd->dev = dev;
> >> + otgd->caps = &config->otg_caps;
> >
> > How about define otgd->caps as a pointer, then don't need copy it.
>
> otgd->caps is a pointer.
>
okay, you are right.
> >
> >> + INIT_WORK(&otgd->work, usb_otg_work);
> >> + otgd->wq = create_singlethread_workqueue("usb_otg");
> >> + if (!otgd->wq) {
> >> + dev_err(dev, "otg: %s: can't create workqueue\n",
> >> + __func__);
> >> + ret = -ENOMEM;
> >> + goto err_wq;
> >> + }
> >> +
> >> + usb_otg_init_timers(otgd, config->otg_timeouts);
> >> +
> >> + /* create copy of original ops */
> >> + otgd->fsm_ops = *config->fsm_ops;
> >
> > The same, use a pointer is enough?
>
> We are creating a copy because we are overriding timer ops.
>
okay.
> >
> >> + /* FIXME: we ignore caller's timer ops */
> >> + otgd->fsm_ops.add_timer = usb_otg_add_timer;
> >> + otgd->fsm_ops.del_timer = usb_otg_del_timer;
> >> + /* set otg ops */
> >> + otgd->fsm.ops = &otgd->fsm_ops;
> >> + otgd->fsm.otg = otgd;
> >> + *
... ...
> >> + * This is used by the USB Host stack to register the Host controller
> >> + * to the OTG core. Host controller must not be started by the
> >> + * caller as it is left upto the OTG state machine to do so.
> >
> > I am confused on how to use this function.
> > - This function should be called before start fsm per usb_otg_start_fsm().
>
> yes.
>
> > - Called by usb_add_hcd(), so we need call usb_add_hcd() before start fsm.
>
> yes.
>
> > - If I want to add hcd when switch to host role, and remove hcd when switch
> > to peripheral, with this design, I cannot use this function?
>
> You add hcd only once during the life of the OTG device. If it is linked to the
> OTG controller the OTG fsm manages the start/stop of hcd using the otg_hcd_ops.
>
> "usb/core/hcd.c"
> static struct otg_hcd_ops otg_hcd_intf = {
> .add = usb_otg_add_hcd,
> .remove = usb_otg_remove_hcd,
> };
>
> Your otg driver must use teh usb_otg_add/remove_hcd to start/stop the controller.
> Using usb_remove_hcd() means the hcd resource is no longer available and the
> otg fsm will be stopped.
>
> > - How about split it out of usb_add_hcd()?
>
> Adding the HCD and starting/stopping the hcd is split into
> usb_add/remove_hcd() and usb_otg_add/remove_hcd() for OTG case.
>
Catch your point now.
Do usb_add_hcd to register hcd before start fsm, and use usb_otg_start_host()
to start/stop host role for otg fsm.
> >
> >> + *
> >> + * Returns: 0 on success, error value otherwise.
> >> + */
> >> +int usb_otg_register_hcd(struct usb_hcd *hcd, unsigned int irqnum,
> >> + unsigned long irqflags, struct otg_hcd_ops *ops)
> >> +{
> >> + struct usb_otg *otgd;
> >> + struct device *hcd_dev = hcd->self.controller;
> >> + struct device *otg_dev = usb_otg_get_device(hcd_dev);
> >> +
> >> + if (!otg_dev)
> >> + return -EINVAL; /* we're definitely not OTG */
> >> +
> >> + /* we're otg but otg controller might not yet be registered */
> >> + mutex_lock(&otg_list_mutex);
> >> + otgd = usb_otg_get_data(otg_dev);
> >> + mutex_unlock(&otg_list_mutex);
> >> + if (!otgd) {
> >> + dev_dbg(hcd_dev,
> >> + "otg: controller not yet registered. waiting..\n");
> >> + /*
> >> + * otg controller might register later. Put the hcd in
> >> + * wait list and call us back when ready
> >> + */
> >> + if (usb_otg_hcd_wait_add(otg_dev, hcd, irqnum, irqflags, ops)) {
> >> + dev_dbg(hcd_dev, "otg: failed to add to wait list\n");
> >> + return -EINVAL;
> >> + }
> >> +
> >> + return 0;
> >> + }
> >> +
> >> + /* HCD will be started by OTG fsm when needed */
> >> + mutex_lock(&otgd->fsm.lock);
> >
> > If call usb_add_hcd() when start host role, deadlock.
>
> No. You must call usb_otg_add_hcd() to start host role.
>
> >
> >> + if (otgd->primary_hcd.hcd) {
> >> + /* probably a shared HCD ? */
> >> + if (usb_otg_hcd_is_primary_hcd(hcd)) {
> >> + dev_err(otg_dev, "otg: primary host already registered\n");
> >> + goto err;
> >> + }
> >> +
> >> + if (hcd->shared_hcd == otgd->primary_hcd.hcd) {
> >> + if (otgd->shared_hcd.hcd) {
> >> + dev_err(otg_dev, "otg: shared host already registered\n");
> >> + goto err;
> >> + }
> >> +
> >> + otgd->shared_hcd.hcd = hcd;
> >> + otgd->shared_hcd.irqnum = irqnum;
> >> + otgd->shared_hcd.irqflags = irqflags;
> >> + otgd->shared_hcd.ops = ops;
> >> + dev_info(otg_dev, "otg: shared host %s registered\n",
> >> + dev_name(hcd->self.controller));
> >> + } else {
> >> + dev_err(otg_dev, "otg: invalid shared host %s\n",
> >> + dev_name(hcd->self.controller));
> >> + goto err;
> >> + }
> >> + } else {
> >> + if (!usb_otg_hcd_is_primary_hcd(hcd)) {
> >> + dev_err(otg_dev, "otg: primary host must be registered first\n");
> >> + goto err;
> >> + }
> >> +
> >> + otgd->primary_hcd.hcd = hcd;
> >> + otgd->primary_hcd.irqnum = irqnum;
> >> + otgd->primary_hcd.irqflags = irqflags;
> >> + otgd->primary_hcd.ops = ops;
> >> + dev_info(otg_dev, "otg: primary host %s registered\n",
> >> + dev_name(hcd->self.controller));
> >> + }
> >> +
> >> + /*
> >> + * we're ready only if we have shared HCD
> >> + * or we don't need shared HCD.
> >> + */
> >> + if (otgd->shared_hcd.hcd || !otgd->primary_hcd.hcd->shared_hcd) {
> >> + otgd->fsm.otg->host = hcd_to_bus(hcd);
> >
> > otgd->host = hcd_to_bus(hcd);
>
> ok. So we set host at both places. struct usb_otg in struct otg_fsm starts to
> feel redundant now. I think we should get rid of it and get the usb_otg struct
> using container_of() instead.
>
Then you may come to force existing otg fsm code to use your OTG core.
> >
> >> + /* FIXME: set bus->otg_port if this is true OTG port with HNP */
> >> +
> >> + /* start FSM */
> >> + usb_otg_start_fsm(&otgd->fsm);
> >> + } else {
> >> + dev_dbg(otg_dev, "otg: can't start till shared host registers\n");
> >> + }
> >> +
> >> + mutex_unlock(&otgd->fsm.lock);
> >> +
> >> + return 0;
> >> +
> >> +err:
> >> + mutex_unlock(&otgd->fsm.lock);
> >> + return -EINVAL;
> >
> > Return non-zero, then if err, do we need call usb_otg_add_hcd() after
> > usb_otg_register_hcd() fails?
>
> You should not call usb_otg_register_hcd() but usb_add_hcd().
> If that fails then you fail as ususal.
My point is if we use usb_add_hcd(), but failed in usb_otg_register_hcd(),
then usb_otg_add_hcd() will be called in *all* error case, is this your
expectation?
if (usb_otg_register_hcd(hcd, irqnum, irqflags, &otg_hcd_intf))
return usb_otg_add_hcd(hcd, irqnum, irqflags);
> >> + * @fsm_running: state machine running/stopped indicator
> >> + */
> >> struct usb_otg {
> >> u8 default_a;
> >>
> >> struct phy *phy;
> >> /* old usb_phy interface */
> >> struct usb_phy *usb_phy;
> >> +
> >
> > add a blank line?
> >
You missed this.
> >> struct usb_bus *host;
> >> struct usb_gadget *gadget;
> >>
> >> enum usb_otg_state state;
> >>
> >> + struct device *dev;
> >> + struct usb_otg_caps *caps;
> >> + struct otg_fsm fsm;
> >> + struct otg_fsm_ops fsm_ops;
> >> +
> >> + /* internal use only */
> >> + struct otg_hcd primary_hcd;
> >> + struct otg_hcd shared_hcd;
> >> + struct otg_gadget_ops *gadget_ops;
> >> + struct otg_timer timers[NUM_OTG_FSM_TIMERS];
> >> + struct list_head list;
> >> + struct work_struct work;
> >> --
> >> 2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists