Re: [PATCH v3 1/2] usb: udc: core: hide struct usb_gadget_driver to gadget driver

On 2021-06-20 21:47, Alan Stern wrote:

On Sun, Jun 20, 2021 at 11:53:18AM +0800, linyyuan@codeaurora.org 
wrote:

quoted

On 2021-06-20 11:46, linyyuan@codeaurora.org wrote:

quoted

On 2021-06-20 10:13, Alan Stern wrote:

quoted

On Sat, Jun 19, 2021 at 11:43:08PM +0800, Linyu Yuan wrote:

quoted

currently most gadget driver have a pointer to save
struct usb_gadget_driver from upper layer,
it allow upper layer set and unset of the pointer.

there is race that upper layer unset the pointer first,
but gadget driver use the pointer later,
and it cause system crash due to NULL pointer access.

This race has already been fixed in Greg's usb-next branch.  See
commit
7dc0c55e9f30 ("USB: UDC core: Add udc_async_callbacks gadget op") and
following commits 04145a03db9d ("USB: UDC: Implement
udc_async_callbacks in dummy-hcd") and b42e8090ba93 ("USB: UDC:
Implement udc_async_callbacks in net2280").

thanks, this is better, lower driver only need change several places.

quoted

You just need to write a corresponding patch implementing the
async_callbacks op for dwc3.

yes, i will do.

quoted

Alan, i want to discuss your suggestion again in b42e8090ba93 ("USB: 
UDC:
Implement udc_async_callbacks in net2280")

+                       if (dev->async_callbacks) { ----> if CPU1 saw 
this
is true
+                               spin_unlock(&dev->lock); ---> CPU2 get 
lock
after this unlock,
it will set async_callbacks to false, then follow call also crash, 
right ?
+                               tmp = dev->driver->setup(&dev->gadget,
&u.r);
+                               spin_lock(&dev->lock);
+                       }

No, this is okay.  The reason is because usb_gadget_remove_driver (CPU2
in your example) does this:

        usb_gadget_disable_async_callbacks(udc);
        if (udc->gadget->irq)
                synchronize_irq(udc->gadget->irq);
        udc->driver->unbind(udc->gadget);
        usb_gadget_udc_stop(udc);

The synchronize_irq call will make CPU2 wait until CPU1 has finished
handling the interrupt for the setup packet.  The system won't crash,
because dev->driver->setup will be called before unbind and udc_stop
instead of after.

still several question,
1. how about suspend calll dev->driver->suspend ?
2. will 04145a03db9d ("USB: UDC: Implement udc_async_callbacks in 
dummy-hcd") backport to LTS branch ?
3. how about coding style ? so following code
if (foo->gadget_driver && foo->gadget_driver->resume)
change to
if (foo->asnyc_callbacks && foo->gadget_driver->resume)

Alan Stern