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.
