Re: [PATCH 05/13] media: v4l: Make media_entity_to_video_device() NULL-safe

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Tue, Jul 08, 2025 at 12:02:29PM +0000, Sakari Ailus wrote:
> On Tue, Jul 08, 2025 at 02:56:16PM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 19, 2025 at 04:14:43PM +0000, Sakari Ailus wrote:
> > > On Thu, Jun 19, 2025 at 06:20:33PM +0300, Laurent Pinchart wrote:
> > > > On Thu, Jun 19, 2025 at 11:15:38AM +0300, Sakari Ailus wrote:
> > > > > Make media_entity_to_video_device(NULL) return NULL, instead of an invalid
> > > > > pointer value.
> > > > > 
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  include/media/v4l2-dev.h | 14 ++++++++++----
> > > > >  1 file changed, 10 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> > > > > index 1b6222fab24e..069c2f14b473 100644
> > > > > --- a/include/media/v4l2-dev.h
> > > > > +++ b/include/media/v4l2-dev.h
> > > > > @@ -313,10 +313,16 @@ struct video_device {
> > > > >   * media_entity_to_video_device - Returns a &struct video_device from
> > > > >   *	the &struct media_entity embedded on it.
> > > > >   *
> > > > > - * @__entity: pointer to &struct media_entity
> > > > > - */
> > > > > -#define media_entity_to_video_device(__entity) \
> > > > > -	container_of(__entity, struct video_device, entity)
> > > > > + * @__entity: pointer to &struct media_entity, may be NULL
> > > > > + */
> > > > > +#define media_entity_to_video_device(__entity)				\
> > > > > +	({								\
> > > > > +		typeof (__entity) __me_to_vdev_ent = __entity;		\

This should be __me_vdev_ent to align the naming with
media_entity_to_v4l2_subdev().

> > > > > +									\
> > > > > +		__me_to_vdev_ent ? container_of(__me_to_vdev_ent,	\
> > > > > +						struct video_device, entity) : \

And here you should write

	__me_to_vdev_ent ?						   \
		container_of(__me_vdev_ent, struct video_device, entity) : \
		NULL;							   \

> > > > > +			NULL;						\
> > > > > +	})
> > > > 
> > > > This makes the macro safer, it's a good idea. Wouldn't it be better
> > > > implemented as a container_of_null() (name to be bikeshedded) though ? I
> > > > don't think media_entity_to_video_device() is the only macro that could
> > > > benefit from this. It could even be integrated in container_of(), but I
> > > > fear that could introduce issues.
> > > 
> > > That sounds like a good idea. I'll first see how this would look like with
> > > container_of_const()...
> > 
> > Thinking some more about this, I think we can move forward without
> > waiting for container_of_null().
> > 
> > Should we however add a check to ensure the entity is a video device (by
> > checking that the function is MEDIA_ENT_F_IO_V4L), and return NULL if
> > it's not ? It would make the macro even safer to use. There would be a
> > small additional runtime cost for call sites that guarantee the entity
> > is a video device.
> > 
> > I checked the current users of the macro, and the vast majority of them
> > are in the .link_validate() operation, where they know that the entity
> > is a video device. There are just a handful of locations where a check
> > precedes the media_entity_to_video_device() call. So maybe it's not
> > worth it ?
> > 
> > I also found no caller that checks for entity != NULL before calling the
> > macro. Is this change actually needed ?
> 
> We have a similar check in media_entity_to_v4l2_subdev() macro.

That's a good point. Different behaviours for the two macros would be
confusing I suppose, so

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

I'm tempted to convert the macro to an inline function, but that would
make it more difficult to switch to container_of_const().

> In the
> patches for streaming control there's a need for an explicit check without
> this patch. That's of course fine as well if we decide so.

-- 
Regards,

Laurent Pinchart