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

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

On Wed, Jul 09, 2025 at 08:03:07PM +0000, Sakari Ailus wrote:
> Hi Laurent,
> 
> On Tue, Jul 08, 2025 at 07:17:47PM +0300, Laurent Pinchart wrote:
> > 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;							   \
> 
> I'm fine wrap after '?' as well, but another wrap is needed due to
> indentation in any case.

I didn't make it clear, my comment was about replicating the style of
media_entity_to_v4l2_subdev().

> > > > > > > +			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>
> 
> Thank you.
> 
> > I'm tempted to convert the macro to an inline function, but that would
> > make it more difficult to switch to container_of_const().
> 
> Let's not do that. Eventually container_of() should become what
> container_of_const() is at the moment.
> 
> But once this is in, I can introduce container_of_null() which we could
> later use. I wonder how it will look like.

-- 
Regards,

Laurent Pinchart