linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
@ 2020-12-03  8:41 Uwe Kleine-König
  2020-12-04 12:41 ` Thierry Reding
  0 siblings, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2020-12-03  8:41 UTC (permalink / raw)
  To: Linus Torvalds
  Cc: linux-kernel, linux-pwm, Thierry Reding, Lee Jones, kernel,
	Michael Walle

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(-)

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-03  8:41 [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks Uwe Kleine-König
@ 2020-12-04 12:41 ` Thierry Reding
  2020-12-04 13:24   ` Lee Jones
  2020-12-04 13:51   ` Uwe Kleine-König
  0 siblings, 2 replies; 7+ messages in thread
From: Thierry Reding @ 2020-12-04 12:41 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: Linus Torvalds, linux-kernel, linux-pwm, Lee Jones, kernel,
	Michael Walle

[-- Attachment #1: Type: text/plain, Size: 2351 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-04 12:41 ` Thierry Reding
@ 2020-12-04 13:24   ` Lee Jones
  2020-12-04 14:03     ` Uwe Kleine-König
  2020-12-04 13:51   ` Uwe Kleine-König
  1 sibling, 1 reply; 7+ messages in thread
From: Lee Jones @ 2020-12-04 13:24 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Uwe Kleine-König, Linus Torvalds, linux-kernel, linux-pwm,
	kernel, Michael Walle

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.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-04 12:41 ` Thierry Reding
  2020-12-04 13:24   ` Lee Jones
@ 2020-12-04 13:51   ` Uwe Kleine-König
  2020-12-04 16:08     ` Thierry Reding
  1 sibling, 1 reply; 7+ messages in thread
From: Uwe Kleine-König @ 2020-12-04 13:51 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-pwm, Linus Torvalds, linux-kernel, Michael Walle, kernel,
	Lee Jones

[-- Attachment #1: Type: text/plain, Size: 3609 bytes --]

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


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-04 13:24   ` Lee Jones
@ 2020-12-04 14:03     ` Uwe Kleine-König
  0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2020-12-04 14:03 UTC (permalink / raw)
  To: Lee Jones
  Cc: Thierry Reding, linux-pwm, linux-kernel, Michael Walle, kernel,
	Linus Torvalds

[-- Attachment #1: Type: text/plain, Size: 2569 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-04 13:51   ` Uwe Kleine-König
@ 2020-12-04 16:08     ` Thierry Reding
  2020-12-04 20:06       ` Uwe Kleine-König
  0 siblings, 1 reply; 7+ messages in thread
From: Thierry Reding @ 2020-12-04 16:08 UTC (permalink / raw)
  To: Uwe Kleine-König
  Cc: linux-pwm, Linus Torvalds, linux-kernel, Michael Walle, kernel,
	Lee Jones

[-- Attachment #1: Type: text/plain, Size: 4434 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks
  2020-12-04 16:08     ` Thierry Reding
@ 2020-12-04 20:06       ` Uwe Kleine-König
  0 siblings, 0 replies; 7+ messages in thread
From: Uwe Kleine-König @ 2020-12-04 20:06 UTC (permalink / raw)
  To: Thierry Reding
  Cc: linux-pwm, Linus Torvalds, linux-kernel, Michael Walle, kernel,
	Lee Jones

[-- Attachment #1: Type: text/plain, Size: 5656 bytes --]

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

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-12-04 20:07 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-03  8:41 [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data in pwm callbacks Uwe Kleine-König
2020-12-04 12:41 ` Thierry Reding
2020-12-04 13:24   ` Lee Jones
2020-12-04 14:03     ` Uwe Kleine-König
2020-12-04 13:51   ` Uwe Kleine-König
2020-12-04 16:08     ` Thierry Reding
2020-12-04 20:06       ` Uwe Kleine-König

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).