lkml.org 
[lkml]   [2020]   [Jan]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v5 1/1] usb: gadget: add raw-gadget interface
    Date

    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]
    \
     
     \ /
      Last update: 2020-01-31 16:23    [W:2.166 / U:0.336 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site