Hi,
Binyamin Sharet <bsharet@xxxxxxxxx> writes:
> 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)..
I guess the best thing would be implement them only when
ALLOW_DELEGATE_DESCRIPTORS is set and provide a stub otherwise.
#ifdef ALLOW_DELEGATE...
static void feature_set(....)
{
....
}
#else
static inline void feature_set(....) { }
#endif
--
balbi
--
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
