On Mon, Jan 13, 2025 at 08:50:59PM +0200, Laurent Pinchart wrote:
> On Thu, Jan 09, 2025 at 07:47:31AM -0300, Thadeu Lima de Souza Cascardo wrote:
> > On Wed, Jan 08, 2025 at 11:14:28PM +0100, Tomasz Sikora wrote:
> > > Hello,
> > > you right
> > > I have in dmsg (line 1228):
> > > [ 12.981124] usb 3-2: Failed to create links for entity 5
> > > [ 12.981126] usb 3-2: Failed to register entities (-22).
> > >
> > > full output in my log.
> >
> > Thanks, Tomasz.
> >
> > Can you test the attached fix? It should still keep the warning about the
> > multiple units with the same ID, but now it would not return an error nor
> > warn when registering the entities.
> >
> > Cascardo.
>
> > From f771f5c4657ed25ae36784bf13992ddbee3161e6 Mon Sep 17 00:00:00 2001
> > From: Thadeu Lima de Souza Cascardo <cascardo@xxxxxxxxxx>
> > Date: Thu, 9 Jan 2025 07:37:41 -0300
> > Subject: [PATCH RFC] media: uvcvideo: restore support for non-compliant devices
> >
> > Some real-world devices have multiple units with the same ID. When creating
> > their media entities, it would lead to warnings and failure to create such
> > entities. However, the V4L2 devices would still be created and work.
> >
> > Restore their support, but still warn about the multiple units with the
> > same ID. Avoid the failure in navigating through the chain by storing
>
> What's "the failure" here ?
>
This is the original failure that we saw with a syzkaller reproducer. But
now, we know this same failure may happen with real devices.
uvc_mc_create_links will use baSourceID to find an entity on the list, but
since there may be multiple entities with the same ID, it may find the
incorrect one (in this case, finding the output terminal instead of the
processing unit).
> > pointers to the entities instead of only their IDs.
>
> Missing SoB and Fixes tags.
>
I skipped them on purpose since this was an RFC.
> The commit message should explain why this is better than reverting
> 3dd075fe8ebb ("media: uvcvideo: Require entities to have a non-zero
> unique ID"). I'm wondering if a revert with a clean fix on top may not
> be easier to review.
>
I am trying to get a proper fix on top because reverting would lead to
these warnings on real hardware.
> > ---
> > drivers/media/usb/uvc/uvc_driver.c | 16 +++++++++++-----
> > drivers/media/usb/uvc/uvc_entity.c | 4 +++-
> > drivers/media/usb/uvc/uvcvideo.h | 1 +
> > 3 files changed, 15 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 1a22364f7da9..dd81067f8d30 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -791,10 +791,8 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > }
> >
> > /* Per UVC 1.1+ spec 3.7.2, the ID is unique. */
> > - if (uvc_entity_by_id(dev, id)) {
> > - dev_err(&dev->udev->dev, "Found multiple Units with ID %u\n", id);
> > - return ERR_PTR(-EINVAL);
> > - }
> > + if (uvc_entity_by_id(dev, id))
> > + dev_warn(&dev->udev->dev, "Found multiple Units with ID %u\n", id);
> >
> > extra_size = roundup(extra_size, sizeof(*entity->pads));
> > if (num_pads)
> > @@ -802,7 +800,7 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> > else
> > num_inputs = 0;
> > size = sizeof(*entity) + extra_size + sizeof(*entity->pads) * num_pads
> > - + num_inputs;
> > + + num_inputs + sizeof(struct uvc_entity *) * num_inputs;
> > entity = kzalloc(size, GFP_KERNEL);
> > if (entity == NULL)
> > return ERR_PTR(-ENOMEM);
> > @@ -840,6 +838,7 @@ static struct uvc_entity *uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
> >
> > entity->bNrInPins = num_inputs;
> > entity->baSourceID = (u8 *)(&entity->pads[num_pads]);
> > + entity->source_entities = (struct uvc_entity **)(&entity->baSourceID[num_inputs]);
> >
> > return entity;
> > }
> > @@ -1503,6 +1502,7 @@ static int uvc_scan_chain_forward(struct uvc_video_chain *chain,
> > }
> >
> > forward->baSourceID[0] = source->id;
> > + forward->source_entities[0] = source;
> > }
> >
> > list_add_tail(&forward->chain, &chain->entities);
> > @@ -1586,6 +1586,8 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
> > return -EINVAL;
> > }
> >
> > + entity->source_entities[i] = term;
> > +
> > uvc_dbg_cont(PROBE, " %d", term->id);
> >
> > list_add_tail(&term->chain, &chain->entities);
> > @@ -1620,6 +1622,8 @@ static int uvc_scan_chain_backward(struct uvc_video_chain *chain,
> > return -EINVAL;
> > }
> >
> > + (*_entity)->source_entities[0] = entity;
> > +
> > *_entity = entity;
> > return 0;
> > }
> > @@ -1783,6 +1787,7 @@ static int uvc_scan_fallback(struct uvc_device *dev)
> > goto error;
> >
> > prev->baSourceID[0] = entity->id;
> > + prev->source_entities[0] = entity;
> > prev = entity;
> > }
> >
> > @@ -1790,6 +1795,7 @@ static int uvc_scan_fallback(struct uvc_device *dev)
> > goto error;
> >
> > prev->baSourceID[0] = iterm->id;
> > + prev->source_entities[0] = iterm;
> >
> > list_add_tail(&chain->list, &dev->chains);
> >
> > diff --git a/drivers/media/usb/uvc/uvc_entity.c b/drivers/media/usb/uvc/uvc_entity.c
> > index cc68dd24eb42..7f42292b7fde 100644
> > --- a/drivers/media/usb/uvc/uvc_entity.c
> > +++ b/drivers/media/usb/uvc/uvc_entity.c
> > @@ -36,7 +36,9 @@ static int uvc_mc_create_links(struct uvc_video_chain *chain,
> > if (!(entity->pads[i].flags & MEDIA_PAD_FL_SINK))
> > continue;
> >
> > - remote = uvc_entity_by_id(chain->dev, entity->baSourceID[i]);
> > + remote = entity->source_entities[i];
> > + if (remote == NULL)
> > + remote = uvc_entity_by_id(chain->dev, entity->baSourceID[i]);
>
> That looks worrying. Why would source_entities[i] be NULL ?
>
> Devices with bad descriptors can lead to crashes, and it's important to
> harden the code. Just warning about duplicate ideas and adding a
> source_entities array that may or may not point to the right source (and
> could point to NULL) doesn't seem to go in the right direction.
>
> Other options include adding a device-specific quirk that overrides the
> incorrect entity IDs, or, possibly better, implementing a heuristic to
> fix those automatically.
>
The heuristic is uvc_scan_fallback. I am trying to balance here between
changing the code too much, which could lead to other hardware stop working
and fixing the two bugs. We could replace baSourceID array with the
source_entities array entirely. But while the baSourceID is built from the
USB descriptors, the source_entities rely on the building of the chain. And
using baSourceID to find the entities in unreliable because of these
devices with multiple entities with the same ID.
Perhaps we should skip when source_entities[i] is NULL, which indicates
that we could not build the chain. But then, again, this introduces more
risk of regressing other hardware, at least without further analysis.
Cascardo.
> > if (remote == NULL || remote->num_pads == 0)
> > return -EINVAL;
> >
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > index 07f9921d83f2..a4ee79e4e85b 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -239,6 +239,7 @@ struct uvc_entity {
> >
> > u8 bNrInPins;
> > u8 *baSourceID;
> > + struct uvc_entity **source_entities;
> >
> > int (*get_info)(struct uvc_device *dev, struct uvc_entity *entity,
> > u8 cs, u8 *caps);
>
> --
> Regards,
>
> Laurent Pinchart
