Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT - Greg Kroah-Hartman

From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Cc: Alan Stern <stern@rowland.harvard.edu>,
	Marc Kleine-Budde <mkl@pengutronix.de>,
	Rhett Aultman <rhett.aultman@samsara.com>,
	linux-usb@vger.kernel.org, linux-can <linux-can@vger.kernel.org>,
	Oliver Neukum <oneukum@suse.com>
Subject: Re: [PATCH v3 1/2] drivers: usb/core/urb: Add URB_FREE_COHERENT
Date: Tue, 21 Jun 2022 19:00:18 +0200	[thread overview]
Message-ID: <YrH5ImIssE/dLZln@kroah.com> (raw)
In-Reply-To: <CAMZ6RqLWKQWrpwfx81dj+Oh5MNcbJ+DdbqC9UQSOEqZcuY5spQ@mail.gmail.com>

On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@rowland.harvard.edu> wrote:
> > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <stern@rowland.harvard.edu> wrote:
> > > > On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> > > > > 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.
> > > >
> > > > urb->transfer_buffer_length is the amount of data that the driver wants
> > > > to send or expects to receive.  urb->actual_length is the amount of data
> > > > that was actually sent or actually received.
> > > >
> > > > Neither of these values has to be the same as the size of the buffer --
> > > > but they better not be bigger!
> > >
> > > Thanks. Now things are a bit clearer.
> > > I guess that for the outcoming URB what I proposed made no sense. For
> > > incoming URB, I guess that most of the drivers want to set
> > > urb::transfer_buffer once for all with the allocated size and never
> > > touch it again.
> >
> > Not necessarily.  Some drivers may behave differently from the way you
> > expect.
> 
> Yes, my point is not to generalise. Agree that there are exceptions.
> 
> > > Maybe the patch only makes sense of the incoming URB. Would it make
> > > sense to keep it but with an additional check to trigger a dmesg
> > > warning if this is used on an outcoming endpoint and with additional
> > > comment that the URB_FREE_COHERENT requires urb::transfer_buffer to be
> > > the allocated size?
> >
> > Well, what really matters is that the transfer_buffer_length value has
> > to be the same as the size of the buffer.  If that's true, the direction
> > of the URB doesn't matter.  So yes, that requirement would definitely
> > need to be documented.
> >
> > On the other hand, there wouldn't be any way to tell automatically if
> > the requirement was violated.
> 
> ACK. That's why I said "add comment" and not "check".
> 
> > And since this function could only be
> > used with some of the URBs you're interested in, does it make sense to
> > add it at all?  The other URBs would still need their buffers to be
> > freed manually.
> 
> The rationale is that similarly to URB_FREE_BUFFER, this would be
> optional. This is why I did not propose to reuse
> URB_NO_TRANSFER_DMA_MAP but instead add a new flag. I propose it
> because I think that many drivers can benefit from it.
> 
> More than that, the real concern is that many developers forget to
> free the DMA allocated memory. c.f. original message of this thread:
> https://lore.kernel.org/linux-can/alpine.DEB.2.22.394.2206031547001.1630869@thelappy/T/#m2ef343d3ee708178b1e37be898884bafa7f49f2f
> 
> And the usual fix requires to create local arrays to store references
> to the transfer buffer and DMA addresses.

Why not just free the memory in the urb completion function that is
called when it is finished?

thanks,

greg k-h