Hi Ricardo, Thank you for the patch.
On Mon, Dec 21, 2020 at 05:48:09PM +0100, Ricardo Ribalda wrote: > The current implementation allocates memory for only one async_control. > If we get a second event before we have processed the previous one, the > old one gets lost. > > Introduce a dynamic memory allocation and a list to handle the > async_controls. Thinking some more about this, do we need to go through the work queue at all for GPIO-based events ? Could the part of uvc_ctrl_status_event_work() before the URB resubmission be moved to another function, which would be called directly by the GPIO threaded IRQ handler ? > Signed-off-by: Ricardo Ribalda <[email protected]> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 49 ++++++++++++++++++++++++++------ > drivers/media/usb/uvc/uvcvideo.h | 19 ++++++++----- > 2 files changed, 52 insertions(+), 16 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c > b/drivers/media/usb/uvc/uvc_ctrl.c > index aa18dcdf8165..69b2fc6ce12c 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1275,11 +1275,9 @@ static void uvc_ctrl_send_slave_event(struct > uvc_video_chain *chain, > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > } > > -static void uvc_ctrl_status_event_work(struct work_struct *work) > +static void __uvc_ctrl_status_event_work(struct uvc_device *dev, > + struct uvc_ctrl_work *w) > { > - struct uvc_device *dev = container_of(work, struct uvc_device, > - async_ctrl.work); > - struct uvc_ctrl_work *w = &dev->async_ctrl; > struct uvc_video_chain *chain = w->chain; > struct uvc_control_mapping *mapping; > struct uvc_control *ctrl = w->ctrl; > @@ -1321,23 +1319,54 @@ static void uvc_ctrl_status_event_work(struct > work_struct *work) > ret); > } > > +static void uvc_ctrl_status_event_work(struct work_struct *work) > +{ > + struct uvc_device *dev = container_of(work, struct uvc_device, > + async_ctrl_work); > + struct uvc_ctrl_work *w; > + > + do { > + mutex_lock(&dev->async_ctrl_lock); > + w = list_first_entry_or_null(&dev->async_ctrl_list, > + struct uvc_ctrl_work, > + list); > + if (w) > + list_del(&w->list); > + mutex_unlock(&dev->async_ctrl_lock); > + > + if (!w) > + return; > + > + __uvc_ctrl_status_event_work(dev, w); > + kfree(w); > + } while (w); > +} > + > bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain, > struct uvc_control *ctrl, const u8 *data) > { > struct uvc_device *dev = chain->dev; > - struct uvc_ctrl_work *w = &dev->async_ctrl; > + struct uvc_ctrl_work *w; > > if (list_empty(&ctrl->info.mappings)) { > ctrl->handle = NULL; > return false; > } > > + w = kzalloc(sizeof(*w), GFP_KERNEL); > + if (WARN(!w, "Not enough memory to trigger uvc event")) > + return false; > + > memcpy(w->data, data, ctrl->info.size); > w->urb = urb; > w->chain = chain; > w->ctrl = ctrl; > > - schedule_work(&w->work); > + mutex_lock(&dev->async_ctrl_lock); > + list_add_tail(&w->list, &dev->async_ctrl_list); > + mutex_unlock(&dev->async_ctrl_lock); > + > + schedule_work(&dev->async_ctrl_work); > > return true; > } > @@ -2277,7 +2306,9 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > struct uvc_entity *entity; > unsigned int i; > > - INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work); > + INIT_WORK(&dev->async_ctrl_work, uvc_ctrl_status_event_work); > + mutex_init(&dev->async_ctrl_lock); > + INIT_LIST_HEAD(&dev->async_ctrl_list); > > /* Walk the entities list and instantiate controls */ > list_for_each_entry(entity, &dev->entities, list) { > @@ -2348,8 +2379,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev) > unsigned int i; > > /* Can be uninitialized if we are aborting on probe error. */ > - if (dev->async_ctrl.work.func) > - cancel_work_sync(&dev->async_ctrl.work); > + if (dev->async_ctrl_work.func) > + cancel_work_sync(&dev->async_ctrl_work); > > /* Free controls and control mappings for all entities. */ > list_for_each_entry(entity, &dev->entities, list) { > diff --git a/drivers/media/usb/uvc/uvcvideo.h > b/drivers/media/usb/uvc/uvcvideo.h > index 0db6c2e0bd98..afcaf49fad1a 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -637,6 +637,14 @@ struct uvc_device_info { > u32 meta_format; > }; > > +struct uvc_ctrl_work { > + struct list_head list; > + struct urb *urb; > + struct uvc_video_chain *chain; > + struct uvc_control *ctrl; > + u8 data[UVC_MAX_STATUS_SIZE]; > +}; > + > struct uvc_device { > struct usb_device *udev; > struct usb_interface *intf; > @@ -673,13 +681,10 @@ struct uvc_device { > struct input_dev *input; > char input_phys[64]; > > - struct uvc_ctrl_work { > - struct work_struct work; > - struct urb *urb; > - struct uvc_video_chain *chain; > - struct uvc_control *ctrl; > - u8 data[UVC_MAX_STATUS_SIZE]; > - } async_ctrl; > + /* Async control */ > + struct work_struct async_ctrl_work; > + struct list_head async_ctrl_list; > + struct mutex async_ctrl_lock; > }; > > enum uvc_handle_state { -- Regards, Laurent Pinchart
