pmdomain: mediatek: fix race condition in power on/power off sequences

  It can happen that during the power off sequence for a power domain
another power on sequence is started, and it can lead to powering on and
off in the same time for the similar power domain.
This can happen if parallel probing occurs: one device starts probing, and
one power domain is probe deferred, this leads to all power domains being
rolled back and powered off, while in the same time another device starts
probing and requests powering on the same power domains or similar.

This was encountered on MT8186, when the sequence is :
Power on SSUSB
Power on SSUSB_P1
Power on DIS
	-> probe deferred
Power off DIS
Power off SSUSB_P1
Power off SSUSB

During the sequence of powering off SSUSB, some new similar sequence starts,
and during the power on of SSUSB, clocks are enabled.
In this case, powering off SSUSB fails from the first sequence, because
power off ACK bit check times out (as clocks are powered back on by the second
sequence). In consequence, powering it on also times out, and it leads to
the whole power domain in a bad state.

To solve this issue, added a mutex that locks the whole power off/power on
sequence such that it would never happen that multiple sequences try to
enable or disable the same power domain in parallel.

Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
---
 drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)
  

Il 29/11/23 12:31, Eugen Hristev ha scritto:
> It can happen that during the power off sequence for a power domain
> another power on sequence is started, and it can lead to powering on and
> off in the same time for the similar power domain.
> This can happen if parallel probing occurs: one device starts probing, and
> one power domain is probe deferred, this leads to all power domains being
> rolled back and powered off, while in the same time another device starts
> probing and requests powering on the same power domains or similar.
> 
> This was encountered on MT8186, when the sequence is :
> Power on SSUSB
> Power on SSUSB_P1
> Power on DIS
> 	-> probe deferred
> Power off DIS
> Power off SSUSB_P1
> Power off SSUSB
> 
> During the sequence of powering off SSUSB, some new similar sequence starts,
> and during the power on of SSUSB, clocks are enabled.
> In this case, powering off SSUSB fails from the first sequence, because
> power off ACK bit check times out (as clocks are powered back on by the second
> sequence). In consequence, powering it on also times out, and it leads to
> the whole power domain in a bad state.
> 
> To solve this issue, added a mutex that locks the whole power off/power on
> sequence such that it would never happen that multiple sequences try to
> enable or disable the same power domain in parallel.
> 
> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>

I don't think that it's a race between genpd_power_on() and genpd_power_off() calls
at all, because genpd *does* have locking after all... at least for probe and for
parents of a power domain (and more anyway).

As far as I remember, what happens when you start .probe()'ing a device is:
platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()

There, you end up with

	if (power_on) {
		genpd_lock(pd);
		ret = genpd_power_on(pd, 0);
		genpd_unlock(pd);
	}

...but when you fail probing, you go with genpd_dev_pm_detach(), which then calls

	/* Check if PM domain can be powered off after removing this device. */
	genpd_queue_power_off_work(pd);

but even then, you end up being in a worker doing

	genpd_lock(genpd);
	genpd_power_off(genpd, false, 0);
	genpd_unlock(genpd);

...so I don't understand why this mutex can resolve the situation here (also: are
you really sure that the race is solved like that?)

I'd say that this probably needs more justification and a trace of the actual
situation here.

Besides, if this really resolves the issue, I would prefer seeing variants of
scpsys_power_{on,off}() functions, because we anyway don't need to lock mutexes
during this driver's probe (add_subdomain calls scpsys_power_on()).
In that case, `scpsys_power_on_unlocked()` would be an idea... but still, please
analyze why your solution works, if it does, because I'm not convinced.

Cheers,
Angelo

> ---
>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
>   1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> index d5f0ee05c794..4f136b47e539 100644
> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
> @@ -9,6 +9,7 @@
>   #include <linux/io.h>
>   #include <linux/iopoll.h>
>   #include <linux/mfd/syscon.h>
> +#include <linux/mutex.h>
>   #include <linux/of.h>
>   #include <linux/of_clk.h>
>   #include <linux/platform_device.h>
> @@ -56,6 +57,7 @@ struct scpsys {
>   	struct device *dev;
>   	struct regmap *base;
>   	const struct scpsys_soc_data *soc_data;
> +	struct mutex mutex;
>   	struct genpd_onecell_data pd_data;
>   	struct generic_pm_domain *domains[];
>   };
> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>   	bool tmp;
>   	int ret;
>   
> +	mutex_lock(&scpsys->mutex);
> +
>   	ret = scpsys_regulator_enable(pd->supply);
> -	if (ret)
> +	if (ret) {
> +		mutex_unlock(&scpsys->mutex);
>   		return ret;
> +	}
>   
>   	ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>   	if (ret)
> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>   			goto err_enable_bus_protect;
>   	}
>   
> +	mutex_unlock(&scpsys->mutex);
>   	return 0;
>   
>   err_enable_bus_protect:
> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>   	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>   err_reg:
>   	scpsys_regulator_disable(pd->supply);
> +	mutex_unlock(&scpsys->mutex);
>   	return ret;
>   }
>   
> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>   	bool tmp;
>   	int ret;
>   
> +	mutex_lock(&scpsys->mutex);
> +
>   	ret = scpsys_bus_protect_enable(pd);
>   	if (ret < 0)
> -		return ret;
> +		goto err_mutex_unlock;
>   
>   	ret = scpsys_sram_disable(pd);
>   	if (ret < 0)
> -		return ret;
> +		goto err_mutex_unlock;
>   
>   	if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, MTK_SCPD_EXT_BUCK_ISO))
>   		regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>   	ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, MTK_POLL_DELAY_US,
>   				 MTK_POLL_TIMEOUT);
>   	if (ret < 0)
> -		return ret;
> +		goto err_mutex_unlock;
>   
>   	clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>   
>   	scpsys_regulator_disable(pd->supply);
>   
> -	return 0;
> +err_mutex_unlock:
> +	mutex_unlock(&scpsys->mutex);
> +	return ret;
>   }
>   
>   static struct
> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device *pdev)
>   		return PTR_ERR(scpsys->base);
>   	}
>   
> +	mutex_init(&scpsys->mutex);
> +
>   	ret = -ENODEV;
>   	for_each_available_child_of_node(np, node) {
>   		struct generic_pm_domain *domain;
  

On 11/29/23 14:52, AngeloGioacchino Del Regno wrote:
> Il 29/11/23 12:31, Eugen Hristev ha scritto:
>> It can happen that during the power off sequence for a power domain
>> another power on sequence is started, and it can lead to powering on and
>> off in the same time for the similar power domain.
>> This can happen if parallel probing occurs: one device starts probing, 
>> and
>> one power domain is probe deferred, this leads to all power domains being
>> rolled back and powered off, while in the same time another device starts
>> probing and requests powering on the same power domains or similar.
>>
>> This was encountered on MT8186, when the sequence is :
>> Power on SSUSB
>> Power on SSUSB_P1
>> Power on DIS
>>     -> probe deferred
>> Power off DIS
>> Power off SSUSB_P1
>> Power off SSUSB
>>
>> During the sequence of powering off SSUSB, some new similar sequence 
>> starts,
>> and during the power on of SSUSB, clocks are enabled.
>> In this case, powering off SSUSB fails from the first sequence, because
>> power off ACK bit check times out (as clocks are powered back on by 
>> the second
>> sequence). In consequence, powering it on also times out, and it leads to
>> the whole power domain in a bad state.
>>
>> To solve this issue, added a mutex that locks the whole power 
>> off/power on
>> sequence such that it would never happen that multiple sequences try to
>> enable or disable the same power domain in parallel.
>>
>> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
> 
> I don't think that it's a race between genpd_power_on() and 
> genpd_power_off() calls
> at all, because genpd *does* have locking after all... at least for 
> probe and for
> parents of a power domain (and more anyway).
> 
> As far as I remember, what happens when you start .probe()'ing a device is:
> platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()
> 
> There, you end up with
> 
>      if (power_on) {
>          genpd_lock(pd);
>          ret = genpd_power_on(pd, 0);
>          genpd_unlock(pd);
>      }
> 
> ...but when you fail probing, you go with genpd_dev_pm_detach(), which 
> then calls
> 
>      /* Check if PM domain can be powered off after removing this 
> device. */
>      genpd_queue_power_off_work(pd);
> 
> but even then, you end up being in a worker doing
> 
>      genpd_lock(genpd);
>      genpd_power_off(genpd, false, 0);
>      genpd_unlock(genpd);
> 
> ...so I don't understand why this mutex can resolve the situation here 
> (also: are
> you really sure that the race is solved like that?)
> 
> I'd say that this probably needs more justification and a trace of the 
> actual
> situation here.
> 
> Besides, if this really resolves the issue, I would prefer seeing 
> variants of
> scpsys_power_{on,off}() functions, because we anyway don't need to lock 
> mutexes
> during this driver's probe (add_subdomain calls scpsys_power_on()).
> In that case, `scpsys_power_on_unlocked()` would be an idea... but 
> still, please
> analyze why your solution works, if it does, because I'm not convinced.

What I see in my tests, is that a power on call for SSUSB domain happens 
while the previous power off sequence did not yet complete, most likely 
while it's waiting in readx_poll_timeout . This leads to inconsistency 
of the power domain, not getting the ACKs next time a power on attempt 
occurs.

I understand what you say about locks, but in this case the powering off 
is not called by the genpd itself, but rather it's called by the 
rollback probe failed mechanism : when the probing fails, 
scpsys_domain_cleanup() is called during the same probing session.
Then it happens that probing begins again and previous cleanup is not 
yet completed. I am not sure whether the lock is still held from the 
previous run, but it's clearly not waiting for a lock to be released to 
be called again.

> 
> Cheers,
> Angelo
> 
>> ---
>>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c 
>> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> index d5f0ee05c794..4f136b47e539 100644
>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/io.h>
>>   #include <linux/iopoll.h>
>>   #include <linux/mfd/syscon.h>
>> +#include <linux/mutex.h>
>>   #include <linux/of.h>
>>   #include <linux/of_clk.h>
>>   #include <linux/platform_device.h>
>> @@ -56,6 +57,7 @@ struct scpsys {
>>       struct device *dev;
>>       struct regmap *base;
>>       const struct scpsys_soc_data *soc_data;
>> +    struct mutex mutex;
>>       struct genpd_onecell_data pd_data;
>>       struct generic_pm_domain *domains[];
>>   };
>> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct 
>> generic_pm_domain *genpd)
>>       bool tmp;
>>       int ret;
>> +    mutex_lock(&scpsys->mutex);
>> +
>>       ret = scpsys_regulator_enable(pd->supply);
>> -    if (ret)
>> +    if (ret) {
>> +        mutex_unlock(&scpsys->mutex);
>>           return ret;
>> +    }
>>       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>>       if (ret)
>> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct 
>> generic_pm_domain *genpd)
>>               goto err_enable_bus_protect;
>>       }
>> +    mutex_unlock(&scpsys->mutex);
>>       return 0;
>>   err_enable_bus_protect:
>> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct 
>> generic_pm_domain *genpd)
>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>   err_reg:
>>       scpsys_regulator_disable(pd->supply);
>> +    mutex_unlock(&scpsys->mutex);
>>       return ret;
>>   }
>> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct 
>> generic_pm_domain *genpd)
>>       bool tmp;
>>       int ret;
>> +    mutex_lock(&scpsys->mutex);
>> +
>>       ret = scpsys_bus_protect_enable(pd);
>>       if (ret < 0)
>> -        return ret;
>> +        goto err_mutex_unlock;
>>       ret = scpsys_sram_disable(pd);
>>       if (ret < 0)
>> -        return ret;
>> +        goto err_mutex_unlock;
>>       if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, 
>> MTK_SCPD_EXT_BUCK_ISO))
>>           regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
>> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct 
>> generic_pm_domain *genpd)
>>       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, 
>> MTK_POLL_DELAY_US,
>>                    MTK_POLL_TIMEOUT);
>>       if (ret < 0)
>> -        return ret;
>> +        goto err_mutex_unlock;
>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>       scpsys_regulator_disable(pd->supply);
>> -    return 0;
>> +err_mutex_unlock:
>> +    mutex_unlock(&scpsys->mutex);
>> +    return ret;
>>   }
>>   static struct
>> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device *pdev)
>>           return PTR_ERR(scpsys->base);
>>       }
>> +    mutex_init(&scpsys->mutex);
>> +
>>       ret = -ENODEV;
>>       for_each_available_child_of_node(np, node) {
>>           struct generic_pm_domain *domain;
> 
>
  

Il 29/11/23 14:28, Eugen Hristev ha scritto:
> On 11/29/23 14:52, AngeloGioacchino Del Regno wrote:
>> Il 29/11/23 12:31, Eugen Hristev ha scritto:
>>> It can happen that during the power off sequence for a power domain
>>> another power on sequence is started, and it can lead to powering on and
>>> off in the same time for the similar power domain.
>>> This can happen if parallel probing occurs: one device starts probing, and
>>> one power domain is probe deferred, this leads to all power domains being
>>> rolled back and powered off, while in the same time another device starts
>>> probing and requests powering on the same power domains or similar.
>>>
>>> This was encountered on MT8186, when the sequence is :
>>> Power on SSUSB
>>> Power on SSUSB_P1
>>> Power on DIS
>>>     -> probe deferred
>>> Power off DIS
>>> Power off SSUSB_P1
>>> Power off SSUSB
>>>
>>> During the sequence of powering off SSUSB, some new similar sequence starts,
>>> and during the power on of SSUSB, clocks are enabled.
>>> In this case, powering off SSUSB fails from the first sequence, because
>>> power off ACK bit check times out (as clocks are powered back on by the second
>>> sequence). In consequence, powering it on also times out, and it leads to
>>> the whole power domain in a bad state.
>>>
>>> To solve this issue, added a mutex that locks the whole power off/power on
>>> sequence such that it would never happen that multiple sequences try to
>>> enable or disable the same power domain in parallel.
>>>
>>> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>
>> I don't think that it's a race between genpd_power_on() and genpd_power_off() calls
>> at all, because genpd *does* have locking after all... at least for probe and for
>> parents of a power domain (and more anyway).
>>
>> As far as I remember, what happens when you start .probe()'ing a device is:
>> platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()
>>
>> There, you end up with
>>
>>      if (power_on) {
>>          genpd_lock(pd);
>>          ret = genpd_power_on(pd, 0);
>>          genpd_unlock(pd);
>>      }
>>
>> ...but when you fail probing, you go with genpd_dev_pm_detach(), which then calls
>>
>>      /* Check if PM domain can be powered off after removing this device. */
>>      genpd_queue_power_off_work(pd);
>>
>> but even then, you end up being in a worker doing
>>
>>      genpd_lock(genpd);
>>      genpd_power_off(genpd, false, 0);
>>      genpd_unlock(genpd);
>>
>> ...so I don't understand why this mutex can resolve the situation here (also: are
>> you really sure that the race is solved like that?)
>>
>> I'd say that this probably needs more justification and a trace of the actual
>> situation here.
>>
>> Besides, if this really resolves the issue, I would prefer seeing variants of
>> scpsys_power_{on,off}() functions, because we anyway don't need to lock mutexes
>> during this driver's probe (add_subdomain calls scpsys_power_on()).
>> In that case, `scpsys_power_on_unlocked()` would be an idea... but still, please
>> analyze why your solution works, if it does, because I'm not convinced.
> 
> What I see in my tests, is that a power on call for SSUSB domain happens while the 
> previous power off sequence did not yet complete, most likely while it's waiting in 
> readx_poll_timeout . This leads to inconsistency of the power domain, not getting 
> the ACKs next time a power on attempt occurs.
> 
> I understand what you say about locks, but in this case the powering off is not 
> called by the genpd itself, but rather it's called by the rollback probe failed 
> mechanism : when the probing fails, scpsys_domain_cleanup() is called during the 
> same probing session.
> Then it happens that probing begins again and previous cleanup is not yet 
> completed. I am not sure whether the lock is still held from the previous run, but 
> it's clearly not waiting for a lock to be released to be called again.
> 

Sorry but I'm a bit lost now: is the problem about probe deferrals of the USB
driver, or about probe deferrals of the mtk-pm-domains driver?

scpsys_domain_cleanup() is only called upon scpsys_probe() failure.

>>
>>> ---
>>>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c 
>>> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>> index d5f0ee05c794..4f136b47e539 100644
>>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>> @@ -9,6 +9,7 @@
>>>   #include <linux/io.h>
>>>   #include <linux/iopoll.h>
>>>   #include <linux/mfd/syscon.h>
>>> +#include <linux/mutex.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_clk.h>
>>>   #include <linux/platform_device.h>
>>> @@ -56,6 +57,7 @@ struct scpsys {
>>>       struct device *dev;
>>>       struct regmap *base;
>>>       const struct scpsys_soc_data *soc_data;
>>> +    struct mutex mutex;
>>>       struct genpd_onecell_data pd_data;
>>>       struct generic_pm_domain *domains[];
>>>   };
>>> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>       bool tmp;
>>>       int ret;
>>> +    mutex_lock(&scpsys->mutex);
>>> +
>>>       ret = scpsys_regulator_enable(pd->supply);
>>> -    if (ret)
>>> +    if (ret) {
>>> +        mutex_unlock(&scpsys->mutex);
>>>           return ret;
>>> +    }
>>>       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>>>       if (ret)
>>> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>               goto err_enable_bus_protect;
>>>       }
>>> +    mutex_unlock(&scpsys->mutex);
>>>       return 0;
>>>   err_enable_bus_protect:
>>> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>   err_reg:
>>>       scpsys_regulator_disable(pd->supply);
>>> +    mutex_unlock(&scpsys->mutex);
>>>       return ret;
>>>   }
>>> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>>       bool tmp;
>>>       int ret;
>>> +    mutex_lock(&scpsys->mutex);
>>> +
>>>       ret = scpsys_bus_protect_enable(pd);
>>>       if (ret < 0)
>>> -        return ret;
>>> +        goto err_mutex_unlock;
>>>       ret = scpsys_sram_disable(pd);
>>>       if (ret < 0)
>>> -        return ret;
>>> +        goto err_mutex_unlock;
>>>       if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, MTK_SCPD_EXT_BUCK_ISO))
>>>           regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
>>> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct generic_pm_domain *genpd)
>>>       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, 
>>> MTK_POLL_DELAY_US,
>>>                    MTK_POLL_TIMEOUT);
>>>       if (ret < 0)
>>> -        return ret;
>>> +        goto err_mutex_unlock;
>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>       scpsys_regulator_disable(pd->supply);
>>> -    return 0;
>>> +err_mutex_unlock:
>>> +    mutex_unlock(&scpsys->mutex);
>>> +    return ret;
>>>   }
>>>   static struct
>>> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device *pdev)
>>>           return PTR_ERR(scpsys->base);
>>>       }
>>> +    mutex_init(&scpsys->mutex);
>>> +
>>>       ret = -ENODEV;
>>>       for_each_available_child_of_node(np, node) {
>>>           struct generic_pm_domain *domain;
>>
  

On 11/29/23 15:37, AngeloGioacchino Del Regno wrote:
> Il 29/11/23 14:28, Eugen Hristev ha scritto:
>> On 11/29/23 14:52, AngeloGioacchino Del Regno wrote:
>>> Il 29/11/23 12:31, Eugen Hristev ha scritto:
>>>> It can happen that during the power off sequence for a power domain
>>>> another power on sequence is started, and it can lead to powering on 
>>>> and
>>>> off in the same time for the similar power domain.
>>>> This can happen if parallel probing occurs: one device starts 
>>>> probing, and
>>>> one power domain is probe deferred, this leads to all power domains 
>>>> being
>>>> rolled back and powered off, while in the same time another device 
>>>> starts
>>>> probing and requests powering on the same power domains or similar.
>>>>
>>>> This was encountered on MT8186, when the sequence is :
>>>> Power on SSUSB
>>>> Power on SSUSB_P1
>>>> Power on DIS
>>>>     -> probe deferred
>>>> Power off DIS
>>>> Power off SSUSB_P1
>>>> Power off SSUSB
>>>>
>>>> During the sequence of powering off SSUSB, some new similar sequence 
>>>> starts,
>>>> and during the power on of SSUSB, clocks are enabled.
>>>> In this case, powering off SSUSB fails from the first sequence, because
>>>> power off ACK bit check times out (as clocks are powered back on by 
>>>> the second
>>>> sequence). In consequence, powering it on also times out, and it 
>>>> leads to
>>>> the whole power domain in a bad state.
>>>>
>>>> To solve this issue, added a mutex that locks the whole power 
>>>> off/power on
>>>> sequence such that it would never happen that multiple sequences try to
>>>> enable or disable the same power domain in parallel.
>>>>
>>>> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power 
>>>> domains")
>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>>
>>> I don't think that it's a race between genpd_power_on() and 
>>> genpd_power_off() calls
>>> at all, because genpd *does* have locking after all... at least for 
>>> probe and for
>>> parents of a power domain (and more anyway).
>>>
>>> As far as I remember, what happens when you start .probe()'ing a 
>>> device is:
>>> platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()
>>>
>>> There, you end up with
>>>
>>>      if (power_on) {
>>>          genpd_lock(pd);
>>>          ret = genpd_power_on(pd, 0);
>>>          genpd_unlock(pd);
>>>      }
>>>
>>> ...but when you fail probing, you go with genpd_dev_pm_detach(), 
>>> which then calls
>>>
>>>      /* Check if PM domain can be powered off after removing this 
>>> device. */
>>>      genpd_queue_power_off_work(pd);
>>>
>>> but even then, you end up being in a worker doing
>>>
>>>      genpd_lock(genpd);
>>>      genpd_power_off(genpd, false, 0);
>>>      genpd_unlock(genpd);
>>>
>>> ...so I don't understand why this mutex can resolve the situation 
>>> here (also: are
>>> you really sure that the race is solved like that?)
>>>
>>> I'd say that this probably needs more justification and a trace of 
>>> the actual
>>> situation here.
>>>
>>> Besides, if this really resolves the issue, I would prefer seeing 
>>> variants of
>>> scpsys_power_{on,off}() functions, because we anyway don't need to 
>>> lock mutexes
>>> during this driver's probe (add_subdomain calls scpsys_power_on()).
>>> In that case, `scpsys_power_on_unlocked()` would be an idea... but 
>>> still, please
>>> analyze why your solution works, if it does, because I'm not convinced.
>>
>> What I see in my tests, is that a power on call for SSUSB domain 
>> happens while the previous power off sequence did not yet complete, 
>> most likely while it's waiting in readx_poll_timeout . This leads to 
>> inconsistency of the power domain, not getting the ACKs next time a 
>> power on attempt occurs.
>>
>> I understand what you say about locks, but in this case the powering 
>> off is not called by the genpd itself, but rather it's called by the 
>> rollback probe failed mechanism : when the probing fails, 
>> scpsys_domain_cleanup() is called during the same probing session.
>> Then it happens that probing begins again and previous cleanup is not 
>> yet completed. I am not sure whether the lock is still held from the 
>> previous run, but it's clearly not waiting for a lock to be released 
>> to be called again.
>>
> 
> Sorry but I'm a bit lost now: is the problem about probe deferrals of 
> the USB
> driver, or about probe deferrals of the mtk-pm-domains driver?
> 
> scpsys_domain_cleanup() is only called upon scpsys_probe() failure.

You are right, my explanation was bad.

It happens during the mtk-pm-domains driver probe.

Not all domains can power up, then everything is rolled back. and this 
happens multiple times
On rare occasions, it happens that another probing sequence starts while 
the previous one was not finished .
I mentioned devices because I had in mind the fact that each device 
requires a power domain, and parallel probing of these devices causes a 
call to mtk-pm-domains driver probe to be called from two different places.

e.g. device 1 probes -> call mtk-pm-domains probe because it requires X 
power domain

device 2 probes -> call mtk-pm-domains probe because it requires Y power 
domain.

First attempt fails but not completed while second attempt starts.

Maybe this is a better explanation of the situation ?
> 
>>>
>>>> ---
>>>>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 
>>>> +++++++++++++++++-----
>>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c 
>>>> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> index d5f0ee05c794..4f136b47e539 100644
>>>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>> @@ -9,6 +9,7 @@
>>>>   #include <linux/io.h>
>>>>   #include <linux/iopoll.h>
>>>>   #include <linux/mfd/syscon.h>
>>>> +#include <linux/mutex.h>
>>>>   #include <linux/of.h>
>>>>   #include <linux/of_clk.h>
>>>>   #include <linux/platform_device.h>
>>>> @@ -56,6 +57,7 @@ struct scpsys {
>>>>       struct device *dev;
>>>>       struct regmap *base;
>>>>       const struct scpsys_soc_data *soc_data;
>>>> +    struct mutex mutex;
>>>>       struct genpd_onecell_data pd_data;
>>>>       struct generic_pm_domain *domains[];
>>>>   };
>>>> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct 
>>>> generic_pm_domain *genpd)
>>>>       bool tmp;
>>>>       int ret;
>>>> +    mutex_lock(&scpsys->mutex);
>>>> +
>>>>       ret = scpsys_regulator_enable(pd->supply);
>>>> -    if (ret)
>>>> +    if (ret) {
>>>> +        mutex_unlock(&scpsys->mutex);
>>>>           return ret;
>>>> +    }
>>>>       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>>>>       if (ret)
>>>> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct 
>>>> generic_pm_domain *genpd)
>>>>               goto err_enable_bus_protect;
>>>>       }
>>>> +    mutex_unlock(&scpsys->mutex);
>>>>       return 0;
>>>>   err_enable_bus_protect:
>>>> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct 
>>>> generic_pm_domain *genpd)
>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>   err_reg:
>>>>       scpsys_regulator_disable(pd->supply);
>>>> +    mutex_unlock(&scpsys->mutex);
>>>>       return ret;
>>>>   }
>>>> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct 
>>>> generic_pm_domain *genpd)
>>>>       bool tmp;
>>>>       int ret;
>>>> +    mutex_lock(&scpsys->mutex);
>>>> +
>>>>       ret = scpsys_bus_protect_enable(pd);
>>>>       if (ret < 0)
>>>> -        return ret;
>>>> +        goto err_mutex_unlock;
>>>>       ret = scpsys_sram_disable(pd);
>>>>       if (ret < 0)
>>>> -        return ret;
>>>> +        goto err_mutex_unlock;
>>>>       if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, 
>>>> MTK_SCPD_EXT_BUCK_ISO))
>>>>           regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
>>>> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct 
>>>> generic_pm_domain *genpd)
>>>>       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, 
>>>> MTK_POLL_DELAY_US,
>>>>                    MTK_POLL_TIMEOUT);
>>>>       if (ret < 0)
>>>> -        return ret;
>>>> +        goto err_mutex_unlock;
>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>       scpsys_regulator_disable(pd->supply);
>>>> -    return 0;
>>>> +err_mutex_unlock:
>>>> +    mutex_unlock(&scpsys->mutex);
>>>> +    return ret;
>>>>   }
>>>>   static struct
>>>> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device 
>>>> *pdev)
>>>>           return PTR_ERR(scpsys->base);
>>>>       }
>>>> +    mutex_init(&scpsys->mutex);
>>>> +
>>>>       ret = -ENODEV;
>>>>       for_each_available_child_of_node(np, node) {
>>>>           struct generic_pm_domain *domain;
>>>
>
  

Il 29/11/23 14:48, Eugen Hristev ha scritto:
> On 11/29/23 15:37, AngeloGioacchino Del Regno wrote:
>> Il 29/11/23 14:28, Eugen Hristev ha scritto:
>>> On 11/29/23 14:52, AngeloGioacchino Del Regno wrote:
>>>> Il 29/11/23 12:31, Eugen Hristev ha scritto:
>>>>> It can happen that during the power off sequence for a power domain
>>>>> another power on sequence is started, and it can lead to powering on and
>>>>> off in the same time for the similar power domain.
>>>>> This can happen if parallel probing occurs: one device starts probing, and
>>>>> one power domain is probe deferred, this leads to all power domains being
>>>>> rolled back and powered off, while in the same time another device starts
>>>>> probing and requests powering on the same power domains or similar.
>>>>>
>>>>> This was encountered on MT8186, when the sequence is :
>>>>> Power on SSUSB
>>>>> Power on SSUSB_P1
>>>>> Power on DIS
>>>>>     -> probe deferred
>>>>> Power off DIS
>>>>> Power off SSUSB_P1
>>>>> Power off SSUSB
>>>>>
>>>>> During the sequence of powering off SSUSB, some new similar sequence starts,
>>>>> and during the power on of SSUSB, clocks are enabled.
>>>>> In this case, powering off SSUSB fails from the first sequence, because
>>>>> power off ACK bit check times out (as clocks are powered back on by the second
>>>>> sequence). In consequence, powering it on also times out, and it leads to
>>>>> the whole power domain in a bad state.
>>>>>
>>>>> To solve this issue, added a mutex that locks the whole power off/power on
>>>>> sequence such that it would never happen that multiple sequences try to
>>>>> enable or disable the same power domain in parallel.
>>>>>
>>>>> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>>>
>>>> I don't think that it's a race between genpd_power_on() and genpd_power_off() 
>>>> calls
>>>> at all, because genpd *does* have locking after all... at least for probe and for
>>>> parents of a power domain (and more anyway).
>>>>
>>>> As far as I remember, what happens when you start .probe()'ing a device is:
>>>> platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()
>>>>
>>>> There, you end up with
>>>>
>>>>      if (power_on) {
>>>>          genpd_lock(pd);
>>>>          ret = genpd_power_on(pd, 0);
>>>>          genpd_unlock(pd);
>>>>      }
>>>>
>>>> ...but when you fail probing, you go with genpd_dev_pm_detach(), which then calls
>>>>
>>>>      /* Check if PM domain can be powered off after removing this device. */
>>>>      genpd_queue_power_off_work(pd);
>>>>
>>>> but even then, you end up being in a worker doing
>>>>
>>>>      genpd_lock(genpd);
>>>>      genpd_power_off(genpd, false, 0);
>>>>      genpd_unlock(genpd);
>>>>
>>>> ...so I don't understand why this mutex can resolve the situation here (also: are
>>>> you really sure that the race is solved like that?)
>>>>
>>>> I'd say that this probably needs more justification and a trace of the actual
>>>> situation here.
>>>>
>>>> Besides, if this really resolves the issue, I would prefer seeing variants of
>>>> scpsys_power_{on,off}() functions, because we anyway don't need to lock mutexes
>>>> during this driver's probe (add_subdomain calls scpsys_power_on()).
>>>> In that case, `scpsys_power_on_unlocked()` would be an idea... but still, please
>>>> analyze why your solution works, if it does, because I'm not convinced.
>>>
>>> What I see in my tests, is that a power on call for SSUSB domain happens while 
>>> the previous power off sequence did not yet complete, most likely while it's 
>>> waiting in readx_poll_timeout . This leads to inconsistency of the power domain, 
>>> not getting the ACKs next time a power on attempt occurs.
>>>
>>> I understand what you say about locks, but in this case the powering off is not 
>>> called by the genpd itself, but rather it's called by the rollback probe failed 
>>> mechanism : when the probing fails, scpsys_domain_cleanup() is called during the 
>>> same probing session.
>>> Then it happens that probing begins again and previous cleanup is not yet 
>>> completed. I am not sure whether the lock is still held from the previous run, 
>>> but it's clearly not waiting for a lock to be released to be called again.
>>>
>>
>> Sorry but I'm a bit lost now: is the problem about probe deferrals of the USB
>> driver, or about probe deferrals of the mtk-pm-domains driver?
>>
>> scpsys_domain_cleanup() is only called upon scpsys_probe() failure.
> 
> You are right, my explanation was bad.
> 
> It happens during the mtk-pm-domains driver probe.
> 
> Not all domains can power up, then everything is rolled back. and this happens 
> multiple times
> On rare occasions, it happens that another probing sequence starts while the 
> previous one was not finished .
> I mentioned devices because I had in mind the fact that each device requires a 
> power domain, and parallel probing of these devices causes a call to mtk-pm-domains 
> driver probe to be called from two different places.
> 
> e.g. device 1 probes -> call mtk-pm-domains probe because it requires X power domain
> 
> device 2 probes -> call mtk-pm-domains probe because it requires Y power domain.
> 
> First attempt fails but not completed while second attempt starts.
> 
> Maybe this is a better explanation of the situation ?

Yeah, now it's a bit clearer!

At this point, I think that you can get away with locking just one path (or two):

/* This is the one giving me lots of suspects */
static void scpsys_remove_one_domain(struct scpsys_domain *pd)
{
	int ret;

***lock***
	if (scpsys_domain_is_on(pd))
		scpsys_power_off(&pd->genpd);
***unlock***

	.....
}

/* This one as well eventually */
static struct
generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node 
*node)
{
	...............

	if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF)) {

		if (scpsys_domain_is_on(pd)) /* Maybe LOCK this one too? */

			dev_warn(scpsys->dev,
				 "%pOF: A default off power domain has been ON\n", node);
	} else {
**		*** lock ***
		ret = scpsys_power_on(&pd->genpd);
**		*** unlock ***
		if (ret < 0) {
			dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
			goto err_put_subsys_clocks;
		}

		if (MTK_SCPD_CAPS(pd, MTK_SCPD_ALWAYS_ON))
			pd->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
	}

	..........
}

Can you please try locking only the remove_one_domain() poweroff call before
trying both?

Reason is that in the add_one_domain() case, we haven't registered the power
domain yet, so locking may not be required there to make things ticking right.

Cheers!

>>
>>>>
>>>>> ---
>>>>>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
>>>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c 
>>>>> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>> index d5f0ee05c794..4f136b47e539 100644
>>>>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>> @@ -9,6 +9,7 @@
>>>>>   #include <linux/io.h>
>>>>>   #include <linux/iopoll.h>
>>>>>   #include <linux/mfd/syscon.h>
>>>>> +#include <linux/mutex.h>
>>>>>   #include <linux/of.h>
>>>>>   #include <linux/of_clk.h>
>>>>>   #include <linux/platform_device.h>
>>>>> @@ -56,6 +57,7 @@ struct scpsys {
>>>>>       struct device *dev;
>>>>>       struct regmap *base;
>>>>>       const struct scpsys_soc_data *soc_data;
>>>>> +    struct mutex mutex;
>>>>>       struct genpd_onecell_data pd_data;
>>>>>       struct generic_pm_domain *domains[];
>>>>>   };
>>>>> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>       bool tmp;
>>>>>       int ret;
>>>>> +    mutex_lock(&scpsys->mutex);
>>>>> +
>>>>>       ret = scpsys_regulator_enable(pd->supply);
>>>>> -    if (ret)
>>>>> +    if (ret) {
>>>>> +        mutex_unlock(&scpsys->mutex);
>>>>>           return ret;
>>>>> +    }
>>>>>       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>>>>>       if (ret)
>>>>> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>               goto err_enable_bus_protect;
>>>>>       }
>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>>       return 0;
>>>>>   err_enable_bus_protect:
>>>>> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>>   err_reg:
>>>>>       scpsys_regulator_disable(pd->supply);
>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>>       return ret;
>>>>>   }
>>>>> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct generic_pm_domain 
>>>>> *genpd)
>>>>>       bool tmp;
>>>>>       int ret;
>>>>> +    mutex_lock(&scpsys->mutex);
>>>>> +
>>>>>       ret = scpsys_bus_protect_enable(pd);
>>>>>       if (ret < 0)
>>>>> -        return ret;
>>>>> +        goto err_mutex_unlock;
>>>>>       ret = scpsys_sram_disable(pd);
>>>>>       if (ret < 0)
>>>>> -        return ret;
>>>>> +        goto err_mutex_unlock;
>>>>>       if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, 
>>>>> MTK_SCPD_EXT_BUCK_ISO))
>>>>>           regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
>>>>> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct generic_pm_domain 
>>>>> *genpd)
>>>>>       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, 
>>>>> MTK_POLL_DELAY_US,
>>>>>                    MTK_POLL_TIMEOUT);
>>>>>       if (ret < 0)
>>>>> -        return ret;
>>>>> +        goto err_mutex_unlock;
>>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>>       scpsys_regulator_disable(pd->supply);
>>>>> -    return 0;
>>>>> +err_mutex_unlock:
>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>> +    return ret;
>>>>>   }
>>>>>   static struct
>>>>> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device *pdev)
>>>>>           return PTR_ERR(scpsys->base);
>>>>>       }
>>>>> +    mutex_init(&scpsys->mutex);
>>>>> +
>>>>>       ret = -ENODEV;
>>>>>       for_each_available_child_of_node(np, node) {
>>>>>           struct generic_pm_domain *domain;
>>>>
>>
  

On 11/29/23 16:41, AngeloGioacchino Del Regno wrote:
> Il 29/11/23 14:48, Eugen Hristev ha scritto:
>> On 11/29/23 15:37, AngeloGioacchino Del Regno wrote:
>>> Il 29/11/23 14:28, Eugen Hristev ha scritto:
>>>> On 11/29/23 14:52, AngeloGioacchino Del Regno wrote:
>>>>> Il 29/11/23 12:31, Eugen Hristev ha scritto:
>>>>>> It can happen that during the power off sequence for a power domain
>>>>>> another power on sequence is started, and it can lead to powering on and
>>>>>> off in the same time for the similar power domain.
>>>>>> This can happen if parallel probing occurs: one device starts probing, and
>>>>>> one power domain is probe deferred, this leads to all power domains being
>>>>>> rolled back and powered off, while in the same time another device starts
>>>>>> probing and requests powering on the same power domains or similar.
>>>>>>
>>>>>> This was encountered on MT8186, when the sequence is :
>>>>>> Power on SSUSB
>>>>>> Power on SSUSB_P1
>>>>>> Power on DIS
>>>>>>     -> probe deferred
>>>>>> Power off DIS
>>>>>> Power off SSUSB_P1
>>>>>> Power off SSUSB
>>>>>>
>>>>>> During the sequence of powering off SSUSB, some new similar sequence starts,
>>>>>> and during the power on of SSUSB, clocks are enabled.
>>>>>> In this case, powering off SSUSB fails from the first sequence, because
>>>>>> power off ACK bit check times out (as clocks are powered back on by the second
>>>>>> sequence). In consequence, powering it on also times out, and it leads to
>>>>>> the whole power domain in a bad state.
>>>>>>
>>>>>> To solve this issue, added a mutex that locks the whole power off/power on
>>>>>> sequence such that it would never happen that multiple sequences try to
>>>>>> enable or disable the same power domain in parallel.
>>>>>>
>>>>>> Fixes: 59b644b01cf4 ("soc: mediatek: Add MediaTek SCPSYS power domains")
>>>>>> Signed-off-by: Eugen Hristev <eugen.hristev@collabora.com>
>>>>>
>>>>> I don't think that it's a race between genpd_power_on() and genpd_power_off() 
>>>>> calls
>>>>> at all, because genpd *does* have locking after all... at least for probe and for
>>>>> parents of a power domain (and more anyway).
>>>>>
>>>>> As far as I remember, what happens when you start .probe()'ing a device is:
>>>>> platform_probe() -> dev_pm_domain_attach() -> genpd_dev_pm_attach()
>>>>>
>>>>> There, you end up with
>>>>>
>>>>>      if (power_on) {
>>>>>          genpd_lock(pd);
>>>>>          ret = genpd_power_on(pd, 0);
>>>>>          genpd_unlock(pd);
>>>>>      }
>>>>>
>>>>> ...but when you fail probing, you go with genpd_dev_pm_detach(), which then calls
>>>>>
>>>>>      /* Check if PM domain can be powered off after removing this device. */
>>>>>      genpd_queue_power_off_work(pd);
>>>>>
>>>>> but even then, you end up being in a worker doing
>>>>>
>>>>>      genpd_lock(genpd);
>>>>>      genpd_power_off(genpd, false, 0);
>>>>>      genpd_unlock(genpd);
>>>>>
>>>>> ...so I don't understand why this mutex can resolve the situation here (also: are
>>>>> you really sure that the race is solved like that?)
>>>>>
>>>>> I'd say that this probably needs more justification and a trace of the actual
>>>>> situation here.
>>>>>
>>>>> Besides, if this really resolves the issue, I would prefer seeing variants of
>>>>> scpsys_power_{on,off}() functions, because we anyway don't need to lock mutexes
>>>>> during this driver's probe (add_subdomain calls scpsys_power_on()).
>>>>> In that case, `scpsys_power_on_unlocked()` would be an idea... but still, please
>>>>> analyze why your solution works, if it does, because I'm not convinced.
>>>>
>>>> What I see in my tests, is that a power on call for SSUSB domain happens while 
>>>> the previous power off sequence did not yet complete, most likely while it's 
>>>> waiting in readx_poll_timeout . This leads to inconsistency of the power domain, 
>>>> not getting the ACKs next time a power on attempt occurs.
>>>>
>>>> I understand what you say about locks, but in this case the powering off is not 
>>>> called by the genpd itself, but rather it's called by the rollback probe failed 
>>>> mechanism : when the probing fails, scpsys_domain_cleanup() is called during the 
>>>> same probing session.
>>>> Then it happens that probing begins again and previous cleanup is not yet 
>>>> completed. I am not sure whether the lock is still held from the previous run, 
>>>> but it's clearly not waiting for a lock to be released to be called again.
>>>>
>>>
>>> Sorry but I'm a bit lost now: is the problem about probe deferrals of the USB
>>> driver, or about probe deferrals of the mtk-pm-domains driver?
>>>
>>> scpsys_domain_cleanup() is only called upon scpsys_probe() failure.
>>
>> You are right, my explanation was bad.
>>
>> It happens during the mtk-pm-domains driver probe.
>>
>> Not all domains can power up, then everything is rolled back. and this happens 
>> multiple times
>> On rare occasions, it happens that another probing sequence starts while the 
>> previous one was not finished .
>> I mentioned devices because I had in mind the fact that each device requires a 
>> power domain, and parallel probing of these devices causes a call to mtk-pm-domains 
>> driver probe to be called from two different places.
>>
>> e.g. device 1 probes -> call mtk-pm-domains probe because it requires X power domain
>>
>> device 2 probes -> call mtk-pm-domains probe because it requires Y power domain.
>>
>> First attempt fails but not completed while second attempt starts.
>>
>> Maybe this is a better explanation of the situation ?
> 
> Yeah, now it's a bit clearer!
> 
> At this point, I think that you can get away with locking just one path (or two):
> 
> /* This is the one giving me lots of suspects */
> static void scpsys_remove_one_domain(struct scpsys_domain *pd)
> {
> 	int ret;
> 
> ***lock***
> 	if (scpsys_domain_is_on(pd))
> 		scpsys_power_off(&pd->genpd);
> ***unlock***
> 
> 	.....
> }
> 
> /* This one as well eventually */
> static struct
> generic_pm_domain *scpsys_add_one_domain(struct scpsys *scpsys, struct device_node 
> *node)
> {
> 	...............
> 
> 	if (MTK_SCPD_CAPS(pd, MTK_SCPD_KEEP_DEFAULT_OFF)) {
> 
> 		if (scpsys_domain_is_on(pd)) /* Maybe LOCK this one too? */
> 
> 			dev_warn(scpsys->dev,
> 				 "%pOF: A default off power domain has been ON\n", node);
> 	} else {
> **		*** lock ***
> 		ret = scpsys_power_on(&pd->genpd);
> **		*** unlock ***
> 		if (ret < 0) {
> 			dev_err(scpsys->dev, "%pOF: failed to power on domain: %d\n", node, ret);
> 			goto err_put_subsys_clocks;
> 		}
> 
> 		if (MTK_SCPD_CAPS(pd, MTK_SCPD_ALWAYS_ON))
> 			pd->genpd.flags |= GENPD_FLAG_ALWAYS_ON;
> 	}
> 
> 	..........
> }
> 
> Can you please try locking only the remove_one_domain() poweroff call before
> trying both?
> 
> Reason is that in the add_one_domain() case, we haven't registered the power
> domain yet, so locking may not be required there to make things ticking right.

We need to have 'critical section' all the access to clocks/registers, so both
paths (power on and power off) must be serialized. So a mutex must be taken before
entering either of those, otherwise it makes no sense. There must be no path left
that leads to power on/ power off that is taken without the mutex.
> 
> Cheers!
> 
>>>
>>>>>
>>>>>> ---
>>>>>>   drivers/pmdomain/mediatek/mtk-pm-domains.c | 24 +++++++++++++++++-----
>>>>>>   1 file changed, 19 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/pmdomain/mediatek/mtk-pm-domains.c 
>>>>>> b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>>> index d5f0ee05c794..4f136b47e539 100644
>>>>>> --- a/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>>> +++ b/drivers/pmdomain/mediatek/mtk-pm-domains.c
>>>>>> @@ -9,6 +9,7 @@
>>>>>>   #include <linux/io.h>
>>>>>>   #include <linux/iopoll.h>
>>>>>>   #include <linux/mfd/syscon.h>
>>>>>> +#include <linux/mutex.h>
>>>>>>   #include <linux/of.h>
>>>>>>   #include <linux/of_clk.h>
>>>>>>   #include <linux/platform_device.h>
>>>>>> @@ -56,6 +57,7 @@ struct scpsys {
>>>>>>       struct device *dev;
>>>>>>       struct regmap *base;
>>>>>>       const struct scpsys_soc_data *soc_data;
>>>>>> +    struct mutex mutex;
>>>>>>       struct genpd_onecell_data pd_data;
>>>>>>       struct generic_pm_domain *domains[];
>>>>>>   };
>>>>>> @@ -238,9 +240,13 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>>       bool tmp;
>>>>>>       int ret;
>>>>>> +    mutex_lock(&scpsys->mutex);
>>>>>> +
>>>>>>       ret = scpsys_regulator_enable(pd->supply);
>>>>>> -    if (ret)
>>>>>> +    if (ret) {
>>>>>> +        mutex_unlock(&scpsys->mutex);
>>>>>>           return ret;
>>>>>> +    }
>>>>>>       ret = clk_bulk_prepare_enable(pd->num_clks, pd->clks);
>>>>>>       if (ret)
>>>>>> @@ -291,6 +297,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>>               goto err_enable_bus_protect;
>>>>>>       }
>>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>>>       return 0;
>>>>>>   err_enable_bus_protect:
>>>>>> @@ -305,6 +312,7 @@ static int scpsys_power_on(struct generic_pm_domain *genpd)
>>>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>>>   err_reg:
>>>>>>       scpsys_regulator_disable(pd->supply);
>>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>>>       return ret;
>>>>>>   }
>>>>>> @@ -315,13 +323,15 @@ static int scpsys_power_off(struct generic_pm_domain 
>>>>>> *genpd)
>>>>>>       bool tmp;
>>>>>>       int ret;
>>>>>> +    mutex_lock(&scpsys->mutex);
>>>>>> +
>>>>>>       ret = scpsys_bus_protect_enable(pd);
>>>>>>       if (ret < 0)
>>>>>> -        return ret;
>>>>>> +        goto err_mutex_unlock;
>>>>>>       ret = scpsys_sram_disable(pd);
>>>>>>       if (ret < 0)
>>>>>> -        return ret;
>>>>>> +        goto err_mutex_unlock;
>>>>>>       if (pd->data->ext_buck_iso_offs && MTK_SCPD_CAPS(pd, 
>>>>>> MTK_SCPD_EXT_BUCK_ISO))
>>>>>>           regmap_set_bits(scpsys->base, pd->data->ext_buck_iso_offs,
>>>>>> @@ -340,13 +350,15 @@ static int scpsys_power_off(struct generic_pm_domain 
>>>>>> *genpd)
>>>>>>       ret = readx_poll_timeout(scpsys_domain_is_on, pd, tmp, !tmp, 
>>>>>> MTK_POLL_DELAY_US,
>>>>>>                    MTK_POLL_TIMEOUT);
>>>>>>       if (ret < 0)
>>>>>> -        return ret;
>>>>>> +        goto err_mutex_unlock;
>>>>>>       clk_bulk_disable_unprepare(pd->num_clks, pd->clks);
>>>>>>       scpsys_regulator_disable(pd->supply);
>>>>>> -    return 0;
>>>>>> +err_mutex_unlock:
>>>>>> +    mutex_unlock(&scpsys->mutex);
>>>>>> +    return ret;
>>>>>>   }
>>>>>>   static struct
>>>>>> @@ -700,6 +712,8 @@ static int scpsys_probe(struct platform_device *pdev)
>>>>>>           return PTR_ERR(scpsys->base);
>>>>>>       }
>>>>>> +    mutex_init(&scpsys->mutex);
>>>>>> +
>>>>>>       ret = -ENODEV;
>>>>>>       for_each_available_child_of_node(np, node) {
>>>>>>           struct generic_pm_domain *domain;
>>>>>
>>>
>