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

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

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