Re: [PATCH] [RFC] usb:gadget: Refcount for gadget pullup

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Felipe,

> Hi Felipe,
> 
> > This commit fixes the way gadget's pullup method (wrapped at
> > usb_gadget_connect/disconnect) is called in the udc-core.
> > 
> > The composite driver allows correct driver registration, even when
> > it calls the usb_gadget_disconnect method (composite driver is
> > defered for further user space configuration - please look into the
> > description of usb_composite_probe at composite.c - line: 1621)
> > 
> > One such example is the CCG (Configurable Composite Gadget) driver
> > (at drivers/staging/ccg), which after its registration has no usb
> > descriptor (i.e. idProduct, idVendor etc.) and functions registered.
> > Those are configured after writing to /sys/module/g_ccg/parameters/
> > or /sys/class/ccg_usb/ccg0/.
> > 
> > Unfortunately, the code at 'usb_gadget_probe_driver' method (some
> > code omitted):
> > 
> > 	if (udc_is_newstyle(udc)) {
> > 		bind(udc->gadget);
> > 		usb_gadget_udc_start(udc->gadget, driver);
> > 		usb_gadget_connect(udc->gadget);
> > 	}
> > 
> > Explicitly calls the usb_gadget_connect method for this driver. It
> > looks that the udc-core enables pullup for a driver, which has no
> > functions and no descriptor filled (those values are feed from
> > userspace).
> > 
> > The solution (at least until the udc-core is reworked) is to add
> > atomic variable, which helps in balancing the number of called
> > usb_gadget_connect/ disconnect functions.
> > 
> > Signed-off-by: Lukasz Majewski <l.majewski@xxxxxxxxxxx>
> > Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> > ---
> >  drivers/usb/gadget/udc-core.c |   17 ++++++++++++++++-
> >  include/linux/usb/gadget.h    |   13 +++++++++++--
> >  2 files changed, 27 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/udc-core.c
> > b/drivers/usb/gadget/udc-core.c index e5e44f8..a26517e 100644
> > --- a/drivers/usb/gadget/udc-core.c
> > +++ b/drivers/usb/gadget/udc-core.c
> > @@ -349,7 +349,22 @@ found:
> >  		}
> >  		usb_gadget_connect(udc->gadget);
> >  	} else {
> > -
> > +		/*
> > +		 * This is a hack for "old style" gadgets:
> > +		 *
> > +		 * The udc_start for "old style" gadgets performs
> > implicitly all
> > +		 * operations done by usb_gadget_connect(but not
> > calling it).
> > +		 * Therefore non composite gadgets (like rndis)
> > works even with
> > +		 * wrong connect_count value ("old style" gadgets
> > don't call
> > +		 * usb_gadget_connect/disconnect).
> > +		 *
> > +		 * On the other hand the CCG (Configurable
> > Composite Gadget)
> > +		 * requires this incrementation since it calls
> > +		 * usb_gadget_disconnect on its probe (it is
> > allowed) and hence
> > +		 * the proper balance is needed when the
> > usb_gadget_connect(i.e.
> > +		 * pullup) is called, when triggered from userspace
> > event.
> > +		 */
> > +		atomic_inc(&udc->gadget->connect_count);
> >  		ret = usb_gadget_start(udc->gadget, driver, bind);
> >  		if (ret)
> >  			goto err1;
> > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> > index 9517466..0801d83 100644
> > --- a/include/linux/usb/gadget.h
> > +++ b/include/linux/usb/gadget.h
> > @@ -534,6 +534,7 @@ struct usb_gadget {
> >  	unsigned			b_hnp_enable:1;
> >  	unsigned			a_hnp_support:1;
> >  	unsigned			a_alt_hnp_support:1;
> > +	atomic_t                        connect_count;
> >  	const char			*name;
> >  	struct device			dev;
> >  };
> > @@ -739,7 +740,11 @@ static inline int usb_gadget_connect(struct
> > usb_gadget *gadget) {
> >  	if (!gadget->ops->pullup)
> >  		return -EOPNOTSUPP;
> > -	return gadget->ops->pullup(gadget, 1);
> > +
> > +	if (atomic_inc_return(&gadget->connect_count) == 1)
> > +		return gadget->ops->pullup(gadget, 1);
> > +
> > +	return 0;
> >  }
> >  
> >  /**
> > @@ -761,7 +766,11 @@ static inline int usb_gadget_disconnect(struct
> > usb_gadget *gadget) {
> >  	if (!gadget->ops->pullup)
> >  		return -EOPNOTSUPP;
> > -	return gadget->ops->pullup(gadget, 0);
> > +
> > +	if (atomic_dec_and_test(&gadget->connect_count))
> > +		return gadget->ops->pullup(gadget, 0);
> > +
> > +	return 0;
> >  }
> >  
> >  
> 
> Any thoughts about solving this problem?
> 

Felipe, can you look into this problem?


-- 
Best regards,

Lukasz Majewski

Samsung Poland R&D Center | Linux Platform Group
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux