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