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

Commit Message

Rhett Aultman June 10, 2022, 9:33 p.m. UTC

From: Vincent Mailhol <[e-mail]>

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 <[e-mail]>
Signed-off-by: Vincent Mailhol <[e-mail]>
Co-developed-by: Rhett Aultman <[e-mail]>
Signed-off-by: Rhett Aultman <[e-mail]>
Reviewed-by: Vincent Mailhol <[e-mail]>
---
 drivers/usb/core/urb.c | 5 ++++-
 include/linux/usb.h    | 3 ++-
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Marc Kleine-Budde June 11, 2022, 3:31 p.m. UTC | #1

On 10.06.2022 17:33:35, Rhett Aultman wrote:
> From: Vincent Mailhol <[e-mail]>
> 
> 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 <[e-mail]>
> Signed-off-by: Vincent Mailhol <[e-mail]>
> Co-developed-by: Rhett Aultman <[e-mail]>
> Signed-off-by: Rhett Aultman <[e-mail]>
> Reviewed-by: Vincent Mailhol <[e-mail]>

FWIW:
Acked-by: Marc Kleine-Budde <[e-mail]>

This patch probably goes upstream via USB. Once this is in net I'll take
the 2nd patch.

regards,
Marc

Vincent MAILHOL June 11, 2022, 4:06 p.m. UTC | #2

On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <[e-mail]> wrote:
> On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > From: Vincent Mailhol <[e-mail]>
> >
> > 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 <[e-mail]>
> > Signed-off-by: Vincent Mailhol <[e-mail]>
> > Co-developed-by: Rhett Aultman <[e-mail]>
> > Signed-off-by: Rhett Aultman <[e-mail]>
> > Reviewed-by: Vincent Mailhol <[e-mail]>
>
> FWIW:
> Acked-by: Marc Kleine-Budde <[e-mail]>
>
> 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).

Yours sincerely,
Vincent Mailhol

Greg Kroah-Hartman June 21, 2022, 1:51 p.m. UTC | #3

On Sun, Jun 12, 2022 at 01:06:37AM +0900, Vincent MAILHOL wrote:
> On Sun. 12 juin 2022 at 00:31, Marc Kleine-Budde <[e-mail]> wrote:
> > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > From: Vincent Mailhol <[e-mail]>
> > >
> > > 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 <[e-mail]>
> > > Signed-off-by: Vincent Mailhol <[e-mail]>
> > > Co-developed-by: Rhett Aultman <[e-mail]>
> > > Signed-off-by: Rhett Aultman <[e-mail]>
> > > Reviewed-by: Vincent Mailhol <[e-mail]>
> >
> > FWIW:
> > Acked-by: Marc Kleine-Budde <[e-mail]>
> >
> > 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.

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.

thanks,

greg k-h

Vincent MAILHOL June 21, 2022, 2:59 p.m. UTC | #4

On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
<[e-mail]> 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 <[e-mail]> wrote:
> > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > From: Vincent Mailhol <[e-mail]>
> > > >
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Vincent Mailhol <[e-mail]>
> > > > Co-developed-by: Rhett Aultman <[e-mail]>
> > > > Signed-off-by: Rhett Aultman <[e-mail]>
> > > > Reviewed-by: Vincent Mailhol <[e-mail]>
> > >
> > > FWIW:
> > > Acked-by: Marc Kleine-Budde <[e-mail]>
> > >
> > > 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?


Yours sincerely,
Vincent Mailhol

Greg Kroah-Hartman June 21, 2022, 3:03 p.m. UTC | #5

On Tue, Jun 21, 2022 at 11:59:16PM +0900, Vincent MAILHOL wrote:
> On Tue. 21 Jun 2022 at 22:56, Greg Kroah-Hartman
> <[e-mail]> 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 <[e-mail]> wrote:
> > > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > > From: Vincent Mailhol <[e-mail]>
> > > > >
> > > > > 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 <[e-mail]>
> > > > > Signed-off-by: Vincent Mailhol <[e-mail]>
> > > > > Co-developed-by: Rhett Aultman <[e-mail]>
> > > > > Signed-off-by: Rhett Aultman <[e-mail]>
> > > > > Reviewed-by: Vincent Mailhol <[e-mail]>
> > > >
> > > > FWIW:
> > > > Acked-by: Marc Kleine-Budde <[e-mail]>
> > > >
> > > > 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

Alan Stern June 21, 2022, 3:11 p.m. UTC | #6

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!

Alan Stern

Vincent MAILHOL June 21, 2022, 3:54 p.m. UTC | #7

On Wed. 22 Jun 2022 at 00:11, Greg Kroah-Hartman
<[e-mail]> 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
> > <[e-mail]> 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 <[e-mail]> wrote:
> > > > > On 10.06.2022 17:33:35, Rhett Aultman wrote:
> > > > > > From: Vincent Mailhol <[e-mail]>
> > > > > >
> > > > > > 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 <[e-mail]>
> > > > > > Signed-off-by: Vincent Mailhol <[e-mail]>
> > > > > > Co-developed-by: Rhett Aultman <[e-mail]>
> > > > > > Signed-off-by: Rhett Aultman <[e-mail]>
> > > > > > Reviewed-by: Vincent Mailhol <[e-mail]>
> > > > >
> > > > > FWIW:
> > > > > Acked-by: Marc Kleine-Budde <[e-mail]>
> > > > >
> > > > > 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.

Vincent MAILHOL June 21, 2022, 3:55 p.m. UTC | #8

On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.

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?

Yours sincerely,
Vincent Mailhol

Alan Stern June 21, 2022, 4:14 p.m. UTC | #9

On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.

> 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.  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.

Alan Stern

Vincent MAILHOL June 21, 2022, 4:40 p.m. UTC | #10

On Wed 22 Jun 2022 at 01:15, Alan Stern <[e-mail]> wrote:
> On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.

I would like to find a solution to remove this burden from the drivers
and have an USB API to easily free the URB when, for example, killing
an anchor.


Yours sincerely,
Vincent Mailhol

Greg Kroah-Hartman June 21, 2022, 5 p.m. UTC | #11

On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> On Wed 22 Jun 2022 at 01:15, Alan Stern <[e-mail]> wrote:
> > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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

Vincent MAILHOL June 21, 2022, 5:14 p.m. UTC | #12

On Wed. 22 Jun 2022 at 02:02, Greg Kroah-Hartman
<[e-mail]> wrote:
> On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> > On Wed 22 Jun 2022 at 01:15, Alan Stern <[e-mail]> wrote:
> > > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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?

I thought it was a bad thing to call usb_free_coherent() in the
completion function and that the best practice was to allocate all the
DMA memory when opening the device, reuse them during runtime and free
everything when closing the device.

Especially, correct me if I am wrong, but isn't there a risk to
trigger below warning if calling usb_free_coherent() in an IRQ context
(which is the case in the completion function)?
https://elixir.bootlin.com/linux/v5.18/source/kernel/dma/mapping.c#L526


Yours sincerely,
Vincent Mailhol

Greg Kroah-Hartman June 21, 2022, 5:46 p.m. UTC | #13

On Wed, Jun 22, 2022 at 02:14:37AM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 02:02, Greg Kroah-Hartman
> <[e-mail]> wrote:
> > On Wed, Jun 22, 2022 at 01:40:10AM +0900, Vincent MAILHOL wrote:
> > > On Wed 22 Jun 2022 at 01:15, Alan Stern <[e-mail]> wrote:
> > > > On Wed, Jun 22, 2022 at 12:55:46AM +0900, Vincent MAILHOL wrote:
> > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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?
> 
> I thought it was a bad thing to call usb_free_coherent() in the
> completion function and that the best practice was to allocate all the
> DMA memory when opening the device, reuse them during runtime and free
> everything when closing the device.

Yes, that is true, but as you are finding out here, doing
fire-and-forget urbs with coherent memory aren't the easiest to use.

Which makes me ask, why do you have to use this type of memory for this
driver?  Why is it special to require it?

> Especially, correct me if I am wrong, but isn't there a risk to
> trigger below warning if calling usb_free_coherent() in an IRQ context
> (which is the case in the completion function)?
> https://elixir.bootlin.com/linux/v5.18/source/kernel/dma/mapping.c#L526

Ah, so your idea here wouldn't work either as if you did a
fire-and-forget urb with this type of buffer, the memory would be freed
in the urb callback in irq context.

So this might not be the interface you want to use at all here.

thanks,

greg k-h

David Laight June 22, 2022, 9:22 a.m. UTC | #14

From: Vincent MAILHOL
> Sent: 21 June 2022 16:56
> 
> On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> 
> 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?

IIRC urb are pretty big.
You'd be unlucky if adding an extra field to hold the allocated
size would ever need more memory.
So it might just be worth saving the allocated size.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Greg Kroah-Hartman June 22, 2022, 9:41 a.m. UTC | #15

On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> From: Vincent MAILHOL
> > Sent: 21 June 2022 16:56
> > 
> > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > 
> > 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?
> 
> IIRC urb are pretty big.

What exactly do you mean by "pretty big" here?  And what is wrong with
that, I have never seen any issues with the current size of that
structure in any benchmark or performance results.  All USB bottlenecks
that I know of are either in the hardware layer, or in the protocol
layer itself (i.e. usb-storage protocol).

> You'd be unlucky if adding an extra field to hold the allocated
> size would ever need more memory.
> So it might just be worth saving the allocated size.

Maybe, yes, then we could transition to allocating the urb and buffer at
the same time like we do partially for iso streams in an urb.  But that
still might be overkill for just this one driver.  I'm curious as to why
a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
its buffers in the first place.

thanks,

greg k-h

David Laight June 22, 2022, 10:03 a.m. UTC | #16

From: Greg Kroah-Hartman
> Sent: 22 June 2022 10:41
...
> > IIRC urb are pretty big.
> 
> What exactly do you mean by "pretty big" here?

Maybe 100-200 bytes?

> And what is wrong with
> that, I have never seen any issues with the current size of that
> structure in any benchmark or performance results.

Nothing really, the cost of allocating a sub-page structure
is pretty much independent of its size.
What I really meant is it isn't (say) 32 bytes where adding
another 4 could be a significant increase.

> All USB bottlenecks
> that I know of are either in the hardware layer, or in the protocol
> layer itself (i.e. usb-storage protocol).

There is a bufferbloat issue for usbnet on XHCI.
It would be better to fill the ring with (probably) 4k buffers
and have something sort out the USB frames from the buffers
and then get the network driver to use (IIRC) build_skb
to generate the skb from the data fragments.

I was only using 100M last time I was testing that and
didn't get performance issues.
Just problems with the USB connections 'bouncing', that
project got canned for other reasons ...

But at higher speeds and high network use it might all
be a problem.

> 
> > You'd be unlucky if adding an extra field to hold the allocated
> > size would ever need more memory.
> > So it might just be worth saving the allocated size.
> 
> Maybe, yes, then we could transition to allocating the urb and buffer at
> the same time like we do partially for iso streams in an urb.  But that
> still might be overkill for just this one driver.  I'm curious as to why
> a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> its buffers in the first place.

Yes, being able to do short transfers from buffer space in the urb
would save all the issues about having to allocate an extra
buffer to avoid DMA from stack.

Indeed for XHCI there is a bit that allows 8 bytes of data to
replace the pointer in the ring structure itself.
I don't remember the driver every doing that.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Vincent MAILHOL June 22, 2022, 10:34 a.m. UTC | #17

On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
<[e-mail]> wrote:
> On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > From: Vincent MAILHOL
> > > Sent: 21 June 2022 16:56
> > >
> > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > >
> > > 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?
> >
> > IIRC urb are pretty big.
>
> What exactly do you mean by "pretty big" here?  And what is wrong with
> that, I have never seen any issues with the current size of that
> structure in any benchmark or performance results.  All USB bottlenecks
> that I know of are either in the hardware layer, or in the protocol
> layer itself (i.e. usb-storage protocol).
>
> > You'd be unlucky if adding an extra field to hold the allocated
> > size would ever need more memory.
> > So it might just be worth saving the allocated size.
>
> Maybe, yes, then we could transition to allocating the urb and buffer at
> the same time like we do partially for iso streams in an urb.  But that
> still might be overkill for just this one driver.

Well, I wouldn't have proposed the patch if it only applied to a
single driver. If we add a urb::allocated_transfer_size as suggested
by David, I believe that the majority of the drivers using DMA memory
will be able to rely on that URB_FREE_COHERENT flag for the garbage
collection.

The caveat, as you pointed before, is that the developper still needs
to be aware of the limitations of DMA and that it should not be freed
in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
functions that would lead to urb_destroy().

> I'm curious as to why
> a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> its buffers in the first place.

The CAN protocol, in its latest revision, allows for transfer speed up
to ~5Mbits. For low performance CPUs, this starts to be a significant
load. Also, the CAN PDU being small (0 to 64 bytes), many small
transfers occur.
Unfortunately I did not do any benchmark myself so I won't be able to
back my explanation with numbers.


Yours sincerely,
Vincent Mailhol

Oliver Neukum June 22, 2022, 11:11 a.m. UTC | #18

On 22.06.22 12:03, David Laight wrote:
> Yes, being able to do short transfers from buffer space in the urb
> would save all the issues about having to allocate an extra
> buffer to avoid DMA from stack.

That is just as hard to do for DMA coherency.

> Indeed for XHCI there is a bit that allows 8 bytes of data to
> replace the pointer in the ring structure itself.
> I don't remember the driver every doing that.

XHCI does use it. There was even a bug with endianness.

	Regards
		Oliver

Greg Kroah-Hartman June 22, 2022, 12:23 p.m. UTC | #19

On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> <[e-mail]> wrote:
> > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > From: Vincent MAILHOL
> > > > Sent: 21 June 2022 16:56
> > > >
> > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > > >
> > > > 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?
> > >
> > > IIRC urb are pretty big.
> >
> > What exactly do you mean by "pretty big" here?  And what is wrong with
> > that, I have never seen any issues with the current size of that
> > structure in any benchmark or performance results.  All USB bottlenecks
> > that I know of are either in the hardware layer, or in the protocol
> > layer itself (i.e. usb-storage protocol).
> >
> > > You'd be unlucky if adding an extra field to hold the allocated
> > > size would ever need more memory.
> > > So it might just be worth saving the allocated size.
> >
> > Maybe, yes, then we could transition to allocating the urb and buffer at
> > the same time like we do partially for iso streams in an urb.  But that
> > still might be overkill for just this one driver.
> 
> Well, I wouldn't have proposed the patch if it only applied to a
> single driver. If we add a urb::allocated_transfer_size as suggested
> by David, I believe that the majority of the drivers using DMA memory
> will be able to rely on that URB_FREE_COHERENT flag for the garbage
> collection.
> 
> The caveat, as you pointed before, is that the developper still needs
> to be aware of the limitations of DMA and that it should not be freed
> in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> functions that would lead to urb_destroy().
> 
> > I'm curious as to why
> > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > its buffers in the first place.
> 
> The CAN protocol, in its latest revision, allows for transfer speed up
> to ~5Mbits. For low performance CPUs, this starts to be a significant
> load. Also, the CAN PDU being small (0 to 64 bytes), many small
> transfers occur.

And is the memcpy the actual issue here?  Even tiny cpus can do large
and small memcopy very very very fast.

> Unfortunately I did not do any benchmark myself so I won't be able to
> back my explanation with numbers.

That might be the simplest solution here :)

thanks,

greg k-h

Vincent MAILHOL June 22, 2022, 3:59 p.m. UTC | #20

On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
<[e-mail]> wrote:
> On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > <[e-mail]> wrote:
> > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > From: Vincent MAILHOL
> > > > > Sent: 21 June 2022 16:56
> > > > >
> > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > > > >
> > > > > 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?
> > > >
> > > > IIRC urb are pretty big.
> > >
> > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > that, I have never seen any issues with the current size of that
> > > structure in any benchmark or performance results.  All USB bottlenecks
> > > that I know of are either in the hardware layer, or in the protocol
> > > layer itself (i.e. usb-storage protocol).
> > >
> > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > size would ever need more memory.
> > > > So it might just be worth saving the allocated size.
> > >
> > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > the same time like we do partially for iso streams in an urb.  But that
> > > still might be overkill for just this one driver.
> >
> > Well, I wouldn't have proposed the patch if it only applied to a
> > single driver. If we add a urb::allocated_transfer_size as suggested
> > by David, I believe that the majority of the drivers using DMA memory
> > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > collection.
> >
> > The caveat, as you pointed before, is that the developper still needs
> > to be aware of the limitations of DMA and that it should not be freed
> > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > functions that would lead to urb_destroy().
> >
> > > I'm curious as to why
> > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > its buffers in the first place.
> >
> > The CAN protocol, in its latest revision, allows for transfer speed up
> > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > transfers occur.
>
> And is the memcpy the actual issue here?  Even tiny cpus can do large
> and small memcopy very very very fast.
>
> > Unfortunately I did not do any benchmark myself so I won't be able to
> > back my explanation with numbers.
>
> That might be the simplest solution here :)

Yes, this would give a clear answer whether or not DMA was needed in
the first place. But I do not own that gs_usb device to do the
benchmark myself (and to be honest I do not have time to dedicate for
this at the moment, maybe I will do it later on some other devices).

Has anyone from the linux-can mailing list ever done such a benchmark?
Else, is there anyone who would like to volunteer?


Yours sincerely,
Vincent Mailhol

Rhett Aultman June 22, 2022, 6:11 p.m. UTC | #21

On Thu, 23 Jun 2022, Vincent MAILHOL wrote:

> On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> <[e-mail]> wrote:
> > On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > > <[e-mail]> wrote:
> > > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > > From: Vincent MAILHOL
> > > > > > Sent: 21 June 2022 16:56
> > > > > >
> > > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > > > > >
> > > > > > 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?
> > > > >
> > > > > IIRC urb are pretty big.
> > > >
> > > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > > that, I have never seen any issues with the current size of that
> > > > structure in any benchmark or performance results.  All USB bottlenecks
> > > > that I know of are either in the hardware layer, or in the protocol
> > > > layer itself (i.e. usb-storage protocol).
> > > >
> > > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > > size would ever need more memory.
> > > > > So it might just be worth saving the allocated size.
> > > >
> > > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > > the same time like we do partially for iso streams in an urb.  But that
> > > > still might be overkill for just this one driver.
> > >
> > > Well, I wouldn't have proposed the patch if it only applied to a
> > > single driver. If we add a urb::allocated_transfer_size as suggested
> > > by David, I believe that the majority of the drivers using DMA memory
> > > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > > collection.
> > >
> > > The caveat, as you pointed before, is that the developper still needs
> > > to be aware of the limitations of DMA and that it should not be freed
> > > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > > functions that would lead to urb_destroy().
> > >
> > > > I'm curious as to why
> > > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > > its buffers in the first place.
> > >
> > > The CAN protocol, in its latest revision, allows for transfer speed up
> > > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > > transfers occur.
> >
> > And is the memcpy the actual issue here?  Even tiny cpus can do large
> > and small memcopy very very very fast.
> >
> > > Unfortunately I did not do any benchmark myself so I won't be able to
> > > back my explanation with numbers.
> >
> > That might be the simplest solution here :)
>
> Yes, this would give a clear answer whether or not DMA was needed in
> the first place. But I do not own that gs_usb device to do the
> benchmark myself (and to be honest I do not have time to dedicate for
> this at the moment, maybe I will do it later on some other devices).
>
> Has anyone from the linux-can mailing list ever done such a benchmark?
> Else, is there anyone who would like to volunteer?

I have access to a couple of gs_usb devices but I am afraid I have no
experience performing this sort of benchmarking and also would have to
squeeze it in as a weekend project or something similar.  That said, if
someone's willing to help step me through it, I can see if it's feasible
for me to do.

That said, the gs_usb driver is mostly following along a very well
established pattern for writing USB CAN devices.  Both the pattern
followed that created the memory leak, as well as the pattern I followed
to resolve the memory leak, were also seen in the esd2 USB CAN driver as
well, and likely others are following suit.  So, I don't know that we'd
need to keep it specific to gs_usb to gain good information here.

Best,
Rhett

Hongren Zheng June 23, 2022, 5:30 p.m. UTC | #22

On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
> 
> 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.

>  #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>  
>  /* The following flags are used internally by usbcore and HCDs */
> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>  #define URB_DIR_OUT		0
>  #define URB_DIR_MASK		URB_DIR_IN
>  
> -- 
> 2.30.2
>

I'm afraid this is a change of uapi as this field is, unfortunately,
exported by usbip to userspace as TCP packets.

This may also cause incompatibility (surprisingly not for this case,
detailed below) between usbip server and client
when one kernel is using the new flags and the other one is not.

If we do change this, we may need to bump usbip protocol version
accordingly.

A copy of Alan Stern's suggestion here for reference
> I don't see anything wrong with this, except that it would be nice to keep 
> the flag values in numerical order.  In other words, set URB_FREE_COHERENT 
> to 0x0200 and change URB_DIR_IN to 0x0400.
> 
> Alan Stern

In usbip, an URB is packed by client (vhci)
into a TCP packet by usbip_pack_cmd_submit,
in which transfer_flags is copied almost as-is,
except it clears the DMA flag.
Then the server (usbip-host) would accept
the packet, mask some flags and
re-submit_urb to usbcore.

In usbip protocol, URB_DIR_IN is not used
as it has a `direction' field, the in-kernel
implementation (usbip-host) also does not
use it as when re-submit_urb the usb_submit_urb
will calculate this specific bit again.

For FREE_COHERENT, since DMA flag is
cleared, it does not affect the protocol
if both server and client are using the new version.

If we are using vhci in newer kernel and
the other side is using the old version,
the USB_FREE_COHERENT flag would be 0x0200,
and will be transmitted via TCP/IP to usbip-host
or a userspace implementation (there are many of them),
which will perceive it as URB_DIR_IN.
usbip-host is not affected, but userspace
program might be affected.

If we are using vhci in old kernel, and
the other side is using the new version,
all the URB_DIR_IN would be conceived by
usbip-host as USB_FREE_COHERENT.
Fortunately, it will be masked by
masking_bogus_flags in stub_rx.c,
and usbip-host will behave correctly,
but userspace program might be affected.

From the analysis above it turned
out that this change does not affect
in-kernel usbip implementation, but in a
quite tricky way.

One way to solve this issue for usbip
is to add some boilerplate transform
from URB_* to USBIP_FLAGS_*
as it is de facto uapi now.

Another way is to use 0x0400 for FREE_COHERENT.
usbip will not take care of this bit as
it would be masked.

Cc Shuah Khan here since she is the maintainer
on usbip.

Regards,

Hongren

Shuah Khan June 23, 2022, 5:45 p.m. UTC | #23

On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
>>
>> 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.
> 
>>   #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
>> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>>   
>>   /* The following flags are used internally by usbcore and HCDs */
>> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
>> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>>   #define URB_DIR_OUT		0
>>   #define URB_DIR_MASK		URB_DIR_IN
>>   
>> -- 
>> 2.30.2
>>
> 
> I'm afraid this is a change of uapi as this field is, unfortunately,
> exported by usbip to userspace as TCP packets.
> 
> This may also cause incompatibility (surprisingly not for this case,
> detailed below) between usbip server and client
> when one kernel is using the new flags and the other one is not.
> 
> If we do change this, we may need to bump usbip protocol version
> accordingly.
> 


> A copy of Alan Stern's suggestion here for reference
>> I don't see anything wrong with this, except that it would be nice to keep
>> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
>> to 0x0200 and change URB_DIR_IN to 0x0400.
>>
>> Alan Stern

Thank you Alan for this detailed analysis of uapi impacts and
usbip host side and vhci incompatibilities. Userspace is going
to be affected. In addition to the usbip tool in the kernel repo,
there are other versions floating around that would break if we
were to change the flags.

> One way to solve this issue for usbip
> is to add some boilerplate transform
> from URB_* to USBIP_FLAGS_*
> as it is de facto uapi now.

It doesn't sound like a there is a compelling reason other than
"it would be nice to keep the flag values in numerical order".

I would not recommend this option. I am not seeing any value to adding
change URB_* to USBIP_FLAGS_* layer without some serious techinical
concerns.

> 
> Another way is to use 0x0400 for FREE_COHERENT.
> usbip will not take care of this bit as
> it would be masked.
> 

I would go with this option adding a clear comment with link to this
discussion.

> Cc Shuah Khan here since she is the maintainer
> on usbip.
> 

Thank you adding me to the discussion.

thanks,
-- Shuah

Alan Stern June 24, 2022, 2:43 p.m. UTC | #24

On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote:
> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
> > On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
> > > 
> > > 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.
> > 
> > >   #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
> > > +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
> > >   /* The following flags are used internally by usbcore and HCDs */
> > > -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
> > > +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
> > >   #define URB_DIR_OUT		0
> > >   #define URB_DIR_MASK		URB_DIR_IN
> > > -- 
> > > 2.30.2
> > > 
> > 
> > I'm afraid this is a change of uapi as this field is, unfortunately,
> > exported by usbip to userspace as TCP packets.
> > 
> > This may also cause incompatibility (surprisingly not for this case,
> > detailed below) between usbip server and client
> > when one kernel is using the new flags and the other one is not.
> > 
> > If we do change this, we may need to bump usbip protocol version
> > accordingly.
> > 
> 
> 
> > A copy of Alan Stern's suggestion here for reference
> > > I don't see anything wrong with this, except that it would be nice to keep
> > > the flag values in numerical order.  In other words, set URB_FREE_COHERENT
> > > to 0x0200 and change URB_DIR_IN to 0x0400.
> > > 
> > > Alan Stern
> 
> Thank you Alan for this detailed analysis of uapi impacts and
> usbip host side and vhci incompatibilities. Userspace is going
> to be affected. In addition to the usbip tool in the kernel repo,
> there are other versions floating around that would break if we
> were to change the flags.
> 
> > One way to solve this issue for usbip
> > is to add some boilerplate transform
> > from URB_* to USBIP_FLAGS_*
> > as it is de facto uapi now.
> 
> It doesn't sound like a there is a compelling reason other than
> "it would be nice to keep the flag values in numerical order".
> 
> I would not recommend this option. I am not seeing any value to adding
> change URB_* to USBIP_FLAGS_* layer without some serious techinical
> concerns.
> 
> > 
> > Another way is to use 0x0400 for FREE_COHERENT.
> > usbip will not take care of this bit as
> > it would be masked.
> > 
> 
> I would go with this option adding a clear comment with link to this
> discussion.
> 
> > Cc Shuah Khan here since she is the maintainer
> > on usbip.
> > 
> 
> Thank you adding me to the discussion.

I can see this causing more problems in the future.  There's no hint in 
include/linux/usb.h that any of the values it defines are part of a user 
API.  If they are, they should be moved to include/uapi/linux/usb/.

In general, if a user program depends on kernel details that are not 
designed to be part of a user API, you should expect that the program 
will sometimes break from one kernel version to another.

Yes, I know Linus insists that kernel changes should not cause 
regressions in userspace, but the line has to be drawn somewhere.  
Otherwise the kernel could never change at all.

Alan Stern

Hongren Zheng June 24, 2022, 4:01 p.m. UTC | #25

On Fri, Jun 24, 2022 at 10:43:34AM -0400, Alan Stern wrote:
> > 
> > > One way to solve this issue for usbip
> > > is to add some boilerplate transform
> > > from URB_* to USBIP_FLAGS_*
> > > as it is de facto uapi now.
> > 
> > It doesn't sound like a there is a compelling reason other than
> > "it would be nice to keep the flag values in numerical order".
> > 
> > I would not recommend this option. I am not seeing any value to adding
> > change URB_* to USBIP_FLAGS_* layer without some serious techinical
> > concerns.

The transfer_flag in usbip is de facto uapi,
That's why I'm proposing the USBIP_FLAGS_* way and
further more I think usbip could move some flags/structs
in usbip_common.h to include/uapi/linux/usb/usbip.h,
instead of the userspace copying them into their own
header.

I will start a new thread if Shuah think that is acceptable.

If this patch is to be landed, I think it should be
landed along with the usbip change so there would be no
userspace change;

Even without this patch, making usbip flags/structs uapi alone
is still worth doing.

> > 
> > > 
> > > Another way is to use 0x0400 for FREE_COHERENT.
> > > usbip will not take care of this bit as
> > > it would be masked.
> > > 
> > 
> > I would go with this option adding a clear comment with link to this
> > discussion.
> > 
> > > Cc Shuah Khan here since she is the maintainer
> > > on usbip.
> > > 
> > 
> > Thank you adding me to the discussion.
> 
> I can see this causing more problems in the future.  There's no hint in 
> include/linux/usb.h that any of the values it defines are part of a user 
> API.  If they are, they should be moved to include/uapi/linux/usb/.

I agree with this argument.

> 
> In general, if a user program depends on kernel details that are not 
> designed to be part of a user API, you should expect that the program 
> will sometimes break from one kernel version to another.
> 
> Yes, I know Linus insists that kernel changes should not cause 
> regressions in userspace, but the line has to be drawn somewhere.  
> Otherwise the kernel could never change at all.
> 
> Alan Stern

Shuah Khan June 24, 2022, 4:31 p.m. UTC | #26

On 6/24/22 8:43 AM, Alan Stern wrote:
> On Thu, Jun 23, 2022 at 11:45:13AM -0600, Shuah Khan wrote:
>> On 6/23/22 11:30 AM, Hongren Zenithal Zheng wrote:
>>> On Fri, Jun 10, 2022 at 05:33:35PM -0400, Rhett Aultman wrote:
>>>>
>>>> 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.
>>>
>>>>    #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
>>>> +#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
>>>>    /* The following flags are used internally by usbcore and HCDs */
>>>> -#define URB_DIR_IN		0x0200	/* Transfer from device to host */
>>>> +#define URB_DIR_IN		0x0400	/* Transfer from device to host */
>>>>    #define URB_DIR_OUT		0
>>>>    #define URB_DIR_MASK		URB_DIR_IN
>>>> -- 
>>>> 2.30.2
>>>>
>>>
>>> I'm afraid this is a change of uapi as this field is, unfortunately,
>>> exported by usbip to userspace as TCP packets.
>>>
>>> This may also cause incompatibility (surprisingly not for this case,
>>> detailed below) between usbip server and client
>>> when one kernel is using the new flags and the other one is not.
>>>
>>> If we do change this, we may need to bump usbip protocol version
>>> accordingly.
>>>
>>
>>
>>> A copy of Alan Stern's suggestion here for reference
>>>> I don't see anything wrong with this, except that it would be nice to keep
>>>> the flag values in numerical order.  In other words, set URB_FREE_COHERENT
>>>> to 0x0200 and change URB_DIR_IN to 0x0400.
>>>>
>>>> Alan Stern
>>
>> Thank you Alan for this detailed analysis of uapi impacts and
>> usbip host side and vhci incompatibilities. Userspace is going
>> to be affected. In addition to the usbip tool in the kernel repo,
>> there are other versions floating around that would break if we
>> were to change the flags.
>>
>>> One way to solve this issue for usbip
>>> is to add some boilerplate transform
>>> from URB_* to USBIP_FLAGS_*
>>> as it is de facto uapi now.
>>
>> It doesn't sound like a there is a compelling reason other than
>> "it would be nice to keep the flag values in numerical order".
>>
>> I would not recommend this option. I am not seeing any value to adding
>> change URB_* to USBIP_FLAGS_* layer without some serious techinical
>> concerns.
>>
>>>
>>> Another way is to use 0x0400 for FREE_COHERENT.
>>> usbip will not take care of this bit as
>>> it would be masked.
>>>
>>
>> I would go with this option adding a clear comment with link to this
>> discussion.
>>
>>> Cc Shuah Khan here since she is the maintainer
>>> on usbip.
>>>
>>
>> Thank you adding me to the discussion.
> 
> I can see this causing more problems in the future.  There's no hint in
> include/linux/usb.h that any of the values it defines are part of a user
> API.  If they are, they should be moved to include/uapi/linux/usb/.
> 

Please elaborate on more problems in the future.

> In general, if a user program depends on kernel details that are not
> designed to be part of a user API, you should expect that the program
> will sometimes break from one kernel version to another.
> 
> Yes, I know Linus insists that kernel changes should not cause
> regressions in userspace, but the line has to be drawn somewhere.
> Otherwise the kernel could never change at all.
> 

I have had to change the usbip sysfs interface api in the past to
address security bugs related to information leaks. I am not saying
no. I am asking if there is a good reason to do this. So far I haven't
heard one.

thanks,
-- Shuah

Alan Stern June 24, 2022, 6:07 p.m. UTC | #27

On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote:
> On 6/24/22 8:43 AM, Alan Stern wrote:
> > > It doesn't sound like a there is a compelling reason other than
> > > "it would be nice to keep the flag values in numerical order".
> > > 
> > > I would not recommend this option. I am not seeing any value to adding
> > > change URB_* to USBIP_FLAGS_* layer without some serious techinical
> > > concerns.
> > > 
> > > > 
> > > > Another way is to use 0x0400 for FREE_COHERENT.
> > > > usbip will not take care of this bit as
> > > > it would be masked.
> > > > 
> > > 
> > > I would go with this option adding a clear comment with link to this
> > > discussion.
> > > 
> > > > Cc Shuah Khan here since she is the maintainer
> > > > on usbip.
> > > > 
> > > 
> > > Thank you adding me to the discussion.
> > 
> > I can see this causing more problems in the future.  There's no hint in
> > include/linux/usb.h that any of the values it defines are part of a user
> > API.  If they are, they should be moved to include/uapi/linux/usb/.
> > 
> 
> Please elaborate on more problems in the future.

In the future people will want to make other changes to 
include/linux/usb.h and they will not be aware that those changes will 
adversely affect usbip, because there is no documentation saying that 
the values defined in usb.h are part of a user API.  That will be a 
problem, because those changes may be serious and important ones, not 
just decorative or stylistic as in this case.

> > In general, if a user program depends on kernel details that are not
> > designed to be part of a user API, you should expect that the program
> > will sometimes break from one kernel version to another.
> > 
> > Yes, I know Linus insists that kernel changes should not cause
> > regressions in userspace, but the line has to be drawn somewhere.
> > Otherwise the kernel could never change at all.
> > 
> 
> I have had to change the usbip sysfs interface api in the past to
> address security bugs related to information leaks. I am not saying
> no. I am asking if there is a good reason to do this. So far I haven't
> heard one.

I agree with Hongren that values defined in include/linux/ should not be 
part of a user API.  There are two choices:

	Move the definitions into include/uapi/linux/, or

	Add code to translate the values between the numbers used in 
	userspace and the numbers used in the kernel.  (This is what
	was done for urb->transfer_flags in devio.c:proc_do_submiturb() 
	near line 1862.)

Alan Stern

Vincent MAILHOL June 26, 2022, 8:21 a.m. UTC | #28

On Thu. 23 Jun 2022 at 03:13, Rhett Aultman <[e-mail]> wrote:
> On Thu, 23 Jun 2022, Vincent MAILHOL wrote:
> > On Wed. 22 Jun 2022 at 21:24, Greg Kroah-Hartman
> > <[e-mail]> wrote:
> > > On Wed, Jun 22, 2022 at 07:34:57PM +0900, Vincent MAILHOL wrote:
> > > > On Wed. 22 Jun 2022 at 18:44, Greg Kroah-Hartman
> > > > <[e-mail]> wrote:
> > > > > On Wed, Jun 22, 2022 at 09:22:12AM +0000, David Laight wrote:
> > > > > > From: Vincent MAILHOL
> > > > > > > Sent: 21 June 2022 16:56
> > > > > > >
> > > > > > > On Wed. 22 Jun 2022 at 00:13, Alan Stern <[e-mail]> 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.
> > > > > > >
> > > > > > > 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?
> > > > > >
> > > > > > IIRC urb are pretty big.
> > > > >
> > > > > What exactly do you mean by "pretty big" here?  And what is wrong with
> > > > > that, I have never seen any issues with the current size of that
> > > > > structure in any benchmark or performance results.  All USB bottlenecks
> > > > > that I know of are either in the hardware layer, or in the protocol
> > > > > layer itself (i.e. usb-storage protocol).
> > > > >
> > > > > > You'd be unlucky if adding an extra field to hold the allocated
> > > > > > size would ever need more memory.
> > > > > > So it might just be worth saving the allocated size.
> > > > >
> > > > > Maybe, yes, then we could transition to allocating the urb and buffer at
> > > > > the same time like we do partially for iso streams in an urb.  But that
> > > > > still might be overkill for just this one driver.
> > > >
> > > > Well, I wouldn't have proposed the patch if it only applied to a
> > > > single driver. If we add a urb::allocated_transfer_size as suggested
> > > > by David, I believe that the majority of the drivers using DMA memory
> > > > will be able to rely on that URB_FREE_COHERENT flag for the garbage
> > > > collection.
> > > >
> > > > The caveat, as you pointed before, is that the developper still needs
> > > > to be aware of the limitations of DMA and that it should not be freed
> > > > in an IRQ context. e.g. no call to usb_kill_anchored_urbs() or other
> > > > functions that would lead to urb_destroy().
> > > >
> > > > > I'm curious as to why
> > > > > a slow and tiny protocol like CAN needs to use usb_alloc_coherent() for
> > > > > its buffers in the first place.
> > > >
> > > > The CAN protocol, in its latest revision, allows for transfer speed up
> > > > to ~5Mbits. For low performance CPUs, this starts to be a significant
> > > > load. Also, the CAN PDU being small (0 to 64 bytes), many small
> > > > transfers occur.
> > >
> > > And is the memcpy the actual issue here?  Even tiny cpus can do large
> > > and small memcopy very very very fast.
> > >
> > > > Unfortunately I did not do any benchmark myself so I won't be able to
> > > > back my explanation with numbers.
> > >
> > > That might be the simplest solution here :)
> >
> > Yes, this would give a clear answer whether or not DMA was needed in
> > the first place. But I do not own that gs_usb device to do the
> > benchmark myself (and to be honest I do not have time to dedicate for
> > this at the moment, maybe I will do it later on some other devices).
> >
> > Has anyone from the linux-can mailing list ever done such a benchmark?
> > Else, is there anyone who would like to volunteer?
>
> I have access to a couple of gs_usb devices but I am afraid I have no
> experience performing this sort of benchmarking and also would have to
> squeeze it in as a weekend project or something similar.  That said, if
> someone's willing to help step me through it, I can see if it's feasible
> for me to do.

I can throw a few hints which might be helpful.

First, you should obviously prepare two versions of the gs_usb driver:
one using usb_alloc_coherent() (the current one), the other using
kmalloc() and compare the two.

Right now, I can think of two relevant benchmarks: transmission
latency and CPU load.

For the transmission latency, I posted one on my tools:
https://lore.kernel.org/linux-can/[e-mail]/T/#u

For the CPU load, I suggest to put the bus on full load, for example using:
| cangen -g0 -p1 can0
(you might also want to play with other parameters such as the length using -L)
Then use an existing tool to get the CPU load figures. I don't know
for sure which tool is a good one to benchmark CPU usage in kernel
land so you will have to research that part. If anyone has a
suggestion…

> That said, the gs_usb driver is mostly following along a very well
> established pattern for writing USB CAN devices.  Both the pattern
> followed that created the memory leak, as well as the pattern I followed
> to resolve the memory leak, were also seen in the esd2 USB CAN driver as
> well, and likely others are following suit.  So, I don't know that we'd
> need to keep it specific to gs_usb to gain good information here.

Yes, I looked at the log, the very first CAN USB driver is ems_usb and
was using DMA memory from the beginning. From that point on, nearly
all the drivers copied the trend (the only exception I am aware of is
peak_usb).

I agree that the scope is wider than the gs_can (thus my proposal to
fix it at API level).


Yours sincerely,
Vincent Mailhol

Shuah Khan June 27, 2022, 10:54 p.m. UTC | #29

On 6/24/22 12:07 PM, Alan Stern wrote:
> On Fri, Jun 24, 2022 at 10:31:06AM -0600, Shuah Khan wrote:
>> On 6/24/22 8:43 AM, Alan Stern wrote:
>>>> It doesn't sound like a there is a compelling reason other than
>>>> "it would be nice to keep the flag values in numerical order".
>>>>
>>>> I would not recommend this option. I am not seeing any value to adding
>>>> change URB_* to USBIP_FLAGS_* layer without some serious techinical
>>>> concerns.
>>>>
>>>>>
>>>>> Another way is to use 0x0400 for FREE_COHERENT.
>>>>> usbip will not take care of this bit as
>>>>> it would be masked.
>>>>>
>>>>
>>>> I would go with this option adding a clear comment with link to this
>>>> discussion.
>>>>
>>>>> Cc Shuah Khan here since she is the maintainer
>>>>> on usbip.
>>>>>
>>>>
>>>> Thank you adding me to the discussion.
>>>
>>> I can see this causing more problems in the future.  There's no hint in
>>> include/linux/usb.h that any of the values it defines are part of a user
>>> API.  If they are, they should be moved to include/uapi/linux/usb/.
>>>
>>
>> Please elaborate on more problems in the future.
> 
> In the future people will want to make other changes to
> include/linux/usb.h and they will not be aware that those changes will
> adversely affect usbip, because there is no documentation saying that
> the values defined in usb.h are part of a user API.  That will be a
> problem, because those changes may be serious and important ones, not
> just decorative or stylistic as in this case.
> 

How often do these values change based on our past experience with these
fields?

>>> In general, if a user program depends on kernel details that are not
>>> designed to be part of a user API, you should expect that the program
>>> will sometimes break from one kernel version to another.
>>>
>>> Yes, I know Linus insists that kernel changes should not cause
>>> regressions in userspace, but the line has to be drawn somewhere.
>>> Otherwise the kernel could never change at all.
>>>
>>
>> I have had to change the usbip sysfs interface api in the past to
>> address security bugs related to information leaks. I am not saying
>> no. I am asking if there is a good reason to do this. So far I haven't
>> heard one.
> 
> I agree with Hongren that values defined in include/linux/ should not be
> part of a user API.  There are two choices:
> 

I agree with this in general. I don't think this is an explicit decision
to make them part of API. It is a consequence of simply copying the
transfer_flags. I am with you both on not being able to recognize the
impact until as this is rather obscure usage hidden away in the packets.
These defines aren't directly referenced.

> 	Move the definitions into include/uapi/linux/, or
> 

Wouldn't this be easier way to handle the change? With this option
the uapi will be well documented.

> 	Add code to translate the values between the numbers used in
> 	userspace and the numbers used in the kernel.  (This is what
> 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> 	near line 1862.)
> 

I looked at the code and looks simple enough. I am okay going this route
if we see issues with the option 1.

thanks,
-- Shuah

Alan Stern June 28, 2022, 1:35 a.m. UTC | #30

On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
> On 6/24/22 12:07 PM, Alan Stern wrote:
> > In the future people will want to make other changes to
> > include/linux/usb.h and they will not be aware that those changes will
> > adversely affect usbip, because there is no documentation saying that
> > the values defined in usb.h are part of a user API.  That will be a
> > problem, because those changes may be serious and important ones, not
> > just decorative or stylistic as in this case.
> > 
> 
> How often do these values change based on our past experience with these
> fields?

I don't know.  You could check the git history to find out for certain.  
My guess would be every eight or ten years.

> > I agree with Hongren that values defined in include/linux/ should not be
> > part of a user API.  There are two choices:
> > 
> 
> I agree with this in general. I don't think this is an explicit decision
> to make them part of API. It is a consequence of simply copying the
> transfer_flags. I am with you both on not being able to recognize the
> impact until as this is rather obscure usage hidden away in the packets.
> These defines aren't directly referenced.
> 
> > 	Move the definitions into include/uapi/linux/, or
> > 
> 
> Wouldn't this be easier way to handle the change? With this option
> the uapi will be well documented.
> 
> > 	Add code to translate the values between the numbers used in
> > 	userspace and the numbers used in the kernel.  (This is what
> > 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> > 	near line 1862.)
> > 
> 
> I looked at the code and looks simple enough. I am okay going this route
> if we see issues with the option 1.

It's up to you; either approach is okay with me.  However, I do think 
that the second option is a little better; I don't see any good reason 
why the kernel should be forced to use the same numeric values for these 
flags forever.  Especially since the only user program that needs to 
know them is usbip, which is fairly closely tied to the kernel; if there 
were more programs using those values then they would constitute a good 
reason for choosing the first option.

Alan Stern

Shuah Khan July 1, 2022, 2:10 a.m. UTC | #31

On 6/27/22 7:35 PM, Alan Stern wrote:
> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>> In the future people will want to make other changes to
>>> include/linux/usb.h and they will not be aware that those changes will
>>> adversely affect usbip, because there is no documentation saying that
>>> the values defined in usb.h are part of a user API.  That will be a
>>> problem, because those changes may be serious and important ones, not
>>> just decorative or stylistic as in this case.
>>>
>>
>> How often do these values change based on our past experience with these
>> fields?
> 
> I don't know.  You could check the git history to find out for certain.
> My guess would be every eight or ten years.
> 
>>> I agree with Hongren that values defined in include/linux/ should not be
>>> part of a user API.  There are two choices:
>>>
>>
>> I agree with this in general. I don't think this is an explicit decision
>> to make them part of API. It is a consequence of simply copying the
>> transfer_flags. I am with you both on not being able to recognize the
>> impact until as this is rather obscure usage hidden away in the packets.
>> These defines aren't directly referenced.
>>
>>> 	Move the definitions into include/uapi/linux/, or
>>>
>>
>> Wouldn't this be easier way to handle the change? With this option
>> the uapi will be well documented.
>>
>>> 	Add code to translate the values between the numbers used in
>>> 	userspace and the numbers used in the kernel.  (This is what
>>> 	was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>> 	near line 1862.)
>>>
>>
>> I looked at the code and looks simple enough. I am okay going this route
>> if we see issues with the option 1.
> 
> It's up to you; either approach is okay with me.  However, I do think
> that the second option is a little better; I don't see any good reason
> why the kernel should be forced to use the same numeric values for these
> flags forever.  Especially since the only user program that needs to
> know them is usbip, which is fairly closely tied to the kernel; if there
> were more programs using those values then they would constitute a good
> reason for choosing the first option.
> 

Thank you Alan and Hongren for your help with this problem. Since there
are no changes to the flags for the time being, I am comfortable going
with the second option.

I will send a patch soon.

thanks,
-- Shuah

Shuah Khan Aug. 1, 2022, 5:42 p.m. UTC | #32

On 6/30/22 8:10 PM, Shuah Khan wrote:
> On 6/27/22 7:35 PM, Alan Stern wrote:
>> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>>> In the future people will want to make other changes to
>>>> include/linux/usb.h and they will not be aware that those changes will
>>>> adversely affect usbip, because there is no documentation saying that
>>>> the values defined in usb.h are part of a user API.  That will be a
>>>> problem, because those changes may be serious and important ones, not
>>>> just decorative or stylistic as in this case.
>>>>
>>>
>>> How often do these values change based on our past experience with these
>>> fields?
>>
>> I don't know.  You could check the git history to find out for certain.
>> My guess would be every eight or ten years.
>>
>>>> I agree with Hongren that values defined in include/linux/ should not be
>>>> part of a user API.  There are two choices:
>>>>
>>>
>>> I agree with this in general. I don't think this is an explicit decision
>>> to make them part of API. It is a consequence of simply copying the
>>> transfer_flags. I am with you both on not being able to recognize the
>>> impact until as this is rather obscure usage hidden away in the packets.
>>> These defines aren't directly referenced.
>>>
>>>>     Move the definitions into include/uapi/linux/, or
>>>>
>>>
>>> Wouldn't this be easier way to handle the change? With this option
>>> the uapi will be well documented.
>>>
>>>>     Add code to translate the values between the numbers used in
>>>>     userspace and the numbers used in the kernel.  (This is what
>>>>     was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>>>     near line 1862.)
>>>>
>>>
>>> I looked at the code and looks simple enough. I am okay going this route
>>> if we see issues with the option 1.
>>
>> It's up to you; either approach is okay with me.  However, I do think
>> that the second option is a little better; I don't see any good reason
>> why the kernel should be forced to use the same numeric values for these
>> flags forever.  Especially since the only user program that needs to
>> know them is usbip, which is fairly closely tied to the kernel; if there
>> were more programs using those values then they would constitute a good
>> reason for choosing the first option.
>>
> 
> Thank you Alan and Hongren for your help with this problem. Since there
> are no changes to the flags for the time being, I am comfortable going
> with the second option.
> 
> I will send a patch soon.
> 

Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
get this done sooner - summer vacations didn't cooperate.

Just an update that I haven't forgotten and it will taken care of.
thanks,
-- Shuah

Vincent MAILHOL Aug. 1, 2022, 6:28 p.m. UTC | #33

On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <[e-mail]> wrote:
> On 6/30/22 8:10 PM, Shuah Khan wrote:
> > On 6/27/22 7:35 PM, Alan Stern wrote:
> >> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
> >>> On 6/24/22 12:07 PM, Alan Stern wrote:
> >>>> In the future people will want to make other changes to
> >>>> include/linux/usb.h and they will not be aware that those changes will
> >>>> adversely affect usbip, because there is no documentation saying that
> >>>> the values defined in usb.h are part of a user API.  That will be a
> >>>> problem, because those changes may be serious and important ones, not
> >>>> just decorative or stylistic as in this case.
> >>>>
> >>>
> >>> How often do these values change based on our past experience with these
> >>> fields?
> >>
> >> I don't know.  You could check the git history to find out for certain.
> >> My guess would be every eight or ten years.
> >>
> >>>> I agree with Hongren that values defined in include/linux/ should not be
> >>>> part of a user API.  There are two choices:
> >>>>
> >>>
> >>> I agree with this in general. I don't think this is an explicit decision
> >>> to make them part of API. It is a consequence of simply copying the
> >>> transfer_flags. I am with you both on not being able to recognize the
> >>> impact until as this is rather obscure usage hidden away in the packets.
> >>> These defines aren't directly referenced.
> >>>
> >>>>     Move the definitions into include/uapi/linux/, or
> >>>>
> >>>
> >>> Wouldn't this be easier way to handle the change? With this option
> >>> the uapi will be well documented.
> >>>
> >>>>     Add code to translate the values between the numbers used in
> >>>>     userspace and the numbers used in the kernel.  (This is what
> >>>>     was done for urb->transfer_flags in devio.c:proc_do_submiturb()
> >>>>     near line 1862.)
> >>>>
> >>>
> >>> I looked at the code and looks simple enough. I am okay going this route
> >>> if we see issues with the option 1.
> >>
> >> It's up to you; either approach is okay with me.  However, I do think
> >> that the second option is a little better; I don't see any good reason
> >> why the kernel should be forced to use the same numeric values for these
> >> flags forever.  Especially since the only user program that needs to
> >> know them is usbip, which is fairly closely tied to the kernel; if there
> >> were more programs using those values then they would constitute a good
> >> reason for choosing the first option.
> >>
> >
> > Thank you Alan and Hongren for your help with this problem. Since there
> > are no changes to the flags for the time being, I am comfortable going
> > with the second option.
> >
> > I will send a patch soon.
> >
>
> Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
> get this done sooner - summer vacations didn't cooperate.
>
> Just an update that I haven't forgotten and it will taken care of.
> thanks,

Thanks for keeping this under your radar. I also have on my TODO list
to send a new version of my patch to add the `URB_FREE_COHERENT' flag
but this time adding an `allocated_length' field to struct urb. I will
wait for your patch to go first. By the way, I will be out for summer
holiday for the next couple of weeks so I wasn't planning to submit
anything soon regardless.

Yours sincerely,
Vincent Mailhol

Shuah Khan Aug. 3, 2022, 11:44 p.m. UTC | #34

On 8/1/22 12:28 PM, Vincent MAILHOL wrote:
> On Tue. 2 Aug. 2022 at 02:48, Shuah Khan <[e-mail]> wrote:
>> On 6/30/22 8:10 PM, Shuah Khan wrote:
>>> On 6/27/22 7:35 PM, Alan Stern wrote:
>>>> On Mon, Jun 27, 2022 at 04:54:17PM -0600, Shuah Khan wrote:
>>>>> On 6/24/22 12:07 PM, Alan Stern wrote:
>>>>>> In the future people will want to make other changes to
>>>>>> include/linux/usb.h and they will not be aware that those changes will
>>>>>> adversely affect usbip, because there is no documentation saying that
>>>>>> the values defined in usb.h are part of a user API.  That will be a
>>>>>> problem, because those changes may be serious and important ones, not
>>>>>> just decorative or stylistic as in this case.
>>>>>>
>>>>>
>>>>> How often do these values change based on our past experience with these
>>>>> fields?
>>>>
>>>> I don't know.  You could check the git history to find out for certain.
>>>> My guess would be every eight or ten years.
>>>>
>>>>>> I agree with Hongren that values defined in include/linux/ should not be
>>>>>> part of a user API.  There are two choices:
>>>>>>
>>>>>
>>>>> I agree with this in general. I don't think this is an explicit decision
>>>>> to make them part of API. It is a consequence of simply copying the
>>>>> transfer_flags. I am with you both on not being able to recognize the
>>>>> impact until as this is rather obscure usage hidden away in the packets.
>>>>> These defines aren't directly referenced.
>>>>>
>>>>>>      Move the definitions into include/uapi/linux/, or
>>>>>>
>>>>>
>>>>> Wouldn't this be easier way to handle the change? With this option
>>>>> the uapi will be well documented.
>>>>>
>>>>>>      Add code to translate the values between the numbers used in
>>>>>>      userspace and the numbers used in the kernel.  (This is what
>>>>>>      was done for urb->transfer_flags in devio.c:proc_do_submiturb()
>>>>>>      near line 1862.)
>>>>>>
>>>>>
>>>>> I looked at the code and looks simple enough. I am okay going this route
>>>>> if we see issues with the option 1.
>>>>
>>>> It's up to you; either approach is okay with me.  However, I do think
>>>> that the second option is a little better; I don't see any good reason
>>>> why the kernel should be forced to use the same numeric values for these
>>>> flags forever.  Especially since the only user program that needs to
>>>> know them is usbip, which is fairly closely tied to the kernel; if there
>>>> were more programs using those values then they would constitute a good
>>>> reason for choosing the first option.
>>>>
>>>
>>> Thank you Alan and Hongren for your help with this problem. Since there
>>> are no changes to the flags for the time being, I am comfortable going
>>> with the second option.
>>>
>>> I will send a patch soon.
>>>
>>
>> Patch is almost ready to be sent out. Changes aren't bad at all. Hoping to
>> get this done sooner - summer vacations didn't cooperate.
>>
>> Just an update that I haven't forgotten and it will taken care of.
>> thanks,
> 
> Thanks for keeping this under your radar. I also have on my TODO list
> to send a new version of my patch to add the `URB_FREE_COHERENT' flag
> but this time adding an `allocated_length' field to struct urb. I will
> wait for your patch to go first. By the way, I will be out for summer
> holiday for the next couple of weeks so I wasn't planning to submit
> anything soon regardless.
> 

Sounds good. I now have the patch ready to be sent out. I will wait for
the merge window to close before I send it out.

thanks,
-- Shuah

Patch

diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index 33d62d7e3929..36c48fb196e0 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -22,6 +22,9 @@  static void urb_destroy(struct kref *kref)
 
 	if (urb->transfer_flags & URB_FREE_BUFFER)
 		kfree(urb->transfer_buffer);
+	else if (urb->transfer_flags & URB_FREE_COHERENT)
+		usb_free_coherent(urb->dev, urb->transfer_buffer_length,
+				  urb->transfer_buffer, urb->transfer_dma);
 
 	kfree(urb);
 }
@@ -504,7 +507,7 @@  int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
 
 	/* Check against a simple/standard policy */
 	allowed = (URB_NO_TRANSFER_DMA_MAP | URB_NO_INTERRUPT | URB_DIR_MASK |
-			URB_FREE_BUFFER);
+			URB_FREE_BUFFER | URB_FREE_COHERENT);
 	switch (xfertype) {
 	case USB_ENDPOINT_XFER_BULK:
 	case USB_ENDPOINT_XFER_INT:
diff --git a/include/linux/usb.h b/include/linux/usb.h
index 60bee864d897..945d68ea1d76 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -1328,9 +1328,10 @@  extern int usb_disabled(void);
 #define URB_NO_INTERRUPT	0x0080	/* HINT: no non-error interrupt
 					 * needed */
 #define URB_FREE_BUFFER		0x0100	/* Free transfer buffer with the URB */
+#define URB_FREE_COHERENT	0x0200  /* Free DMA memory of transfer buffer */
 
 /* The following flags are used internally by usbcore and HCDs */
-#define URB_DIR_IN		0x0200	/* Transfer from device to host */
+#define URB_DIR_IN		0x0400	/* Transfer from device to host */
 #define URB_DIR_OUT		0
 #define URB_DIR_MASK		URB_DIR_IN