Re: [PATCH] mfd: twl-core: allocate modules with twl_private

[Lee Jones <lee@kernel.org>]

Please consider these reviews.

/* Sashiko Automation: Issues Found (3 Findings) */

On Mon, 29 Jun 2026, Rosen Penev wrote:

> Use a flexible array member to allocate the twl_client array together
> with struct twl_private. This removes a separate devm_kcalloc() and
> keeps the module data tied to the main private structure.
> 
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/mfd/twl-core.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/mfd/twl-core.c b/drivers/mfd/twl-core.c
> index c024a28b057e..a7754b7ccae0 100644
> --- a/drivers/mfd/twl-core.c
> +++ b/drivers/mfd/twl-core.c
> @@ -159,7 +159,7 @@ struct twl_private {
>  	unsigned int twl_id;
>  
>  	struct twl_mapping *twl_map;
> -	struct twl_client *twl_modules;
> +	struct twl_client twl_modules[];
>  };
>  
>  static struct twl_private *twl_priv;
> @@ -760,7 +760,8 @@ twl_probe(struct i2c_client *client)
>  		goto free;
>  	}
>  
> -	twl_priv = devm_kzalloc(&client->dev, sizeof(struct twl_private),
> +	num_slaves = twl_get_num_slaves();
> +	twl_priv = devm_kzalloc(&client->dev, struct_size(twl_priv, twl_modules, num_slaves),
>  				GFP_KERNEL);

[Severity: Critical]
Does evaluating num_slaves before twl_priv is allocated result in an
incorrect slave count for TWL4030 devices?

twl_get_num_slaves() calls twl_class_is_4030(), which checks
twl_priv->twl_id. Because twl_priv is NULL before this allocation, the check
fails and incorrectly defaults to 3 instead of 4.

During device removal, twl_get_num_slaves() will correctly evaluate to 4,
causing the removal loop to read and write out-of-bounds of this 3-element
flexible array.


>  	if (!twl_priv) {
>  		status = -ENOMEM;
> @@ -777,16 +778,6 @@ twl_probe(struct i2c_client *client)
>  		twl_regmap_config = twl4030_regmap_config;
>  	}
>  
> -	num_slaves = twl_get_num_slaves();
> -	twl_priv->twl_modules = devm_kcalloc(&client->dev,
> -					 num_slaves,
> -					 sizeof(struct twl_client),
> -					 GFP_KERNEL);
> -	if (!twl_priv->twl_modules) {
> -		status = -ENOMEM;
> -		goto free;
> -	}
> -
>  	for (i = 0; i < num_slaves; i++) {
>  		struct twl_client *twl = &twl_priv->twl_modules[i];

[Severity: High]
This is a pre-existing issue, but is there a missing cleanup step for the
global twl_priv pointer?

If twl_probe() fails or the driver is unbound, devres automatically frees
the twl_priv structure. However, the driver fails to set the global
twl_priv pointer back to NULL in twl_remove():

drivers/mfd/twl-core.c:twl_remove() {
    ...
    twl_priv->ready = false;
}

Subsequent attempts to bind the driver will hit the singleton check in
twl_probe():

    if (twl_priv) {
        dev_dbg(&client->dev, "only one instance of %s allowed\n",
            DRIVER_NAME);
        return -EBUSY;
    }

Can this leave a dangling pointer to devres-freed memory and permanently block
future probe attempts?




[Severity: High]
This isn't a bug introduced by this patch, but could this leave a dangling
function pointer on system shutdown?

During twl_probe(), pm_power_off is globally assigned to twl6030_power_off:

drivers/mfd/twl-core.c:twl_probe() {
    ...
    if (of_device_is_system_power_controller(node)) {
        if (!pm_power_off)
            pm_power_off = twl6030_power_off;
    ...
}

If the driver is later unbound or fails to probe after this assignment,
twl_remove() does not set pm_power_off back to NULL. 

When the system later shuts down, might it execute the dangling hook and
cause a kernel panic?

>  
> -- 
> 2.54.0
> 

-- 
Lee Jones