Re: [PATCH v4 21/29] media: mc: Don't care about unsettable flags in MEDIA_IOC_LINK_SETUP

[Jacopo Mondi <jacopo.mondi@ideasonboard.com>]

Hi Sakari

On Fri, Apr 10, 2026 at 03:56:58PM +0300, Sakari Ailus wrote:
> Hi Jacopo,
>
> Thanks for the review.
>
> On Fri, Apr 10, 2026 at 12:31:58PM +0200, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Wed, Apr 08, 2026 at 06:39:30PM +0300, Sakari Ailus wrote:
> > > The implementation of MEDIA_IOC_LINK_SETUP currently requires that all
> > > flags that are set by the driver are correctly set as the driver expects.
> >
> > I would
> >
> > The implementation of MEDIA_IOC_LINK_SETUP currently requires that all
> > flags that are not configurable are not modified by userspace.
>
> Sounds good.
>
> >
> > > This poses a problem for adding new flags as programs could not work with
> > > links that have unknown flags even when the use of these flags wouldn't
> > > affect the program.
> > >
> > > Ignore the non-settable link flags.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/mc/mc-entity.c | 8 ++++----
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/mc/mc-entity.c b/drivers/media/mc/mc-entity.c
> > > index 717569bd1a8c..287eded356bb 100644
> > > --- a/drivers/media/mc/mc-entity.c
> > > +++ b/drivers/media/mc/mc-entity.c
> > > @@ -1319,7 +1319,7 @@ static int __media_entity_setup_link_notify(struct media_link *link, u32 flags)
> > >
> > >  int __media_entity_setup_link(struct media_link *link, u32 flags)
> >
> > Let me get this right: "link" comes from userspace. "flags" are the
> > flags set by driver when creating the link, right ?
>
> It's the other way around; this is called via MEDIA_IOC_SETUP_LINK IOCTL.
>

Ah
static long media_device_setup_link(struct media_device *mdev, void *arg)
{
   struct media_link_desc *linkd = arg;

   ...

   return __media_entity_setup_link(link, linkd->flags);
}

I've read it the other way around

> >
> > >  {
> > > -	const u32 mask = MEDIA_LNK_FL_ENABLED;
> > > +	const u32 settable_flags = MEDIA_LNK_FL_ENABLED;
> > >  	struct media_device *mdev;
> > >  	struct media_pad *source, *sink;
> > >  	int ret = -EBUSY;
> > > @@ -1327,9 +1327,9 @@ int __media_entity_setup_link(struct media_link *link, u32 flags)
> > >  	if (link == NULL)
> > >  		return -EINVAL;
> > >
> > > -	/* The non-modifiable link flags must not be modified. */
> > > -	if ((link->flags & ~mask) != (flags & ~mask))
> > > -		return -EINVAL;
> > > +	/* Only allow changing user-settable flags. */
> > > +	flags &= settable_flags;
> > > +	flags |= link->flags & ~settable_flags;
> > >
> > >  	if (link->flags & MEDIA_LNK_FL_IMMUTABLE)
> >
> > If my above understanding is correct and if you drop the above check,
> > doesn't this mean userspace can now clear MEDIA_LNK_FL_IMMUTABLE ?
> >
> > Should link->flags be replaced by "flags" here ?
>
> The intent here is to drop whatever non-settable flags the user provided
> and replace them with what is set on the link.
>
> >
> > >  		return link->flags == flags ? 0 : -EINVAL;
> >
> > And this should probably be changed as well. Unfortunately, if we
> > want to allow userspace to ignore the forthcoming MEDIA_LNK_FL_VALIDATE_LATE
> > flag I guess we need to make it an exception (which is not great, I know).
> >
> > Hope I got it right...
> >

I didn't

Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

>
> --
> Regards,
>
> Sakari Ailus
>