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

[Jiada Wang]

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