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

[Hongren Zenithal Zheng <i@zenithal.me>]

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