linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
@ 2019-04-11 10:03 Jiada Wang
  2019-04-16 17:48 ` Eugeniu Rosca
  2019-04-16 19:22 ` Daniel Lezcano
  0 siblings, 2 replies; 10+ messages in thread
From: Jiada Wang @ 2019-04-11 10:03 UTC (permalink / raw)
  To: rui.zhang, edubezval, daniel.lezcano; +Cc: linux-pm, linux-kernel, jiada_wang

Currently IRQ is remain enabled after .remove, later if device is probed,
IRQ is requested before .thermal_init, this may cause IRQ function be
triggered but not able to clear IRQ status, thus cause system to hang.

this patch by moving request of IRQ after device initialization to
avoid this issue.

Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
---
 drivers/thermal/rcar_gen3_thermal.c | 48 ++++++++++++++++-------------
 1 file changed, 26 insertions(+), 22 deletions(-)

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 88fa41cf16e8..4d095d7f9763 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -375,28 +375,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, priv);
 
-	/*
-	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
-	 * to trigger on the low and high trip points of the current
-	 * temp window at this point.
-	 */
-	for (i = 0; i < 2; i++) {
-		irq = platform_get_irq(pdev, i);
-		if (irq < 0)
-			return irq;
-
-		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
-					 dev_name(dev), i);
-		if (!irqname)
-			return -ENOMEM;
-
-		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
-						rcar_gen3_thermal_irq_thread,
-						IRQF_SHARED, irqname, priv);
-		if (ret)
-			return ret;
-	}
-
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 
@@ -458,6 +436,32 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		goto error_unregister;
 	}
 
+	/*
+	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
+	 * to trigger on the low and high trip points of the current
+	 * temp window at this point.
+	 */
+	for (i = 0; i < 2; i++) {
+		irq = platform_get_irq(pdev, i);
+		if (irq < 0) {
+			ret = irq;
+			goto error_unregister;
+		}
+
+		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
+					 dev_name(dev), i);
+		if (!irqname) {
+			ret = -ENOMEM;
+			goto error_unregister;
+		}
+
+		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
+						rcar_gen3_thermal_irq_thread,
+						IRQF_SHARED, irqname, priv);
+		if (ret)
+			goto error_unregister;
+	}
+
 	rcar_thermal_irq_set(priv, true);
 
 	return 0;
-- 
2.19.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-11 10:03 [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization Jiada Wang
@ 2019-04-16 17:48 ` Eugeniu Rosca
  2019-04-17  4:40   ` Jiada Wang
  2019-04-23 10:01   ` Simon Horman
  2019-04-16 19:22 ` Daniel Lezcano
  1 sibling, 2 replies; 10+ messages in thread
From: Eugeniu Rosca @ 2019-04-16 17:48 UTC (permalink / raw)
  To: Jiada Wang
  Cc: linux-pm, linux-kernel, Zhang Rui, Eduardo Valentin,
	Simon Horman, Niklas Söderlund, Geert Uytterhoeven,
	Sergei Shtylyov, Marek Vasut, Kuninori Morimoto, Hien Dang,
	Fabrizio Castro, Dien Pham, Daniel Lezcano, Biju Das,
	George G. Davis, Joshua Frkuska, Eugeniu Rosca, Eugeniu Rosca

Hi Jiada,

Adding below people, since they've made recent contributions to the
driver and might be interested in your patch:

git log master --since="1 year" -- drivers/thermal/rcar_gen3_thermal.c \
    | grep -o "\-by:.*" | sed 's/\-by: //' | sort | uniq -c | sort -rn
      7 Eduardo Valentin <edubezval@gmail.com>
      6 Simon Horman <horms+renesas@verge.net.au>
      5 Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
      2 Geert Uytterhoeven <geert+renesas@glider.be>
      1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
      1 Marek Vasut <marek.vasut+renesas@gmail.com>
      1 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
      1 Hien Dang <hien.dang.eb@renesas.com>
      1 Fabrizio Castro <fabrizio.castro@bp.renesas.com>
      1 Dien Pham <dien.pham.ry@renesas.com>
      1 Daniel Lezcano <daniel.lezcano@linaro.org>
      1 Biju Das <biju.das@bp.renesas.com>

I confirm that loading and unloading the rcar3 thermal driver in a
loop produces soft lockup using v5.1-rc5-10-g618d919cae2f on
H3-ES2.0-Salvator-X. 

Full log and .config can be found here:
https://gist.github.com/erosca/1f76b6dd897cdc39581fca475155e363

I post an excerpt from the above [1] (why not including it in the
description?). Also, why not rephrasing the commit summary line in such
a way that everybody understands this patch fixes a severe issue, e.g.
"thermal: rcar_gen3_thermal: Fix soft lockup on probe" ?

BTW, with this patch applied I left the thermal driver being
loaded/unloaded on the target for over one hour w/o seeing the issue
reproduced. So, while there might be slight variations in how the final
solution looks like, I think the patch already deserves a:

Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>

[1] Soft lockup reproduced with v5.1-rc5-10-g618d919cae2f

root@rcar-gen3:~# while true; do rmmod rcar_gen3_thermal; modprobe rcar_gen3_thermal; done
[   43.439043] rcar_gen3_thermal e6198000.thermal: TSC0: Loaded 0 trip points
[   43.451670] rcar_gen3_thermal e6198000.thermal: TSC1: Loaded 0 trip points
[   43.463974] rcar_gen3_thermal e6198000.thermal: TSC2: Loaded 0 trip points

[..]

[  553.966104] rcar_gen3_thermal e6198000.thermal: TSC0: Loaded 0 trip points
[  553.978759] rcar_gen3_thermal e6198000.thermal: TSC1: Loaded 0 trip points
[  553.991058] rcar_gen3_thermal e6198000.thermal: TSC2: Loaded 0 trip points
[  562.235306] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD25)
[  567.353336] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD13)
[  572.473318] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD13)
[  577.593328] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  579.189148] rcu: INFO: rcu_preempt self-detected stall on CPU
[  579.195329] rcu:     0-....: (1 GPs behind) idle=b76/1/0x4000000000000004 softirq=263851/263851 fqs=6251 last_accelerate: e095/4240, Nonlazy posted: ...
[  579.209711] rcu:      (t=25008 jiffies g=346801 q=468)
[  579.214801] Task dump for CPU 0:
[  579.218178] modprobe        R  running task        0  6337   1420 0x0000002a
[  579.225514] Call trace:
[  579.228103]  dump_backtrace+0x0/0x1dc
[  579.231934]  show_stack+0x24/0x30
[  579.235410]  sched_show_task+0x31c/0x36c
[  579.239507]  dump_cpu_task+0xb0/0xc0
[  579.243248]  rcu_dump_cpu_stacks+0x220/0x238
[  579.247702]  rcu_sched_clock_irq+0x8a4/0x141c
[  579.252249]  update_process_times+0x34/0x64
[  579.256617]  tick_sched_handle+0x80/0x98
[  579.260714]  tick_sched_timer+0x64/0xbc
[  579.264722]  __hrtimer_run_queues+0x5c0/0xb84
[  579.269266]  hrtimer_interrupt+0x1ec/0x454
[  579.273547]  arch_timer_handler_phys+0x40/0x58
[  579.278185]  handle_percpu_devid_irq+0x174/0x6e8
[  579.282999]  generic_handle_irq+0x3c/0x54
[  579.287185]  __handle_domain_irq+0x114/0x118
[  579.291639]  gic_handle_irq+0x70/0xac
[  579.295465]  el1_irq+0xbc/0x180
[  579.298756]  __asan_load8+0x8c/0x9c
[  579.302403]  rcu_is_watching+0x80/0x8c
[  579.306322]  rebalance_domains+0x12c/0x584
[  579.310599]  run_rebalance_domains+0x1f4/0x298
[  579.315231]  __do_softirq+0x4c0/0xab8
[  579.319061]  irq_exit+0x148/0x1d8
[  579.322530]  __handle_domain_irq+0xc0/0x118
[  579.326894]  gic_handle_irq+0x70/0xac
[  579.330720]  el1_irq+0xbc/0x180
[  579.334012]  lock_is_held_type+0xec/0x144
[  579.338201]  rcu_read_lock_sched_held+0x90/0x98
[  579.342927]  kmem_cache_alloc+0x328/0x3e0
[  579.347114]  create_object+0x5c/0x39c
[  579.350944]  kmemleak_alloc+0x54/0x88
[  579.354774]  __kmalloc_track_caller+0x1c8/0x434
[  579.359499]  devres_alloc_node+0x40/0x8c
[  579.363597]  __devm_request_region+0x48/0xc8
[  579.368055]  devm_ioremap_resource+0xcc/0x148
[  579.372626]  rcar_gen3_thermal_probe+0x288/0x618 [rcar_gen3_thermal]
[  579.379231]  platform_drv_probe+0x70/0xe4
[  579.383420]  really_probe+0x2d8/0x3d8
[  579.387249]  driver_probe_device+0x154/0x164
[  579.391705]  device_driver_attach+0x98/0xa0
[  579.396070]  __driver_attach+0xf0/0xf4
[  579.399987]  bus_for_each_dev+0x114/0x13c
[  579.404173]  driver_attach+0x38/0x44
[  579.407912]  bus_add_driver+0x234/0x288
[  579.411919]  driver_register+0x148/0x190
[  579.416015]  __platform_driver_register+0x84/0x90
[  579.420931]  rcar_gen3_thermal_driver_init+0x28/0x1000 [rcar_gen3_thermal]
[  579.428074]  do_one_initcall+0x124/0x68c
[  579.432173]  do_init_module+0xb4/0x300
[  579.436090]  load_module+0x2c90/0x2f18
[  579.440008]  __se_sys_finit_module+0x128/0x148
[  579.444642]  __arm64_sys_finit_module+0x4c/0x5c
[  579.449367]  el0_svc_common+0xd0/0x16c
[  579.453283]  el0_svc_handler+0x94/0xa0
[  579.457200]  el0_svc+0x8/0xc
[  582.713314] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  587.833305] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  592.953323] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  598.073430] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  603.193306] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
[  604.242120] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [modprobe:6337]
[..]

Best regards,
Eugeniu.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-11 10:03 [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization Jiada Wang
  2019-04-16 17:48 ` Eugeniu Rosca
@ 2019-04-16 19:22 ` Daniel Lezcano
  2019-04-17  3:01   ` Jiada Wang
  1 sibling, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2019-04-16 19:22 UTC (permalink / raw)
  To: Jiada Wang, rui.zhang, edubezval; +Cc: linux-pm, linux-kernel

On 11/04/2019 12:03, Jiada Wang wrote:
> Currently IRQ is remain enabled after .remove, later if device is probed,
> IRQ is requested before .thermal_init, this may cause IRQ function be
> triggered but not able to clear IRQ status, thus cause system to hang.
> 
> this patch by moving request of IRQ after device initialization to
> avoid this issue.

Why not disable the interrupt and clear the irq status in the .remove
callback, so the place is clean when probing again?


        struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);

        rcar_thermal_irq_set(priv, false);

should do the trick no ?

> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 48 ++++++++++++++++-------------
>  1 file changed, 26 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
> index 88fa41cf16e8..4d095d7f9763 100644
> --- a/drivers/thermal/rcar_gen3_thermal.c
> +++ b/drivers/thermal/rcar_gen3_thermal.c
> @@ -375,28 +375,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  
>  	platform_set_drvdata(pdev, priv);
>  
> -	/*
> -	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
> -	 * to trigger on the low and high trip points of the current
> -	 * temp window at this point.
> -	 */
> -	for (i = 0; i < 2; i++) {
> -		irq = platform_get_irq(pdev, i);
> -		if (irq < 0)
> -			return irq;
> -
> -		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> -					 dev_name(dev), i);
> -		if (!irqname)
> -			return -ENOMEM;
> -
> -		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
> -						rcar_gen3_thermal_irq_thread,
> -						IRQF_SHARED, irqname, priv);
> -		if (ret)
> -			return ret;
> -	}
> -
>  	pm_runtime_enable(dev);
>  	pm_runtime_get_sync(dev);
>  
> @@ -458,6 +436,32 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>  		goto error_unregister;
>  	}
>  
> +	/*
> +	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
> +	 * to trigger on the low and high trip points of the current
> +	 * temp window at this point.
> +	 */
> +	for (i = 0; i < 2; i++) {
> +		irq = platform_get_irq(pdev, i);
> +		if (irq < 0) {
> +			ret = irq;
> +			goto error_unregister;
> +		}
> +
> +		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
> +					 dev_name(dev), i);
> +		if (!irqname) {
> +			ret = -ENOMEM;
> +			goto error_unregister;
> +		}
> +
> +		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
> +						rcar_gen3_thermal_irq_thread,
> +						IRQF_SHARED, irqname, priv);
> +		if (ret)
> +			goto error_unregister;
> +	}
> +
>  	rcar_thermal_irq_set(priv, true);
>  
>  	return 0;
> 


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-16 19:22 ` Daniel Lezcano
@ 2019-04-17  3:01   ` Jiada Wang
  2019-04-17  8:05     ` Daniel Lezcano
  0 siblings, 1 reply; 10+ messages in thread
From: Jiada Wang @ 2019-04-17  3:01 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, edubezval; +Cc: linux-pm, linux-kernel

Hi Daniel

On 2019/04/17 4:22, Daniel Lezcano wrote:
> On 11/04/2019 12:03, Jiada Wang wrote:
>> Currently IRQ is remain enabled after .remove, later if device is probed,
>> IRQ is requested before .thermal_init, this may cause IRQ function be
>> triggered but not able to clear IRQ status, thus cause system to hang.
>>
>> this patch by moving request of IRQ after device initialization to
>> avoid this issue.
> 
> Why not disable the interrupt and clear the irq status in the .remove
> callback, so the place is clean when probing again?
> 
> 
>          struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
> 
>          rcar_thermal_irq_set(priv, false);
> 
> should do the trick no ?
> 
yes, this issue also can be addressed by disable the interrupt in .remove.

But there is race condition between .remove and irq thread function,
(which enables IRQ)
so driver need to ensure threaded irq has been disabled before call 
rcar_thermal_irq_set(priv, false) in .remove.
this adds additional complexity.

I am fine with both solutions,
what is your opinion?

Thanks,
Jiada

>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>>   drivers/thermal/rcar_gen3_thermal.c | 48 ++++++++++++++++-------------
>>   1 file changed, 26 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
>> index 88fa41cf16e8..4d095d7f9763 100644
>> --- a/drivers/thermal/rcar_gen3_thermal.c
>> +++ b/drivers/thermal/rcar_gen3_thermal.c
>> @@ -375,28 +375,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>>   
>>   	platform_set_drvdata(pdev, priv);
>>   
>> -	/*
>> -	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
>> -	 * to trigger on the low and high trip points of the current
>> -	 * temp window at this point.
>> -	 */
>> -	for (i = 0; i < 2; i++) {
>> -		irq = platform_get_irq(pdev, i);
>> -		if (irq < 0)
>> -			return irq;
>> -
>> -		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>> -					 dev_name(dev), i);
>> -		if (!irqname)
>> -			return -ENOMEM;
>> -
>> -		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
>> -						rcar_gen3_thermal_irq_thread,
>> -						IRQF_SHARED, irqname, priv);
>> -		if (ret)
>> -			return ret;
>> -	}
>> -
>>   	pm_runtime_enable(dev);
>>   	pm_runtime_get_sync(dev);
>>   
>> @@ -458,6 +436,32 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
>>   		goto error_unregister;
>>   	}
>>   
>> +	/*
>> +	 * Request 2 (of the 3 possible) IRQs, the driver only needs to
>> +	 * to trigger on the low and high trip points of the current
>> +	 * temp window at this point.
>> +	 */
>> +	for (i = 0; i < 2; i++) {
>> +		irq = platform_get_irq(pdev, i);
>> +		if (irq < 0) {
>> +			ret = irq;
>> +			goto error_unregister;
>> +		}
>> +
>> +		irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>> +					 dev_name(dev), i);
>> +		if (!irqname) {
>> +			ret = -ENOMEM;
>> +			goto error_unregister;
>> +		}
>> +
>> +		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
>> +						rcar_gen3_thermal_irq_thread,
>> +						IRQF_SHARED, irqname, priv);
>> +		if (ret)
>> +			goto error_unregister;
>> +	}
>> +
>>   	rcar_thermal_irq_set(priv, true);
>>   
>>   	return 0;
>>
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-16 17:48 ` Eugeniu Rosca
@ 2019-04-17  4:40   ` Jiada Wang
  2019-04-23 10:01   ` Simon Horman
  1 sibling, 0 replies; 10+ messages in thread
From: Jiada Wang @ 2019-04-17  4:40 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: linux-pm, linux-kernel, Zhang Rui, Eduardo Valentin,
	Simon Horman, Niklas Söderlund, Geert Uytterhoeven,
	Sergei Shtylyov, Marek Vasut, Kuninori Morimoto, Hien Dang,
	Fabrizio Castro, Dien Pham, Daniel Lezcano, Biju Das,
	George G. Davis, Joshua Frkuska, Eugeniu Rosca

Hi Eugeniu

thanks for your test & comments and adding more people for review

I will add necessary backtrace information to description and
rephrase commit summary in V2 patch

Thanks,
Jiada

On 2019/04/17 2:48, Eugeniu Rosca wrote:
> Hi Jiada,
> 
> Adding below people, since they've made recent contributions to the
> driver and might be interested in your patch:
> 
> git log master --since="1 year" -- drivers/thermal/rcar_gen3_thermal.c \
>      | grep -o "\-by:.*" | sed 's/\-by: //' | sort | uniq -c | sort -rn
>        7 Eduardo Valentin <edubezval@gmail.com>
>        6 Simon Horman <horms+renesas@verge.net.au>
>        5 Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>        2 Geert Uytterhoeven <geert+renesas@glider.be>
>        1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>        1 Marek Vasut <marek.vasut+renesas@gmail.com>
>        1 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>        1 Hien Dang <hien.dang.eb@renesas.com>
>        1 Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>        1 Dien Pham <dien.pham.ry@renesas.com>
>        1 Daniel Lezcano <daniel.lezcano@linaro.org>
>        1 Biju Das <biju.das@bp.renesas.com>
> 
> I confirm that loading and unloading the rcar3 thermal driver in a
> loop produces soft lockup using v5.1-rc5-10-g618d919cae2f on
> H3-ES2.0-Salvator-X.
> 
> Full log and .config can be found here:
> https://gist.github.com/erosca/1f76b6dd897cdc39581fca475155e363
> 
> I post an excerpt from the above [1] (why not including it in the
> description?). Also, why not rephrasing the commit summary line in such
> a way that everybody understands this patch fixes a severe issue, e.g.
> "thermal: rcar_gen3_thermal: Fix soft lockup on probe" ?
> 
> BTW, with this patch applied I left the thermal driver being
> loaded/unloaded on the target for over one hour w/o seeing the issue
> reproduced. So, while there might be slight variations in how the final
> solution looks like, I think the patch already deserves a:
> 
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> [1] Soft lockup reproduced with v5.1-rc5-10-g618d919cae2f
> 
> root@rcar-gen3:~# while true; do rmmod rcar_gen3_thermal; modprobe rcar_gen3_thermal; done
> [   43.439043] rcar_gen3_thermal e6198000.thermal: TSC0: Loaded 0 trip points
> [   43.451670] rcar_gen3_thermal e6198000.thermal: TSC1: Loaded 0 trip points
> [   43.463974] rcar_gen3_thermal e6198000.thermal: TSC2: Loaded 0 trip points
> 
> [..]
> 
> [  553.966104] rcar_gen3_thermal e6198000.thermal: TSC0: Loaded 0 trip points
> [  553.978759] rcar_gen3_thermal e6198000.thermal: TSC1: Loaded 0 trip points
> [  553.991058] rcar_gen3_thermal e6198000.thermal: TSC2: Loaded 0 trip points
> [  562.235306] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD25)
> [  567.353336] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD13)
> [  572.473318] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD13)
> [  577.593328] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  579.189148] rcu: INFO: rcu_preempt self-detected stall on CPU
> [  579.195329] rcu:     0-....: (1 GPs behind) idle=b76/1/0x4000000000000004 softirq=263851/263851 fqs=6251 last_accelerate: e095/4240, Nonlazy posted: ...
> [  579.209711] rcu:      (t=25008 jiffies g=346801 q=468)
> [  579.214801] Task dump for CPU 0:
> [  579.218178] modprobe        R  running task        0  6337   1420 0x0000002a
> [  579.225514] Call trace:
> [  579.228103]  dump_backtrace+0x0/0x1dc
> [  579.231934]  show_stack+0x24/0x30
> [  579.235410]  sched_show_task+0x31c/0x36c
> [  579.239507]  dump_cpu_task+0xb0/0xc0
> [  579.243248]  rcu_dump_cpu_stacks+0x220/0x238
> [  579.247702]  rcu_sched_clock_irq+0x8a4/0x141c
> [  579.252249]  update_process_times+0x34/0x64
> [  579.256617]  tick_sched_handle+0x80/0x98
> [  579.260714]  tick_sched_timer+0x64/0xbc
> [  579.264722]  __hrtimer_run_queues+0x5c0/0xb84
> [  579.269266]  hrtimer_interrupt+0x1ec/0x454
> [  579.273547]  arch_timer_handler_phys+0x40/0x58
> [  579.278185]  handle_percpu_devid_irq+0x174/0x6e8
> [  579.282999]  generic_handle_irq+0x3c/0x54
> [  579.287185]  __handle_domain_irq+0x114/0x118
> [  579.291639]  gic_handle_irq+0x70/0xac
> [  579.295465]  el1_irq+0xbc/0x180
> [  579.298756]  __asan_load8+0x8c/0x9c
> [  579.302403]  rcu_is_watching+0x80/0x8c
> [  579.306322]  rebalance_domains+0x12c/0x584
> [  579.310599]  run_rebalance_domains+0x1f4/0x298
> [  579.315231]  __do_softirq+0x4c0/0xab8
> [  579.319061]  irq_exit+0x148/0x1d8
> [  579.322530]  __handle_domain_irq+0xc0/0x118
> [  579.326894]  gic_handle_irq+0x70/0xac
> [  579.330720]  el1_irq+0xbc/0x180
> [  579.334012]  lock_is_held_type+0xec/0x144
> [  579.338201]  rcu_read_lock_sched_held+0x90/0x98
> [  579.342927]  kmem_cache_alloc+0x328/0x3e0
> [  579.347114]  create_object+0x5c/0x39c
> [  579.350944]  kmemleak_alloc+0x54/0x88
> [  579.354774]  __kmalloc_track_caller+0x1c8/0x434
> [  579.359499]  devres_alloc_node+0x40/0x8c
> [  579.363597]  __devm_request_region+0x48/0xc8
> [  579.368055]  devm_ioremap_resource+0xcc/0x148
> [  579.372626]  rcar_gen3_thermal_probe+0x288/0x618 [rcar_gen3_thermal]
> [  579.379231]  platform_drv_probe+0x70/0xe4
> [  579.383420]  really_probe+0x2d8/0x3d8
> [  579.387249]  driver_probe_device+0x154/0x164
> [  579.391705]  device_driver_attach+0x98/0xa0
> [  579.396070]  __driver_attach+0xf0/0xf4
> [  579.399987]  bus_for_each_dev+0x114/0x13c
> [  579.404173]  driver_attach+0x38/0x44
> [  579.407912]  bus_add_driver+0x234/0x288
> [  579.411919]  driver_register+0x148/0x190
> [  579.416015]  __platform_driver_register+0x84/0x90
> [  579.420931]  rcar_gen3_thermal_driver_init+0x28/0x1000 [rcar_gen3_thermal]
> [  579.428074]  do_one_initcall+0x124/0x68c
> [  579.432173]  do_init_module+0xb4/0x300
> [  579.436090]  load_module+0x2c90/0x2f18
> [  579.440008]  __se_sys_finit_module+0x128/0x148
> [  579.444642]  __arm64_sys_finit_module+0x4c/0x5c
> [  579.449367]  el0_svc_common+0xd0/0x16c
> [  579.453283]  el0_svc_handler+0x94/0xa0
> [  579.457200]  el0_svc+0x8/0xc
> [  582.713314] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  587.833305] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  592.953323] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  598.073430] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  603.193306] renesas_sdhi_internal_dmac ee100000.sd: timeout waiting for hardware interrupt (CMD12)
> [  604.242120] watchdog: BUG: soft lockup - CPU#0 stuck for 22s! [modprobe:6337]
> [..]
> 
> Best regards,
> Eugeniu.
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-17  3:01   ` Jiada Wang
@ 2019-04-17  8:05     ` Daniel Lezcano
  2019-04-18 11:36       ` Jiada Wang
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Lezcano @ 2019-04-17  8:05 UTC (permalink / raw)
  To: Jiada Wang, rui.zhang, edubezval; +Cc: linux-pm, linux-kernel

On 17/04/2019 05:01, Jiada Wang wrote:
> Hi Daniel
> 
> On 2019/04/17 4:22, Daniel Lezcano wrote:
>> On 11/04/2019 12:03, Jiada Wang wrote:
>>> Currently IRQ is remain enabled after .remove, later if device is
>>> probed,
>>> IRQ is requested before .thermal_init, this may cause IRQ function be
>>> triggered but not able to clear IRQ status, thus cause system to hang.
>>>
>>> this patch by moving request of IRQ after device initialization to
>>> avoid this issue.
>>
>> Why not disable the interrupt and clear the irq status in the .remove
>> callback, so the place is clean when probing again?
>>
>>
>>          struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
>>
>>          rcar_thermal_irq_set(priv, false);
>>
>> should do the trick no ?
>>
> yes, this issue also can be addressed by disable the interrupt in .remove.
> 
> But there is race condition between .remove and irq thread function,
> (which enables IRQ)
> so driver need to ensure threaded irq has been disabled before call
> rcar_thermal_irq_set(priv, false) in .remove.
> this adds additional complexity.
> 
> I am fine with both solutions,
> what is your opinion?

My opinion is it is the tree hiding the forest.

After a quick look at the irq setup and handling, it appears the
implementation is cumbersome. This part should be fixed before the rest:

 - check IRQF_ONESHOT flag
 - remove the lock in the interrupt handlers
 - remove rcar_gen3_thermal_irq() which is pointless
 - check the IRQF_SHARED flag is correct (I doubt)

As the function devm_request_threaded_irq() is called 3 times, you
should add the priv->tscs[i]->zone in the private data and the irq
thread handler should look like:

static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
{
	struct thermal_zone_device *tz = data;

	thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);

	[ ... ]

        return IRQ_HANDLED;
}

When the implementation is fixed, then we can take care of the .remove

>>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>>> ---
>>>   drivers/thermal/rcar_gen3_thermal.c | 48 ++++++++++++++++-------------
>>>   1 file changed, 26 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/thermal/rcar_gen3_thermal.c
>>> b/drivers/thermal/rcar_gen3_thermal.c
>>> index 88fa41cf16e8..4d095d7f9763 100644
>>> --- a/drivers/thermal/rcar_gen3_thermal.c
>>> +++ b/drivers/thermal/rcar_gen3_thermal.c
>>> @@ -375,28 +375,6 @@ static int rcar_gen3_thermal_probe(struct
>>> platform_device *pdev)
>>>         platform_set_drvdata(pdev, priv);
>>>   -    /*
>>> -     * Request 2 (of the 3 possible) IRQs, the driver only needs to
>>> -     * to trigger on the low and high trip points of the current
>>> -     * temp window at this point.
>>> -     */
>>> -    for (i = 0; i < 2; i++) {
>>> -        irq = platform_get_irq(pdev, i);
>>> -        if (irq < 0)
>>> -            return irq;
>>> -
>>> -        irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>>> -                     dev_name(dev), i);
>>> -        if (!irqname)
>>> -            return -ENOMEM;
>>> -
>>> -        ret = devm_request_threaded_irq(dev, irq,
>>> rcar_gen3_thermal_irq,
>>> -                        rcar_gen3_thermal_irq_thread,
>>> -                        IRQF_SHARED, irqname, priv);
>>> -        if (ret)
>>> -            return ret;
>>> -    }
>>> -
>>>       pm_runtime_enable(dev);
>>>       pm_runtime_get_sync(dev);
>>>   @@ -458,6 +436,32 @@ static int rcar_gen3_thermal_probe(struct
>>> platform_device *pdev)
>>>           goto error_unregister;
>>>       }
>>>   +    /*
>>> +     * Request 2 (of the 3 possible) IRQs, the driver only needs to
>>> +     * to trigger on the low and high trip points of the current
>>> +     * temp window at this point.
>>> +     */
>>> +    for (i = 0; i < 2; i++) {
>>> +        irq = platform_get_irq(pdev, i);
>>> +        if (irq < 0) {
>>> +            ret = irq;
>>> +            goto error_unregister;
>>> +        }
>>> +
>>> +        irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>>> +                     dev_name(dev), i);
>>> +        if (!irqname) {
>>> +            ret = -ENOMEM;
>>> +            goto error_unregister;
>>> +        }
>>> +
>>> +        ret = devm_request_threaded_irq(dev, irq,
>>> rcar_gen3_thermal_irq,
>>> +                        rcar_gen3_thermal_irq_thread,
>>> +                        IRQF_SHARED, irqname, priv);
>>> +        if (ret)
>>> +            goto error_unregister;
>>> +    }
>>> +
>>>       rcar_thermal_irq_set(priv, true);
>>>         return 0;
>>>
>>
>>


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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-17  8:05     ` Daniel Lezcano
@ 2019-04-18 11:36       ` Jiada Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Jiada Wang @ 2019-04-18 11:36 UTC (permalink / raw)
  To: Daniel Lezcano, rui.zhang, edubezval
  Cc: linux-pm, linux-kernel, erosca, Frkuska, Joshua

Hi Daniel

Thanks for your comments

On 2019/04/17 17:05, Daniel Lezcano wrote:
> On 17/04/2019 05:01, Jiada Wang wrote:
>> Hi Daniel
>>
>> On 2019/04/17 4:22, Daniel Lezcano wrote:
>>> On 11/04/2019 12:03, Jiada Wang wrote:
>>>> Currently IRQ is remain enabled after .remove, later if device is
>>>> probed,
>>>> IRQ is requested before .thermal_init, this may cause IRQ function be
>>>> triggered but not able to clear IRQ status, thus cause system to hang.
>>>>
>>>> this patch by moving request of IRQ after device initialization to
>>>> avoid this issue.
>>>
>>> Why not disable the interrupt and clear the irq status in the .remove
>>> callback, so the place is clean when probing again?
>>>
>>>
>>>           struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
>>>
>>>           rcar_thermal_irq_set(priv, false);
>>>
>>> should do the trick no ?
>>>
>> yes, this issue also can be addressed by disable the interrupt in .remove.
>>
>> But there is race condition between .remove and irq thread function,
>> (which enables IRQ)
>> so driver need to ensure threaded irq has been disabled before call
>> rcar_thermal_irq_set(priv, false) in .remove.
>> this adds additional complexity.
>>
>> I am fine with both solutions,
>> what is your opinion?
> 
> My opinion is it is the tree hiding the forest.
> 
> After a quick look at the irq setup and handling, it appears the
> implementation is cumbersome. This part should be fixed before the rest:
> 
>   - check IRQF_ONESHOT flag
>   - remove the lock in the interrupt handlers
>   - remove rcar_gen3_thermal_irq() which is pointless
>   - check the IRQF_SHARED flag is correct (I doubt)
> 

yes, I think rather than IRQF_SHARED, IRQF_ONESHOT flag need to be used.
these suggestions make sense, I will add these changes in v2 patch-set

> As the function devm_request_threaded_irq() is called 3 times, you
> should add the priv->tscs[i]->zone in the private data and the irq
> thread handler should look like:
> 
> static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
> {
> 	struct thermal_zone_device *tz = data;
> 
> 	thermal_zone_device_update(tz, THERMAL_EVENT_UNSPECIFIED);
> 
> 	[ ... ]
> 
>          return IRQ_HANDLED;
> }
> 
hmmm, IRQ is requested 2 times to monitor low and high temperature of 
all tzs.

Thanks,
Jiada

> When the implementation is fixed, then we can take care of the .remove
> 
>>>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>>>> ---
>>>>    drivers/thermal/rcar_gen3_thermal.c | 48 ++++++++++++++++-------------
>>>>    1 file changed, 26 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/thermal/rcar_gen3_thermal.c
>>>> b/drivers/thermal/rcar_gen3_thermal.c
>>>> index 88fa41cf16e8..4d095d7f9763 100644
>>>> --- a/drivers/thermal/rcar_gen3_thermal.c
>>>> +++ b/drivers/thermal/rcar_gen3_thermal.c
>>>> @@ -375,28 +375,6 @@ static int rcar_gen3_thermal_probe(struct
>>>> platform_device *pdev)
>>>>          platform_set_drvdata(pdev, priv);
>>>>    -    /*
>>>> -     * Request 2 (of the 3 possible) IRQs, the driver only needs to
>>>> -     * to trigger on the low and high trip points of the current
>>>> -     * temp window at this point.
>>>> -     */
>>>> -    for (i = 0; i < 2; i++) {
>>>> -        irq = platform_get_irq(pdev, i);
>>>> -        if (irq < 0)
>>>> -            return irq;
>>>> -
>>>> -        irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>>>> -                     dev_name(dev), i);
>>>> -        if (!irqname)
>>>> -            return -ENOMEM;
>>>> -
>>>> -        ret = devm_request_threaded_irq(dev, irq,
>>>> rcar_gen3_thermal_irq,
>>>> -                        rcar_gen3_thermal_irq_thread,
>>>> -                        IRQF_SHARED, irqname, priv);
>>>> -        if (ret)
>>>> -            return ret;
>>>> -    }
>>>> -
>>>>        pm_runtime_enable(dev);
>>>>        pm_runtime_get_sync(dev);
>>>>    @@ -458,6 +436,32 @@ static int rcar_gen3_thermal_probe(struct
>>>> platform_device *pdev)
>>>>            goto error_unregister;
>>>>        }
>>>>    +    /*
>>>> +     * Request 2 (of the 3 possible) IRQs, the driver only needs to
>>>> +     * to trigger on the low and high trip points of the current
>>>> +     * temp window at this point.
>>>> +     */
>>>> +    for (i = 0; i < 2; i++) {
>>>> +        irq = platform_get_irq(pdev, i);
>>>> +        if (irq < 0) {
>>>> +            ret = irq;
>>>> +            goto error_unregister;
>>>> +        }
>>>> +
>>>> +        irqname = devm_kasprintf(dev, GFP_KERNEL, "%s:ch%d",
>>>> +                     dev_name(dev), i);
>>>> +        if (!irqname) {
>>>> +            ret = -ENOMEM;
>>>> +            goto error_unregister;
>>>> +        }
>>>> +
>>>> +        ret = devm_request_threaded_irq(dev, irq,
>>>> rcar_gen3_thermal_irq,
>>>> +                        rcar_gen3_thermal_irq_thread,
>>>> +                        IRQF_SHARED, irqname, priv);
>>>> +        if (ret)
>>>> +            goto error_unregister;
>>>> +    }
>>>> +
>>>>        rcar_thermal_irq_set(priv, true);
>>>>          return 0;
>>>>
>>>
>>>
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-16 17:48 ` Eugeniu Rosca
  2019-04-17  4:40   ` Jiada Wang
@ 2019-04-23 10:01   ` Simon Horman
  2019-04-23 11:24     ` Eugeniu Rosca
  1 sibling, 1 reply; 10+ messages in thread
From: Simon Horman @ 2019-04-23 10:01 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Jiada Wang, linux-pm, linux-kernel, Zhang Rui, Eduardo Valentin,
	Niklas Söderlund, Geert Uytterhoeven, Sergei Shtylyov,
	Marek Vasut, Kuninori Morimoto, Hien Dang, Fabrizio Castro,
	Dien Pham, Daniel Lezcano, Biju Das, George G. Davis,
	Joshua Frkuska, Eugeniu Rosca

On Tue, Apr 16, 2019 at 07:48:30PM +0200, Eugeniu Rosca wrote:
> Hi Jiada,
> 
> Adding below people, since they've made recent contributions to the
> driver and might be interested in your patch:
> 
> git log master --since="1 year" -- drivers/thermal/rcar_gen3_thermal.c \
>     | grep -o "\-by:.*" | sed 's/\-by: //' | sort | uniq -c | sort -rn
>       7 Eduardo Valentin <edubezval@gmail.com>
>       6 Simon Horman <horms+renesas@verge.net.au>
>       5 Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
>       2 Geert Uytterhoeven <geert+renesas@glider.be>
>       1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>       1 Marek Vasut <marek.vasut+renesas@gmail.com>
>       1 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>       1 Hien Dang <hien.dang.eb@renesas.com>
>       1 Fabrizio Castro <fabrizio.castro@bp.renesas.com>
>       1 Dien Pham <dien.pham.ry@renesas.com>
>       1 Daniel Lezcano <daniel.lezcano@linaro.org>
>       1 Biju Das <biju.das@bp.renesas.com>
> 
> I confirm that loading and unloading the rcar3 thermal driver in a
> loop produces soft lockup using v5.1-rc5-10-g618d919cae2f on
> H3-ES2.0-Salvator-X. 
> 
> Full log and .config can be found here:
> https://gist.github.com/erosca/1f76b6dd897cdc39581fca475155e363
> 
> I post an excerpt from the above [1] (why not including it in the
> description?). Also, why not rephrasing the commit summary line in such
> a way that everybody understands this patch fixes a severe issue, e.g.
> "thermal: rcar_gen3_thermal: Fix soft lockup on probe" ?
> 
> BTW, with this patch applied I left the thermal driver being
> loaded/unloaded on the target for over one hour w/o seeing the issue
> reproduced. So, while there might be slight variations in how the final
> solution looks like, I think the patch already deserves a:
> 
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> [1] Soft lockup reproduced with v5.1-rc5-10-g618d919cae2f

Thanks,

Unfortunately I do not see the patch in my inbox.
Would you care to repost it including the following recipients.

linux-pm@vger.kernel.org
Zhang Rui <rui.zhang@intel.com>
Eduardo Valentin <edubezval@gmail.com>
linux-renesas-soc@vger.kernel.org
Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Feel free to include the suggestions made by Eugeniu above and post a v2.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-23 10:01   ` Simon Horman
@ 2019-04-23 11:24     ` Eugeniu Rosca
  2019-04-24  8:41       ` Simon Horman
  0 siblings, 1 reply; 10+ messages in thread
From: Eugeniu Rosca @ 2019-04-23 11:24 UTC (permalink / raw)
  To: Simon Horman
  Cc: Jiada Wang, linux-pm, linux-kernel, Zhang Rui, Eduardo Valentin,
	Niklas Söderlund, Geert Uytterhoeven, Sergei Shtylyov,
	Marek Vasut, Kuninori Morimoto, Hien Dang, Fabrizio Castro,
	Dien Pham, Daniel Lezcano, Biju Das, George G. Davis,
	Joshua Frkuska, Eugeniu Rosca, Eugeniu Rosca

Hi Simon,

On Tue, Apr 23, 2019 at 12:01:07PM +0200, Simon Horman wrote:
> On Tue, Apr 16, 2019 at 07:48:30PM +0200, Eugeniu Rosca wrote:
> > Hi Jiada,
> > 
> > Adding below people, since they've made recent contributions to the
> > driver and might be interested in your patch:
> > 
> > git log master --since="1 year" -- drivers/thermal/rcar_gen3_thermal.c \
> >     | grep -o "\-by:.*" | sed 's/\-by: //' | sort | uniq -c | sort -rn
> >       7 Eduardo Valentin <edubezval@gmail.com>
> >       6 Simon Horman <horms+renesas@verge.net.au>
> >       5 Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> >       2 Geert Uytterhoeven <geert+renesas@glider.be>
> >       1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> >       1 Marek Vasut <marek.vasut+renesas@gmail.com>
> >       1 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> >       1 Hien Dang <hien.dang.eb@renesas.com>
> >       1 Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> >       1 Dien Pham <dien.pham.ry@renesas.com>
> >       1 Daniel Lezcano <daniel.lezcano@linaro.org>
> >       1 Biju Das <biju.das@bp.renesas.com>
> > 
> > I confirm that loading and unloading the rcar3 thermal driver in a
> > loop produces soft lockup using v5.1-rc5-10-g618d919cae2f on
> > H3-ES2.0-Salvator-X. 
> > 
> > Full log and .config can be found here:
> > https://gist.github.com/erosca/1f76b6dd897cdc39581fca475155e363
> > 
> > I post an excerpt from the above [1] (why not including it in the
> > description?). Also, why not rephrasing the commit summary line in such
> > a way that everybody understands this patch fixes a severe issue, e.g.
> > "thermal: rcar_gen3_thermal: Fix soft lockup on probe" ?
> > 
> > BTW, with this patch applied I left the thermal driver being
> > loaded/unloaded on the target for over one hour w/o seeing the issue
> > reproduced. So, while there might be slight variations in how the final
> > solution looks like, I think the patch already deserves a:
> > 
> > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > 
> > [1] Soft lockup reproduced with v5.1-rc5-10-g618d919cae2f
> 
> Thanks,
> 
> Unfortunately I do not see the patch in my inbox.
> Would you care to repost it including the following recipients.
> 
> linux-pm@vger.kernel.org
> Zhang Rui <rui.zhang@intel.com>
> Eduardo Valentin <edubezval@gmail.com>
> linux-renesas-soc@vger.kernel.org
> Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> 
> Feel free to include the suggestions made by Eugeniu above and post a v2.

There is a v2 pushed recently:
https://patchwork.kernel.org/cover/10911999/
("[v2,0/2] thermal: rcar_gen3_thermal: fix IRQ issues").

It seems to include both the preparatory work suggested by Daniel
in https://patchwork.kernel.org/patch/10895557/#22592583,
as well as the soft lockup fix itself.

Best regard,
Eugeniu.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization
  2019-04-23 11:24     ` Eugeniu Rosca
@ 2019-04-24  8:41       ` Simon Horman
  0 siblings, 0 replies; 10+ messages in thread
From: Simon Horman @ 2019-04-24  8:41 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Jiada Wang, linux-pm, linux-kernel, Zhang Rui, Eduardo Valentin,
	Niklas Söderlund, Geert Uytterhoeven, Sergei Shtylyov,
	Marek Vasut, Kuninori Morimoto, Hien Dang, Fabrizio Castro,
	Dien Pham, Daniel Lezcano, Biju Das, George G. Davis,
	Joshua Frkuska, Eugeniu Rosca

On Tue, Apr 23, 2019 at 01:24:08PM +0200, Eugeniu Rosca wrote:
> Hi Simon,
> 
> On Tue, Apr 23, 2019 at 12:01:07PM +0200, Simon Horman wrote:
> > On Tue, Apr 16, 2019 at 07:48:30PM +0200, Eugeniu Rosca wrote:
> > > Hi Jiada,
> > > 
> > > Adding below people, since they've made recent contributions to the
> > > driver and might be interested in your patch:
> > > 
> > > git log master --since="1 year" -- drivers/thermal/rcar_gen3_thermal.c \
> > >     | grep -o "\-by:.*" | sed 's/\-by: //' | sort | uniq -c | sort -rn
> > >       7 Eduardo Valentin <edubezval@gmail.com>
> > >       6 Simon Horman <horms+renesas@verge.net.au>
> > >       5 Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > >       2 Geert Uytterhoeven <geert+renesas@glider.be>
> > >       1 Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> > >       1 Marek Vasut <marek.vasut+renesas@gmail.com>
> > >       1 Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> > >       1 Hien Dang <hien.dang.eb@renesas.com>
> > >       1 Fabrizio Castro <fabrizio.castro@bp.renesas.com>
> > >       1 Dien Pham <dien.pham.ry@renesas.com>
> > >       1 Daniel Lezcano <daniel.lezcano@linaro.org>
> > >       1 Biju Das <biju.das@bp.renesas.com>
> > > 
> > > I confirm that loading and unloading the rcar3 thermal driver in a
> > > loop produces soft lockup using v5.1-rc5-10-g618d919cae2f on
> > > H3-ES2.0-Salvator-X. 
> > > 
> > > Full log and .config can be found here:
> > > https://gist.github.com/erosca/1f76b6dd897cdc39581fca475155e363
> > > 
> > > I post an excerpt from the above [1] (why not including it in the
> > > description?). Also, why not rephrasing the commit summary line in such
> > > a way that everybody understands this patch fixes a severe issue, e.g.
> > > "thermal: rcar_gen3_thermal: Fix soft lockup on probe" ?
> > > 
> > > BTW, with this patch applied I left the thermal driver being
> > > loaded/unloaded on the target for over one hour w/o seeing the issue
> > > reproduced. So, while there might be slight variations in how the final
> > > solution looks like, I think the patch already deserves a:
> > > 
> > > Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > 
> > > [1] Soft lockup reproduced with v5.1-rc5-10-g618d919cae2f
> > 
> > Thanks,
> > 
> > Unfortunately I do not see the patch in my inbox.
> > Would you care to repost it including the following recipients.
> > 
> > linux-pm@vger.kernel.org
> > Zhang Rui <rui.zhang@intel.com>
> > Eduardo Valentin <edubezval@gmail.com>
> > linux-renesas-soc@vger.kernel.org
> > Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > 
> > Feel free to include the suggestions made by Eugeniu above and post a v2.
> 
> There is a v2 pushed recently:
> https://patchwork.kernel.org/cover/10911999/
> ("[v2,0/2] thermal: rcar_gen3_thermal: fix IRQ issues").
> 
> It seems to include both the preparatory work suggested by Daniel
> in https://patchwork.kernel.org/patch/10895557/#22592583,
> as well as the soft lockup fix itself.

Thanks, I see v2 now.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2019-04-24  8:41 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-11 10:03 [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization Jiada Wang
2019-04-16 17:48 ` Eugeniu Rosca
2019-04-17  4:40   ` Jiada Wang
2019-04-23 10:01   ` Simon Horman
2019-04-23 11:24     ` Eugeniu Rosca
2019-04-24  8:41       ` Simon Horman
2019-04-16 19:22 ` Daniel Lezcano
2019-04-17  3:01   ` Jiada Wang
2019-04-17  8:05     ` Daniel Lezcano
2019-04-18 11:36       ` Jiada Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).