Re: [PATCH v2 05/13] power-domain: Add refcounting

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hello Simon,

On 05/12/2024 at 10:56:44 -07, Simon Glass <sjg@chromium.org> wrote:

> Hi Miquel,
>
> On Thu, 5 Dec 2024 at 06:54, Miquel Raynal <miquel.raynal@bootlin.com> wrote:
>>
>> It is very surprising that such an uclass, specifically designed to
>> handle resources that may be shared by different devices, is not keeping
>> the count of the number of times a power domain has been
>> enabled/disabled to avoid shutting it down unexpectedly or disabling it
>> several times.
>>
>> Doing this causes troubles on eg. i.MX8MP because disabling power
>> domains can be done in a recursive loops were the same power domain
>> disabled up to 4 times in a row. PGCs seem to have tight FSM internal
>> timings to respect and it is easy to produce a race condition that puts
>> the power domains in an unstable state, leading to ADB400 errors and
>> later crashes in Linux.
>>
>> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
>> ---
>>  drivers/power/domain/power-domain-uclass.c | 37 ++++++++++++++++++++++++++++--
>>  include/power-domain.h                     |  4 ++--
>>  2 files changed, 37 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/power/domain/power-domain-uclass.c b/drivers/power/domain/power-domain-uclass.c
>> index 938bd8cbc9ffd1ba2109d702f886b6a99288d063..55a3d95d83c5aaac31acbd877199c87f8f6fb880 100644
>> --- a/drivers/power/domain/power-domain-uclass.c
>> +++ b/drivers/power/domain/power-domain-uclass.c
>> @@ -12,6 +12,10 @@
>>  #include <power-domain-uclass.h>
>>  #include <dm/device-internal.h>
>>
>> +struct power_domain_priv {
>> +       int on_count;
>> +};
>> +
>>  static inline struct power_domain_ops *power_domain_dev_ops(struct udevice *dev)
>>  {
>>         return (struct power_domain_ops *)dev->driver->ops;
>> @@ -110,19 +114,47 @@ int power_domain_free(struct power_domain *power_domain)
>>  int power_domain_on(struct power_domain *power_domain)
>>  {
>>         struct power_domain_ops *ops = power_domain_dev_ops(power_domain->dev);
>> +       struct power_domain_priv *priv = dev_get_uclass_priv(power_domain->dev);
>> +       int ret;
>>
>>         debug("%s(power_domain=%p)\n", __func__, power_domain);
>>
>> -       return ops->on ? ops->on(power_domain) : 0;
>> +       if (priv->on_count++ > 0)
>> +               return 0;
>
> -EALREADY - see drivers/power/regulator/regulator-uclass.c

Actually I don't see the point in returning this? What is the purpose?
Users should not care nor know about this, it is all internal to the
uclass, we do _not_ want users to mess with this.

If someone ever needs it, they can add a layer of "hiding" for all users
but them, but as said just above, I really think it is a bad idea to
propagate this outside of the uclass.

(same in the _off path).

Thanks,
Miquèl