Hi, Jonathan,

On 04.02.2025 16:33, Jonathan Cameron wrote:

On Wed, 15 Jan 2025 16:42:37 +0100
Ulf Hansson [off-list ref] wrote:

quoted

On Thu, 9 Jan 2025 at 18:34, Daniel Lezcano [off-list ref] wrote:

quoted

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/ (local)
?

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

quoted

quoted

On 03/01/2025 17:38, Claudiu wrote:  

quoted

From: Claudiu Beznea <claudiu.beznea.uj@bp.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@bp.renesas.com>  

Reviewed-by: Ulf Hansson <redacted>

Kind regards
Uffe

quoted

quoted

---
  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