linux-kernel - Re: [PATCH v3 06/10] pmdomain: samsung: convert to regmap_read_poll

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: <5c19e4ef-c4fd-4bf5-88b3-46c86751b14e@samsung.com>
Date: Wed, 22 Oct 2025 15:45:07 +0200
From: Marek Szyprowski <m.szyprowski@...sung.com>
To: André Draszik <andre.draszik@...aro.org>, Krzysztof
	Kozlowski <krzk@...nel.org>, Alim Akhtar <alim.akhtar@...sung.com>, Rob
	Herring <robh@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Krzysztof
	Kozlowski <krzk+dt@...nel.org>, Ulf Hansson <ulf.hansson@...aro.org>
Cc: Peter Griffin <peter.griffin@...aro.org>, Tudor Ambarus
	<tudor.ambarus@...aro.org>, Will McVicker <willmcvicker@...gle.com>,
	kernel-team@...roid.com, linux-arm-kernel@...ts.infradead.org,
	linux-samsung-soc@...r.kernel.org, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH v3 06/10] pmdomain: samsung: convert to
 regmap_read_poll_timeout()

On 21.10.2025 22:38, Marek Szyprowski wrote:
> On 16.10.2025 17:58, André Draszik wrote:
>> Replace the open-coded PD status polling with
>> regmap_read_poll_timeout(). This change simplifies the code without
>> altering functionality.
>>
>> Signed-off-by: André Draszik <andre.draszik@...aro.org>
>> ---
>>   drivers/pmdomain/samsung/exynos-pm-domains.c | 29 
>> ++++++++--------------------
>>   1 file changed, 8 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/pmdomain/samsung/exynos-pm-domains.c 
>> b/drivers/pmdomain/samsung/exynos-pm-domains.c
>> index 
>> 383126245811cb8e4dbae3b99ced3f06d3093f35..431548ad9a7e40c0a77ac6672081b600c90ddd4e 
>> 100644
>> --- a/drivers/pmdomain/samsung/exynos-pm-domains.c
>> +++ b/drivers/pmdomain/samsung/exynos-pm-domains.c
>> @@ -13,7 +13,6 @@
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>>   #include <linux/pm_domain.h>
>> -#include <linux/delay.h>
>>   #include <linux/of.h>
>>   #include <linux/pm_runtime.h>
>>   #include <linux/regmap.h>
>> @@ -35,7 +34,8 @@ struct exynos_pm_domain {
>>   static int exynos_pd_power(struct generic_pm_domain *domain, bool 
>> power_on)
>>   {
>>       struct exynos_pm_domain *pd;
>> -    u32 timeout, pwr;
>> +    unsigned int val;
>> +    u32 pwr;
>>       int err;
>>         pd = container_of(domain, struct exynos_pm_domain, pd);
>> @@ -45,25 +45,12 @@ static int exynos_pd_power(struct 
>> generic_pm_domain *domain, bool power_on)
>>       if (err)
>>           return err;
>>   -    /* Wait max 1ms */
>> -    timeout = 10;
>> -    while (timeout-- > 0) {
>> -        unsigned int val;
>> -
>> -        err = regmap_read(pd->regmap, 0x4, &val);
>> -        if (err || ((val & pd->local_pwr_cfg) != pwr)) {
>> -            cpu_relax();
>> -            usleep_range(80, 100);
>> -            continue;
>> -        }
>> -
>> -        return 0;
>> -    }
>> -
>> -    if (!err)
>> -        err = -ETIMEDOUT;
>> -    pr_err("Power domain %s %sable failed: %d\n", domain->name,
>> -           power_on ? "en" : "dis", err);
>> +    err = regmap_read_poll_timeout(pd->regmap, 0x4, val,
>> +                       (val & pd->local_pwr_cfg) == pwr,
>> +                       100, 1 * USEC_PER_MSEC);
>> +    if (err)
>> +        pr_err("Power domain %s %sable failed: %d (%#.2x)\n",
>> +               domain->name, power_on ? "en" : "dis", err, val);
>
> I've posted my 'tested-by' tag for this patchset, but in meantime I 
> found that this patch causes regression from time to time on old 
> Exynos SoCs (especially when all debugs are disabled). It looks that 
> there are some subtle differences between reading the status register 
> up to 10 times with cpu_relax()+usleep_range() and the 
> regmap_read_poll_timeout(). I will try to analyze this a bit more and 
> provide details, but I suspect that the old loop might take a bit 
> longer than the 1ms from the comment above this code.

It looks that during the early boot all calls to 
regmap_read_poll_timeout() lasts exactly 10ms (measured with ktime_get() 
and ktime_to_ms()), what means that timekeeping source doesn't provide 
resolution high enough for the 1ms timeout. This in turn results in 
premature end of regmap_read_poll_timeout() loop after only one cycle of 
read+wait+read, what is not always enough for power domain to turn on/off.

According to the commit 7349a69cf312 ("iopoll: Do not use timekeeping in 
read_poll_timeout_atomic()"), ktime_get(), which is used also by 
regmap_read_poll_timeout(), is not reliable in all contexts, so I think 
that this patch should be dropped as there is no easy way to fix this.

The alternative would be to use regmap_read_poll_timeout_atomic(), which 
need to be fixed the same way as regmap_read_poll_timeout_atomic() by 
the mentioned commit, but in such case we would effectively switch from 
usleep to udelay.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland