Re: [PATCH v4] usb: gadget: f_fs: add capability for dfu run-time descriptor

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Mon, Aug 5, 2024 at 3:01 AM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Sun, Aug 04, 2024 at 08:06:40PM -0400, crwulff@xxxxxxxxx wrote:
> > From: David Sands <david.sands@xxxxxxxxx>
> >
> > From: David Sands <david.sands@xxxxxxxxx>
>
> Twice?

Oops. Not sure what happened, but I'll try to make it not happen next time.

>
> > Add the ability for FunctionFS driver to be able to create DFU Run-Time
> > descriptors.
>
> As others said, please spell out "DFU" and I do not think that
> "Run-Time" needs Capital letters, or a '-', right?
>
> Also include here a lot more description of how this is to be used.

Ok, I will expand on this (and the associated documentation.)

>
> >
> > Signed-off-by: David Sands <david.sands@xxxxxxxxx>
> > Co-developed-by: Chris Wulff <crwulff@xxxxxxxxx>
> > Signed-off-by: Chris Wulff <crwulff@xxxxxxxxx>
> > ---
> > v4: Clean up unneeded change, switch to BIT macros, more documentation
> > v3: Documentation, additional constants and constant order fixed
> > https://lore.kernel.org/all/CO1PR17MB54197F118CBC8783D289B97DE1102@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > v2: https://lore.kernel.org/linux-usb/CO1PR17MB54198D086B61F7392FA9075FE10E2@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > v1: https://lore.kernel.org/linux-usb/CO1PR17MB5419AC3907C74E28D80C5021E1082@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> > ---
> >  Documentation/usb/functionfs-desc.rst | 22 ++++++++++++++++++++++
> >  Documentation/usb/functionfs.rst      |  2 ++
> >  Documentation/usb/index.rst           |  1 +
> >  drivers/usb/gadget/function/f_fs.c    | 12 ++++++++++--
> >  include/uapi/linux/usb/ch9.h          |  8 ++++++--
> >  include/uapi/linux/usb/functionfs.h   | 25 +++++++++++++++++++++++++
> >  6 files changed, 66 insertions(+), 4 deletions(-)
> >  create mode 100644 Documentation/usb/functionfs-desc.rst
> >
> > diff --git a/Documentation/usb/functionfs-desc.rst b/Documentation/usb/functionfs-desc.rst
> > new file mode 100644
> > index 000000000000..73d2b8a3f02c
> > --- /dev/null
> > +++ b/Documentation/usb/functionfs-desc.rst
> > @@ -0,0 +1,22 @@
> > +======================
> > +FunctionFS Descriptors
> > +======================
> > +
> > +Interface Descriptors
> > +---------------------
> > +
> > +Standard USB interface descriptors may be added. The class/subclass of the
> > +most recent interface descriptor determines what type of class-specific
> > +descriptors are accepted.
> > +
> > +Class-Specific Descriptors
> > +--------------------------
> > +
>
> Why an empty section?

It was just a heading-2 for the class-specific descriptor section (with each
of the class-specific descriptors being heading-3). I can add a bit of
text though.

>
> > +DFU Functional Descriptor
> > +~~~~~~~~~~~~~~~~~~~~~~~~~
> > +
> > +When the interface class is USB_CLASS_APP_SPEC and  the interface subclass
>
> Extra space?
>
>
> > +is USB_SUBCLASS_DFU, a DFU functional descriptor can be provided.
>
> Provided how?

I will expand on this a bit more. Most of the functionfs descriptor
behavior wasn't documented. The functionfs page talks about how
these are written to the ep0 file, but doesn't mention anything about
what descriptors can be written other than mentioning that ep# files
are created when endpoint descriptors are written.

>
> > +
> > +.. kernel-doc:: include/uapi/linux/usb/functionfs.h
> > +   :doc: usb_dfu_functional_descriptor
> > diff --git a/Documentation/usb/functionfs.rst b/Documentation/usb/functionfs.rst
> > index d05a775bc45b..4f96e4b93d7b 100644
> > --- a/Documentation/usb/functionfs.rst
> > +++ b/Documentation/usb/functionfs.rst
> > @@ -70,6 +70,8 @@ have been written to their ep0's.
> >  Conversely, the gadget is unregistered after the first USB function
> >  closes its endpoints.
> >
> > +For more information about FunctionFS descriptors see :doc:`functionfs-desc`
> > +
> >  DMABUF interface
> >  ================
> >
> > diff --git a/Documentation/usb/index.rst b/Documentation/usb/index.rst
> > index 27955dad95e1..826492c813ac 100644
> > --- a/Documentation/usb/index.rst
> > +++ b/Documentation/usb/index.rst
> > @@ -11,6 +11,7 @@ USB support
> >      dwc3
> >      ehci
> >      functionfs
> > +    functionfs-desc
>
> That's an odd name for a DFU-specific file, right?
>
> Where are the Documentation/ABI/ entries?

functionfs-desc was intended to be for more than DFU. I was thinking
it would be nice to also talk about other descriptors that can be
written to functionfs since I couldn't find any documentation on
that, but I didn't want to add documentation for a bunch of existing
stuff to this same patch. It seems like that would be better submitted
separately (which I can work on if you think it's useful.) I only included
the descriptors that were relevant to DFU.

I will see what I can add for the ABI documentation as well.

>
> >      gadget_configfs
> >      gadget_hid
> >      gadget_multi
> > diff --git a/drivers/usb/gadget/function/f_fs.c b/drivers/usb/gadget/function/f_fs.c
> > index d8b096859337..ba5c6e4827ba 100644
> > --- a/drivers/usb/gadget/function/f_fs.c
> > +++ b/drivers/usb/gadget/function/f_fs.c
> > @@ -2478,7 +2478,7 @@ typedef int (*ffs_os_desc_callback)(enum ffs_os_desc_type entity,
> >
> >  static int __must_check ffs_do_single_desc(char *data, unsigned len,
> >                                          ffs_entity_callback entity,
> > -                                        void *priv, int *current_class)
> > +                                        void *priv, int *current_class, int *current_subclass)
> >  {
> >       struct usb_descriptor_header *_ds = (void *)data;
> >       u8 length;
> > @@ -2535,6 +2535,7 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
> >               if (ds->iInterface)
> >                       __entity(STRING, ds->iInterface);
> >               *current_class = ds->bInterfaceClass;
> > +             *current_subclass = ds->bInterfaceSubClass;
> >       }
> >               break;
> >
> > @@ -2559,6 +2560,12 @@ static int __must_check ffs_do_single_desc(char *data, unsigned len,
> >                       if (length != sizeof(struct ccid_descriptor))
> >                               goto inv_length;
> >                       break;
> > +             } else if (*current_class == USB_CLASS_APP_SPEC &&
> > +                        *current_subclass == USB_SUBCLASS_DFU) {
> > +                     pr_vdebug("dfu functional descriptor\n");
> > +                     if (length != sizeof(struct usb_dfu_functional_descriptor))
> > +                             goto inv_length;
> > +                     break;
> >               } else {
> >                       pr_vdebug("unknown descriptor: %d for class %d\n",
> >                             _ds->bDescriptorType, *current_class);
> > @@ -2621,6 +2628,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
> >       const unsigned _len = len;
> >       unsigned long num = 0;
> >       int current_class = -1;
> > +     int current_subclass = -1;
> >
> >       for (;;) {
> >               int ret;
> > @@ -2640,7 +2648,7 @@ static int __must_check ffs_do_descs(unsigned count, char *data, unsigned len,
> >                       return _len - len;
> >
> >               ret = ffs_do_single_desc(data, len, entity, priv,
> > -                     &current_class);
> > +                     &current_class, &current_subclass);
> >               if (ret < 0) {
> >                       pr_debug("%s returns %d\n", __func__, ret);
> >                       return ret;
> > diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
> > index 44d73ba8788d..91f0f7e214a5 100644
> > --- a/include/uapi/linux/usb/ch9.h
> > +++ b/include/uapi/linux/usb/ch9.h
> > @@ -254,6 +254,9 @@ struct usb_ctrlrequest {
> >  #define USB_DT_DEVICE_CAPABILITY     0x10
> >  #define USB_DT_WIRELESS_ENDPOINT_COMP        0x11
> >  #define USB_DT_WIRE_ADAPTER          0x21
> > +/* From USB Device Firmware Upgrade Specification, Revision 1.1 */
> > +#define USB_DT_DFU_FUNCTIONAL                0x21
> > +/* these are from the Wireless USB spec */
> >  #define USB_DT_RPIPE                 0x22
> >  #define USB_DT_CS_RADIO_CONTROL              0x23
> >  /* From the T10 UAS specification */
> > @@ -329,9 +332,10 @@ struct usb_device_descriptor {
> >  #define USB_CLASS_USB_TYPE_C_BRIDGE  0x12
> >  #define USB_CLASS_MISC                       0xef
> >  #define USB_CLASS_APP_SPEC           0xfe
> > -#define USB_CLASS_VENDOR_SPEC                0xff
> > +#define USB_SUBCLASS_DFU                     0x01
> >
> > -#define USB_SUBCLASS_VENDOR_SPEC     0xff
> > +#define USB_CLASS_VENDOR_SPEC                0xff
> > +#define USB_SUBCLASS_VENDOR_SPEC             0xff
> >
> >  /*-------------------------------------------------------------------------*/
> >
> > diff --git a/include/uapi/linux/usb/functionfs.h b/include/uapi/linux/usb/functionfs.h
> > index 9f88de9c3d66..40f87cbabf7a 100644
> > --- a/include/uapi/linux/usb/functionfs.h
> > +++ b/include/uapi/linux/usb/functionfs.h
> > @@ -37,6 +37,31 @@ struct usb_endpoint_descriptor_no_audio {
> >       __u8  bInterval;
> >  } __attribute__((packed));
> >
> > +/**
> > + * struct usb_dfu_functional_descriptor - DFU Functional descriptor
> > + * @bLength:         Size of the descriptor (bytes)
> > + * @bDescriptorType: USB_DT_DFU_FUNCTIONAL
> > + * @bmAttributes:    DFU attributes
> > + * @wDetachTimeOut:  Maximum time to wait after DFU_DETACH (ms, le16)
> > + * @wTransferSize:   Maximum number of bytes per control-write (le16)
> > + * @bcdDFUVersion:   DFU Spec version (BCD, le16)
> > + */
> > +struct usb_dfu_functional_descriptor {
> > +     __u8  bLength;
> > +     __u8  bDescriptorType;
> > +     __u8  bmAttributes;
> > +     __le16 wDetachTimeOut;
> > +     __le16 wTransferSize;
> > +     __le16 bcdDFUVersion;
> > +} __attribute__ ((packed));
> > +
> > +/* from DFU functional descriptor bmAttributes */
> > +#define DFU_FUNC_ATT_CAN_DOWNLOAD    BIT(0)
> > +#define DFU_FUNC_ATT_CAN_UPLOAD              BIT(1)
> > +#define DFU_FUNC_ATT_MANIFEST_TOLERANT       BIT(2)
> > +#define DFU_FUNC_ATT_WILL_DETACH     BIT(3)
>
> Wrong macro for bit fields for uapi .h files :(

Oh. I'm surprised there is more than one macro for bits. I will
change this to _BITUL().

>
> thanks,
>
> greg k-h





[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux