Re: [PATCH] i2c: core: Allocate temporary client dynamically

[Su Hui <suhui@xxxxxxxxxxxx>]

On 2025/2/20 23:12, Geert Uytterhoeven wrote:
> drivers/i2c/i2c-core-base.c: In function ‘i2c_detect.isra’:
> drivers/i2c/i2c-core-base.c:2544:1: warning: the frame size of 1312 bytes is larger than 1024 bytes [-Wframe-larger-than=]
>   2544 | }
>        | ^
>
> Fix this by allocating the temporary client structure dynamically, as it
> is a rather large structure (1216 bytes, depending on kernel config).
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx>
> ---
> Mostly compile-tested, as apparently I have no I2C devices for which
> driver->detect and driver->address_list are valid.
>
> Apparently an alternative solution was posted before, but that does not
> fully address the potential stack size issue:
> "[PATCH] i2c: core: mark i2c_detect_address noinline_for_stack".
> https://lore.kernel.org/20250210080217.2772467-1-suhui@xxxxxxxxxxxx
> ---
>   drivers/i2c/i2c-core-base.c | 15 ++++++++++-----
>   1 file changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 35a221e2c11c1460..7ad1ad5c8c3f5694 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -2506,7 +2506,7 @@ static int i2c_detect_address(struct i2c_client *temp_client,
>   static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
>   {
>   	const unsigned short *address_list;
> -	struct i2c_client temp_client;
> +	struct i2c_client *temp_client;
>   	int i, err = 0;
>   
>   	address_list = driver->address_list;
> @@ -2527,19 +2527,24 @@ static int i2c_detect(struct i2c_adapter *adapter, struct i2c_driver *driver)
>   		return 0;
>   
>   	/* Set up a temporary client to help detect callback */
> -	memset(&temp_client, 0, sizeof(temp_client));
> -	temp_client.adapter = adapter;
> +	temp_client = kzalloc(sizeof(*temp_client), GFP_KERNEL);
> +	if (!temp_client)
> +		return -ENOMEM;
> +
> +	temp_client->adapter = adapter;
>   
>   	for (i = 0; address_list[i] != I2C_CLIENT_END; i += 1) {
>   		dev_dbg(&adapter->dev,
>   			"found normal entry for adapter %d, addr 0x%02x\n",
>   			i2c_adapter_id(adapter), address_list[i]);
> -		temp_client.addr = address_list[i];
> -		err = i2c_detect_address(&temp_client, driver);
> +		temp_client->addr = address_list[i];
> +		err = i2c_detect_address(temp_client, driver);
>   		if (unlikely(err))
>   			break;
>   	}
>   
> +	kfree(temp_client);
> +
>   	return err;
>   }

Looks good to me.

Reviewed-by: Su Hui <suhui@xxxxxxxxxxxx>

Su Hui