atc260x has broken locking

atc260x has broken locking

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 1178 bytes --]

When looking at this functions I found the locking to be broken for the atomic 
case (comments stripped):

static void regmap_lock_mutex(void *__mutex)
{
	struct mutex *mutex = __mutex;

	if (system_state > SYSTEM_RUNNING && irqs_disabled())
		mutex_trylock(mutex);
	else
		mutex_lock(mutex);
}

static void regmap_unlock_mutex(void *__mutex)
{
	struct mutex *mutex = __mutex;

	mutex_unlock(mutex);
}

When the mutex is already locked and the atomic context is hit then the lock 
will not be acquired, this is never noticed, and it afterwards is unlocked 
anyway.

The comment says this is inspired from i2c_in_atomic_xfer_mode() to detect the 
atomic case, but the important caller __i2c_lock_bus_helper() actually does 
check and pass on the return value of mutex_trylock(), which is missing here.

Greetings,

Eike
-- 
Rolf Eike Beer, emlix GmbH, https://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]

Re: atc260x has broken locking

[Cristian Ciocaltea]

Hi Eike,

On Wed, Feb 23, 2022 at 12:07:48PM +0100, Rolf Eike Beer wrote:
> When looking at this functions I found the locking to be broken for the atomic 
> case (comments stripped):
> 
> static void regmap_lock_mutex(void *__mutex)
> {
> 	struct mutex *mutex = __mutex;
> 
> 	if (system_state > SYSTEM_RUNNING && irqs_disabled())
> 		mutex_trylock(mutex);
> 	else
> 		mutex_lock(mutex);
> }
> 
> static void regmap_unlock_mutex(void *__mutex)
> {
> 	struct mutex *mutex = __mutex;
> 
> 	mutex_unlock(mutex);
> }
> 
> When the mutex is already locked and the atomic context is hit then the lock 
> will not be acquired, this is never noticed, and it afterwards is unlocked 
> anyway.
> 
> The comment says this is inspired from i2c_in_atomic_xfer_mode() to detect the 
> atomic case, but the important caller __i2c_lock_bus_helper() actually does 
> check and pass on the return value of mutex_trylock(), which is missing here.

This is indeed a limitation of the current implementation because the
main goal was to offer initial support for SBC poweroff and reboot
use cases, which were the only cases where the atomic context kicks in.

Hence it was more important to make sure those operations are triggered
rather than failing due to an error condition which is hard to be
handled properly - e.g. getting a behaviour similar with the '-EGAIN'
scenario in the I2C implementation.

As a matter of fact the tests I made so far using a RoseapplePi board
didn't reveal any problems, but I will try to do some more extensive
testing and see if the issue becomes visible eventually. Then it would
be easier to try some possible solutions/workarounds.

Out of pure curiosity, on which hardware do you plan to use this, if you
haven't already?

Thanks,
Cristian

> Greetings,
> 
> Eike
> -- 
> Rolf Eike Beer, emlix GmbH, https://www.emlix.com
> Fon +49 551 30664-0, Fax +49 551 30664-11
> Gothaer Platz 3, 37083 Göttingen, Germany
> Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
> Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055
> 
> emlix - smart embedded open source

Re: atc260x has broken locking

[Rolf Eike Beer]

[-- Attachment #1: Type: text/plain, Size: 2361 bytes --]

Am Freitag, 25. Februar 2022, 00:14:03 CET schrieb Cristian Ciocaltea:
> Hi Eike,
> 
> On Wed, Feb 23, 2022 at 12:07:48PM +0100, Rolf Eike Beer wrote:
> > When looking at this functions I found the locking to be broken for the
> > atomic case (comments stripped):
> > 
> > static void regmap_lock_mutex(void *__mutex)
> > {
> > 
> > 	struct mutex *mutex = __mutex;
> > 	
> > 	if (system_state > SYSTEM_RUNNING && irqs_disabled())
> > 	
> > 		mutex_trylock(mutex);
> > 	
> > 	else
> > 	
> > 		mutex_lock(mutex);
> > 
> > }
> > 
> > static void regmap_unlock_mutex(void *__mutex)
> > {
> > 
> > 	struct mutex *mutex = __mutex;
> > 	
> > 	mutex_unlock(mutex);
> > 
> > }
> > 
> > When the mutex is already locked and the atomic context is hit then the
> > lock will not be acquired, this is never noticed, and it afterwards is
> > unlocked anyway.
> > 
> > The comment says this is inspired from i2c_in_atomic_xfer_mode() to detect
> > the atomic case, but the important caller __i2c_lock_bus_helper()
> > actually does check and pass on the return value of mutex_trylock(),
> > which is missing here.
> This is indeed a limitation of the current implementation because the
> main goal was to offer initial support for SBC poweroff and reboot
> use cases, which were the only cases where the atomic context kicks in.
> 
> Hence it was more important to make sure those operations are triggered
> rather than failing due to an error condition which is hard to be
> handled properly - e.g. getting a behaviour similar with the '-EGAIN'
> scenario in the I2C implementation.

Which makes sense as the unlock is in fact never reached then I guess because 
the system reboots or shuts down before. Maybe this should end up as comment 
somewhere in the code.

> Out of pure curiosity, on which hardware do you plan to use this, if you
> haven't already?

On no hardware. I annotated mutex_trylock() as __must_check and this was the 
only place that broke the build afterwards.

Eike
-- 
Rolf Eike Beer, emlix GmbH, https://www.emlix.com
Fon +49 551 30664-0, Fax +49 551 30664-11
Gothaer Platz 3, 37083 Göttingen, Germany
Sitz der Gesellschaft: Göttingen, Amtsgericht Göttingen HR B 3160
Geschäftsführung: Heike Jordan, Dr. Uwe Kracke – Ust-IdNr.: DE 205 198 055

emlix - smart embedded open source

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 313 bytes --]