| Message ID | 20241127-uvc-fix-async-v2-4-510aab9570dd@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Two fixes for async controls | expand |
Hi Ricardo, Thank you for the patch. On Wed, Nov 27, 2024 at 12:14:52PM +0000, Ricardo Ribalda wrote: > ctrl->handle will only be different than NULL for controls that have > mappings. This is because that assignment is only done inside > uvc_ctrl_set() for mapped controls. That seems right. I checked the unplug paths, and the mappings are destroyed from uvc_delete(), after all file handles get closed. I think this patch should go first in the series, as otherwise there's a temporary state where you rely on handle never being touched without holding the proper lock, and this code path violates that requirement. Is there anything I may be missing that would cause issues with bisection if I move this patch at the beginning of the series ? > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 99ddc5e9dff8..b9c9ff629ab6 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1645,10 +1645,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > struct uvc_device *dev = chain->dev; > struct uvc_ctrl_work *w = &dev->async_ctrl; > > - if (list_empty(&ctrl->info.mappings)) { > - ctrl->handle = NULL; > + if (list_empty(&ctrl->info.mappings)) > return false; > - } > > w->data = data; > w->urb = urb;
Hi On Thu, 28 Nov 2024 at 22:26, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Wed, Nov 27, 2024 at 12:14:52PM +0000, Ricardo Ribalda wrote: > > ctrl->handle will only be different than NULL for controls that have > > mappings. This is because that assignment is only done inside > > uvc_ctrl_set() for mapped controls. > > That seems right. I checked the unplug paths, and the mappings are > destroyed from uvc_delete(), after all file handles get closed. > > I think this patch should go first in the series, as otherwise there's a > temporary state where you rely on handle never being touched without > holding the proper lock, and this code path violates that requirement. > Is there anything I may be missing that would cause issues with > bisection if I move this patch at the beginning of the series ? > You probably want to add the Cc: stable and Fixes: tags if you make this patch the first one. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 4 +--- > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 99ddc5e9dff8..b9c9ff629ab6 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1645,10 +1645,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > > struct uvc_device *dev = chain->dev; > > struct uvc_ctrl_work *w = &dev->async_ctrl; > > > > - if (list_empty(&ctrl->info.mappings)) { > > - ctrl->handle = NULL; > > + if (list_empty(&ctrl->info.mappings)) > > return false; > > - } > > > > w->data = data; > > w->urb = urb; > > -- > Regards, > > Laurent Pinchart
On Thu, Nov 28, 2024 at 10:34:38PM +0100, Ricardo Ribalda wrote: > On Thu, 28 Nov 2024 at 22:26, Laurent Pinchart wrote: > > On Wed, Nov 27, 2024 at 12:14:52PM +0000, Ricardo Ribalda wrote: > > > ctrl->handle will only be different than NULL for controls that have > > > mappings. This is because that assignment is only done inside > > > uvc_ctrl_set() for mapped controls. > > > > That seems right. I checked the unplug paths, and the mappings are > > destroyed from uvc_delete(), after all file handles get closed. > > > > I think this patch should go first in the series, as otherwise there's a > > temporary state where you rely on handle never being touched without > > holding the proper lock, and this code path violates that requirement. > > Is there anything I may be missing that would cause issues with > > bisection if I move this patch at the beginning of the series ? > > You probably want to add the Cc: stable and Fixes: tags if you make > this patch the first one. Good point. Given that ctrl->handle for controls without mappings can't be touched anywhere else, I think it's safe to keep this patch last, and not backport it. There may be a theoretical issue with mappings dynamically added at runtime, as the list could then become non-empty and the condition change, but the condition should never turn from false to true, only from true to false, so I think it's safe. I'll keep the patch last. > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 4 +--- > > > 1 file changed, 1 insertion(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 99ddc5e9dff8..b9c9ff629ab6 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -1645,10 +1645,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, > > > struct uvc_device *dev = chain->dev; > > > struct uvc_ctrl_work *w = &dev->async_ctrl; > > > > > > - if (list_empty(&ctrl->info.mappings)) { > > > - ctrl->handle = NULL; > > > + if (list_empty(&ctrl->info.mappings)) > > > return false; > > > - } > > > > > > w->data = data; > > > w->urb = urb;
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 99ddc5e9dff8..b9c9ff629ab6 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1645,10 +1645,8 @@ bool uvc_ctrl_status_event_async(struct urb *urb, struct uvc_video_chain *chain, struct uvc_device *dev = chain->dev; struct uvc_ctrl_work *w = &dev->async_ctrl; - if (list_empty(&ctrl->info.mappings)) { - ctrl->handle = NULL; + if (list_empty(&ctrl->info.mappings)) return false; - } w->data = data; w->urb = urb;
ctrl->handle will only be different than NULL for controls that have mappings. This is because that assignment is only done inside uvc_ctrl_set() for mapped controls. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)