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
