linux-kernel - Re: [PATCH] regulator: core: Only increment use_count when enable

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20231104140932.4946-1-zr.zhang@vivo.com>
Date:   Sat,  4 Nov 2023 22:09:32 +0800
From:   Rui Zhang <zr.zhang@...o.com>
To:     broonie@...nel.org
Cc:     dianders@...omium.org, lgirdwood@...il.com,
        linux-kernel@...r.kernel.org, opensource.kernel@...o.com,
        zr.zhang@...o.com
Subject: Re: [PATCH] regulator: core: Only increment use_count when enable_count changes

It was very great to get feedback from you.

On 2023/11/3 20:51, Mark Brown wrote:
> On Fri, Nov 03, 2023 at 03:42:31PM +0800, Rui Zhang wrote:
>
>> The use_count of a regulator should only be incremented when the
>> enable_count changes from 0 to 1. Similarly, the use_count should
>> only be decremented when the enable_count changes from 1 to 0.
> Why?

Sorry. It might be an inappropriate expression.
I think my tone should be softer.

I believe that tracking active consumers would be an useful approach.
In case of enable/disable interfaces, the framework could benefit from
considering whether the resource should be provided particularly when
there are active consumers involved. Enabling the resource when
there is an active consumer might be generally preferable.

That's why recording the number of active consumers throught use_count
could prvide an accurate reflection of the the resource needs at that time.

>
>> In the previous implementation, use_count was sometimes decremented
>> to 0 when some consumer called unbalanced disable,
>> leading to unexpected disable even the regulator is enabled by
>> other consumers. With this change, the use_count accurately reflects
>> the number of users which the regulator is enabled.
> If a consumer does an unbalanced disable the consumer is buggy and the
> reference counting is wrong overall.  The bug is in the consumer driver
> doing the unbalanced disable.

I completely agree with you that the buggy consumer is the root cause of
the problem.

It would be better if the regulator core can prevent the spreading of the
malfunction. We all hope that failures could be limited to a minimum range
and not result in a system-wide disaster.

Addtionally, this change could make it easier to identify the buggy consumer.

For example, there are Foo_A and Foo_B as consumers, both of whom have been
enabled at some point. If Foo_B disables twice reducing the use_count to 0.
(Regulator core will complain about underflow of enable_count.)
Then after a long time, Foo_A disables, the regulator core will complain
about the unbalanced disable. It seems that Foo_A might be the buggy consumer
even it is actually the victim.
However, we would need to ensure that logs from prior to this event from
Foo_B are available to accurately determine the root cause.