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

Sakari Ailus
2026-04-10T12:56:51Z

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.

>
> > {
> > - 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...
>

--
Regards,

Sakari Ailus