[PATCH v2 0/3] thermal: Fix workqueue-related issues in drivers

[PATCH v2 0/3] thermal: Fix workqueue-related issues in drivers

[Geert Uytterhoeven]

	Hi,

This patch series fixes workqueue-related issues in the Renesas R-Car
Thermal and Dialog DA9062/9061 PMIC drivers, where the workqueue may run
while the device is suspended, or unbound.
The R-Car Thermal driver fixes have been tested on R-Car M2-W and R-Mobile
APE6.
The DA9062/9061 fixes have been compile-tested only.

Changes compared to v1:
  - Add Reviewed-by.

Note: The Intel PowerClamp driver also uses schedule_delayed_work(), but I
believe that is OK, as the thermal registers are part of the CPU.

Thanks!

Geert Uytterhoeven (3):
  thermal: rcar_thermal: Prevent hardware access during system suspend
  thermal: rcar_thermal: Prevent doing work after unbind
  thermal: da9062/61: Prevent hardware access during system suspend

 drivers/thermal/da9062-thermal.c | 4 ++--
 drivers/thermal/rcar_thermal.c   | 5 +++--
 2 files changed, 5 insertions(+), 4 deletions(-)

-- 
2.17.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH v2 1/3] thermal: rcar_thermal: Prevent hardware access during system suspend

[Geert Uytterhoeven]

On r8a7791/koelsch, sometimes the following message is printed during
system suspend:

    rcar_thermal e61f0000.thermal: thermal sensor was broken

This happens if the workqueue runs while the device is already
suspended.  Fix this by using the freezable system workqueue instead,
cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from happening
during system suspend").

Fixes: e0a5172e9eec7f0d ("thermal: rcar: add interrupt support")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
v2:
  - Add Reviewed-by.
---
 drivers/thermal/rcar_thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index 78f932822d381c9d..ea132e122b174757 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -434,8 +434,8 @@ static irqreturn_t rcar_thermal_irq(int irq, void *data)
 	rcar_thermal_for_each_priv(priv, common) {
 		if (rcar_thermal_had_changed(priv, status)) {
 			rcar_thermal_irq_disable(priv);
-			schedule_delayed_work(&priv->work,
-					      msecs_to_jiffies(300));
+			queue_delayed_work(system_freezable_wq, &priv->work,
+					   msecs_to_jiffies(300));
 		}
 	}
 
-- 
2.17.1

[PATCH v2 2/3] thermal: rcar_thermal: Prevent doing work after unbind

[Geert Uytterhoeven]

When testing bind/unbind on r8a7791/koelsch:

    WARNING: CPU: 1 PID: 697 at lib/debugobjects.c:329 debug_print_object+0x8c/0xb4
    ODEBUG: free active (active state 0) object type: timer_list hint: delayed_work_timer_fn+0x0/0x10

This happens if the workqueue runs after the device has been unbound.
Fix this by cancelling any queued work during remove.

Fixes: e0a5172e9eec7f0d ("thermal: rcar: add interrupt support")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
v2:
  - Add Reviewed-by.
---
 drivers/thermal/rcar_thermal.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/thermal/rcar_thermal.c b/drivers/thermal/rcar_thermal.c
index ea132e122b174757..616ba2fccf410d3b 100644
--- a/drivers/thermal/rcar_thermal.c
+++ b/drivers/thermal/rcar_thermal.c
@@ -453,6 +453,7 @@ static int rcar_thermal_remove(struct platform_device *pdev)
 
 	rcar_thermal_for_each_priv(priv, common) {
 		rcar_thermal_irq_disable(priv);
+		cancel_delayed_work_sync(&priv->work);
 		if (priv->chip->use_of_thermal)
 			thermal_remove_hwmon_sysfs(priv->zone);
 		else
-- 
2.17.1

[PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during system suspend

[Geert Uytterhoeven]

The workqueue used for monitoring the hardware may run while the device
is already suspended.  Fix this by using the freezable system workqueue
instead, cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from
happening during system suspend").

Fixes: 608567aac3206ae8 ("thermal: da9062/61: Thermal junction temperature monitoring driver")
Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Untested due to lack of hardware.

v2:
  - No changes.
---
 drivers/thermal/da9062-thermal.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-thermal.c
index dd8dd947b7f0737c..01b0cb9944577851 100644
--- a/drivers/thermal/da9062-thermal.c
+++ b/drivers/thermal/da9062-thermal.c
@@ -106,7 +106,7 @@ static void da9062_thermal_poll_on(struct work_struct *work)
 					   THERMAL_EVENT_UNSPECIFIED);
 
 		delay = msecs_to_jiffies(thermal->zone->passive_delay);
-		schedule_delayed_work(&thermal->work, delay);
+		queue_delayed_work(system_freezable_wq, &thermal->work, delay);
 		return;
 	}
 
@@ -125,7 +125,7 @@ static irqreturn_t da9062_thermal_irq_handler(int irq, void *data)
 	struct da9062_thermal *thermal = data;
 
 	disable_irq_nosync(thermal->irq);
-	schedule_delayed_work(&thermal->work, 0);
+	queue_delayed_work(system_freezable_wq, &thermal->work, 0);
 
 	return IRQ_HANDLED;
 }
-- 
2.17.1

RE: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during system suspend

[Steve Twiss]

Hi Geert,

Many thanks for looking at this.

On 12 October 2018 08:20 Geert Uytterhoeven wrote:

> Subject: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during
> system suspend
> 
> The workqueue used for monitoring the hardware may run while the device
> is already suspended.  Fix this by using the freezable system workqueue
> instead, cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from
> happening during system suspend").

My thinking was:  this device is a PMIC and it will power the system. So when
the device is turned off, the S/W will also not be running.

Although my assumption only works if the PMIC device is the primary system
power -- this has always been the case so far. And although I don't have any
evidence this will change, it may become untrue in the future of course.

> Fixes: 608567aac3206ae8 ("thermal: da9062/61: Thermal junction temperature
> monitoring driver")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Untested due to lack of hardware.

So, I have not been able to make any time to test this patch yet -- and with
current workloads this might take a bit of time before I get to it.

Regards,
Steve

> v2:
>   - No changes.
> ---
>  drivers/thermal/da9062-thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-
> thermal.c
> index dd8dd947b7f0737c..01b0cb9944577851 100644
> --- a/drivers/thermal/da9062-thermal.c
> +++ b/drivers/thermal/da9062-thermal.c
> @@ -106,7 +106,7 @@ static void da9062_thermal_poll_on(struct work_struct
> *work)
>  					   THERMAL_EVENT_UNSPECIFIED);
> 
>  		delay = msecs_to_jiffies(thermal->zone->passive_delay);
> -		schedule_delayed_work(&thermal->work, delay);
> +		queue_delayed_work(system_freezable_wq, &thermal->work,
> delay);
>  		return;
>  	}
> 
> @@ -125,7 +125,7 @@ static irqreturn_t da9062_thermal_irq_handler(int irq,
> void *data)
>  	struct da9062_thermal *thermal = data;
> 
>  	disable_irq_nosync(thermal->irq);
> -	schedule_delayed_work(&thermal->work, 0);
> +	queue_delayed_work(system_freezable_wq, &thermal->work, 0);
> 
>  	return IRQ_HANDLED;
>  }
> --
> 2.17.1

RE: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during system suspend

[Steve Twiss]

Hi Geert,

Many thanks for looking at this.

On 12 October 2018 08:20 Geert Uytterhoeven wrote:

> Subject: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during
> system suspend
> 
> The workqueue used for monitoring the hardware may run while the device
> is already suspended.  Fix this by using the freezable system workqueue
> instead, cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from
> happening during system suspend").

My thinking was:  this device is a PMIC and it will power the system. So when
the device is turned off, the S/W will also not be running.

Although my assumption only works if the PMIC device is the primary system
power -- this has always been the case so far. And although I don't have any
evidence this will change, it may become untrue in the future of course.

> Fixes: 608567aac3206ae8 ("thermal: da9062/61: Thermal junction temperature
> monitoring driver")
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Untested due to lack of hardware.

So, I have not been able to make any time to test this patch yet -- and with
current workloads this might take a bit of time before I get to it.

Regards,
Steve

> v2:
>   - No changes.
> ---
>  drivers/thermal/da9062-thermal.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/thermal/da9062-thermal.c b/drivers/thermal/da9062-
> thermal.c
> index dd8dd947b7f0737c..01b0cb9944577851 100644
> --- a/drivers/thermal/da9062-thermal.c
> +++ b/drivers/thermal/da9062-thermal.c
> @@ -106,7 +106,7 @@ static void da9062_thermal_poll_on(struct work_struct
> *work)
>  					   THERMAL_EVENT_UNSPECIFIED);
> 
>  		delay = msecs_to_jiffies(thermal->zone->passive_delay);
> -		schedule_delayed_work(&thermal->work, delay);
> +		queue_delayed_work(system_freezable_wq, &thermal->work,
> delay);
>  		return;
>  	}
> 
> @@ -125,7 +125,7 @@ static irqreturn_t da9062_thermal_irq_handler(int irq,
> void *data)
>  	struct da9062_thermal *thermal = data;
> 
>  	disable_irq_nosync(thermal->irq);
> -	schedule_delayed_work(&thermal->work, 0);
> +	queue_delayed_work(system_freezable_wq, &thermal->work, 0);
> 
>  	return IRQ_HANDLED;
>  }
> --
> 2.17.1

Re: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during system suspend

[Geert Uytterhoeven]

Hi Steve,

On Wed, Oct 17, 2018 at 10:57 AM Steve Twiss
<stwiss.opensource@diasemi.com> wrote:
> On 12 October 2018 08:20 Geert Uytterhoeven wrote:
> > Subject: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during
> > system suspend
> >
> > The workqueue used for monitoring the hardware may run while the device
> > is already suspended.  Fix this by using the freezable system workqueue
> > instead, cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from
> > happening during system suspend").
>
> My thinking was:  this device is a PMIC and it will power the system. So when
> the device is turned off, the S/W will also not be running.
>
> Although my assumption only works if the PMIC device is the primary system
> power -- this has always been the case so far. And although I don't have any
> evidence this will change, it may become untrue in the future of course.

This is not about powering off the system, but about suspending the system,
which suspends all drivers.

The issue is that the normal workqueue may run while the system is being
suspended.  Accessing the DA9062 may or may not work at that time,
depending on the i2c controller being usable or not.

Due to the DA9062 being an i2c device, I agree this is different than
for rcar-thermal, where the rcar-thermal device itself cannot be
accessed because it is suspended.

> > Fixes: 608567aac3206ae8 ("thermal: da9062/61: Thermal junction temperature
> > monitoring driver")
> > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > ---
> > Untested due to lack of hardware.
>
> So, I have not been able to make any time to test this patch yet -- and with
> current workloads this might take a bit of time before I get to it.

The main thing to test is what happens when da9062_thermal_poll_on() is
called while the i2c controller is already suspended, and whether that
is mitigated by my patch.
Looking at the function, I guess it starts spewing error messages, and
will continue triggering itself, by virtue of enabling the interrupt again,
without having been able to disable its cause.

> > --- a/drivers/thermal/da9062-thermal.c
> > +++ b/drivers/thermal/da9062-thermal.c
> > @@ -106,7 +106,7 @@ static void da9062_thermal_poll_on(struct work_struct
> > *work)
> >                                          THERMAL_EVENT_UNSPECIFIED);
> >
> >               delay = msecs_to_jiffies(thermal->zone->passive_delay);
> > -             schedule_delayed_work(&thermal->work, delay);
> > +             queue_delayed_work(system_freezable_wq, &thermal->work,
> > delay);
> >               return;
> >       }
> >
> > @@ -125,7 +125,7 @@ static irqreturn_t da9062_thermal_irq_handler(int irq,
> > void *data)
> >       struct da9062_thermal *thermal = data;
> >
> >       disable_irq_nosync(thermal->irq);
> > -     schedule_delayed_work(&thermal->work, 0);
> > +     queue_delayed_work(system_freezable_wq, &thermal->work, 0);
> >
> >       return IRQ_HANDLED;
> >  }

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

RE: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during system suspend

[Steve Twiss]

Hi Geert,

On 17 October 2018 10:14 Geert Uytterhoeven wrote:

> Subject: Re: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during
> system suspend
> 
> Hi Steve,
> 
> On Wed, Oct 17, 2018 at 10:57 AM Steve Twiss wrote:
> > On 12 October 2018 08:20 Geert Uytterhoeven wrote:
> > > Subject: [PATCH v2 3/3] thermal: da9062/61: Prevent hardware access during
> > > system suspend
> > >
> > > The workqueue used for monitoring the hardware may run while the device
> > > is already suspended.  Fix this by using the freezable system workqueue
> > > instead, cfr. commit 51e20d0e3a60cf46 ("thermal: Prevent polling from
> > > happening during system suspend").
> >
> > My thinking was:  this device is a PMIC and it will power the system. So when
> > the device is turned off, the S/W will also not be running.
> >
> > Although my assumption only works if the PMIC device is the primary system
> > power -- this has always been the case so far. And although I don't have any
> > evidence this will change, it may become untrue in the future of course.
> 
> This is not about powering off the system, but about suspending the system,
> which suspends all drivers.
> 
> The issue is that the normal workqueue may run while the system is being
> suspended.  Accessing the DA9062 may or may not work at that time,
> depending on the i2c controller being usable or not.

I see now. Yes.

> 
> Due to the DA9062 being an i2c device, I agree this is different than
> for rcar-thermal, where the rcar-thermal device itself cannot be
> accessed because it is suspended.
> 
> > > Fixes: 608567aac3206ae8 ("thermal: da9062/61: Thermal junction temperature
> > > monitoring driver")
> > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > ---
> > > Untested due to lack of hardware.
> >
> > So, I have not been able to make any time to test this patch yet -- and with
> > current workloads this might take a bit of time before I get to it.
> 
> The main thing to test is what happens when da9062_thermal_poll_on() is
> called while the i2c controller is already suspended, and whether that
> is mitigated by my patch.
> Looking at the function, I guess it starts spewing error messages, and
> will continue triggering itself, by virtue of enabling the interrupt again,
> without having been able to disable its cause.

We have definitely seen similar things like this before, exactly as you described
when going into suspend and the I2C controller disappears. 
There will be a window of opportunity for this to happen. 

I have still not tested, but:

Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>

I will make time to test your changes.

Regards,
Steve

> 
> > > --- a/drivers/thermal/da9062-thermal.c
> > > +++ b/drivers/thermal/da9062-thermal.c
> > > @@ -106,7 +106,7 @@ static void da9062_thermal_poll_on(struct
> work_struct
> > > *work)
> > >                                          THERMAL_EVENT_UNSPECIFIED);
> > >
> > >               delay = msecs_to_jiffies(thermal->zone->passive_delay);
> > > -             schedule_delayed_work(&thermal->work, delay);
> > > +             queue_delayed_work(system_freezable_wq, &thermal->work,
> > > delay);
> > >               return;
> > >       }
> > >
> > > @@ -125,7 +125,7 @@ static irqreturn_t da9062_thermal_irq_handler(int irq,
> > > void *data)
> > >       struct da9062_thermal *thermal = data;
> > >
> > >       disable_irq_nosync(thermal->irq);
> > > -     schedule_delayed_work(&thermal->work, 0);
> > > +     queue_delayed_work(system_freezable_wq, &thermal->work, 0);
> > >
> > >       return IRQ_HANDLED;
> > >  }
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds