On Mon, 12 Sep 2016, Binyamin Sharet (bsharet) wrote:
> > On 8 Sep 2016, at 23:24, Alan Stern <st...@rowland.harvard.edu> wrote:
> >
> > On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:
> >
> >>
> >>> On 8 Sep 2016, at 22:20, Alan Stern <st...@rowland.harvard.edu> wrote:
> >>>
> >>> On Thu, 8 Sep 2016, Binyamin Sharet (bsharet) wrote:
> >>>
> >>>>> I was thinking more like:
> >>>>>
> >>>>> struct usb_gadgetfs_ioctl_arg {
> >>>>> uint16_t length;
> >>>>> uint8_t reserved[2];
> >>>>>
> >>>>> uint8_t data[0];
> >>>>> }
> >>>>>
> >>>>> but the principle is pretty much the same.
> >>>>>
> >>>>> Alan Stern
> >>>>>
> >>>>
> >>>> Won’t the user lose the relevant information (e.g. feature
> >>>> structure) by using this model?
> >>>
> >>> What feature structure? Aren't your feature lists just vectors of 64
> >>> bits? They can be stored in the .data field above.
> >>>
> >>> Alan Stern
> >>>
> >>
> >> Not “just” - they are platform-dependant uint64_t. which means they
> >> won’t look the same on systems with different endianness. If the
> >> user is unaware of this, it can cause confusion w/r/t which bit is which.
> >>
> >> We can use 8-bit vectors instead and skip the endianness issue,
> >> but why define a generic usb_gadgetfs_ioctl_args structure instead
> >> of “features struct” for feature-related ioctls and different structs for
> >> other types of ioctl (if we’ll have such in the future)?
> >
> > Good point. Yes, you can have different interfaces for different
> > ioctls.
> >
> > This whole question arose because I complained that the features API
> > looked too extravagant, and Felipe said that he didn't want to run out
> > of available fields in case any features in the future need them.
> >
> > But if you're worried about that, why limit yourself to 64 features and
> > 24 bytes of extra data? It doesn't make sense to think that some fixed
> > limit might end up being too small, but then restrict yourself to a
> > different fixed limit.
> >
> > Alan Stern
>
> What about changing the ioctl API for the “features” handling, and instead
> of passing an entire bitmap from user to kernel and vice versa, the user
> will only operate on a single feature (by feature id)?
>
> This way we’ll have a much simpler (and intuitive) API with the user -
> is_feature_supported(u64), is_feature_enabled(u64), enable_feature(u64)
> and disabled_feature(u64).
>
> So the internal representation of the supported/enabled features will not
> matter much to the user.
That's fine with me. But since you're passing feature ID numbers to
the kernel, instead of feature bitmaps, the argument type should be
plain int, not u64. Unless you think somebody will implement more than
2 billion features... :-)
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
