Hi, Andrey Konovalov <andreyknvl@xxxxxxxxxx> 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:
signature.asc
Description: PGP signature
