Re: [PATCH] mfd: max77620: Avoid regmap mutex deadlock in power-off handler

[Diogo Ivo]

On 5/20/26 16:42, Dmitry Osipenko wrote:
> 20.05.2026 17:28, Diogo Ivo пишет:
> > max77620_pm_power_off() is called via the sys-off framework as a
> > SYS_OFF_MODE_POWER_OFF handler, which runs in an atomic notifier chain
> > with IRQs disabled after smp_send_stop(). regmap_update_bits() acquires
> > the regmap mutex in this path; if another CPU held that mutex when it
> > was stopped, the power-off sequence deadlocks.
> >
> > Replace regmap_update_bits() with i2c_smbus_write_byte_data(), which
> > bypasses the regmap lock entirely. The I2C core detects the atomic
> > context via i2c_in_atomic_xfer_mode() and uses i2c_trylock_bus() rather
> > than a blocking acquisition, avoiding the deadlock.
> >
> > Tested on Pixel C, powers off correctly.
> >
> > Assisted-by: Claude:claude-sonnet-4-6
> > Fixes: 744b13107d0d ("mfd: max77620: Provide system power-off functionality")
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Diogo Ivo <diogo.ivo@xxxxxxxxxxxxxxxxxx>
> > ---
> > This patch was tested on a local branch that sets pm_power_off =
> > max77620_pm_power_off() unconditionally so that the function runs.
> > I haven't checked whether the other bits in ONOFFCNFG1 are safe to
> > discard at power-off time as I don't have access to the datasheet.
> > If someone with access to the datasheet confirms they're not I'll
> > respin the patch taking that into account.
> > ---
> >   drivers/mfd/max77620.c | 10 +++++++---
> >   1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mfd/max77620.c b/drivers/mfd/max77620.c
> > index 3af2974b3023..8c768968a317 100644
> > --- a/drivers/mfd/max77620.c
> > +++ b/drivers/mfd/max77620.c
> > @@ -487,10 +487,14 @@ static int max77620_read_es_version(struct max77620_chip *chip)
> >   static void max77620_pm_power_off(void)
> >   {
> >   	struct max77620_chip *chip = max77620_scratch;
> > +	struct i2c_client *client = to_i2c_client(chip->dev);
> >   
> > -	regmap_update_bits(chip->rmap, MAX77620_REG_ONOFFCNFG1,
> > -			   MAX77620_ONOFFCNFG1_SFT_RST,
> > -			   MAX77620_ONOFFCNFG1_SFT_RST);
> > +	/*
> > +	 * Atomic context: IRQs disabled. Use raw I2C write, bypassing
> > +	 * regmap locking entirely.
> > +	 */
> > +	i2c_smbus_write_byte_data(client, MAX77620_REG_ONOFFCNFG1,
> > +				  MAX77620_ONOFFCNFG1_SFT_RST);
> >   }
> >   
> >   static int max77620_probe(struct i2c_client *client)
> >
> > ---
> > base-commit: 27fa82620cbaa89a7fc11ac3057701d598813e87
> > change-id: 20260520-max77620_poweroff-08e39429835f
> >
> > Best regards,
> > --
> > Diogo Ivo <diogo.ivo@xxxxxxxxxxxxxxxxxx>
>
> Kernel parks secondary CPUs before powering off system, hence there
> shouldn't be a locking contention.

This patch was motivated by the Sashiko review I got in [1]. Its point
here is that there is a possibility for a deadlock scenario in which
a secondary CPU obtains the mutex for the regmap and then smp_send_stop()
is called before this secondary CPU gets a chance to release the mutex,
making it so that when the primary CPU tries to acquire it to issue the
write it hangs. Is there something that I am misunderstanding here?

> Have you checked whether regmap_write_bits() works?

Now, in case this is all true this problem is still not something that
will usually happen, only when this specific situation holds so
generally even regmap_update_bits() was working, and in [1] I sent it
out exactly like that. Changing it to regmap_write_bits() would not make
any difference.

[1]: 
https://lore.kernel.org/linux-tegra/20260514-smaug-poweroff-v1-0-30f9a4688966@xxxxxxxxxxxxxxxxxx/