| Message ID | 20210611115558.796338-2-jarkko.nikula@linux.intel.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Headers | show |
| Series | [1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused | expand |
On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > > Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); ... > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > + struct pci_dev *pdev = to_pci_dev(dev); > struct intel_qep *qep = pci_get_drvdata(pdev); Why not change both lines to dev_get_drvdata()? > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > + struct pci_dev *pdev = to_pci_dev(dev); > struct intel_qep *qep = pci_get_drvdata(pdev); Ditto
On Sun, 13 Jun 2021 13:36:16 +0300 Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: > > > > Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); > > ... > > > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > > + struct pci_dev *pdev = to_pci_dev(dev); > > struct intel_qep *qep = pci_get_drvdata(pdev); > > Why not change both lines to dev_get_drvdata()? > > > - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > > + struct pci_dev *pdev = to_pci_dev(dev); > > struct intel_qep *qep = pci_get_drvdata(pdev); > > Ditto > Question when doing this is whether it is better to pair pci_get_drvdata with pci_set_drvdata rather than assuming it will always map to dev_get_drvdata(). I personally don't feel too strongly about this either way, but I know others are unhappy with mixing them. Of course, could use dev_set_drvdata() in the first place then it would be less of an issue. Jonathan
On 6/13/21 1:36 PM, Andy Shevchenko wrote: > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: >> >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); > > ... > >> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); >> + struct pci_dev *pdev = to_pci_dev(dev); >> struct intel_qep *qep = pci_get_drvdata(pdev); > > Why not change both lines to dev_get_drvdata()? > I thought it before and Uwe had a good point why it isn't necessarily a good idea: https://www.spinics.net/lists/linux-pwm/msg15325.html Jarkko
+Uwe Kleine-König On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote: > On 6/13/21 1:36 PM, Andy Shevchenko wrote: > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > > <jarkko.nikula@linux.intel.com> wrote: > >> > >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); > > > > ... > > > >> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > >> + struct pci_dev *pdev = to_pci_dev(dev); > >> struct intel_qep *qep = pci_get_drvdata(pdev); > > > > Why not change both lines to dev_get_drvdata()? > > > I thought it before and Uwe had a good point why it isn't necessarily a > good idea: > > https://www.spinics.net/lists/linux-pwm/msg15325.html I understand this point, but the problem is that we often use different callbacks for different layers. For example, the PM callbacks are operating with generic 'struct device' and using the PCI device there is non-flexible layering violation, so in my opinion it's the opposite to what Uwe says. I.o.w. we need to use corresponding API to what we have in the callbacks. If the callback comes from generic level ==> generic APIs more appropriate. Anyway, it's such a minor detail, that I'm not going to insist on either way independently on the arguments.
On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote: > +Uwe Kleine-König > > On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula > <jarkko.nikula@linux.intel.com> wrote: > > On 6/13/21 1:36 PM, Andy Shevchenko wrote: > > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > > > <jarkko.nikula@linux.intel.com> wrote: > > >> > > >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); > > > > > > ... > > > > > >> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > > >> + struct pci_dev *pdev = to_pci_dev(dev); > > >> struct intel_qep *qep = pci_get_drvdata(pdev); > > > > > > Why not change both lines to dev_get_drvdata()? > > > > > I thought it before and Uwe had a good point why it isn't necessarily a > > good idea: > > > > https://www.spinics.net/lists/linux-pwm/msg15325.html > > I understand this point, but the problem is that we often use > different callbacks for different layers. For example, the PM > callbacks are operating with generic 'struct device' and using the PCI > device there is non-flexible layering violation, so in my opinion it's > the opposite to what Uwe says. I.o.w. we need to use corresponding API > to what we have in the callbacks. If the callback comes from generic > level ==> generic APIs more appropriate. Without having looked at the driver in question: I (think I) understand both sides here and both variants have their own downside. - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata() is a wrapper around dev_set_drvdata for the pcidev's struct device. - Using pci_get_drvdata() adds overhead (for humans only though, the compiler doesn't care and creates the same code) and having the pci device in the callback isn't necessary. My personal opinion is: The first is the graver layer violation because it relies on an implementation detail in the PCI framework. The latter is relying on a fact that is local to the driver only: All devices this driver is bound to are pci devices. The main benefit of explicitly using pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow knowledge of the PCI stuff can easier match a pci_get_drvdata() to the pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so while it uses a function call/code line more, it is more explicit and more obviously correct. And regarding your argument about the matching API: I think the above code uses the matching API, that is make a pci_dev from a device using to_pci_dev(). So this is about weighting up- and downsides. How you judge them is subjective. (Though my judgement is naturally the better one :-) Just my 0.02€ Uwe
On Mon, 14 Jun 2021 11:37:22 +0200 Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote: > > +Uwe Kleine-König > > > > On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula > > <jarkko.nikula@linux.intel.com> wrote: > > > On 6/13/21 1:36 PM, Andy Shevchenko wrote: > > > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > > > > <jarkko.nikula@linux.intel.com> wrote: > > > >> > > > >> Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); > > > > > > > > ... > > > > > > > >> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > > > >> + struct pci_dev *pdev = to_pci_dev(dev); > > > >> struct intel_qep *qep = pci_get_drvdata(pdev); > > > > > > > > Why not change both lines to dev_get_drvdata()? > > > > > > > I thought it before and Uwe had a good point why it isn't necessarily a > > > good idea: > > > > > > https://www.spinics.net/lists/linux-pwm/msg15325.html > > > > I understand this point, but the problem is that we often use > > different callbacks for different layers. For example, the PM > > callbacks are operating with generic 'struct device' and using the PCI > > device there is non-flexible layering violation, so in my opinion it's > > the opposite to what Uwe says. I.o.w. we need to use corresponding API > > to what we have in the callbacks. If the callback comes from generic > > level ==> generic APIs more appropriate. > > Without having looked at the driver in question: I (think I) understand > both sides here and both variants have their own downside. > > - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata() > is a wrapper around dev_set_drvdata for the pcidev's struct device. > > - Using pci_get_drvdata() adds overhead (for humans only though, the > compiler doesn't care and creates the same code) and having the pci > device in the callback isn't necessary. > > My personal opinion is: The first is the graver layer violation because > it relies on an implementation detail in the PCI framework. The latter > is relying on a fact that is local to the driver only: All devices this > driver is bound to are pci devices. The main benefit of explicitly using > pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow > knowledge of the PCI stuff can easier match a pci_get_drvdata() to the > pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so > while it uses a function call/code line more, it is more explicit and > more obviously correct. > > And regarding your argument about the matching API: I think the above > code uses the matching API, that is make a pci_dev from a device using > to_pci_dev(). > > So this is about weighting up- and downsides. How you judge them is > subjective. (Though my judgement is naturally the better one :-) Personally I'm happy with either dev_set_drvdata / dev_get_drvdata or pci_set_drvdata / pci_get_drvdata In this particular case there is a convenient struct device *dev local variable anyway in the probe() (IIRC) so could have done option 1 with no loss of readability and a tiny saving in code. Not worth changing it though is my 0.02€ Jonathan > > Just my 0.02€ > Uwe >
On Mon, Jun 14, 2021 at 1:45 PM Jonathan Cameron <jic23@kernel.org> wrote: > On Mon, 14 Jun 2021 11:37:22 +0200 > Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > On Mon, Jun 14, 2021 at 11:57:46AM +0300, Andy Shevchenko wrote: > > > +Uwe Kleine-König > > > On Mon, Jun 14, 2021 at 11:24 AM Jarkko Nikula > > > <jarkko.nikula@linux.intel.com> wrote: > > > > On 6/13/21 1:36 PM, Andy Shevchenko wrote: > > > > > On Fri, Jun 11, 2021 at 2:57 PM Jarkko Nikula > > > > > <jarkko.nikula@linux.intel.com> wrote: ... > > > > >> - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); > > > > >> + struct pci_dev *pdev = to_pci_dev(dev); > > > > >> struct intel_qep *qep = pci_get_drvdata(pdev); > > > > > > > > > > Why not change both lines to dev_get_drvdata()? > > > > > > > > > I thought it before and Uwe had a good point why it isn't necessarily a > > > > good idea: > > > > > > > > https://www.spinics.net/lists/linux-pwm/msg15325.html > > > > > > I understand this point, but the problem is that we often use > > > different callbacks for different layers. For example, the PM > > > callbacks are operating with generic 'struct device' and using the PCI > > > device there is non-flexible layering violation, so in my opinion it's > > > the opposite to what Uwe says. I.o.w. we need to use corresponding API > > > to what we have in the callbacks. If the callback comes from generic > > > level ==> generic APIs more appropriate. > > > > Without having looked at the driver in question: I (think I) understand > > both sides here and both variants have their own downside. > > > > - Using dev_get_drvdata() makes use of the fact that pci_set_drvdata() > > is a wrapper around dev_set_drvdata for the pcidev's struct device. > > > > - Using pci_get_drvdata() adds overhead (for humans only though, the > > compiler doesn't care and creates the same code) and having the pci > > device in the callback isn't necessary. > > > > My personal opinion is: The first is the graver layer violation because > > it relies on an implementation detail in the PCI framework. The latter > > is relying on a fact that is local to the driver only: All devices this > > driver is bound to are pci devices. The main benefit of explicitly using > > pci_get_drvdata ∘ to_pci_dev I see is that someone having only shallow > > knowledge of the PCI stuff can easier match a pci_get_drvdata() to the > > pci_set_drvdata() called in .probe() than a dev_get_drvdata() and so > > while it uses a function call/code line more, it is more explicit and > > more obviously correct. > > > > And regarding your argument about the matching API: I think the above > > code uses the matching API, that is make a pci_dev from a device using > > to_pci_dev(). > > > > So this is about weighting up- and downsides. How you judge them is > > subjective. (Though my judgement is naturally the better one :-) > > Personally I'm happy with either > > dev_set_drvdata / dev_get_drvdata > or > pci_set_drvdata / pci_get_drvdata > > In this particular case there is a convenient struct device *dev local > variable anyway in the probe() (IIRC) so could have done option 1 with > no loss of readability and a tiny saving in code. As I said this is unflexible. For example, we have quite a few drivers that split in the way of core part (as library) + glue driver(s) How to implement callbacks that will use the same pairs of the callbacks? I don't think it's possible in a good and neat way. On top of that I think using the knowledge of the device nature in the generic callbacks _is_ a layering violation. TL;DR: the simple rule of thumb may be: if the callback uses struct device, that dev_get_drvdata(), otherwise based on what you have got as a parameter. Does it make sense? > Not worth changing it though is my 0.02€
diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c index a8d3dccecc0f..8d7ae28fbd67 100644 --- a/drivers/counter/intel-qep.c +++ b/drivers/counter/intel-qep.c @@ -475,7 +475,7 @@ static void intel_qep_remove(struct pci_dev *pci) static int __maybe_unused intel_qep_suspend(struct device *dev) { - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); + struct pci_dev *pdev = to_pci_dev(dev); struct intel_qep *qep = pci_get_drvdata(pdev); qep->qepcon = intel_qep_readl(qep, INTEL_QEPCON); @@ -487,7 +487,7 @@ static int __maybe_unused intel_qep_suspend(struct device *dev) static int __maybe_unused intel_qep_resume(struct device *dev) { - struct pci_dev *pdev = container_of(dev, struct pci_dev, dev); + struct pci_dev *pdev = to_pci_dev(dev); struct intel_qep *qep = pci_get_drvdata(pdev); /*
Use to_pci_dev() helper instead of container_of(d, struct pci_dev, dev); Suggested-by: Jonathan Cameron <jic23@kernel.org> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/counter/intel-qep.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)