Message-ID: <20201204200618.4t4briks4iv4vcgz@pengutronix.de>
Date:   Fri, 4 Dec 2020 21:06:18 +0100
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Thierry Reding <thierry.reding@...il.com>
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

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/ |

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

Powered by blists - more mailing lists