Hello.
Michal Nazarewicz wrote:
> This commit fixes a possible race condition in IDC class when
> usb_del_udc() is called concurrently to
> usb_gadget_register_driver() or usb_gadget_unregister_driver().
> * NOT FOR MERGING, NOT TESTED, NOT EVEN COMPILED *
> Signed-off-by: Michal Nazarewicz <m.nazarewicz@xxxxxxxxxxx>
Only some cosmetic stuff, partly not even related to this patch...
>> Also, the more I think about it, the more I'm convinced that this needs
>> some synchronisation. If usb_del_udc() is called at the same time as
>> usb_gadget_unregister_driver() funky stuff can happen. Funky stuff will
>> also happen if usb_del_udc() is called at the same time as
>> usb_gadget_register_driver().
>>
>> I think, spin lock should be replaced with a mutex and the all of the
>> operations need to be performed when holding the mutex. Alternatively,
>> mutex can be used in addition to spin lock and then usb_add_udc() could
>> work without locking the mutex. Or, per-UDC mutex needs to be created
>> and registering, unregistering and deleting would be performed while
>> holding this mutex.
> After more thought I came up with a scheme where usage of atomic
> operations (xchg and cmpxchg) is enough to guarantee correctness.
> The idea is that the driver field can have additional value:
> &udc_claimed. This value means that usb_del_udc(),
> usb_gadget_register_driver() or usb_gadget_unregister_driver() does
> something with the UDC. So basically, UDC can be in either of three
> states: free, busy or claimed where claimed means the above.
> usb_gadget_register_driver() treats claimed UDCs as busy,
> usb_gadget_unregister_driver() as ones of no interest, and
> usb_del_udc() waits till the UDC is unclaimed.
> To make it possible for usb_del_udc() to wait till
> usb_*register_gadget_driver() finishes work on UDC a wait queue is
> added.
> The busy flag has been removed.
> diff --git a/drivers/usb/gadget/udc-core.c b/drivers/usb/gadget/udc-core.c
> index 9334a91..6e40005 100644
> --- a/drivers/usb/gadget/udc-core.c
> +++ b/drivers/usb/gadget/udc-core.c
> @@ -31,6 +31,8 @@ static struct class *udc_class;
> static struct device_type udc_device_type;
> static LIST_HEAD(udc_list);
> static spinlock_t udc_lock;
> +static DECLARE_WAIT_QUEUE_HEAD(udc_wq);
> +static struct usb_gadget_driver udc_claimed;
>
> /* ------------------------------------------------------------------------- */
>
> @@ -98,8 +100,7 @@ int usb_add_udc(struct device *parent, struct usb_udc *udc)
> unsigned long flags;
> int ret;
>
> - if (!udc->gadget || !udc->gadget->ops ||
> - !udc->gadget->ops->start) {
> + if (!udc->gadget || !udc->gadget->ops || !udc->gadget->ops->start) {
> dev_dbg(parent, "unitializaed gadget\n");
Only "uninitialized". This is probably remark for Felipe...
> @@ -158,6 +159,34 @@ err1:
> }
> EXPORT_SYMBOL_GPL(usb_add_udc);
>
> +
> +static struct usb_gadget_driver *udc_claim_driver_wait(struct usb_udc *udc)
> +{
> + struct usb_gadget_driver *driver;
> + wait_event(udc_wq, (driver = xchg(&udc->driver, &udc_claimed))
> + != &udc_claimed);
> + return driver;
> +}
> +
> +static int udc_claim_driver_if(struct usb_udc *udc,
> + struct usb_gadget_driver *old)
> +{
> + return cmpxchg(&udc->driver, old, &udc_claimed) == old
> + ? 0 : -EBUSY;
> +}
> +
> +static void udc_unclaim_driver(struct usb_udc *udc,
> + struct usb_gadget_driver *driver)
> +{
> + smp_wmb();
> + udc->driver = driver;
> + wake_up_all(&udc_wq);
> +}
> +
> +static void __usb_gadget_unregister_driver(struct usb_udc *udc,
> + struct usb_gadget_driver *driver);
> +
> +
Probably too many empty lines at the start and end of this hunk...
WBR, Sergei
--
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
