From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Andrey Konovalov <andreyknvl@google.com>
Cc: USB list <linux-usb@vger.kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Alan Stern <stern@rowland.harvard.edu>,
Jonathan Corbet <corbet@lwn.net>, Felipe Balbi <balbi@kernel.org>,
Dmitry Vyukov <dvyukov@google.com>,
Alexander Potapenko <glider@google.com>,
Marco Elver <elver@google.com>
Subject: Re: [PATCH v3 1/1] usb: gadget: add raw-gadget interface
Date: Wed, 18 Dec 2019 19:19:21 +0100 [thread overview]
Message-ID: <20191218181921.GA882018@kroah.com> (raw)
In-Reply-To: <CAAeHK+zXegV1GmSKD8Y3-hTbKUQceWdfo+GJPxSSzYr0zQTYKw@mail.gmail.com>
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
next prev parent reply other threads:[~2019-12-18 18:19 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-11 18:02 [PATCH v3 0/1] usb: gadget: add raw-gadget interface Andrey Konovalov
2019-12-11 18:02 ` [PATCH v3 1/1] " Andrey Konovalov
2019-12-18 13:23 ` Greg Kroah-Hartman
2019-12-18 17:28 ` Andrey Konovalov
2019-12-18 18:19 ` Greg Kroah-Hartman [this message]
2019-12-18 19:22 ` Andrey Konovalov
2019-12-18 20:06 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191218181921.GA882018@kroah.com \
--to=gregkh@linuxfoundation.org \
--cc=andreyknvl@google.com \
--cc=balbi@kernel.org \
--cc=corbet@lwn.net \
--cc=dvyukov@google.com \
--cc=elver@google.com \
--cc=glider@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=stern@rowland.harvard.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).