| Message ID | 20201203084142.3810204-1-u.kleine-koenig@pengutronix.de |
|---|---|
| State | Accepted |
| Commit | 062c9cdf60a1e581b1002d372f1cf8e745fe3c16 |
| Headers | show |
| Series | [RESEND,for,5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks | expand |
On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote: > 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") > Tested-by: Michael Walle <michael@walle.cc> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > --- > Hello Linus, > > Thierry (who usually sends PWM patches to you) didn't react to this > patch sent to the pwm Mailinglist last week > (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de) > yet. > > Given v5.10 isn't far away any more and I don't know when Thierry will > take a look and act, I'm sending this directly to you. The affected > driver was new in 5.10-rc1 and at least once the unpatched driver > created an oops: > > https://lavalab.kontron.com/scheduler/job/108#L950 > > Michael Walle who tested this patch is the original author of the > driver. IMHO it would be good to have this fixed before 5.10. > > If you prefer a pull request, I can setup something (but I don't have > access to Thierry's tree, so it will be for a repository that's new to > you). > > Best regards > Uwe > > drivers/pwm/pwm-sl28cpld.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) I thought I had seen you discuss this with Lee and gotten the impression that you were going to respin this to move the platform_set_drvdata() to an earlier point, which I think is the more correct approach. container_of() isn't exactly wrong, but it's really just papering over the fact that platform_set_drvdata() is in the wrong place, so I'd prefer a patch that does that instead. Now, I can no longer find a link to the discussion that I recall, so it was either on IRC (where I don't have any logs) or I'm loosing my mind. I'll prepare a patch that moves platform_set_drvdata() for Michael to test. If that works I'll send a PR with fixes to Linus early next week. Thierry
On Fri, 04 Dec 2020, Thierry Reding wrote: > On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote: > > 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") > > Tested-by: Michael Walle <michael@walle.cc> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello Linus, > > > > Thierry (who usually sends PWM patches to you) didn't react to this > > patch sent to the pwm Mailinglist last week > > (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de) > > yet. > > > > Given v5.10 isn't far away any more and I don't know when Thierry will > > take a look and act, I'm sending this directly to you. The affected > > driver was new in 5.10-rc1 and at least once the unpatched driver > > created an oops: > > > > https://lavalab.kontron.com/scheduler/job/108#L950 > > > > Michael Walle who tested this patch is the original author of the > > driver. IMHO it would be good to have this fixed before 5.10. > > > > If you prefer a pull request, I can setup something (but I don't have > > access to Thierry's tree, so it will be for a repository that's new to > > you). > > > > Best regards > > Uwe > > > > drivers/pwm/pwm-sl28cpld.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > I thought I had seen you discuss this with Lee and gotten the impression > that you were going to respin this to move the platform_set_drvdata() to > an earlier point, which I think is the more correct approach. > > container_of() isn't exactly wrong, but it's really just papering over > the fact that platform_set_drvdata() is in the wrong place, so I'd > prefer a patch that does that instead. > > Now, I can no longer find a link to the discussion that I recall, so it > was either on IRC (where I don't have any logs) or I'm loosing my mind. Don't worry, you are (probably!) still quite sane. The discussion happened over IRC. I highlighted my concerns, but Uwe didn't respond to them. This patch was the next time I saw anything on the subject. > I'll prepare a patch that moves platform_set_drvdata() for Michael to > test. If that works I'll send a PR with fixes to Linus early next week.
Hello Thierry, On Fri, Dec 04, 2020 at 01:41:16PM +0100, Thierry Reding wrote: > On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote: > > 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") > > Tested-by: Michael Walle <michael@walle.cc> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > --- > > Hello Linus, > > > > Thierry (who usually sends PWM patches to you) didn't react to this > > patch sent to the pwm Mailinglist last week > > (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de) > > yet. > > > > Given v5.10 isn't far away any more and I don't know when Thierry will > > take a look and act, I'm sending this directly to you. The affected > > driver was new in 5.10-rc1 and at least once the unpatched driver > > created an oops: > > > > https://lavalab.kontron.com/scheduler/job/108#L950 > > > > Michael Walle who tested this patch is the original author of the > > driver. IMHO it would be good to have this fixed before 5.10. > > > > If you prefer a pull request, I can setup something (but I don't have > > access to Thierry's tree, so it will be for a repository that's new to > > you). > > > > Best regards > > Uwe > > > > drivers/pwm/pwm-sl28cpld.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > I thought I had seen you discuss this with Lee and gotten the impression > that you were going to respin this to move the platform_set_drvdata() to > an earlier point, which I think is the more correct approach. Lee asked on irc why I didn't move the platform_set_drvdata to an earlier stage and I told him why. Then the conversation was over. > container_of() isn't exactly wrong, but it's really just papering over > the fact that platform_set_drvdata() is in the wrong place, so I'd > prefer a patch that does that instead. platfrom_set_drvdata is in a perfectly fine position if you only rely on it in the platform_driver's remove callback which is the case with my patch. I wrote in my commit log | Instead of setting the driver data earlier use the traditional | container_of approach as this way the driver data is conceptually and | computational nearer. which is still think to be true. The main thing I don't like about the platform_set_drvdata approach is that you have to rely on dev_get_drvdata() returning the value set with platform_set_drvdata() which IMHO is an implementation detail of the platform driver code. > Now, I can no longer find a link to the discussion that I recall, so it > was either on IRC (where I don't have any logs) or I'm loosing my mind. It was on IRC but I thought to have written an email about this, too. But I don't find it either. > I'll prepare a patch that moves platform_set_drvdata() for Michael to > test. If that works I'll send a PR with fixes to Linus early next week. You're late, Linus already merged my patch. Best regards Uwe
Hello Lee, On Fri, Dec 04, 2020 at 01:24:36PM +0000, Lee Jones wrote: > On Fri, 04 Dec 2020, Thierry Reding wrote: > > Now, I can no longer find a link to the discussion that I recall, so it > > was either on IRC (where I don't have any logs) or I'm loosing my mind. > > Don't worry, you are (probably!) still quite sane. > > The discussion happened over IRC. FTR, the conversation was as follows (with lag = Lee, tags = Thierry and ukleinek = me): 1606741876 < ukleinek> tagr, lag: would you mind if I send 20201124212432.3117322-1-u.kleine-koenig@pengutronix.de to Linus? 1606741894 < ukleinek> tagr: or if you do mind, can you please send it? [...] 1606742364 < lag> ukleinek: I assume this is the container_of() patch? 1606742370 < ukleinek> lag: right 1606742402 < lag> ukleinek: It seems very wrong that a leaf controller driver's ops would be called before it has probed 1606742410 < lag> ukleinek: How is that a thing? 1606742428 < ukleinek> lag: the ops can be called as soon as pwmchip_add completes 1606742443 < lag> ukleinek: Where is pwmchip_add() called 1606742470 < lag> I guess I can grep that myself 1606742480 < ukleinek> lag: just before platform_set_drvdata(pdev, priv) in sl28cpld_pwm_probe() 1606742755 < lag> ukleinek: What about moving pwmchip_add() after platform_set_drvdata() or vice versa? 1606742789 < ukleinek> lag: did you read the commit log? 1606742845 < lag> ukleinek: I did 1606742981 < ukleinek> lag: then I don't get your question 1606743049 < lag> ukleinek: Why is using container_of, which is generally horrible and only used if there is no other way to obtain data, better than changing order of the calls such that the dependencies are met 1606743127 < ukleinek> container_of is a well understood concept and it's cheaper than dev_get_drvdata 1606743198 < ukleinek> and conceptually it's easier too. (But that might only be me) 1606743241 < ukleinek> for one thing it cannot happen that I get a wrong pointer because platform_set_drvdata was called too late 1606743281 < ukleinek> also it makes use of the fact that platform_set_drvdata sets the driver's driver data and not something in the platform device > I highlighted my concerns, but Uwe didn't respond to them. This patch > was the next time I saw anything on the subject. So I did respond and if you didn't see it the problem is on your end. Best regards Uwe
On Fri, Dec 04, 2020 at 02:51:02PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Fri, Dec 04, 2020 at 01:41:16PM +0100, Thierry Reding wrote: > > On Thu, Dec 03, 2020 at 09:41:42AM +0100, Uwe Kleine-König wrote: > > > 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") > > > Tested-by: Michael Walle <michael@walle.cc> > > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> > > > --- > > > Hello Linus, > > > > > > Thierry (who usually sends PWM patches to you) didn't react to this > > > patch sent to the pwm Mailinglist last week > > > (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de) > > > yet. > > > > > > Given v5.10 isn't far away any more and I don't know when Thierry will > > > take a look and act, I'm sending this directly to you. The affected > > > driver was new in 5.10-rc1 and at least once the unpatched driver > > > created an oops: > > > > > > https://lavalab.kontron.com/scheduler/job/108#L950 > > > > > > Michael Walle who tested this patch is the original author of the > > > driver. IMHO it would be good to have this fixed before 5.10. > > > > > > If you prefer a pull request, I can setup something (but I don't have > > > access to Thierry's tree, so it will be for a repository that's new to > > > you). > > > > > > Best regards > > > Uwe > > > > > > drivers/pwm/pwm-sl28cpld.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > I thought I had seen you discuss this with Lee and gotten the impression > > that you were going to respin this to move the platform_set_drvdata() to > > an earlier point, which I think is the more correct approach. > > Lee asked on irc why I didn't move the platform_set_drvdata to an > earlier stage and I told him why. Then the conversation was over. Okay, looking at the logs that you posted, the discussion didn't quite end the way I remembered it. Still, I would've expected a bit more discussion and a chance to reach consensus before you went off on your own and submitted this patch "out-of-band". > > container_of() isn't exactly wrong, but it's really just papering over > > the fact that platform_set_drvdata() is in the wrong place, so I'd > > prefer a patch that does that instead. > > platfrom_set_drvdata is in a perfectly fine position if you only rely on > it in the platform_driver's remove callback which is the case with my > patch. I wrote in my commit log In general it's still a bad idea to call platform_set_drvdata() after you register with the framework, so I think that's a worthwhile change irrespective of your fix. > | Instead of setting the driver data earlier use the traditional > | container_of approach as this way the driver data is conceptually and > | computational nearer. > > which is still think to be true. The main thing I don't like about the > platform_set_drvdata approach is that you have to rely on > dev_get_drvdata() returning the value set with platform_set_drvdata() > which IMHO is an implementation detail of the platform driver code. Well yeah, but it's an implementation detail that pretty much all platform drivers rely on and that's been like this ever since platform_{get,set}_drvdata() were introduced over 15 years ago. So it's not like this is suddenly going to stop working. > > Now, I can no longer find a link to the discussion that I recall, so it > > was either on IRC (where I don't have any logs) or I'm loosing my mind. > > It was on IRC but I thought to have written an email about this, too. > But I don't find it either. > > > I'll prepare a patch that moves platform_set_drvdata() for Michael to > > test. If that works I'll send a PR with fixes to Linus early next week. > > You're late, Linus already merged my patch. Oh well... I'll just send my patch along with the rest of the batch for v5.11 then. Thierry
Hello Thierry, On Fri, Dec 04, 2020 at 05:08:34PM +0100, Thierry Reding wrote: > On Fri, Dec 04, 2020 at 02:51:02PM +0100, Uwe Kleine-König wrote: > > On Fri, Dec 04, 2020 at 01:41:16PM +0100, Thierry Reding wrote: > > > I thought I had seen you discuss this with Lee and gotten the impression > > > that you were going to respin this to move the platform_set_drvdata() to > > > an earlier point, which I think is the more correct approach. > > > > Lee asked on irc why I didn't move the platform_set_drvdata to an > > earlier stage and I told him why. Then the conversation was over. > > Okay, looking at the logs that you posted, the discussion didn't quite > end the way I remembered it. Still, I would've expected a bit more > discussion and a chance to reach consensus before you went off on your > own and submitted this patch "out-of-band". I interpreted Lee's silence as "hmm, ok, we don't agree". And giving more weight to the opinion of the one who invested the time to create a patch and communicate with the reporter (i.e. me) I considered it ok to keep my opinion. And you didn't comment for over a week to the patch, Michael's and my ping via mail and me addressing you via irc. Together with this patch being urgent in my eyes (and this being the only urgent patch on my radar) I thought it ok to send it directly to Linus two days after I tried to contact you without success. Other than that: Given that you (and Lee?) seem to feel strong about dev_get_drvdata being better than container_of, indeed a bit more discussion would have been appropriate. But I see the problem on your side because Lee stopped discussing after reading my arguments and you didn't even start. And for the record: I also feel strong that container_of is better than dev_get_drvdata for the following reasons: - container_of is computationally cheaper than dev_get_drvdata; - container_of provides (a bit) type safety, compared to dev_get_drvdata providing none; - container_of is more universal, as you cannot easily use dev_get_drvdata e.g. in the PWM driver provided by drivers/gpio/gpio-mvebu.c. - Maybe subjectively I think that conceptually container_of is nicer. That's "use the struct that contains the pwm_chip" compared to "use the struct registered in probe belonging to this pwm_chip's coupled device". > > > container_of() isn't exactly wrong, but it's really just papering over > > > the fact that platform_set_drvdata() is in the wrong place, so I'd > > > prefer a patch that does that instead. > > > > platfrom_set_drvdata is in a perfectly fine position if you only rely on > > it in the platform_driver's remove callback which is the case with my > > patch. I wrote in my commit log > > In general it's still a bad idea to call platform_set_drvdata() after > you register with the framework, so I think that's a worthwhile change > irrespective of your fix. *shrug* > > | Instead of setting the driver data earlier use the traditional > > | container_of approach as this way the driver data is conceptually and > > | computational nearer. > > > > which is still think to be true. The main thing I don't like about the > > platform_set_drvdata approach is that you have to rely on > > dev_get_drvdata() returning the value set with platform_set_drvdata() > > which IMHO is an implementation detail of the platform driver code. > > Well yeah, but it's an implementation detail that pretty much all > platform drivers rely on and that's been like this ever since > platform_{get,set}_drvdata() were introduced over 15 years ago. > > So it's not like this is suddenly going to stop working. Yes, however that hardly makes it prettier in my book. Two statistics about that area: - All PWM drivers in drivers/pwm/ use dev_get_drvdata only in PM callbacks but not PWM ops. Compared to the latter PM callbacks don't get a pwm_chip argument and so cannot use container_of. - The following drivers use platform_set_drvdata only after pwmchip_add: - pwm-ab8500.c - pwm-atmel.c - pwm-atmel-hlcdc.c - pwm-atmel-tcb.c - pwm-berlin.c - pwm-cros-ec.c - pwm-ep93xx.c - pwm-fsl-ftm.c - pwm-hibvt.c - pwm-lpc18xx-sct.c - pwm-lpc32xx.c - pwm-lpss-platform.c - pwm-meson.c - pwm-mtk-disp.c - pwm-mxs.c - pwm-omap-dmtimer.c - pwm-pxa.c - pwm-sifive.c - pwm-sl28cpld.c - pwm-sti.c - pwm-stm32.c - pwm-stm32-lp.c - pwm-stmpe.c - pwm-sun4i.c - pwm-tiecap.c - pwm-tiehrpwm.c - pwm-twl.c - pwm-twl-led.c - pwm-vt8500.c - pwm-zx.c None of these currently has a problem with respect to ordering of platform_set_drvdata vs pwmchip_add (now that pwm-sl28cpld.c is fixed). For completeness: The following drivers use platform_set_drvdata before pwmchip_add: - pwm-bcm2835.c - pwm-bcm-iproc.c - pwm-bcm-kona.c - pwm-brcmstb.c - pwm-clps711x.c - pwm-crc.c - pwm-img.c - pwm-imx1.c - pwm-imx27.c - pwm-imx-tpm.c - pwm-iqs620a.c - pwm-jz4740.c - pwm-lp3943.c - pwm-mediatek.c - pwm-rcar.c - pwm-renesas-tpu.c - pwm-rockchip.c - pwm-samsung.c - pwm-spear.c - pwm-sprd.c - pwm-tegra.c So arguing that pwm-sl28cpld.c should continue to use dev_get_drvdata in the PWM callbacks and fixing only this single driver's order between platform_set_drvdata and pwmchip_add now seems strange to me. 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") Tested-by: Michael Walle <michael@walle.cc> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> --- Hello Linus, Thierry (who usually sends PWM patches to you) didn't react to this patch sent to the pwm Mailinglist last week (https://lore.kernel.org/r/20201124212432.3117322-1-u.kleine-koenig@pengutronix.de) yet. Given v5.10 isn't far away any more and I don't know when Thierry will take a look and act, I'm sending this directly to you. The affected driver was new in 5.10-rc1 and at least once the unpatched driver created an oops: https://lavalab.kontron.com/scheduler/job/108#L950 Michael Walle who tested this patch is the original author of the driver. IMHO it would be good to have this fixed before 5.10. If you prefer a pull request, I can setup something (but I don't have access to Thierry's tree, so it will be for a repository that's new to you). Best regards Uwe drivers/pwm/pwm-sl28cpld.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)