On Wed. 22 Jun 2022 at 00:11, Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> wrote: > 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@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote: > > > > > On 10.06.2022 17:33:35, Rhett Aultman wrote: > > > > > > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > > > > > > > > > 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@xxxxxxxxxx> > > > > > > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > > > Co-developed-by: Rhett Aultman <rhett.aultman@xxxxxxxxxxx> > > > > > > Signed-off-by: Rhett Aultman <rhett.aultman@xxxxxxxxxxx> > > > > > > Reviewed-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > > > > > > > > > FWIW: > > > > > Acked-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > > > > > > > > > > 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? Here, just assuming that usb_alloc_coherent() was given urb::transfer_dma as an argument. Maybe I missed some edge cases but I never saw drivers doing it differently.
