Re: [PATCH v3 1/1] usb: gadget: add raw-gadget interface

[Andrey Konovalov]

On Wed, Dec 18, 2019 at 7:19 PM Greg Kroah-Hartman
<gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Wed, Dec 18, 2019 at 06:28:19PM +0100, Andrey Konovalov wrote:
> > On Wed, Dec 18, 2019 at 2:23 PM Greg Kroah-Hartman
> > <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Wed, Dec 11, 2019 at 07:02:41PM +0100, Andrey Konovalov wrote:
> > > > USB Raw Gadget is a kernel module that provides a userspace interface for
> > > > the USB Gadget subsystem. Essentially it allows to emulate USB devices
> > > > from userspace. Enabled with CONFIG_USB_RAW_GADGET. Raw Gadget is
> > > > currently a strictly debugging feature and shouldn't be used in
> > > > production.
> > > >
> > > > Raw Gadget is similar to GadgetFS, but provides a more low-level and
> > > > direct access to the USB Gadget layer for the userspace. The key
> > > > differences are:
> > > >
> > > > 1. Every USB request is passed to the userspace to get a response, while
> > > >    GadgetFS responds to some USB requests internally based on the provided
> > > >    descriptors. However note, that the UDC driver might respond to some
> > > >    requests on its own and never forward them to the Gadget layer.
> > > >
> > > > 2. GadgetFS performs some sanity checks on the provided USB descriptors,
> > > >    while Raw Gadget allows you to provide arbitrary data as responses to
> > > >    USB requests.
> > > >
> > > > 3. Raw Gadget provides a way to select a UDC device/driver to bind to,
> > > >    while GadgetFS currently binds to the first available UDC.
> > > >
> > > > 4. Raw Gadget uses predictable endpoint names (handles) across different
> > > >    UDCs (as long as UDCs have enough endpoints of each required transfer
> > > >    type).
> > > >
> > > > 5. Raw Gadget has ioctl-based interface instead of a filesystem-based one.
> > >
> > > Looks good to me, only minor comments below.
> >
> > Great, thanks!
> >
> > About reworking the logging to use dev_err/dbg(): can I pass the
> > global miscdevice struct into those macros? Or should I pass a pointer
> > to this struct into all of the functions that print log messages? The
> > latter seems unnecessarily complex, unless there's a reason to do
> > that.
>
> Ah, you are right, you only have one misc device here.  No, that's not
> good, but you can use it for some messages (your ioctl errors), but
> ideally you will have a struct device somewhere for each of the
> "instances" you create, right?  That is what you should use for that.

OK, I think I got the idea. Will do in v4.

>
> > > > +struct raw_dev {
> > > > +     struct kref                     count;
> > > > +     spinlock_t                      lock;
> > > > +
> > > > +     const char                      *udc_name;
> > > > +     struct usb_gadget_driver        driver;
> > >
> > > A dev embeds a driver?
> > >
> > > Not a pointer?
> > >
> > > But you have a kref, so the reference count of this object is there,
> > > right?
> >
> > I didn't get this comment, could you elaborate? I can make it a
> > pointer, but for each raw_dev we have a unique usb_gadget_driver
> > instance, so embedding it as is is simpler.
>
> Ok, that's fine.  But it feels odd creating a driver dynamically to me,
> but it should work (as you show.)  It doesn't give you something to use
> for the dev_* messages directly, ah, but you do have something:
>
> > > > +
> > > > +     /* Protected by lock: */
> > > > +     enum dev_state                  state;
> > > > +     bool                            gadget_registered;
> > > > +     struct usb_gadget               *gadget;
>
> There, use that pointer for your dev_* messages, and you should be fine.
>
> > > > +static void gadget_unbind(struct usb_gadget *gadget)
> > > > +{
> > > > +     struct raw_dev *dev = get_gadget_data(gadget);
> > > > +     unsigned long flags;
> > > > +
> > > > +     spin_lock_irqsave(&dev->lock, flags);
> > > > +     set_gadget_data(gadget, NULL);
> > > > +     spin_unlock_irqrestore(&dev->lock, flags);
> > > > +     /* Matches kref_get() in gadget_bind(). */
> > > > +     kref_put(&dev->count, dev_free);
> > >
> > > What protects the kref from being called 'put' twice on the same
> > > pointer at the same time?  There should be some lock somewhere, right?
> >
> > Hm, kref_put() does refcount_dec_and_test(), which in turns calls
> > atomic_dec_and_test(), so this is protected against concurrent puts
> > (which is the whole idea of kref?), and no locking is needed. Unless I
> > misunderstand something.
>
> It's late, but there should be some lock somewhere to prevent a race
> around this type of thing.  That's why we have kref_put_mutex() and
> kref_put_lock().
>
> Odds are you are fine here, but just something to be aware of...

Ah, I see. So AFAIU kref_put_lock/mutex() are meant to be used in
cases when there might be a concurrent user that doesn't have the
reference counter incremented, but holds the lock? We don't do this
kind of stuff here.