Message-ID: <X8pfAmiXa8MqF9Gl@ulmo>
Date:   Fri, 4 Dec 2020 17:08:34 +0100
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     linux-pwm@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        linux-kernel@...r.kernel.org, Michael Walle <michael@...le.cc>,
        kernel@...gutronix.de, Lee Jones <lee.jones@...aro.org>
Subject: Re: [PATCH RESEND for 5.10] pwm: sl28cpld: fix getting driver data
 in pwm callbacks

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@...le.cc>
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@...gutronix.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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists