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

[Felipe Balbi <balbi@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 2979 bytes --]


Hi,

Andrey Konovalov <andreyknvl@google.com> writes:
>> > diff --git a/drivers/usb/gadget/legacy/raw_gadget.c b/drivers/usb/gadget/legacy/raw_gadget.c
>> > new file mode 100644
>> > index 000000000000..51796af48069
>> > --- /dev/null
>> > +++ b/drivers/usb/gadget/legacy/raw_gadget.c
>> > @@ -0,0 +1,1068 @@
>> > +// SPDX-License-Identifier: GPL-2.0
>>
>> V2 only
>
> Like this: SPDX-License-Identifier: GPL-2.0 only ?

Right, you need to choose if you want 2.0-only or 2.0-or-later and make
sure spdx and module_license() agree.

https://spdx.org/licenses/GPL-2.0-only.html

What you had before, implies GPL-2.0-only...

>> > +MODULE_LICENSE("GPL");

but this is GPL 2+

/me goes look

Actually Thomas Gleixner changed the meaning of MODULE_LICENSE("GPL"),
so I don't really know how this should look today.

>> > +static int raw_event_queue_add(struct raw_event_queue *queue,
>> > +     enum usb_raw_event_type type, size_t length, const void *data)
>> > +{
>> > +     unsigned long flags;
>> > +     struct usb_raw_event *event;
>> > +
>> > +     spin_lock_irqsave(&queue->lock, flags);
>> > +     if (WARN_ON(queue->size >= RAW_EVENT_QUEUE_SIZE)) {
>> > +             spin_unlock_irqrestore(&queue->lock, flags);
>> > +             return -ENOMEM;
>> > +     }
>> > +     event = kmalloc(sizeof(*event) + length, GFP_ATOMIC);
>>
>> I would very much prefer dropping GFP_ATOMIC here. Must you have this
>> allocation under a spinlock?
>
> The issue here is not the spinlock, but that this might be called in
> interrupt context. The number of atomic allocations here is restricted
> by 128, and we can reduce the limit even further (until some point in
> the future when and if we'll report more different events). Another
> option would be to preallocate the required number of objects
> (although we don't know the required size in advance, so we'll waste
> some memory) and use those. What would you prefer?

I think you shouldn't do either :-) Here's what I think you should do:

1. support O_NONBLOCK. This just means conditionally removing your
   wait_for_completion_interruptible().

2. Every time user calls write(), you usb_ep_alloc(), allocate a buffer
   with the write size, copy buffer to kernel space,
   usb_ep_queue(). When complete() callback is called, then you free the
   request. This would allow us to amortize the cost of copy_from_user()
   with several requests being queued to USB controller.

3. Have a pre-allocated list of requests (128?) for read(). Enqueue them
   all during set_alt(). When user calls read() you will:

   a) check if there are completed requests to be copied over to
      userspace. Recycle the request.

   b) if there are no completed requests, then it depends on O_NONBLOCK

      i) If O_NONBLOCK, return -EWOULDBLOCK
      ii) otherwise, wait_for_completion

I think this can all be done without any GFP_ATOMIC allocations.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]