Re: [PATCH v2 3/3 RESEND] mfd: max8907: Convert to use device managed poweroff handler

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] mfd: max8907: regmap mutex deadlock and sleep in atomic power-off
- [Low] mfd: max8907: missing <linux/property.h> for device_property_read_bool()
- [Low] mfd: max8907: misleading error message in power-off handler
--

commit 5506b550cb60d6cb6be8b862f1268bdb1e810f08
Author: Svyatoslav Ryhel <clamor95@gmail.com>

mfd: max8907: Convert to use device managed poweroff handler

Replace the legacy global 'pm_power_off' function pointer assignment with
the devm_register_sys_off_handler() helper.

> diff --git a/drivers/mfd/max8907.c b/drivers/mfd/max8907.c
> index 363ac80c0593b..9c40f0551a8ce 100644
> --- a/drivers/mfd/max8907.c
> +++ b/drivers/mfd/max8907.c
[ ... ]
> -static struct max8907 *max8907_pm_off;
> -static void max8907_power_off(void)
> +static int max8907_power_off(struct sys_off_data *data)
>  {
> -	regmap_update_bits(max8907_pm_off->regmap_gen, MAX8907_REG_RESET_CNFG,
> -			MAX8907_MASK_POWER_OFF, MAX8907_MASK_POWER_OFF);
> +	struct max8907 *max8907 = data->cb_data;
> +	int ret;
> +
> +	ret = regmap_update_bits(max8907->regmap_gen, MAX8907_REG_RESET_CNFG,
> +				 MAX8907_MASK_POWER_OFF, MAX8907_MASK_POWER_OFF);

[Severity: High]
Does this code risk a deadlock or sleep in an atomic context?

The handler max8907_power_off() is registered with SYS_OFF_MODE_POWER_OFF.
This means it will be called via an atomic notifier chain during system
shutdown, after machine_power_off() stops CPUs and disables interrupts.
However, regmap_update_bits() acquires a mutex and invokes I2C transfers
that can sleep.

Should this use SYS_OFF_MODE_POWER_OFF_PREPARE instead? That mode uses a
blocking notifier chain and runs in process context before CPUs are stopped.

> +	if (ret)
> +		dev_err(max8907->dev, "failed to restart device!\n");

[Severity: Low]
Is this error message accurate?

This error path logs a failure to "restart device", but max8907_power_off()
is explicitly used for powering off the device, not restarting it.

> +
> +	return NOTIFY_DONE;
>  }
>  
>  static int max8907_i2c_probe(struct i2c_client *i2c)
>  {
>  	struct max8907 *max8907;
>  	int ret;
[ ... ]
> @@ -204,6 +209,16 @@ static int max8907_i2c_probe(struct i2c_client *i2c)
>  		goto err_regmap_gen;
>  	}
>  
> +	if (device_property_read_bool(&i2c->dev, "system-power-controller") ||
> +	    device_property_read_bool(&i2c->dev, "maxim,system-power-controller")) {

[Severity: Low]
Could this lead to a build failure due to a missing header?

The patch replaces of_property_read_bool() with device_property_read_bool()
in max8907_i2c_probe(). The new function is defined in <linux/property.h>,
but that header is not explicitly included in this file. Relying on implicit
transitive headers might break builds on some architectures or
configurations.

> +		ret = devm_register_sys_off_handler(&i2c->dev, SYS_OFF_MODE_POWER_OFF,
> +						    SYS_OFF_PRIO_DEFAULT,
> +						    &max8907_power_off, max8907);
> +		if (ret)
> +			return dev_err_probe(&i2c->dev, ret,
> +					     "failed to register poweroff handler\n");
> +	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014121544.138958-1-clamor95@gmail.com?part=3