Re: [PATCH v1 1/1] thermal: rcar_gen3_thermal: request IRQ after device initialization

[Daniel Lezcano]

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@xxxxxxxxxx>
>>> ---
>>> Â 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