| Message ID | 20190424051145.23072-1-jiada_wang@mentor.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | thermal: rcar_gen3_thermal: fix IRQ issues | expand |
Hi Jiada, I think this series looks good. Unfortunately I'm out of office for the next week and are thus unable to test it. Please don't let this block this series but if it's still on the ML when I'm back I will do a proper review and test of it. On 2019-04-24 14:11:43 +0900, Jiada Wang wrote: > 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. > > --- > v4: remove 'spinlock_t lock' > add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type") > fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove") > > v3: fix to use correct code base > remove unused "flag" variable in rcar_gen3_thermal_irq > > 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 | 41 +++++++---------------------- > 1 file changed, 9 insertions(+), 32 deletions(-) > > -- > 2.19.2 >
On Wed, Apr 24, 2019 at 02:11:43PM +0900, Jiada Wang wrote: > 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. > > --- > v4: remove 'spinlock_t lock' > add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type") > fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove") > > v3: fix to use correct code base > remove unused "flag" variable in rcar_gen3_thermal_irq > > 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 | 41 +++++++---------------------- > 1 file changed, 9 insertions(+), 32 deletions(-) > > -- > 2.19.2 For the record, below is the diff between v3 and v4 [1]. It accurately reflects my review comments in https://patchwork.kernel.org/patch/10913165/#22601305 . In addition, I made sure there are either false positives or no new issues reported by: - sparse v0.5.2-1-ga3c4716a703a - smatch v0.5.0-4785-g4968bcad1c08 - cppcheck 1.88 dev - make W=123 - make coccicheck I repeated the same test steps as described in https://patchwork.kernel.org/cover/10913163/#22602335 and the results were the same: Reviewed-and-Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com> [1] diff --git a/drivers/thermal/rcar_gen3_thermal.c b/drivers/thermal/rcar_gen3_thermal.c index c63a86d3dac6..280230951dfe 100644 --- a/drivers/thermal/rcar_gen3_thermal.c +++ b/drivers/thermal/rcar_gen3_thermal.c @@ -14,7 +14,6 @@ #include <linux/of_device.h> #include <linux/platform_device.h> #include <linux/pm_runtime.h> -#include <linux/spinlock.h> #include <linux/sys_soc.h> #include <linux/thermal.h> @@ -82,7 +81,6 @@ struct rcar_gen3_thermal_tsc { struct rcar_gen3_thermal_priv { struct rcar_gen3_thermal_tsc *tscs[TSC_MAX_NUM]; unsigned int num_tscs; - spinlock_t lock; /* Protect interrupts on and off */ void (*thermal_init)(struct rcar_gen3_thermal_tsc *tsc); }; @@ -231,8 +229,8 @@ 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; - int i; u32 status; + int i; for (i = 0; i < priv->num_tscs; i++) { status = rcar_gen3_thermal_read(priv->tscs[i], REG_GEN3_IRQSTR); @@ -352,8 +350,6 @@ static int rcar_gen3_thermal_probe(struct platform_device *pdev) if (soc_device_match(r8a7795es1)) priv->thermal_init = rcar_gen3_thermal_init_r8a7795es1; - spin_lock_init(&priv->lock); - platform_set_drvdata(pdev, priv); /*
Hi Jiada, Thanks for your patches. On 2019-04-24 14:11:43 +0900, Jiada Wang wrote: > 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. I really like this series, nice work. Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > --- > v4: remove 'spinlock_t lock' > add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type") > fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove") > > v3: fix to use correct code base > remove unused "flag" variable in rcar_gen3_thermal_irq > > 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 | 41 +++++++---------------------- > 1 file changed, 9 insertions(+), 32 deletions(-) > > -- > 2.19.2 >
Hi Niklas, On Wed, May 08, 2019 at 01:54:03AM +0200, Niklas Söderlund wrote: > Hi Jiada, [..] > I really like this series, nice work. > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Is there anything off-the-shelf available for testing the rcar3 thermal driver, to avoid reinventing the wheel via https://patchwork.kernel.org/cover/10913163/#22602335 Thank you.
Hi Eugeniu, On 2019-05-10 12:42:31 +0200, Eugeniu Rosca wrote: > Hi Niklas, > > On Wed, May 08, 2019 at 01:54:03AM +0200, Niklas Söderlund wrote: > > Hi Jiada, > [..] > > I really like this series, nice work. > > > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > Is there anything off-the-shelf available for testing the rcar3 > thermal driver, to avoid reinventing the wheel via > https://patchwork.kernel.org/cover/10913163/#22602335 Not that I know of, unfortunately :-( I have a private home hacked testing framework (don't we all?) based on tcl+expect where I have two basic tests for rcar_gen3_thermal. I'm willing to share the tests if you by chance want them, but be warned that they are highly specialised for my needs and I'm reluctant to publish my whole hack tool as it just a ugly hack ;-) On a high level the tests I have are 1. thermal-load Generates load on target and observes the temperature is increased using the /sys/class/thermal/thermal_zone*/temp" interface. This seems similar to the test case your reference using stress-ng. 2. thermal-cooling Emulate the passive trip point temperatures using the /sys/class/thermal/*/emul_temp interface and observe that the specified cooling state is achieved. I should add a third test to make sure IRQ fires but this is just a pet project for me so maybe I will get around to it sometime... If you know of anything around to test thermal drivers or if you create something please let me know so I can add it to my tests. And let me know if you want my hacks for inspiration for your own testing.
Hi Niklas, On Fri, May 10, 2019 at 01:36:08PM +0200, Niklas Söderlund wrote: > Hi Eugeniu, > > On 2019-05-10 12:42:31 +0200, Eugeniu Rosca wrote: > > Hi Niklas, > > > > On Wed, May 08, 2019 at 01:54:03AM +0200, Niklas Söderlund wrote: > > > Hi Jiada, > > [..] > > > I really like this series, nice work. > > > > > > Tested-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > > > Is there anything off-the-shelf available for testing the rcar3 > > thermal driver, to avoid reinventing the wheel via > > https://patchwork.kernel.org/cover/10913163/#22602335 > > Not that I know of, unfortunately :-( > > I have a private home hacked testing framework (don't we all?) based on > tcl+expect where I have two basic tests for rcar_gen3_thermal. I'm > willing to share the tests if you by chance want them, but be warned > that they are highly specialised for my needs and I'm reluctant to > publish my whole hack tool as it just a ugly hack ;-) > > On a high level the tests I have are > > 1. thermal-load > Generates load on target and observes the temperature is increased > using the /sys/class/thermal/thermal_zone*/temp" interface. This > seems similar to the test case your reference using stress-ng. > > 2. thermal-cooling > Emulate the passive trip point temperatures using the > /sys/class/thermal/*/emul_temp interface and observe that the > specified cooling state is achieved. > > I should add a third test to make sure IRQ fires but this is just a pet > project for me so maybe I will get around to it sometime... > > If you know of anything around to test thermal drivers or if you create > something please let me know so I can add it to my tests. And let me > know if you want my hacks for inspiration for your own testing. Thanks for this summary. It would be definitely convenient to have a set of tests covering the most important features of the driver. I was particularly thinking of the test procedure in light of below: - I still can reproduce a few UBSAN (signed integer overflow) and KASAN (use-after-free) reports with the most recent vanilla driver. - There are a couple of thermal commits in rcar-3.9.x pending for mainline submission: https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=fe7d0d1c77f9 ("thermal: rcar_gen3_thermal: Use FUSE values if they are available") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=2776ccd63649 ("thermal: rcar_gen3_thermal: Fix interrupt count issue") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=9146af785f41 ("thermal: rcar_gen3_thermal: Enable selection between polling/interrupt mode") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=55b262766ec2 ("thermal: rcar_gen3_thermal: PIO-INT can be selected for each TSC separately") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=d323d9de0683 ("thermal: rcar_gen3_thermal: Add support for r8a77990") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=fb8efb8bac29 ("thermal: rcar_gen3_thermal: Fix interrupts are not raised issue on E3") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=5627c42a1bd5 ("thermal: rcar_gen3_thermal: Use DIV_ROUND_CLOSEST correctly as its description") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=d4e41702e53b ("thermal: rcar_gen3_thermal: [H3/M3N] Update calculation formula due to HW evaluation") https://git.kernel.org/pub/scm/linux/kernel/git/horms/renesas-bsp.git/commit/?id=958bd36e03b7 ("thermal: rcar_gen3_thermal: [E3] Update calculation formula due to HW evaluation") Long story short, I think we will review more thermal commits in hopefully not so distant future and it would be helpful to reach some common understanding what kind of testing the new patches should pass. Your summary already gives some insight in that direction. Thanks.
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. --- v4: remove 'spinlock_t lock' add Fixes tag in ("thermal: rcar_gen3_thermal: fix interrupt type") fix typos in ("thermal: rcar_gen3_thermal: disable interrupt in .remove") v3: fix to use correct code base remove unused "flag" variable in rcar_gen3_thermal_irq 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 | 41 +++++++---------------------- 1 file changed, 9 insertions(+), 32 deletions(-)