[PATCH v2 0/2] thermal: rcar_gen3_thermal: fix IRQ issues

[PATCH v2 0/2] thermal: rcar_gen3_thermal: fix IRQ issues

[Jiada Wang]

There are issues with interrupt handling in rcar_gen3_thermal driver.

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.

Since the irq line isn't shared between different devices,
so the proper interrupt type flag should be IRQF_ONESHOT.

This patch-set fix these interrupt handling retated issues. 

---
v2: use irq type IRQF_ONESHOT instead of IRQF_SHARED
    disable interrupt in .remove

v1: initial version

Jiada Wang (2):
  thermal: rcar_gen3_thermal: fix interrupt type
  thermal: rcar_gen3_thermal: disable interrupt in .remove

 drivers/thermal/rcar_gen3_thermal.c | 43 +++++++++--------------------
 1 file changed, 13 insertions(+), 30 deletions(-)

-- 
2.19.2

[PATCH v2 1/2] thermal: rcar_gen3_thermal: fix interrupt type

[Jiada Wang]

Currently IRQF_SHARED type interrupt line is allocated, but it
is not appropriate, as the interrupt line isn't shared between
different devices, instead IRQF_ONESHOT is the proper type.

By changing interrupt type to IRQF_ONESHOT, now irq handler is
no longer needed, as clear of interrupt status can be done in
threaded interrupt context.

Because IRQF_ONESHOT type interrupt line is kept disabled until
the threaded handler has been run, so there is no need to protect
read/write of REG_GEN3_IRQSTR with lock.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index 64035bcec42c..a11032e42f36 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -236,41 +236,21 @@ static void rcar_thermal_irq_set(struct rcar_gen3_thermal_priv *priv, bool on)
 static irqreturn_t rcar_gen3_thermal_irq(int irq, void *data)
 {
 	struct rcar_gen3_thermal_priv *priv = data;
+	unsigned long flags;
+	int i;
 	u32 status;
-	int i, ret = IRQ_HANDLED;
 
-	spin_lock(&priv->lock);
 	for (i = 0; i < priv->num_tscs; i++) {
 		status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR);
 		rcar_gen3_thermal_write(priv->tscs[i], REG_GEN3_IRQSTR, 0);
-		if (status)
-			ret = IRQ_WAKE_THREAD;
-	}
-
-	if (ret == IRQ_WAKE_THREAD)
-		rcar_thermal_irq_set(priv, false);
-
-	spin_unlock(&priv->lock);
 
-	return ret;
-}
-
-static irqreturn_t rcar_gen3_thermal_irq_thread(int irq, void *data)
-{
-	struct rcar_gen3_thermal_priv *priv = data;
-	unsigned long flags;
-	int i;
-
-	for (i = 0; i < priv->num_tscs; i++) {
-		rcar_gen3_thermal_set_irq_temp(priv->tscs[i]);
-		thermal_zone_device_update(priv->tscs[i]->zone,
-					   THERMAL_EVENT_UNSPECIFIED);
+		if (status) {
+			rcar_gen3_thermal_set_irq_temp(priv->tscs[i]);
+			thermal_zone_device_update(priv->tscs[i]->zone,
+						   THERMAL_EVENT_UNSPECIFIED);
+		}
 	}
 
-	spin_lock_irqsave(&priv->lock, flags);
-	rcar_thermal_irq_set(priv, true);
-	spin_unlock_irqrestore(&priv->lock, flags);
-
 	return IRQ_HANDLED;
 }
 
@@ -397,9 +377,9 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev)
 		if (!irqname)
 			return -ENOMEM;
 
-		ret = devm_request_threaded_irq(dev, irq, rcar_gen3_thermal_irq,
-						rcar_gen3_thermal_irq_thread,
-						IRQF_SHARED, irqname, priv);
+		ret = devm_request_threaded_irq(dev, irq, NULL,
+						rcar_gen3_thermal_irq,
+						IRQF_ONESHOT, irqname, priv);
 		if (ret)
 			return ret;
 	}
-- 
2.19.2

[PATCH v2 2/2] thermal: rcar_gen3_thermal: disable interrupt in .remove

[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
called before device is initialized.

this patch by disable interrupt in .remove, to ensure irq function
only be called after device is fully initialized.

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

diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c
index a11032e42f36..eb4e65b7ad8b 100644
--- a/drivers/thermal/rcar_gen3_thermal.c
+++ b/drivers/thermal/rcar_gen3_thermal.c
@@ -318,6 +318,9 @@ MODULE_DEVICE_TABLE(of, rcar_gen3_thermal_dt_ids);
 static int rcar_gen3_thermal_remove(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
+	struct rcar_gen3_thermal_priv *priv = dev_get_drvdata(dev);
+
+	rcar_thermal_irq_set(priv, false);
 
 	pm_runtime_put(dev);
 	pm_runtime_disable(dev);
-- 
2.19.2

Re: [PATCH v2 1/2] thermal: rcar_gen3_thermal: fix interrupt type

[Eugeniu Rosca]

Hi Jiada,

On Tue, Apr 23, 2019 at 03:12:17PM +0900, Jiada Wang wrote:
> Currently IRQF_SHARED type interrupt line is allocated, but it
> is not appropriate, as the interrupt line isn't shared between
> different devices, instead IRQF_ONESHOT is the proper type.
> 
> By changing interrupt type to IRQF_ONESHOT, now irq handler is
> no longer needed, as clear of interrupt status can be done in
> threaded interrupt context.
> 
> Because IRQF_ONESHOT type interrupt line is kept disabled until
> the threaded handler has been run, so there is no need to protect
> read/write of REG_GEN3_IRQSTR with lock.
> 
> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
> ---
>  drivers/thermal/rcar_gen3_thermal.c | 40 ++++++++---------------------
>  1 file changed, 10 insertions(+), 30 deletions(-)
>

[..]

I might be doing something wrong, but I couldn't apply this patch on top
of v5.1-rc6-4-g7142eaa58b49. All below commands failed (git v2.21.0):
 - git am this.patch
 - git apply this.patch
 - patch -p1 < this.patch

-- 
Best regards,
Eugeniu.

Re: [PATCH v2 1/2] thermal: rcar_gen3_thermal: fix interrupt type

[Jiada Wang]

Hi Eugeniu

On 2019/04/23 22:07, Eugeniu Rosca wrote:
> Hi Jiada,
> 
> On Tue, Apr 23, 2019 at 03:12:17PM +0900, Jiada Wang wrote:
>> Currently IRQF_SHARED type interrupt line is allocated, but it
>> is not appropriate, as the interrupt line isn't shared between
>> different devices, instead IRQF_ONESHOT is the proper type.
>>
>> By changing interrupt type to IRQF_ONESHOT, now irq handler is
>> no longer needed, as clear of interrupt status can be done in
>> threaded interrupt context.
>>
>> Because IRQF_ONESHOT type interrupt line is kept disabled until
>> the threaded handler has been run, so there is no need to protect
>> read/write of REG_GEN3_IRQSTR with lock.
>>
>> Signed-off-by: Jiada Wang <jiada_wang@mentor.com>
>> ---
>>   drivers/thermal/rcar_gen3_thermal.c | 40 ++++++++---------------------
>>   1 file changed, 10 insertions(+), 30 deletions(-)
>>
> 
> [..]
> 
> I might be doing something wrong, but I couldn't apply this patch on top
Oops, I think I used wrong code base,
I will send out v3 patch-set soon

sorry about that

Thanks,
Jiada
> of v5.1-rc6-4-g7142eaa58b49. All below commands failed (git v2.21.0):
>   - git am this.patch
>   - git apply this.patch
>   - patch -p1 < this.patch
>