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.
> +{
> + 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.
> 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];
--
balbi
Attachment:
signature.asc
Description: PGP signature
