| 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
| ||
|
Message-ID: <567adde6-a348-41c0-b415-80daf16d3dbb@tuxon.dev>
Date: Wed, 5 Feb 2025 10:33:39 +0200
From: Claudiu Beznea <claudiu.beznea@...on.dev>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>,
Ulf Hansson <ulf.hansson@...aro.org>
Cc: Daniel Lezcano <daniel.lezcano@...aro.org>, rafael@...nel.org,
rui.zhang@...el.com, lukasz.luba@....com, robh@...nel.org,
krzk+dt@...nel.org, conor+dt@...nel.org, geert+renesas@...der.be,
magnus.damm@...il.com, mturquette@...libre.com, sboyd@...nel.org,
p.zabel@...gutronix.de, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-clk@...r.kernel.org, Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
Subject: Re: [PATCH 2/6] thermal: of: Export non-devres helper to
register/unregister thermal zone
Hi, Jonathan,
On 04.02.2025 16:33, Jonathan Cameron wrote:
> On Wed, 15 Jan 2025 16:42:37 +0100
> Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
>> On Thu, 9 Jan 2025 at 18:34, Daniel Lezcano <daniel.lezcano@...aro.org> wrote:
>>>
>>>
>>> Ulf,
>>>
>>> can you have a look at this particular patch please ?
>>>
>>> Perhaps this scenario already happened in the past and there is an
>>> alternative to fix it instead of this proposed change
>>
>> I think the patch makes sense.
>>
>> If there is a PM domain that is attached to the device that is
>> managing the clocks for the thermal zone, the detach procedure
>> certainly needs to be well controlled/synchronized.
>>
> Does this boil down to the same issue as
> https://lore.kernel.org/linux-iio/20250128105908.0000353b@huawei.com/
> ?
Yes, as described in the cover letter.
>
> Just to point out there is another way like is done in i2c:
> https://elixir.bootlin.com/linux/v6.12.6/source/drivers/i2c/i2c-core-base.c#L630
>
> Register a devres_release_group() in bus probe() and release it before
> the dev_pm_domain_detach() call. That keeps the detach procedure well
> controlled and synchronized as it is entirely in control of the driver.
>From the IIO thread I got that Ulf doesn't consider it a good approach for
all the cases.
Thank you,
Claudiu
>
> That IIO thread has kind of died out for now though with no resolution
> so far.
>
> Jonathan
>
>
>>>
>>>
>>> On 03/01/2025 17:38, Claudiu wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>>>>
>>>> On the Renesas RZ/G3S (and other Renesas SoCs, e.g., RZ/G2{L, LC, UL}),
>>>> clocks are managed through PM domains. These PM domains, registered on
>>>> behalf of the clock controller driver, are configured with
>>>> GENPD_FLAG_PM_CLK. In most of the Renesas drivers used by RZ SoCs, the
>>>> clocks are enabled/disabled using runtime PM APIs.
>>>>
>>>> During probe, devices are attached to the PM domain controlling their
>>>> clocks. Similarly, during removal, devices are detached from the PM domain.
>>>>
>>>> The detachment call stack is as follows:
>>>>
>>>> device_driver_detach() ->
>>>> device_release_driver_internal() ->
>>>> __device_release_driver() ->
>>>> device_remove() ->
>>>> platform_remove() ->
>>>> dev_pm_domain_detach()
>>>>
>>>> In the upcoming Renesas RZ/G3S thermal driver, the
>>>> struct thermal_zone_device_ops::change_mode API is implemented to
>>>> start/stop the thermal sensor unit. Register settings are updated within
>>>> the change_mode API.
>>>>
>>>> In case devres helpers are used for thermal zone register/unregister the
>>>> struct thermal_zone_device_ops::change_mode API is invoked when the
>>>> driver is unbound. The identified call stack is as follows:
>>>>
>>>> device_driver_detach() ->
>>>> device_release_driver_internal() ->
>>>> device_unbind_cleanup() ->
>>>> devres_release_all() ->
>>>> devm_thermal_of_zone_release() ->
>>>> thermal_zone_device_disable() ->
>>>> thermal_zone_device_set_mode() ->
>>>> rzg3s_thermal_change_mode()
>>>>
>>>> The device_unbind_cleanup() function is called after the thermal device is
>>>> detached from the PM domain (via dev_pm_domain_detach()).
>>>>
>>>> The rzg3s_thermal_change_mode() implementation calls
>>>> pm_runtime_resume_and_get()/pm_runtime_put_autosuspend() before/after
>>>> accessing the registers. However, during the unbind scenario, the
>>>> devm_thermal_of_zone_release() is invoked after dev_pm_domain_detach().
>>>> Consequently, the clocks are not enabled, as the device is removed from
>>>> the PM domain at this time, leading to an Asynchronous SError Interrupt.
>>>> The system cannot be used after this.
>>>>
>>>> Add thermal_of_zone_register()/thermal_of_zone_unregister(). These will
>>>> be used in the upcomming RZ/G3S thermal driver.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@...renesas.com>
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@...aro.org>
>>
>> Kind regards
>> Uffe
>>
>>>> ---
>>>> drivers/thermal/thermal_of.c | 8 +++++---
>>>> include/linux/thermal.h | 14 ++++++++++++++
>>>> 2 files changed, 19 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/thermal_of.c b/drivers/thermal/thermal_of.c
>>>> index fab11b98ca49..8fc35d20db60 100644
>>>> --- a/drivers/thermal/thermal_of.c
>>>> +++ b/drivers/thermal/thermal_of.c
>>>> @@ -329,11 +329,12 @@ static bool thermal_of_should_bind(struct thermal_zone_device *tz,
>>>> *
>>>> * @tz: a pointer to the thermal zone structure
>>>> */
>>>> -static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>>>> +void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>>>> {
>>>> thermal_zone_device_disable(tz);
>>>> thermal_zone_device_unregister(tz);
>>>> }
>>>> +EXPORT_SYMBOL_GPL(thermal_of_zone_unregister);
>>>>
>>>> /**
>>>> * thermal_of_zone_register - Register a thermal zone with device node
>>>> @@ -355,8 +356,8 @@ static void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>>>> * - ENOMEM: if one structure can not be allocated
>>>> * - Other negative errors are returned by the underlying called functions
>>>> */
>>>> -static struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
>>>> - const struct thermal_zone_device_ops *ops)
>>>> +struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
>>>> + const struct thermal_zone_device_ops *ops)
>>>> {
>>>> struct thermal_zone_device_ops of_ops = *ops;
>>>> struct thermal_zone_device *tz;
>>>> @@ -429,6 +430,7 @@ static struct thermal_zone_device *thermal_of_zone_register(struct device_node *
>>>>
>>>> return ERR_PTR(ret);
>>>> }
>>>> +EXPORT_SYMBOL_GPL(thermal_of_zone_register);
>>>>
>>>> static void devm_thermal_of_zone_release(struct device *dev, void *res)
>>>> {
>>>> diff --git a/include/linux/thermal.h b/include/linux/thermal.h
>>>> index 69f9bedd0ee8..adbb4092a064 100644
>>>> --- a/include/linux/thermal.h
>>>> +++ b/include/linux/thermal.h
>>>> @@ -195,13 +195,23 @@ struct thermal_zone_params {
>>>>
>>>> /* Function declarations */
>>>> #ifdef CONFIG_THERMAL_OF
>>>> +struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
>>>> + const struct thermal_zone_device_ops *ops);
>>>> struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
>>>> const struct thermal_zone_device_ops *ops);
>>>>
>>>> +void thermal_of_zone_unregister(struct thermal_zone_device *tz);
>>>> void devm_thermal_of_zone_unregister(struct device *dev, struct thermal_zone_device *tz);
>>>>
>>>> #else
>>>>
>>>> +static inline
>>>> +struct thermal_zone_device *thermal_of_zone_register(struct device_node *sensor, int id, void *data,
>>>> + const struct thermal_zone_device_ops *ops)
>>>> +{
>>>> + return ERR_PTR(-ENOTSUPP);
>>>> +}
>>>> +
>>>> static inline
>>>> struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, int id, void *data,
>>>> const struct thermal_zone_device_ops *ops)
>>>> @@ -209,6 +219,10 @@ struct thermal_zone_device *devm_thermal_of_zone_register(struct device *dev, in
>>>> return ERR_PTR(-ENOTSUPP);
>>>> }
>>>>
>>>> +static inline void thermal_of_zone_unregister(struct thermal_zone_device *tz)
>>>> +{
>>>> +}
>>>> +
>>>> static inline void devm_thermal_of_zone_unregister(struct device *dev,
>>>> struct thermal_zone_device *tz)
>>>> {
>>>
>>>
>>> --
>>> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
>>>
>>> Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
>>> <http://twitter.com/#!/linaroorg> Twitter |
>>> <http://www.linaro.org/linaro-blog/> Blog
>>
>
Powered by blists - more mailing lists