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

[Alan Stern <stern@rowland.harvard.edu>]

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