| Message ID | 20201124212432.3117322-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: sl28cpld: fix getting driver data in pwm callbacks | expand |
Am 2020-11-24 22:24, schrieb Uwe Kleine-König: > Currently .get_state() and .apply() use dev_get_drvdata() on the struct > device related to the pwm chip. This only works after .probe() called > platform_set_drvdata() which in this driver happens only after > pwmchip_add() and so comes possibly too late. > > Instead of setting the driver data earlier use the traditional > container_of approach as this way the driver data is conceptually and > computational nearer. > > Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> I wasn't able to reproduce the original bug, it seems it is really timing/kernel/kernel config dependent. Thus I could only test if it still working and could not verify that the original bug is fixed. Nonetheless: Tested-by: Michael Walle <michael@walle.cc> Btw. the backtrace is available here: https://lavalab.kontron.com/scheduler/job/108#L950 -michael
On Tue, Nov 24, 2020 at 11:22:33PM +0100, Michael Walle wrote: > Am 2020-11-24 22:24, schrieb Uwe Kleine-König: > > Currently .get_state() and .apply() use dev_get_drvdata() on the struct > > device related to the pwm chip. This only works after .probe() called > > platform_set_drvdata() which in this driver happens only after > > pwmchip_add() and so comes possibly too late. > > > > Instead of setting the driver data earlier use the traditional > > container_of approach as this way the driver data is conceptually and > > computational nearer. > > > > Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > I wasn't able to reproduce the original bug, it seems it is really > timing/kernel/kernel config dependent. Thus I could only test if it > still working and could not verify that the original bug is fixed. > Nonetheless: > > Tested-by: Michael Walle <michael@walle.cc> > > > Btw. the backtrace is available here: > https://lavalab.kontron.com/scheduler/job/108#L950 This is triggered from deferred_probe_work, so my best guess is that both the pwm-fan (here the consumer) and the pwm hardware driver were probed in parallel with a timing that made pwm-fan hit the short window when driver data was unset. I wonder if we {sh,c}ould expand PWM_DEBUG to hit this kind of error. E.g. let pwmchip_add() request and get_state each PWM. Best regards Uwe
Am 2020-11-24 22:24, schrieb Uwe Kleine-König: > Currently .get_state() and .apply() use dev_get_drvdata() on the struct > device related to the pwm chip. This only works after .probe() called > platform_set_drvdata() which in this driver happens only after > pwmchip_add() and so comes possibly too late. > > Instead of setting the driver data earlier use the traditional > container_of approach as this way the driver data is conceptually and > computational nearer. > > Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller") > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello, > > in v4 of the driver Michael still used container_of and then changed to > use dev_get_drvdata() as Lee suggested. I didn't notice this > suggestion, > otherwise I would have expressed my variance about this already > earlier. > > I noticed this problem because Michael contacted me via irc and showed > me the resulting oops, so I think applying this before 5.10 would be > good. It's not entirely clear to me why .get_state() is called that > early in his case, but the theory is clear: The callbacks can be called > as soon as pwmchip_add() is called. > > Best regards > Uwe Ping. Would be nice if this makes it into v5.10. -michael
Hello Michael, On Mon, Nov 30, 2020 at 09:17:52AM +0100, Michael Walle wrote: > Am 2020-11-24 22:24, schrieb Uwe Kleine-König: > > Currently .get_state() and .apply() use dev_get_drvdata() on the struct > > device related to the pwm chip. This only works after .probe() called > > platform_set_drvdata() which in this driver happens only after > > pwmchip_add() and so comes possibly too late. > > > > Instead of setting the driver data earlier use the traditional > > container_of approach as this way the driver data is conceptually and > > computational nearer. > > > > Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller") > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello, > > > > in v4 of the driver Michael still used container_of and then changed to > > use dev_get_drvdata() as Lee suggested. I didn't notice this suggestion, > > otherwise I would have expressed my variance about this already earlier. > > > > I noticed this problem because Michael contacted me via irc and showed > > me the resulting oops, so I think applying this before 5.10 would be > > good. It's not entirely clear to me why .get_state() is called that > > early in his case, but the theory is clear: The callbacks can be called > > as soon as pwmchip_add() is called. > > > > Best regards > > Uwe > > Ping. Would be nice if this makes it into v5.10. Yes, fully agreed. @Thierry: Do you care to send it to Linus? Or are you ok if I do that? Best regards Uwe
diff --git a/drivers/pwm/pwm-sl28cpld.c b/drivers/pwm/pwm-sl28cpld.c index 5046b6b7fd35..b4c651fc749c 100644 --- a/drivers/pwm/pwm-sl28cpld.c +++ b/drivers/pwm/pwm-sl28cpld.c @@ -84,12 +84,14 @@ struct sl28cpld_pwm { struct regmap *regmap; u32 offset; }; +#define sl28cpld_pwm_from_chip(_chip) \ + container_of(_chip, struct sl28cpld_pwm, pwm_chip) static void sl28cpld_pwm_get_state(struct pwm_chip *chip, struct pwm_device *pwm, struct pwm_state *state) { - struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); + struct sl28cpld_pwm *priv = sl28cpld_pwm_from_chip(chip); unsigned int reg; int prescaler; @@ -118,7 +120,7 @@ static void sl28cpld_pwm_get_state(struct pwm_chip *chip, static int sl28cpld_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, const struct pwm_state *state) { - struct sl28cpld_pwm *priv = dev_get_drvdata(chip->dev); + struct sl28cpld_pwm *priv = sl28cpld_pwm_from_chip(chip); unsigned int cycle, prescaler; bool write_duty_cycle_first; int ret;
Currently .get_state() and .apply() use dev_get_drvdata() on the struct device related to the pwm chip. This only works after .probe() called platform_set_drvdata() which in this driver happens only after pwmchip_add() and so comes possibly too late. Instead of setting the driver data earlier use the traditional container_of approach as this way the driver data is conceptually and computational nearer. Fixes: 9db33d221efc ("pwm: Add support for sl28cpld PWM controller") Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello, in v4 of the driver Michael still used container_of and then changed to use dev_get_drvdata() as Lee suggested. I didn't notice this suggestion, otherwise I would have expressed my variance about this already earlier. I noticed this problem because Michael contacted me via irc and showed me the resulting oops, so I think applying this before 5.10 would be good. It's not entirely clear to me why .get_state() is called that early in his case, but the theory is clear: The callbacks can be called as soon as pwmchip_add() is called. Best regards Uwe drivers/pwm/pwm-sl28cpld.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)