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@vger.kernel.org
Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
---
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@tecnico.ulisboa.pt>

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@vger.kernel.org
> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> ---
> 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@tecnico.ulisboa.pt>

Kernel parks secondary CPUs before powering off system, hence there
shouldn't be a locking contention.

Have you checked whether regmap_write_bits() works?

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@vger.kernel.org
>> Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
>> ---
>> 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@tecnico.ulisboa.pt>
> 
> 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@tecnico.ulisboa.pt/

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@vger.kernel.org
> > > Signed-off-by: Diogo Ivo <diogo.ivo@tecnico.ulisboa.pt>
> > > ---
> > > 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@tecnico.ulisboa.pt>
> > 
> > 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@tecnico.ulisboa.pt/

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

I'm deferring to Mark.

-- 
Lee Jones

On Wed, May 20, 2026 at 05:19:00PM +0100, Lee Jones wrote:
> On Wed, 20 May 2026, Diogo Ivo wrote:

> > 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?
> > 

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

In the case where the CPU is being powered off during a regmap write
there is a potential issue - as Diogo says if we're in the middle of
holding the lock and we power off the CPU that owns the lock then it
will never be able to release the lock.  I would expect the same issue
to apply to a bus like I2C or SPI though, they'll hold a lock while
they're in the middle of doing bus I/O unless you use some special API.

On 5/20/26 18:23, Mark Brown wrote:
> On Wed, May 20, 2026 at 05:19:00PM +0100, Lee Jones wrote:
>> On Wed, 20 May 2026, Diogo Ivo wrote:
> 
>>> 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?
>>>
> 
>> It's my understanding that using the Regmap wrappers _prevents_ locking
>> issues, rather than causes them.
> 
> In the case where the CPU is being powered off during a regmap write
> there is a potential issue - as Diogo says if we're in the middle of
> holding the lock and we power off the CPU that owns the lock then it
> will never be able to release the lock.  I would expect the same issue
> to apply to a bus like I2C or SPI though, they'll hold a lock while
> they're in the middle of doing bus I/O unless you use some special API.

In that case can we call __i2c_smbus_xfer() directy from
max77620_pm_power_off()? Or is that unsafe because it can interfere with
an ongoing transfer? In essence my question is what can we do about
this, Sashiko suggested to move the handler to 
SYS_OFF_MODE_POWER_OFF_PREPARE
but I believe that that would/could be too soon.

Best regards,
Diogo

20.05.2026 19:23, Mark Brown пишет:
> On Wed, May 20, 2026 at 05:19:00PM +0100, Lee Jones wrote:
>> On Wed, 20 May 2026, Diogo Ivo wrote:
> 
>>> 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?
>>>
> 
>> It's my understanding that using the Regmap wrappers _prevents_ locking
>> issues, rather than causes them.
> 
> In the case where the CPU is being powered off during a regmap write
> there is a potential issue - as Diogo says if we're in the middle of
> holding the lock and we power off the CPU that owns the lock then it
> will never be able to release the lock.  I would expect the same issue
> to apply to a bus like I2C or SPI though, they'll hold a lock while
> they're in the middle of doing bus I/O unless you use some special API.

Sounds bad

Diogo, check if shutdown works with added nosmp to kernel's cmdline.

BTW, you can use i2c_smbus_read_byte_data+i2c_smbus_write_byte_data to
keep the old regmap_update_bits behaviour.

On 5/20/26 18:44, Dmitry Osipenko wrote:
> 20.05.2026 19:23, Mark Brown пишет:
>> On Wed, May 20, 2026 at 05:19:00PM +0100, Lee Jones wrote:
>>> On Wed, 20 May 2026, Diogo Ivo wrote:
>>
>>>> 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?
>>>>
>>
>>> It's my understanding that using the Regmap wrappers _prevents_ locking
>>> issues, rather than causes them.
>>
>> In the case where the CPU is being powered off during a regmap write
>> there is a potential issue - as Diogo says if we're in the middle of
>> holding the lock and we power off the CPU that owns the lock then it
>> will never be able to release the lock.  I would expect the same issue
>> to apply to a bus like I2C or SPI though, they'll hold a lock while
>> they're in the middle of doing bus I/O unless you use some special API.
> 
> Sounds bad
> 
> Diogo, check if shutdown works with added nosmp to kernel's cmdline.

So to be clear shutdown already works with regmap_update_bits() and I
have never encountered this deadlock in my testing as the write to power
off the PMIC needs to happen at a very specific timing. I imagine adding
nosmp will just guarantee that the deadlock can never happen.

> BTW, you can use i2c_smbus_read_byte_data+i2c_smbus_write_byte_data to
> keep the old regmap_update_bits behaviour.

My question here is more if this is actually needed or we can skip the
read. In any case the patch that Lee merged is with regmap_update_bits()
so for the time being this is not a problem.

Best regards,
Diogo