Hi,
Binyamin Sharet <bsharet@xxxxxxxxx> writes:
> On 09/07/2016 03:36 PM, Felipe Balbi wrote:
>> Hi,
>>
>> Binyamin Sharet <bsharet@xxxxxxxxx> writes:
>>> Feature control mechanism allows addition of dynamic features to
>>> gadgetfs.
>>>
>>> It provides a user-mode driver the ability to control those features,
>>> by querying the supported and enabled features and enable/disable
>>> features in runtime via ioctl on ep0 fd.
>>> ---
>>> drivers/usb/gadget/legacy/inode.c | 67
>>> +++++++++++++++++++++++++++++++++++++--
>>> include/uapi/linux/usb/gadgetfs.h | 29 +++++++++++++++++
>>> 2 files changed, 94 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/usb/gadget/legacy/inode.c
>>> b/drivers/usb/gadget/legacy/inode.c
>>> index 16104b5e..f54200d 100644
>>> --- a/drivers/usb/gadget/legacy/inode.c
>>> +++ b/drivers/usb/gadget/legacy/inode.c
>>> @@ -144,6 +144,8 @@ struct dev_data {
>>> wait_queue_head_t wait;
>>> struct super_block *sb;
>>> struct dentry *dentry;
>>> + struct usb_gadgetfs_features enabled_features;
>>> + struct usb_gadgetfs_features supported_features;
>>>
>>> /* except this scratch i/o buffer for ep0 */
>>> u8 rbuf [256];
>>> @@ -1235,14 +1237,75 @@ out:
>>> return mask;
>>> }
>>>
>>> +/* feature control */
>>> +
>>> +static bool
>>> +is_feature_set(struct usb_gadgetfs_features * features, unsigned int
>>> feature)
>> patch is line-wrapped. Please fix it. Also, you should be taking u64 as
>> argument here.
> Why u64? this is the number of the feature (e.g. bit index), not
> a bitmap, and we only support 256 features.
Oh, I see what you're doing now. Sorry, I completely misunderstood.
>>> +{
>>> + uint64_t feature_bit;
>> not userspace types in kernel ;-)
>>
>>> + /* overflow */
>>> + if(feature >= (sizeof(*features) * 8))
>> run checkpatch.pl on this patch. Formatting is pretty bad.
>>
>>> + return false;
>>> + feature_bit = 1 << (feature % 64);
>>> + return (features->bitmap[feature / 64] & feature_bit) != 0;
>>> +}
>>> +
>>> +/* clear a feature in a feature bitmap */
>>> +static bool
>>> +feature_clear(struct usb_gadgetfs_features * features, unsigned int
>>> feature)
>>> +{
>>> + uint64_t feature_bit;
>>> + /* overflow */
>>> + if(feature >= (sizeof(*features) * 8))
>>> + return false;
>>> + feature_bit = 1 << (feature % 64);
>>> + features->bitmap[feature / 64] &= ~feature_bit;
>>> + return true;
>>> +}
>>> +
>>> +/* set a feature in a feature bitmap */
>>> +static bool
>>> +feature_set(struct usb_gadgetfs_features * features, unsigned int feature)
>>> +{
>>> + uint64_t feature_bit;
>>> + /* overflow */
>>> + if(feature >= (sizeof(*features) * 8))
>>> + return false;
>>> + feature_bit = 1 << (feature % 64);
>>> + features->bitmap[feature / 64] |= feature_bit;
>>> + return true;
>>> +}
>> you can't allow for set/clear/get on "reserved" bits. Also, I had
>> mentioned that we should hardcode bit 0 to 1 when this is enabled.
>>
> I think I misunderstood you. Do you mean that bit 0 of the features is
> "features supported" and not "delegate descriptors"?
yes :-) I mentioned that when we discussed this weeks ago and even gave
the example of USB descriptors's bmAttributes field which have a bit
always enabled. This bit can be used by userspace to make sure that the
kernel not only responds to the ioctl() properly, but that it really
supports our feature IOCTLs. Note that we were, previously, passing
ioctl to the UDC driver, and we can't assume they're not using that.
>>> diff --git a/include/uapi/linux/usb/gadgetfs.h
>>> b/include/uapi/linux/usb/gadgetfs.h
>>> index 0bb12e0..9d304e0 100644
>>> --- a/include/uapi/linux/usb/gadgetfs.h
>>> +++ b/include/uapi/linux/usb/gadgetfs.h
>>> @@ -85,4 +85,33 @@ struct usb_gadgetfs_event {
>>> */
>>> #define GADGETFS_CLEAR_HALT _IO('g', 3)
>>>
>>> +
>>> +
>>> +struct usb_gadgetfs_features {
>>> + uint64_t bitmap[4];
>> this should really be:
>>
>> uint64_t features0;
>>
>> /* Keep 24 bytes reserved for possible future usage */
>> uint8_t RESERVED[24];
>>
> Thanks for the comments.
np
--
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
