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.
>> +{
>> + 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"?
>> 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.
--
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
