Re: [PATCH v3 05/29] media: v4l2-subdev: Allow allocating frame descriptors based on the need

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hej Linus,

On Mon, Aug 31, 2026 at 03:18:50PM +0200, Linus Walleij wrote:
> Hi Sakari,
> 
> thanks for your patch!

Tack för kritiken!

> 
> On Mon, Aug 24, 2026 at 2:14 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> 
> > Frame descriptors entries require a small amount of memory per entry (20
> > bytes), but if the number of entries in a frame descriptor is large, an
> > unreasonably large amount of memory would need to be allocated in the
> > stack. Therefore the number of entries has been limited to 8.
> >
> > Support larger frame descriptors by making the entry field a pointer that
> > by default points to a pre-allocated array while the get_frame_desc() pad
> > o may allocate as much memory as required, up to V4L2_FRAME_DESC_ENTRY_MAX
> > which is changed to 64.
> >
> > The caller is also responsible for releasing the allocated memory by
> > calling v4l2_subdev_free_frame_desc().
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> (...)
> 
> > @@ -63,10 +63,6 @@ static bool v4l2_subdev_enable_streams_api;
> >  /*
> >   * Maximum stream ID is 63 for now, as we use u64 bitmask to represent a set
> >   * of streams.
> > - *
> > - * Note that V4L2_FRAME_DESC_ENTRY_MAX is related: V4L2_FRAME_DESC_ENTRY_MAX
> > - * restricts the total number of streams in a pad, although the stream ID is
> > - * not restricted.
> >   */
> >  #define V4L2_SUBDEV_MAX_STREAM_ID 63
> >
> > @@ -354,6 +350,7 @@ static int call_set_frame_interval(struct v4l2_subdev *sd,
> >  static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >                                struct v4l2_mbus_frame_desc *fd)
> >  {
> > +       unsigned int type;
> 
> This is again an enum, right?

Yes, seems so...

> 
> > @@ -362,16 +359,26 @@ static int call_get_frame_desc(struct v4l2_subdev *sd, unsigned int pad,
> >                 return -EOPNOTSUPP;
> >  #endif
> >
> > -       memset(fd, 0, sizeof(*fd));
> 
> So passing an unititialized or re-used struct v4l2_mbus_frame_desc foo
> used to be fine...
> 
> > +       type = fd->type;
> > +       memset_after(fd, 0, type);
> 
> ...and is not fine anymore.
> 
> I guess later patches in this series fixes up all users so no-one
> passes in some garbage here?

After the set, we have all drivers converted to use
v4l2_subdev_get_frame_desc(), so this means some amount of inter-set
breakage. I wouldn't see this as a serious issue but if someone thinks so,
then we may need to introduce an intermediate wrapper that sets the type
for this.

I'll also include patches for the recently merged drivers calling
get_frame_desc() in v4.

> 
> > +               if (desc->num_entries > desc->len_entries) {
> > +                       dev_dbg(sd->dev,
> 
> When you get to this check, isn't that after this loop:
> 
> for (i = 0; i < fd->num_entries; i++) { (...)
> 
> so you should check num_entries agains len_entries before this
> loop?
> 
> (I might be misreading the patch, maybe I should actually apply
> it and inspect the result.)

The earlier arrangement was that the caller made the allocation but that's
no longer the case, so this is a fatal error now. Still, if num_entries
exceeds len_entries, the access of unallocated memory has probably already
been committed, so this check would still take effect retrospectively even
if moved to call_get_frame_desc(). I'll do that now in any case, the other
sensible option would be just removing it.

-- 
Med trevliga hälsningar,

Sakari Ailus