Hi
On Tue, 14 Jan 2025 at 02:51, Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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 ?
>
> > pointers to the entities instead of only their IDs.
>
> Missing SoB and Fixes tags.
>
> 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.
>
> > ---
> > 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.
Can't we simply do something like this:
diff --git a/drivers/media/usb/uvc/uvc_driver.c
b/drivers/media/usb/uvc/uvc_driver.c
index a10d4f4d9f95..b55dc440db26 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -135,6 +135,9 @@ struct uvc_entity *uvc_entity_by_id(struct
uvc_device *dev, int id)
{
struct uvc_entity *entity;
+ if (id == UVC_INVALID_ENTITY_ID)
+ return NULL;
+
list_for_each_entry(entity, &dev->entities, list) {
if (entity->id == id)
return entity;
@@ -802,13 +805,13 @@ static struct uvc_entity
*uvc_alloc_new_entity(struct uvc_device *dev, u16 type,
/* Per UVC 1.1+ spec 3.7.2, the ID should be non-zero. */
if (id == 0) {
dev_err(&dev->udev->dev, "Found Unit with invalid ID 0.\n");
- return ERR_PTR(-EINVAL);
+ id = UVC_INVALID_ENTITY_ID;
}
/* 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);
+ id = UVC_INVALID_ENTITY_ID;
}
extra_size = roundup(extra_size, sizeof(*entity->pads));
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index 5e388f05f3fc..2ba8e32260ca 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -41,6 +41,8 @@
#define UVC_EXT_GPIO_UNIT 0x7ffe
#define UVC_EXT_GPIO_UNIT_ID 0x100
+#define UVC_INVALID_ENTITY_ID 0xffff
+
/* ------------------------------------------------------------------------
* Driver specific constants.
*/
>
> > 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
--
Ricardo Ribalda
