On 09/07/2016 03:51 PM, Felipe Balbi wrote: > Binyamin Sharet <bsharet@xxxxxxxxx> writes: > >> On 09/07/2016 03:40 PM, Felipe Balbi wrote: >>> Binyamin Sharet <bsharet@xxxxxxxxx> writes: >>> >>>> Added USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS to Kconfig, this >>>> option is available under USB_GADGETFS and requires EXPERT. >>>> Enabling this option will not change the default behavior of >>>> GadgetFS, but will allow a user to enable descriptor delegation >>>> using ioctls. >>>> --- >>>> drivers/usb/gadget/legacy/Kconfig | 12 ++++++++++++ >>>> drivers/usb/gadget/legacy/inode.c | 12 ++++++++++-- >>>> 2 files changed, 22 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/usb/gadget/legacy/Kconfig >>>> b/drivers/usb/gadget/legacy/Kconfig >>>> index 0b36878..c062384 100644 >>>> --- a/drivers/usb/gadget/legacy/Kconfig >>>> +++ b/drivers/usb/gadget/legacy/Kconfig >>>> @@ -184,6 +184,18 @@ config USB_GADGETFS >>>> Say "y" to link the driver statically, or "m" to build a >>>> dynamically linked module called "gadgetfs". >>>> >>>> +config USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS >>>> + bool "Delegate get descriptor requests to user" if EXPERT >>>> + depends on USB_GADGETFS >>> why not: >>> >>> bool " Delegate all requests to user" >>> depends on USB_GADGETFS && EXPERT >>> >>> ? >> I have done it this way so it will not be displayed unless EXPERT >> is enabled. > if it depends on EXPERT it won't show ;-) > >>>> diff --git a/drivers/usb/gadget/legacy/inode.c >>>> b/drivers/usb/gadget/legacy/inode.c >>>> index f8a3e75..9a62584 100644 >>>> --- a/drivers/usb/gadget/legacy/inode.c >>>> +++ b/drivers/usb/gadget/legacy/inode.c >>>> @@ -178,9 +178,13 @@ static struct dev_data *dev_new (void) >>>> spin_lock_init (&dev->lock); >>>> INIT_LIST_HEAD (&dev->epfiles); >>>> init_waitqueue_head (&dev->wait); >>>> + >>>> +#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS >>>> feature_set(&dev->supported_features, >>>> GADGETFS_FEATURE_DELEGATE_DESCRIPTORS); >>>> - return dev; >>>> +#endif >>> feature_set shouldn't be here until $subject is merged. >>> >>>> @@ -1414,10 +1418,14 @@ gadgetfs_setup (struct usb_gadget *gadget, const >>>> struct usb_ctrlrequest *ctrl) >>>> case USB_REQ_GET_DESCRIPTOR: >>>> if (ctrl->bRequestType != USB_DIR_IN) >>>> goto unrecognized; >>>> + >>>> +#ifdef CONFIG_USB_GADGETFS_ALLOW_DELEGATE_DESCRIPTORS >>>> if (is_feature_set(&dev->enabled_features, >>>> GADGETFS_FEATURE_DELEGATE_DESCRIPTORS)) >>> this shouldn't be here until $subject. >>> >> What do you mean by "until $subject is merged"? >> Should this patch be submitted only in the future? > no, no :-) > > I mean that the conditional already existed. You're only wrapping it > with the ifdef. This means we create a point in the history where > features are enabled without ALLOW_DELEGATE_DESCRIPTORS. We don't want > that, so move feature_set() and if (is_feature_set()) usage to $this > patch. > Thanks! Should I put feature_clear and is_feature_set under #ifdefs as well? They are unused and cause warning at the moment, but it can gets really ugly to set them under ifdef (thinking about future features).. -- Binyamin Sharet, Cisco, STARE-C -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
