Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <mkl@pengutronix.de> wrote:
> > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > From: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > >
> > > 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 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() on the
> > > transfer buffer when the URB is killed, similarly to how
> > > URB_FREE_BUFFER triggers a call to kfree().
> > >
> > > In order to have all the flags in numerical order, URB_DIR_IN is
> > > renumbered from 0x0200 to 0x0400 so that URB_FREE_COHERENT can reuse
> > > value 0x0200.
> > >
> > > Co-developed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > Signed-off-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com>
> > > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>
> >
> > FWIW:
> > Acked-by: Marc Kleine-Budde <mkl@pengutronix.de>
> >
> > This patch probably goes upstream via USB. Once this is in net I'll take
> > the 2nd patch.
> 
> Question to Greg: can this first patch also be applied to the stable
> branches? Technically, this is a new feature but it will be used to
> solve several memory leaks on existing drivers (the gs_usb is only one
> example).

We take in dependent patches into the stable trees all the time when
needed, that's not an issue here.

What is an issue here is that this feels odd as other USB developers
said previously.

My big objection here is what validates that the size of the transfer
buffer here is really the size of the buffer to be freed?  Is that
always set properly to be the length that was allocated?  That might
just be the size of the last transfer using this buffer, but there is no
guarantee that I can see of that says this really is the length of the
allocated buffer, which is why usb_free_coherent() requires a size
parameter.

If that guarantee is always right, then we should be able to drop the
size option in usb_free_coherent(), and I don't think that's really
possible.

thanks,

greg k-h