| Message ID | 20231113101718.6098-2-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-subdev: Fix link validation issue | expand |
Hi Laurent, On Mon, Nov 13, 2023 at 12:17:17PM +0200, Laurent Pinchart wrote: > The v4l2_subdev_link_validate_get_format() function warns if the pad > given as argument belongs to a non-subdev entity. This can't happen, as > the function is called from v4l2_subdev_link_validate() only (indirectly > through v4l2_subdev_link_validate_locked()), and that function ensures > that both ends of the link are subdevs. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index be86b906c985..67d43206ce32 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1184,14 +1184,6 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, > struct v4l2_subdev *sd; > int ret; > > - if (!is_media_entity_v4l2_subdev(pad->entity)) { > - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, > - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", > - pad->entity->function, pad->entity->name); > - > - return -EINVAL; > - } > - > sd = media_entity_to_v4l2_subdev(pad->entity); It'd be good to check sd is non-NULL here, although pad presumably is always a pad of a sub-device entity. > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
On Mon, Nov 13, 2023 at 10:42:16AM +0000, Sakari Ailus wrote: > Hi Laurent, > > On Mon, Nov 13, 2023 at 12:17:17PM +0200, Laurent Pinchart wrote: > > The v4l2_subdev_link_validate_get_format() function warns if the pad > > given as argument belongs to a non-subdev entity. This can't happen, as > > the function is called from v4l2_subdev_link_validate() only (indirectly > > through v4l2_subdev_link_validate_locked()), and that function ensures > > that both ends of the link are subdevs. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- > > drivers/media/v4l2-core/v4l2-subdev.c | 8 -------- > > 1 file changed, 8 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > index be86b906c985..67d43206ce32 100644 > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > @@ -1184,14 +1184,6 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, > > struct v4l2_subdev *sd; > > int ret; > > > > - if (!is_media_entity_v4l2_subdev(pad->entity)) { > > - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, > > - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", > > - pad->entity->function, pad->entity->name); > > - > > - return -EINVAL; > > - } > > - > > sd = media_entity_to_v4l2_subdev(pad->entity); > > It'd be good to check sd is non-NULL here, although pad presumably is > always a pad of a sub-device entity. It's guaranteed by the caller. Is there a specific reason you think a check would be good ? > > > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
On 13/11/2023 12:17, Laurent Pinchart wrote: > The v4l2_subdev_link_validate_get_format() function warns if the pad > given as argument belongs to a non-subdev entity. This can't happen, as > the function is called from v4l2_subdev_link_validate() only (indirectly > through v4l2_subdev_link_validate_locked()), and that function ensures > that both ends of the link are subdevs. > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/media/v4l2-core/v4l2-subdev.c | 8 -------- > 1 file changed, 8 deletions(-) > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > index be86b906c985..67d43206ce32 100644 > --- a/drivers/media/v4l2-core/v4l2-subdev.c > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > @@ -1184,14 +1184,6 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, > struct v4l2_subdev *sd; > int ret; > > - if (!is_media_entity_v4l2_subdev(pad->entity)) { > - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, > - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", > - pad->entity->function, pad->entity->name); > - > - return -EINVAL; > - } > - > sd = media_entity_to_v4l2_subdev(pad->entity); > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE; At some point I also wondered why we need to check for non-subdev entity. But, it was there already, so I left it... Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> Tomi
Hi Sakari, On Mon, Nov 13, 2023 at 12:50:16PM +0200, Laurent Pinchart wrote: > On Mon, Nov 13, 2023 at 10:42:16AM +0000, Sakari Ailus wrote: > > On Mon, Nov 13, 2023 at 12:17:17PM +0200, Laurent Pinchart wrote: > > > The v4l2_subdev_link_validate_get_format() function warns if the pad > > > given as argument belongs to a non-subdev entity. This can't happen, as > > > the function is called from v4l2_subdev_link_validate() only (indirectly > > > through v4l2_subdev_link_validate_locked()), and that function ensures > > > that both ends of the link are subdevs. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > > --- > > > drivers/media/v4l2-core/v4l2-subdev.c | 8 -------- > > > 1 file changed, 8 deletions(-) > > > > > > diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c > > > index be86b906c985..67d43206ce32 100644 > > > --- a/drivers/media/v4l2-core/v4l2-subdev.c > > > +++ b/drivers/media/v4l2-core/v4l2-subdev.c > > > @@ -1184,14 +1184,6 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, > > > struct v4l2_subdev *sd; > > > int ret; > > > > > > - if (!is_media_entity_v4l2_subdev(pad->entity)) { > > > - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, > > > - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", > > > - pad->entity->function, pad->entity->name); > > > - > > > - return -EINVAL; > > > - } > > > - > > > sd = media_entity_to_v4l2_subdev(pad->entity); > > > > It'd be good to check sd is non-NULL here, although pad presumably is > > always a pad of a sub-device entity. > > It's guaranteed by the caller. Is there a specific reason you think a > check would be good ? Would you still want a check ? > > > > > > fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c index be86b906c985..67d43206ce32 100644 --- a/drivers/media/v4l2-core/v4l2-subdev.c +++ b/drivers/media/v4l2-core/v4l2-subdev.c @@ -1184,14 +1184,6 @@ v4l2_subdev_link_validate_get_format(struct media_pad *pad, u32 stream, struct v4l2_subdev *sd; int ret; - if (!is_media_entity_v4l2_subdev(pad->entity)) { - WARN(pad->entity->function != MEDIA_ENT_F_IO_V4L, - "Driver bug! Wrong media entity type 0x%08x, entity %s\n", - pad->entity->function, pad->entity->name); - - return -EINVAL; - } - sd = media_entity_to_v4l2_subdev(pad->entity); fmt->which = V4L2_SUBDEV_FORMAT_ACTIVE;
The v4l2_subdev_link_validate_get_format() function warns if the pad given as argument belongs to a non-subdev entity. This can't happen, as the function is called from v4l2_subdev_link_validate() only (indirectly through v4l2_subdev_link_validate_locked()), and that function ensures that both ends of the link are subdevs. Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> --- drivers/media/v4l2-core/v4l2-subdev.c | 8 -------- 1 file changed, 8 deletions(-)