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

From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Neukum <oneukum@suse.com>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Rhett Aultman <rhett.aultman@samsara.com>,
	Wolfgang Grandegger <wg@grandegger.com>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	linux-can <linux-can@vger.kernel.org>,
	linux-usb@vger.kernel.org
Subject: Re: [RFC PATCH] USB: core: urb: add new transfer flag URB_FREE_COHERENT
Date: Tue, 7 Jun 2022 19:18:08 +0900	[thread overview]
Message-ID: <CAMZ6Rq+CtOQ7Gn62QQqDd=_0dvCNhh5g_oXB6LmHEd0MfRym1g@mail.gmail.com> (raw)
In-Reply-To: <66b14321-667a-46a3-27db-cb8682bd5476@suse.com>

On Tue. 7 Jun 2022 at 18:49, Oliver Neukum <oneukum@suse.com> wrote:
> On 05.06.22 15:45, Vincent MAILHOL wrote:
> >
> > This is how I see things:
> >   * In the open() function, the driver will do the coherent allocation
> > for its transfer_buffers, fill those into URBs and add all the URBs in
> > an anchor.
> >   * During runtime, the driver will keep recycling the same URBs (no
> > need to kill URB nor to usb_free_coherent() the transfer_buffer).
> Yes.
> >   * Finally, in the close() function, the driver has to kill the URBs
> > and usb_free_coherent() the transfer_buffers. As far as I understand,
> > no helper functions allow us to do all that, thus requiring the driver
> > to iterate through the anchor to manually usb_free_coherent() the
> > transfer buffer.
> Yes. But you cannot nicely solve that with a flag as you proposed. You
> would need to use a helper function.
> > So, the intent of this patch is to provide a method to both kill the
> > URBs and usb_free_coherent() the transfer buffer at once. The
> Well, you don't directly. Your patch frees the buffer together with the URB.
> That has some uses, but you still would need to iterate over the URBs
> Yes, there is a helper for that, but then you cover one and only one
> use case, that is, you leave no way to free the buffers without
> at the same time discrading the URBs.
>
> You can do that, but it strikes me as unelegant.

Elegancy is also my concern.

My RFC originated from this patch:
https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/

Here the proposed solution was to keep a pointer of all the
transfer_buffer in a local array to be able to free them when closing.
I really found that original patch to be unelegant which led me to
propose this RFC.
Comparatively, I still think my patch to be a more elegant solution,
and the original author also seems to share my thoughts.

If my patch is unelegant, then what would be the elegant/state of the
art way to free all this DMA allocated memory?
(pointing to any reference driver implementation should be enough for
me to understand).

Yours sincerely,
Vincent Mailhol