On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote: > On Wed 22 Jun 2022 at 01:15, Alan Stern <stern@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx> 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
