Re: [PATCH] media: i2c: maxim-serdes: reduce max_des_update_streams() stack usage

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

Hi Arnd,

On Thu, Oct 15, 2026 at 12:06:28PM +0200, Arnd Bergmann wrote:
> On Thu, Oct 15, 2026, at 11:40, Sakari Ailus wrote:
> > On Thu, Oct 15, 2026 at 08:45:26AM +0200, Arnd Bergmann wrote:
> >> On Wed, Oct 14, 2026, at 13:33, Sakari Ailus wrote:
> >> > On Wed, Oct 14, 2026 at 12:31:33PM +0200, Arnd Bergmann wrote:
> >> Right. I see there are more allocations in max_des_update_pipe_remaps(),
> >> and max_des_update_pipe_vc_remaps() called through max_des_update_links()
> >> that could fail as well, and may have the additional problem of
> >> leaking allocations every time a stream is enabled and disabled.
> >> 
> >> Can you tell if those should be fixed as well?
> >
> > There seem to be devm_*alloc*() functions being used but that doesn't look
> > like a memory leak -- there's devm_kfree() use as well for the same
> > pointers.
> 
> I see. Yes, that is a bit unusual but not a bug.
> 
> Seeing that the max_serdes_vc_remap array is only eight bytes long, the
> devm_kzalloc() does seem like a very heavyweight operation here, that one
> can just go on the stack and into struct max_des_pipe in place of
> the pointer ;-)

Indeed.

> 
> >> Are ->enable_streams() and ->disable_streams() always serialized
> >> or can they be called concurrently on the same subdev? If there
> >> is only one of them at a time, the allocation could just be
> >> done ahead of time in max_des_allocate().
> >
> > Access to sub-device state which these ops operate on is serialised using
> > the sub-device state mutex i.e. these aren't called concurrently (on the
> > same sub-device).
> >
> > Also cc Dumitru.
> 
> Ok, so maybe the easiest way would be to just put everything into 
> max_des_priv, including all the currently dynamically allocated
> arrays. Since all the maximum sizes are 4 or 8 in the driver
> and most of the structures are short, using fixed-length arrays
> will make it use less memory than the minimum of two cache lines
> per devres object.

We're also trying to remove state outside sub-device state. I guess this is
still driver internal and not visible in the try states so it's not
strictly mandatory yet but still preferred.

I haven't fully read the code so I don't know if it'd be feasible to use
the information gathered directly rather than through an intermediate data
structure that is now the case.

I'd try smaller types now to see if the data structure would fit nicely
into the stack and leave the rest for later.

-- 
Regards,

Sakari Ailus