Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT

[Vincent MAILHOL <mailhol.vincent@wanadoo.fr>]

On Sun. 5 juin 2022 at 01:40, Alan Stern <stern@rowland.harvard.edu> wrote :
> On Sat, Jun 04, 2022 at 11:41:57PM +0900, Vincent Mailhol wrote:
> > When allocating URB memory with kmalloc(), drivers can simply set the
> > URB_FREE_BUFFER flag in urb::transfer_flags and that way, the memory
> > will be freed in the background when calling killing the URB (for
> > example with usb_kill_anchored_urbs()).
> >
> > However, there are no equivalent mechanism when allocating DMA memory
> > (with usb_alloc_coherent()).
> >
> > This patch adds a new flag: URB_FREE_COHERENT. Setting this flag will
> > cause the kernel to automatically call usb_free_coherent() when the
> > URB is killed, similarly to how URB_FREE_BUFFER triggers a call to
> > kfree().
> >
> > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > ---
> > Hi Rhett Aultman,
> >
> > I put the code snippet I previously sent into a patch. It is not
> > tested (this is why I post it as RFC). Please feel free to add this to
> > your series.
> > ---
> >  drivers/usb/core/urb.c | 3 +++
> >  include/linux/usb.h    | 1 +
> >  2 files changed, 4 insertions(+)
> >
> > diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> > index 33d62d7e3929..1460fdac0b18 100644
> > --- a/drivers/usb/core/urb.c
> > +++ b/drivers/usb/core/urb.c
> > @@ -22,6 +22,9 @@ static void urb_destroy(struct kref *kref)
> >
> >       if (urb->transfer_flags & URB_FREE_BUFFER)
> >               kfree(urb->transfer_buffer);
> > +     else if (urb->transfer_flags & URB_FREE_COHERENT)
> > +             usb_free_coherent(urb->dev, urb->transfer_buffer_length,
> > +                               urb->transfer_buffer, urb->transfer_dma);
> >
> >       kfree(urb);
> >  }
> > diff --git a/include/linux/usb.h b/include/linux/usb.h
> > index 60bee864d897..2200b3785fdb 100644
> > --- a/include/linux/usb.h
> > +++ b/include/linux/usb.h
> > @@ -1328,6 +1328,7 @@ extern int usb_disabled(void);
> >  #define URB_NO_INTERRUPT     0x0080  /* HINT: no non-error interrupt
> >                                        * needed */
> >  #define URB_FREE_BUFFER              0x0100  /* Free transfer buffer with the URB */
> > +#define URB_FREE_COHERENT    0x0400  /* Free DMA memory of transfer buffer */
> >
> >  /* The following flags are used internally by usbcore and HCDs */
> >  #define URB_DIR_IN           0x0200  /* Transfer from device to host */
>
> I don't see anything wrong with this, except that it would be nice to keep
> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
> to 0x0200 and change URB_DIR_IN to 0x0400.

Indeed, the URB_DIR_IN macro is not part of the UAPI so it should be
safe to renumber it. Maybe I was confused by the USB_DIR_IN which is
part of UAPI so some part of my subcocient was telling me not to
change it...

Thanks for pointing this out, I will send a v2 right away (and I will
keep the RFC tag because this is not yet tested).


Yours sincerely,
Vincent Mailhol