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

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

On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > 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.
> 
> Thanks for the comment.
> 
> I (probably wrongly) assumed that urb::transfer_buffer_length was the
> allocated length and urb::actual_length was what was actually being
> transferred. Right now, I am just confused. Seems that I need to study
> a bit more and understand the real purpose of
> urb::transfer_buffer_length because I still fail to understand in
> which situation this can be different from the allocated length.
> 
> > 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.
> 
> I do not follow you on this comment. usb_free_coherent() does not have
> a reference to the URB, right? How would it be supposed to retrieve
> urb::transfer_buffer_length?

Ah, good point.  Along those lines, how do you know what the `dma`
parameter should be set to as well?

thanks,

greg k-h