Messages in this thread | | | From | Felipe Balbi <> | Subject | Re: [PATCH v5 1/1] usb: gadget: add raw-gadget interface | Date | Fri, 31 Jan 2020 17:22:09 +0200 |
| |
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 [unhandled content-type:application/pgp-signature]
| |