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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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@linuxfoundation.org> 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.

> > > +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...

thanks,

greg k-h