| Message ID | 20230406062549.2461917-1-badhri@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started | expand |
On Thu, Apr 06, 2023 at 06:25:48AM +0000, Badhri Jagan Sridharan wrote: > usb_udc_connect_control does not check to see if the udc has already > been started. This causes gadget->ops->pullup to be called through > usb_gadget_connect when invoked from usb_udc_vbus_handler even before > usb_gadget_udc_start is called. Guard this by checking for udc->started > in usb_udc_connect_control before invoking usb_gadget_connect. > > Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate > related functions with connect_lock. usb_gadget_connect_locked, > usb_gadget_disconnect_locked, usb_udc_connect_control_locked, > usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are called with > this lock held as they can be simulataneously invoked from different code > paths. > > Adding an additional check to make sure udc is started(udc->started) > before pullup callback is invoked. > > Cc: stable@vger.kernel.org > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > --- > * Fixed commit message comments. > * Renamed udc_connect_control_lock to connect_lock and made it per > device. > * udc->vbus, udc->started, gadget->connect, gadget->deactivate are all > now guarded by connect_lock. > * Code now checks for udc->started to be set before invoking pullup > callback. > --- > drivers/usb/gadget/udc/core.c | 140 +++++++++++++++++++++++----------- > 1 file changed, 96 insertions(+), 44 deletions(-) > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > index 3dcbba739db6..41d3a1998cff 100644 > --- a/drivers/usb/gadget/udc/core.c > +++ b/drivers/usb/gadget/udc/core.c > @@ -37,6 +37,10 @@ static struct bus_type gadget_bus_type; > * @vbus: for udcs who care about vbus status, this value is real vbus status; > * for udcs who do not care about vbus status, this value is always true > * @started: the UDC's started state. True if the UDC had started. > + * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related > + * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked, > + * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are > + * called with this lock held. > * > * This represents the internal data structure which is used by the UDC-class > * to hold information about udc driver and gadget together. > @@ -48,6 +52,7 @@ struct usb_udc { > struct list_head list; > bool vbus; > bool started; > + struct mutex connect_lock; > }; > > static struct class *udc_class; > @@ -687,17 +692,8 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > } > EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); > > -/** > - * usb_gadget_connect - software-controlled connect to USB host > - * @gadget:the peripheral being connected > - * > - * Enables the D+ (or potentially D-) pullup. The host will start > - * enumerating this gadget when the pullup is active and a VBUS session > - * is active (the link is powered). > - * > - * Returns zero on success, else negative errno. > - */ > -int usb_gadget_connect(struct usb_gadget *gadget) > +/* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */ Shouldn't you just use the __must_hold() marking here to document this so that the tools can properly check and validate it as well? thanks, greg k-h
Hi Badhri,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d629c0e221cd99198b843d8351a0a9bfec6c0423]
url: https://github.com/intel-lab-lkp/linux/commits/Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
base: d629c0e221cd99198b843d8351a0a9bfec6c0423
patch link: https://lore.kernel.org/r/20230406062549.2461917-1-badhri%40google.com
patch subject: [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started
config: s390-allyesconfig (https://download.01.org/0day-ci/archive/20230406/202304061758.Tz5RJDZU-lkp@intel.com/config)
compiler: s390-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
git checkout 2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=s390 SHELL=/bin/bash drivers/usb/gadget/udc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061758.Tz5RJDZU-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/usb/gadget/udc/core.c:696:5: warning: no previous prototype for 'usb_gadget_connect_locked' [-Wmissing-prototypes]
696 | int usb_gadget_connect_locked(struct usb_gadget *gadget)
| ^~~~~~~~~~~~~~~~~~~~~~~~~
>> drivers/usb/gadget/udc/core.c:749:5: warning: no previous prototype for 'usb_gadget_disconnect_locked' [-Wmissing-prototypes]
749 | int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/usb_gadget_connect_locked +696 drivers/usb/gadget/udc/core.c
694
695 /* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */
> 696 int usb_gadget_connect_locked(struct usb_gadget *gadget)
697 {
698 int ret = 0;
699
700 if (!gadget->ops->pullup) {
701 ret = -EOPNOTSUPP;
702 goto out;
703 }
704
705 if (gadget->deactivated || !gadget->udc->started) {
706 /*
707 * If gadget is deactivated we only save new state.
708 * Gadget will be connected automatically after activation.
709 *
710 * udc first needs to be started before gadget can be pulled up.
711 */
712 gadget->connected = true;
713 goto out;
714 }
715
716 ret = gadget->ops->pullup(gadget, 1);
717 if (!ret)
718 gadget->connected = 1;
719
720 out:
721 trace_usb_gadget_connect(gadget, ret);
722
723 return ret;
724 }
725
726 /**
727 * usb_gadget_connect - software-controlled connect to USB host
728 * @gadget:the peripheral being connected
729 *
730 * Enables the D+ (or potentially D-) pullup. The host will start
731 * enumerating this gadget when the pullup is active and a VBUS session
732 * is active (the link is powered).
733 *
734 * Returns zero on success, else negative errno.
735 */
736 int usb_gadget_connect(struct usb_gadget *gadget)
737 {
738 int ret;
739
740 mutex_lock(&gadget->udc->connect_lock);
741 ret = usb_gadget_connect_locked(gadget);
742 mutex_unlock(&gadget->udc->connect_lock);
743
744 return ret;
745 }
746 EXPORT_SYMBOL_GPL(usb_gadget_connect);
747
748 /* Internal version of usb_gadget_disconnect needs to be called with udc->connect_lock held. */
> 749 int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
750 {
751 int ret = 0;
752
753 if (!gadget->ops->pullup) {
754 ret = -EOPNOTSUPP;
755 goto out;
756 }
757
758 if (!gadget->connected)
759 goto out;
760
761 if (gadget->deactivated || !gadget->udc->started) {
762 /*
763 * If gadget is deactivated we only save new state.
764 * Gadget will stay disconnected after activation.
765 *
766 * udc should have been started before gadget being pulled down.
767 */
768 gadget->connected = false;
769 goto out;
770 }
771
772 ret = gadget->ops->pullup(gadget, 0);
773 if (!ret)
774 gadget->connected = 0;
775
776 mutex_lock(&udc_lock);
777 if (gadget->udc->driver)
778 gadget->udc->driver->disconnect(gadget);
779 mutex_unlock(&udc_lock);
780
781 out:
782 trace_usb_gadget_disconnect(gadget, ret);
783
784 return ret;
785 }
786
Hi Badhri,
kernel test robot noticed the following build warnings:
[auto build test WARNING on d629c0e221cd99198b843d8351a0a9bfec6c0423]
url: https://github.com/intel-lab-lkp/linux/commits/Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
base: d629c0e221cd99198b843d8351a0a9bfec6c0423
patch link: https://lore.kernel.org/r/20230406062549.2461917-1-badhri%40google.com
patch subject: [PATCH v2 1/2] usb: gadget: udc: core: Invoke usb_gadget_connect only when started
config: hexagon-randconfig-r041-20230403 (https://download.01.org/0day-ci/archive/20230406/202304061713.mqRzNBGz-lkp@intel.com/config)
compiler: clang version 17.0.0 (https://github.com/llvm/llvm-project 67409911353323ca5edf2049ef0df54132fa1ca7)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Badhri-Jagan-Sridharan/usb-gadget-udc-core-Prevent-redundant-calls-to-pullup/20230406-142708
git checkout 2f12e8b0c9bf3d25df88c73b614c3e8d84bd7338
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/usb/gadget/udc/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304061713.mqRzNBGz-lkp@intel.com/
All warnings (new ones prefixed by >>):
In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:547:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __raw_readb(PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:560:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:37:51: note: expanded from macro '__le16_to_cpu'
#define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
^
In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:573:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
~~~~~~~~~~ ^
include/uapi/linux/byteorder/little_endian.h:35:51: note: expanded from macro '__le32_to_cpu'
#define __le32_to_cpu(x) ((__force __u32)(__le32)(x))
^
In file included from drivers/usb/gadget/udc/core.c:17:
In file included from include/linux/dma-mapping.h:10:
In file included from include/linux/scatterlist.h:9:
In file included from arch/hexagon/include/asm/io.h:334:
include/asm-generic/io.h:584:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writeb(value, PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:594:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
include/asm-generic/io.h:604:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
__raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
~~~~~~~~~~ ^
>> drivers/usb/gadget/udc/core.c:696:5: warning: no previous prototype for function 'usb_gadget_connect_locked' [-Wmissing-prototypes]
int usb_gadget_connect_locked(struct usb_gadget *gadget)
^
drivers/usb/gadget/udc/core.c:696:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int usb_gadget_connect_locked(struct usb_gadget *gadget)
^
static
>> drivers/usb/gadget/udc/core.c:749:5: warning: no previous prototype for function 'usb_gadget_disconnect_locked' [-Wmissing-prototypes]
int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
^
drivers/usb/gadget/udc/core.c:749:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
^
static
8 warnings generated.
vim +/usb_gadget_connect_locked +696 drivers/usb/gadget/udc/core.c
694
695 /* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */
> 696 int usb_gadget_connect_locked(struct usb_gadget *gadget)
697 {
698 int ret = 0;
699
700 if (!gadget->ops->pullup) {
701 ret = -EOPNOTSUPP;
702 goto out;
703 }
704
705 if (gadget->deactivated || !gadget->udc->started) {
706 /*
707 * If gadget is deactivated we only save new state.
708 * Gadget will be connected automatically after activation.
709 *
710 * udc first needs to be started before gadget can be pulled up.
711 */
712 gadget->connected = true;
713 goto out;
714 }
715
716 ret = gadget->ops->pullup(gadget, 1);
717 if (!ret)
718 gadget->connected = 1;
719
720 out:
721 trace_usb_gadget_connect(gadget, ret);
722
723 return ret;
724 }
725
726 /**
727 * usb_gadget_connect - software-controlled connect to USB host
728 * @gadget:the peripheral being connected
729 *
730 * Enables the D+ (or potentially D-) pullup. The host will start
731 * enumerating this gadget when the pullup is active and a VBUS session
732 * is active (the link is powered).
733 *
734 * Returns zero on success, else negative errno.
735 */
736 int usb_gadget_connect(struct usb_gadget *gadget)
737 {
738 int ret;
739
740 mutex_lock(&gadget->udc->connect_lock);
741 ret = usb_gadget_connect_locked(gadget);
742 mutex_unlock(&gadget->udc->connect_lock);
743
744 return ret;
745 }
746 EXPORT_SYMBOL_GPL(usb_gadget_connect);
747
748 /* Internal version of usb_gadget_disconnect needs to be called with udc->connect_lock held. */
> 749 int usb_gadget_disconnect_locked(struct usb_gadget *gadget)
750 {
751 int ret = 0;
752
753 if (!gadget->ops->pullup) {
754 ret = -EOPNOTSUPP;
755 goto out;
756 }
757
758 if (!gadget->connected)
759 goto out;
760
761 if (gadget->deactivated || !gadget->udc->started) {
762 /*
763 * If gadget is deactivated we only save new state.
764 * Gadget will stay disconnected after activation.
765 *
766 * udc should have been started before gadget being pulled down.
767 */
768 gadget->connected = false;
769 goto out;
770 }
771
772 ret = gadget->ops->pullup(gadget, 0);
773 if (!ret)
774 gadget->connected = 0;
775
776 mutex_lock(&udc_lock);
777 if (gadget->udc->driver)
778 gadget->udc->driver->disconnect(gadget);
779 mutex_unlock(&udc_lock);
780
781 out:
782 trace_usb_gadget_disconnect(gadget, ret);
783
784 return ret;
785 }
786
On Wed, Apr 5, 2023 at 11:37 PM Greg KH <gregkh@linuxfoundation.org> wrote: > > On Thu, Apr 06, 2023 at 06:25:48AM +0000, Badhri Jagan Sridharan wrote: > > usb_udc_connect_control does not check to see if the udc has already > > been started. This causes gadget->ops->pullup to be called through > > usb_gadget_connect when invoked from usb_udc_vbus_handler even before > > usb_gadget_udc_start is called. Guard this by checking for udc->started > > in usb_udc_connect_control before invoking usb_gadget_connect. > > > > Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate > > related functions with connect_lock. usb_gadget_connect_locked, > > usb_gadget_disconnect_locked, usb_udc_connect_control_locked, > > usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are called with > > this lock held as they can be simulataneously invoked from different code > > paths. > > > > Adding an additional check to make sure udc is started(udc->started) > > before pullup callback is invoked. > > > > Cc: stable@vger.kernel.org > > Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") > > Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> > > --- > > * Fixed commit message comments. > > * Renamed udc_connect_control_lock to connect_lock and made it per > > device. > > * udc->vbus, udc->started, gadget->connect, gadget->deactivate are all > > now guarded by connect_lock. > > * Code now checks for udc->started to be set before invoking pullup > > callback. > > --- > > drivers/usb/gadget/udc/core.c | 140 +++++++++++++++++++++++----------- > > 1 file changed, 96 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c > > index 3dcbba739db6..41d3a1998cff 100644 > > --- a/drivers/usb/gadget/udc/core.c > > +++ b/drivers/usb/gadget/udc/core.c > > @@ -37,6 +37,10 @@ static struct bus_type gadget_bus_type; > > * @vbus: for udcs who care about vbus status, this value is real vbus status; > > * for udcs who do not care about vbus status, this value is always true > > * @started: the UDC's started state. True if the UDC had started. > > + * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related > > + * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked, > > + * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are > > + * called with this lock held. > > * > > * This represents the internal data structure which is used by the UDC-class > > * to hold information about udc driver and gadget together. > > @@ -48,6 +52,7 @@ struct usb_udc { > > struct list_head list; > > bool vbus; > > bool started; > > + struct mutex connect_lock; > > }; > > > > static struct class *udc_class; > > @@ -687,17 +692,8 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > > } > > EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); > > > > -/** > > - * usb_gadget_connect - software-controlled connect to USB host > > - * @gadget:the peripheral being connected > > - * > > - * Enables the D+ (or potentially D-) pullup. The host will start > > - * enumerating this gadget when the pullup is active and a VBUS session > > - * is active (the link is powered). > > - * > > - * Returns zero on success, else negative errno. > > - */ > > -int usb_gadget_connect(struct usb_gadget *gadget) > > +/* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */ > > Shouldn't you just use the __must_hold() marking here to document this > so that the tools can properly check and validate it as well? Sure ! Have fixed it in v3. I also made these functions static in v4. Thanks, Badhri > > thanks, > > greg k-h
diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c index 3dcbba739db6..41d3a1998cff 100644 --- a/drivers/usb/gadget/udc/core.c +++ b/drivers/usb/gadget/udc/core.c @@ -37,6 +37,10 @@ static struct bus_type gadget_bus_type; * @vbus: for udcs who care about vbus status, this value is real vbus status; * for udcs who do not care about vbus status, this value is always true * @started: the UDC's started state. True if the UDC had started. + * @connect_lock: protects udc->vbus, udc->started, gadget->connect, gadget->deactivate related + * functions. usb_gadget_connect_locked, usb_gadget_disconnect_locked, + * usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are + * called with this lock held. * * This represents the internal data structure which is used by the UDC-class * to hold information about udc driver and gadget together. @@ -48,6 +52,7 @@ struct usb_udc { struct list_head list; bool vbus; bool started; + struct mutex connect_lock; }; static struct class *udc_class; @@ -687,17 +692,8 @@ int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) } EXPORT_SYMBOL_GPL(usb_gadget_vbus_disconnect); -/** - * usb_gadget_connect - software-controlled connect to USB host - * @gadget:the peripheral being connected - * - * Enables the D+ (or potentially D-) pullup. The host will start - * enumerating this gadget when the pullup is active and a VBUS session - * is active (the link is powered). - * - * Returns zero on success, else negative errno. - */ -int usb_gadget_connect(struct usb_gadget *gadget) +/* Internal version of usb_gadget_connect needs to be called with udc_connect_control_lock held. */ +int usb_gadget_connect_locked(struct usb_gadget *gadget) { int ret = 0; @@ -706,10 +702,12 @@ int usb_gadget_connect(struct usb_gadget *gadget) goto out; } - if (gadget->deactivated) { + if (gadget->deactivated || !gadget->udc->started) { /* * If gadget is deactivated we only save new state. * Gadget will be connected automatically after activation. + * + * udc first needs to be started before gadget can be pulled up. */ gadget->connected = true; goto out; @@ -724,22 +722,31 @@ int usb_gadget_connect(struct usb_gadget *gadget) return ret; } -EXPORT_SYMBOL_GPL(usb_gadget_connect); /** - * usb_gadget_disconnect - software-controlled disconnect from USB host - * @gadget:the peripheral being disconnected - * - * Disables the D+ (or potentially D-) pullup, which the host may see - * as a disconnect (when a VBUS session is active). Not all systems - * support software pullup controls. + * usb_gadget_connect - software-controlled connect to USB host + * @gadget:the peripheral being connected * - * Following a successful disconnect, invoke the ->disconnect() callback - * for the current gadget driver so that UDC drivers don't need to. + * Enables the D+ (or potentially D-) pullup. The host will start + * enumerating this gadget when the pullup is active and a VBUS session + * is active (the link is powered). * * Returns zero on success, else negative errno. */ -int usb_gadget_disconnect(struct usb_gadget *gadget) +int usb_gadget_connect(struct usb_gadget *gadget) +{ + int ret; + + mutex_lock(&gadget->udc->connect_lock); + ret = usb_gadget_connect_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); + + return ret; +} +EXPORT_SYMBOL_GPL(usb_gadget_connect); + +/* Internal version of usb_gadget_disconnect needs to be called with udc->connect_lock held. */ +int usb_gadget_disconnect_locked(struct usb_gadget *gadget) { int ret = 0; @@ -751,10 +758,12 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) if (!gadget->connected) goto out; - if (gadget->deactivated) { + if (gadget->deactivated || !gadget->udc->started) { /* * If gadget is deactivated we only save new state. * Gadget will stay disconnected after activation. + * + * udc should have been started before gadget being pulled down. */ gadget->connected = false; goto out; @@ -774,6 +783,30 @@ int usb_gadget_disconnect(struct usb_gadget *gadget) return ret; } + +/** + * usb_gadget_disconnect - software-controlled disconnect from USB host + * @gadget:the peripheral being disconnected + * + * Disables the D+ (or potentially D-) pullup, which the host may see + * as a disconnect (when a VBUS session is active). Not all systems + * support software pullup controls. + * + * Following a successful disconnect, invoke the ->disconnect() callback + * for the current gadget driver so that UDC drivers don't need to. + * + * Returns zero on success, else negative errno. + */ +int usb_gadget_disconnect(struct usb_gadget *gadget) +{ + int ret; + + mutex_lock(&gadget->udc->connect_lock); + ret = usb_gadget_disconnect_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); + + return ret; +} EXPORT_SYMBOL_GPL(usb_gadget_disconnect); /** @@ -794,10 +827,11 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) if (gadget->deactivated) goto out; + mutex_lock(&gadget->udc->connect_lock); if (gadget->connected) { - ret = usb_gadget_disconnect(gadget); + ret = usb_gadget_disconnect_locked(gadget); if (ret) - goto out; + goto unlock; /* * If gadget was being connected before deactivation, we want @@ -807,6 +841,8 @@ int usb_gadget_deactivate(struct usb_gadget *gadget) } gadget->deactivated = true; +unlock: + mutex_unlock(&gadget->udc->connect_lock); out: trace_usb_gadget_deactivate(gadget, ret); @@ -830,6 +866,7 @@ int usb_gadget_activate(struct usb_gadget *gadget) if (!gadget->deactivated) goto out; + mutex_lock(&gadget->udc->connect_lock); gadget->deactivated = false; /* @@ -837,7 +874,8 @@ int usb_gadget_activate(struct usb_gadget *gadget) * while it was being deactivated, we call usb_gadget_connect(). */ if (gadget->connected) - ret = usb_gadget_connect(gadget); + ret = usb_gadget_connect_locked(gadget); + mutex_unlock(&gadget->udc->connect_lock); out: trace_usb_gadget_activate(gadget, ret); @@ -1078,12 +1116,13 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state); /* ------------------------------------------------------------------------- */ -static void usb_udc_connect_control(struct usb_udc *udc) +/* Acquire udc_connect_control_lock before calling this function. */ +static void usb_udc_connect_control_locked(struct usb_udc *udc) { - if (udc->vbus) - usb_gadget_connect(udc->gadget); + if (udc->vbus && udc->started) + usb_gadget_connect_locked(udc->gadget); else - usb_gadget_disconnect(udc->gadget); + usb_gadget_disconnect_locked(udc->gadget); } /** @@ -1099,10 +1138,12 @@ void usb_udc_vbus_handler(struct usb_gadget *gadget, bool status) { struct usb_udc *udc = gadget->udc; + mutex_lock(&udc->connect_lock); if (udc) { udc->vbus = status; - usb_udc_connect_control(udc); + usb_udc_connect_control_locked(udc); } + mutex_unlock(&udc->connect_lock); } EXPORT_SYMBOL_GPL(usb_udc_vbus_handler); @@ -1124,7 +1165,7 @@ void usb_gadget_udc_reset(struct usb_gadget *gadget, EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); /** - * usb_gadget_udc_start - tells usb device controller to start up + * usb_gadget_udc_start_locked - tells usb device controller to start up * @udc: The UDC to be started * * This call is issued by the UDC Class driver when it's about @@ -1136,7 +1177,7 @@ EXPORT_SYMBOL_GPL(usb_gadget_udc_reset); * * Returns zero on success, else negative errno. */ -static inline int usb_gadget_udc_start(struct usb_udc *udc) +static inline int usb_gadget_udc_start_locked(struct usb_udc *udc) { int ret; @@ -1153,7 +1194,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) } /** - * usb_gadget_udc_stop - tells usb device controller we don't need it anymore + * usb_gadget_udc_stop_locked - tells usb device controller we don't need it anymore * @udc: The UDC to be stopped * * This call is issued by the UDC Class driver after calling @@ -1163,7 +1204,7 @@ static inline int usb_gadget_udc_start(struct usb_udc *udc) * far as powering off UDC completely and disable its data * line pullups. */ -static inline void usb_gadget_udc_stop(struct usb_udc *udc) +static inline void usb_gadget_udc_stop_locked(struct usb_udc *udc) { if (!udc->started) { dev_err(&udc->dev, "UDC had already stopped\n"); @@ -1322,6 +1363,7 @@ int usb_add_gadget(struct usb_gadget *gadget) udc->gadget = gadget; gadget->udc = udc; + mutex_init(&udc->connect_lock); udc->started = false; @@ -1523,11 +1565,15 @@ static int gadget_bind_driver(struct device *dev) if (ret) goto err_bind; - ret = usb_gadget_udc_start(udc); - if (ret) + mutex_lock(&udc->connect_lock); + ret = usb_gadget_udc_start_locked(udc); + if (ret) { + mutex_unlock(&udc->connect_lock); goto err_start; + } usb_gadget_enable_async_callbacks(udc); - usb_udc_connect_control(udc); + usb_udc_connect_control_locked(udc); + mutex_unlock(&udc->connect_lock); kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); return 0; @@ -1558,12 +1604,14 @@ static void gadget_unbind_driver(struct device *dev) kobject_uevent(&udc->dev.kobj, KOBJ_CHANGE); - usb_gadget_disconnect(gadget); + mutex_lock(&udc->connect_lock); + usb_gadget_disconnect_locked(gadget); usb_gadget_disable_async_callbacks(udc); if (gadget->irq) synchronize_irq(gadget->irq); udc->driver->unbind(gadget); - usb_gadget_udc_stop(udc); + usb_gadget_udc_stop_locked(udc); + mutex_unlock(&udc->connect_lock); mutex_lock(&udc_lock); driver->is_bound = false; @@ -1649,11 +1697,15 @@ static ssize_t soft_connect_store(struct device *dev, } if (sysfs_streq(buf, "connect")) { - usb_gadget_udc_start(udc); - usb_gadget_connect(udc->gadget); + mutex_lock(&udc->connect_lock); + usb_gadget_udc_start_locked(udc); + usb_gadget_connect_locked(udc->gadget); + mutex_unlock(&udc->connect_lock); } else if (sysfs_streq(buf, "disconnect")) { - usb_gadget_disconnect(udc->gadget); - usb_gadget_udc_stop(udc); + mutex_lock(&udc->connect_lock); + usb_gadget_disconnect_locked(udc->gadget); + usb_gadget_udc_stop_locked(udc); + mutex_unlock(&udc->connect_lock); } else { dev_err(dev, "unsupported command '%s'\n", buf); ret = -EINVAL;
usb_udc_connect_control does not check to see if the udc has already been started. This causes gadget->ops->pullup to be called through usb_gadget_connect when invoked from usb_udc_vbus_handler even before usb_gadget_udc_start is called. Guard this by checking for udc->started in usb_udc_connect_control before invoking usb_gadget_connect. Guarding udc->vbus, udc->started, gadget->connect, gadget->deactivate related functions with connect_lock. usb_gadget_connect_locked, usb_gadget_disconnect_locked, usb_udc_connect_control_locked, usb_gadget_udc_start_locked, usb_gadget_udc_stop_locked are called with this lock held as they can be simulataneously invoked from different code paths. Adding an additional check to make sure udc is started(udc->started) before pullup callback is invoked. Cc: stable@vger.kernel.org Fixes: 628ef0d273a6 ("usb: udc: add usb_udc_vbus_handler") Signed-off-by: Badhri Jagan Sridharan <badhri@google.com> --- * Fixed commit message comments. * Renamed udc_connect_control_lock to connect_lock and made it per device. * udc->vbus, udc->started, gadget->connect, gadget->deactivate are all now guarded by connect_lock. * Code now checks for udc->started to be set before invoking pullup callback. --- drivers/usb/gadget/udc/core.c | 140 +++++++++++++++++++++++----------- 1 file changed, 96 insertions(+), 44 deletions(-) base-commit: d629c0e221cd99198b843d8351a0a9bfec6c0423