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