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

[Lee Jones]

Mark,

On Wed, 20 May 2026, Diogo Ivo wrote:
> 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/

It's my understanding that using the Regmap wrappers _prevents_ locking
issues, rather than causes them.

I'm deferring to Mark.

-- 
Lee Jones