On Wed, May 31, 2023 at 10:55 AM Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> wrote: > > On Wed, May 31, 2023 at 04:02:02AM +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(). > > After a merged version of patches 1/3 and 3/3 have been applied, it > seems like most of this will not be needed any more. Maybe not any of > it. Without the connect_lock introduced in this patch, wouldn't the usb_gadget_connect()/ usb_gadget_disconnect() through soft_connect_store() race against usb_gadget_connect()/ usb_gadget_disconnect() through usb_udc_connect_control() ? On a side note, I am working on merging patches 1/3 and 3/3. Thanks, Badhri > > usb_udc_connect_control() gets called from only two places. One of them > is in gadget_bind_driver(), where we know that the UDC has been started > and connecting is allowed. The other place is the vbus work routine > queued by usb_udc_vbus_handler(). If that place checks the new > allow_connect flag before calling usb_gadget_connect(), nothing more > will be needed. You just have to make sure that the allow_connect flag > is set in gadget_bind_driver between the start and connect_control > calls, and it is cleared in gadget_unbind_driver before the > cancel_work_sync call. > > It's possible that a new mutex will be needed to synchronize accesses to > the allow_connect flag. That's something you will have to study and > decide on. But if you can avoid adding one, that would be best. > > > 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. > > It's a general principle of kernel programming that locks protect data, > not code. So if this patch were to be accepted, you would have to > change this description to say that connect_lock guards various flags, > not various function calls. > > Alan Stern
