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

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

[Arnd Bergmann]

From: Arnd Bergmann <arnd@arndb.de>

The function can break the stack frame warning limit in
some configurations:

drivers/media/i2c/maxim-serdes/max_des.c: In function 'max_des_update_streams.isra':
drivers/media/i2c/maxim-serdes/max_des.c:2550:1: error: the frame size of 1648 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
 2550 | }
      | ^

This is a simple large structure on the stack that shouldn't be
there. Move both instances into a dynamically allocated structure
instead.

Fixes: 22923bc4512c ("media: i2c: add Maxim GMSL2/3 deserializer framework")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/media/i2c/maxim-serdes/max_des.c | 28 ++++++++++++++----------
 1 file changed, 17 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
index 6b4ffd0e2415..4347ca24b5a1 100644
--- a/drivers/media/i2c/maxim-serdes/max_des.c
+++ b/drivers/media/i2c/maxim-serdes/max_des.c
@@ -2164,7 +2164,7 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
 					struct v4l2_mbus_frame_desc *fd,
 					unsigned int pad)
 {
-	struct max_des_remap_context context = { 0 };
+	struct max_des_remap_context __free(kfree) *context = kzalloc_obj(*context);
 	struct max_des_priv *priv = sd_to_priv(sd);
 	struct v4l2_subdev_route *route;
 	int ret;
@@ -2172,7 +2172,10 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
 	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
 	fd->num_entries = 0;
 
-	ret = max_des_populate_remap_context(priv, &context, state);
+	if (!context)
+		return -ENOMEM;
+
+	ret = max_des_populate_remap_context(priv, context, state);
 	if (ret)
 		return ret;
 
@@ -2187,7 +2190,7 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
 		if (ret)
 			return ret;
 
-		ret = max_des_get_src_dst_vc_id(&context, hw.pipe->index, hw.phy->index,
+		ret = max_des_get_src_dst_vc_id(context, hw.pipe->index, hw.phy->index,
 						hw.entry.bus.csi2.vc, &dst_vc_id);
 		if (ret)
 			return ret;
@@ -2450,18 +2453,21 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
 				  u32 pad, u64 updated_streams_mask, bool enable)
 {
 	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
-	struct max_des_remap_context context = { 0 };
+	struct max_des_remap_context *context __free(kfree) = kzalloc_obj(*context);
 	struct max_des_mode_context mode_context = { 0 };
 	struct max_des *des = priv->des;
 	unsigned int num_pads = max_des_num_pads(des);
 	u64 *streams_masks;
 	int ret;
 
-	ret = max_des_populate_remap_context(priv, &context, state);
+	if (!context)
+		return -ENOMEM;
+
+	ret = max_des_populate_remap_context(priv, context, state);
 	if (ret)
 		return ret;
 
-	ret = max_des_populate_mode_context(priv, &mode_context, state, context.mode);
+	ret = max_des_populate_mode_context(priv, &mode_context, state, context->mode);
 	if (ret)
 		return ret;
 
@@ -2471,11 +2477,11 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
 	if (ret)
 		return ret;
 
-	ret = max_des_set_pipes_phy(priv, &context);
+	ret = max_des_set_pipes_phy(priv, context);
 	if (ret)
 		goto err_free_streams_masks;
 
-	ret = max_des_set_tunnel(priv, &context);
+	ret = max_des_set_tunnel(priv, context);
 	if (ret)
 		goto err_free_streams_masks;
 
@@ -2483,7 +2489,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
 	if (ret)
 		goto err_free_streams_masks;
 
-	ret = max_des_set_vc_remaps(priv, &context, state, streams_masks);
+	ret = max_des_set_vc_remaps(priv, context, state, streams_masks);
 	if (ret)
 		goto err_free_streams_masks;
 
@@ -2502,7 +2508,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
 	if (ret)
 		goto err_revert_streams_disable;
 
-	ret = max_des_update_links(priv, &context, state, streams_masks);
+	ret = max_des_update_links(priv, context, state, streams_masks);
 	if (ret)
 		goto err_revert_active_disable;
 
@@ -2533,7 +2539,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
 	max_des_update_tpg(priv, state, priv->streams_masks);
 
 err_revert_links_update:
-	max_des_update_links(priv, &context, state, priv->streams_masks);
+	max_des_update_links(priv, context, state, priv->streams_masks);
 
 err_revert_active_disable:
 	max_des_update_active(priv, priv->streams_masks, true);
-- 
2.53.0

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

[Sakari Ailus]

Hi Arnd,

On Wed, Oct 14, 2026 at 12:31:33PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The function can break the stack frame warning limit in
> some configurations:
> 
> drivers/media/i2c/maxim-serdes/max_des.c: In function 'max_des_update_streams.isra':
> drivers/media/i2c/maxim-serdes/max_des.c:2550:1: error: the frame size of 1648 bytes is larger than 1536 bytes [-Werror=frame-larger-than=]
>  2550 | }
>       | ^
> 
> This is a simple large structure on the stack that shouldn't be
> there. Move both instances into a dynamically allocated structure
> instead.
> 
> Fixes: 22923bc4512c ("media: i2c: add Maxim GMSL2/3 deserializer framework")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/media/i2c/maxim-serdes/max_des.c | 28 ++++++++++++++----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/maxim-serdes/max_des.c b/drivers/media/i2c/maxim-serdes/max_des.c
> index 6b4ffd0e2415..4347ca24b5a1 100644
> --- a/drivers/media/i2c/maxim-serdes/max_des.c
> +++ b/drivers/media/i2c/maxim-serdes/max_des.c
> @@ -2164,7 +2164,7 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
>  					struct v4l2_mbus_frame_desc *fd,
>  					unsigned int pad)
>  {
> -	struct max_des_remap_context context = { 0 };
> +	struct max_des_remap_context __free(kfree) *context = kzalloc_obj(*context);
>  	struct max_des_priv *priv = sd_to_priv(sd);
>  	struct v4l2_subdev_route *route;
>  	int ret;
> @@ -2172,7 +2172,10 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
>  	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>  	fd->num_entries = 0;
>  
> -	ret = max_des_populate_remap_context(priv, &context, state);
> +	if (!context)
> +		return -ENOMEM;
> +
> +	ret = max_des_populate_remap_context(priv, context, state);
>  	if (ret)
>  		return ret;
>  
> @@ -2187,7 +2190,7 @@ static int max_des_get_frame_desc_state(struct v4l2_subdev *sd,
>  		if (ret)
>  			return ret;
>  
> -		ret = max_des_get_src_dst_vc_id(&context, hw.pipe->index, hw.phy->index,
> +		ret = max_des_get_src_dst_vc_id(context, hw.pipe->index, hw.phy->index,
>  						hw.entry.bus.csi2.vc, &dst_vc_id);
>  		if (ret)
>  			return ret;
> @@ -2450,18 +2453,21 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>  				  u32 pad, u64 updated_streams_mask, bool enable)
>  {
>  	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> -	struct max_des_remap_context context = { 0 };
> +	struct max_des_remap_context *context __free(kfree) = kzalloc_obj(*context);

max_des_update_streams() is used in implementing disable_streams()
sub-device pad op where meaningful error handling isn't possible.

It seems there are other ways it can fail but this adds one more.

The bulk of the struct consists of the vc_ids_map unsigned int array, but
the maximum value VCs can have is 31. Also the size of unsigned long
depends on the architecture, so a fixed-size type would be probably a
better choice.

Also using bit fields instead of bool would save space.

>  	struct max_des_mode_context mode_context = { 0 };
>  	struct max_des *des = priv->des;
>  	unsigned int num_pads = max_des_num_pads(des);
>  	u64 *streams_masks;
>  	int ret;
>  
> -	ret = max_des_populate_remap_context(priv, &context, state);
> +	if (!context)
> +		return -ENOMEM;
> +
> +	ret = max_des_populate_remap_context(priv, context, state);
>  	if (ret)
>  		return ret;
>  
> -	ret = max_des_populate_mode_context(priv, &mode_context, state, context.mode);
> +	ret = max_des_populate_mode_context(priv, &mode_context, state, context->mode);
>  	if (ret)
>  		return ret;
>  
> @@ -2471,11 +2477,11 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		return ret;
>  
> -	ret = max_des_set_pipes_phy(priv, &context);
> +	ret = max_des_set_pipes_phy(priv, context);
>  	if (ret)
>  		goto err_free_streams_masks;
>  
> -	ret = max_des_set_tunnel(priv, &context);
> +	ret = max_des_set_tunnel(priv, context);
>  	if (ret)
>  		goto err_free_streams_masks;
>  
> @@ -2483,7 +2489,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		goto err_free_streams_masks;
>  
> -	ret = max_des_set_vc_remaps(priv, &context, state, streams_masks);
> +	ret = max_des_set_vc_remaps(priv, context, state, streams_masks);
>  	if (ret)
>  		goto err_free_streams_masks;
>  
> @@ -2502,7 +2508,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>  	if (ret)
>  		goto err_revert_streams_disable;
>  
> -	ret = max_des_update_links(priv, &context, state, streams_masks);
> +	ret = max_des_update_links(priv, context, state, streams_masks);
>  	if (ret)
>  		goto err_revert_active_disable;
>  
> @@ -2533,7 +2539,7 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>  	max_des_update_tpg(priv, state, priv->streams_masks);
>  
>  err_revert_links_update:
> -	max_des_update_links(priv, &context, state, priv->streams_masks);
> +	max_des_update_links(priv, context, state, priv->streams_masks);
>  
>  err_revert_active_disable:
>  	max_des_update_active(priv, priv->streams_masks, true);
> 

-- 
Regards,

Sakari Ailus

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

[Arnd Bergmann]

On Wed, Oct 14, 2026, at 13:33, Sakari Ailus wrote:
> On Wed, Oct 14, 2026 at 12:31:33PM +0200, Arnd Bergmann wrote:
>>  			return ret;
>> @@ -2450,18 +2453,21 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
>>  				  u32 pad, u64 updated_streams_mask, bool enable)
>>  {
>>  	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
>> -	struct max_des_remap_context context = { 0 };
>> +	struct max_des_remap_context *context __free(kfree) = kzalloc_obj(*context);
>
> max_des_update_streams() is used in implementing disable_streams()
> sub-device pad op where meaningful error handling isn't possible.
>
> It seems there are other ways it can fail but this adds one more.

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?

> The bulk of the struct consists of the vc_ids_map unsigned int array, but
> the maximum value VCs can have is 31. Also the size of unsigned long
> depends on the architecture, so a fixed-size type would be probably a
> better choice.
>
> Also using bit fields instead of bool would save space.

That would likely bring it under the warning limit, but I wonder
if there is another way to avoid the stack allocation entirely.

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().

      Arnd

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

[Sakari Ailus]

Hi Arnd,

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:
> >>  			return ret;
> >> @@ -2450,18 +2453,21 @@ static int max_des_update_streams(struct v4l2_subdev *sd,
> >>  				  u32 pad, u64 updated_streams_mask, bool enable)
> >>  {
> >>  	struct max_des_priv *priv = v4l2_get_subdevdata(sd);
> >> -	struct max_des_remap_context context = { 0 };
> >> +	struct max_des_remap_context *context __free(kfree) = kzalloc_obj(*context);
> >
> > max_des_update_streams() is used in implementing disable_streams()
> > sub-device pad op where meaningful error handling isn't possible.
> >
> > It seems there are other ways it can fail but this adds one more.
> 
> 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.

> 
> > The bulk of the struct consists of the vc_ids_map unsigned int array, but
> > the maximum value VCs can have is 31. Also the size of unsigned long
> > depends on the architecture, so a fixed-size type would be probably a
> > better choice.
> >
> > Also using bit fields instead of bool would save space.
> 
> That would likely bring it under the warning limit, but I wonder
> if there is another way to avoid the stack allocation entirely.
> 
> 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.

-- 
Regards,

Sakari Ailus

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

[Sakari Ailus]

On Thu, Oct 15, 2026 at 12:40:07PM +0300, Sakari Ailus wrote:
> Also cc Dumitru.

Dumitru: could you update MAINTAINERS? Cosmin's e-mail is bouncing with a
permanent error but his e-mail is still listed for M: field for the
maxim-serdes driver.

-- 
Sakari Ailus

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

[Arnd Bergmann]

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 ;-)

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

      Arnd