[PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused

[PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused

[Jarkko Nikula]

Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.

Suggested-by: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/counter/intel-qep.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
index ab10ba33f46a..a8d3dccecc0f 100644
--- a/drivers/counter/intel-qep.c
+++ b/drivers/counter/intel-qep.c
@@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
 	intel_qep_writel(qep, INTEL_QEPCON, 0);
 }
 
-#ifdef CONFIG_PM
-static int intel_qep_suspend(struct device *dev)
+static int __maybe_unused intel_qep_suspend(struct device *dev)
 {
 	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
 	struct intel_qep *qep = pci_get_drvdata(pdev);
@@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
 	return 0;
 }
 
-static int intel_qep_resume(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 intel_qep *qep = pci_get_drvdata(pdev);
@@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
 
 	return 0;
 }
-#endif
 
 static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
 			    intel_qep_suspend, intel_qep_resume, NULL);
-- 
2.30.2

[PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Jarkko Nikula]

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

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);
 
 	/*
-- 
2.30.2

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Andy Shevchenko]

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Jonathan Cameron]

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

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Jarkko Nikula]

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

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Andy Shevchenko]

+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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Uwe Kleine-König]

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

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

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

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Jonathan Cameron]

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
> 

Re: [PATCH 2/2] counter: intel-qep: Use to_pci_dev() helper

[Andy Shevchenko]

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€


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused

[Jonathan Cameron]

On Fri, 11 Jun 2021 14:55:57 +0300
Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:

> Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.
> 
> Suggested-by: Jonathan Cameron <jic23@kernel.org>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Both applied to the togreg branch of iio.git and pushed out
as testing to let 0-day poke at them.

William, if you want to give feedback on these, still time for
me to add tags etc. They just seem trivial enough its not worth
wasting your time :)

Jonathan


> ---
>  drivers/counter/intel-qep.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> index ab10ba33f46a..a8d3dccecc0f 100644
> --- a/drivers/counter/intel-qep.c
> +++ b/drivers/counter/intel-qep.c
> @@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
>  	intel_qep_writel(qep, INTEL_QEPCON, 0);
>  }
>  
> -#ifdef CONFIG_PM
> -static int intel_qep_suspend(struct device *dev)
> +static int __maybe_unused intel_qep_suspend(struct device *dev)
>  {
>  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
>  	struct intel_qep *qep = pci_get_drvdata(pdev);
> @@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
>  	return 0;
>  }
>  
> -static int intel_qep_resume(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 intel_qep *qep = pci_get_drvdata(pdev);
> @@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
>  			    intel_qep_suspend, intel_qep_resume, NULL);

Re: [PATCH 1/2] counter: intel-qep: Mark PM callbacks with __maybe_unused

[William Breathitt Gray]

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

On Fri, Jun 11, 2021 at 06:23:34PM +0100, Jonathan Cameron wrote:
> On Fri, 11 Jun 2021 14:55:57 +0300
> Jarkko Nikula <jarkko.nikula@linux.intel.com> wrote:
> 
> > Remove CONFIG_PM ifdef and mark PM callbacks with __maybe_unused.
> > 
> > Suggested-by: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> 
> Both applied to the togreg branch of iio.git and pushed out
> as testing to let 0-day poke at them.
> 
> William, if you want to give feedback on these, still time for
> me to add tags etc. They just seem trivial enough its not worth
> wasting your time :)
> 
> Jonathan

Yes, these are pretty trivial and both look good to me. Feel free to add
my Ack to both patches if there's still time.

Acked-by: William Breathitt Gray <vilhelm.gray@gmail.com>

> > ---
> >  drivers/counter/intel-qep.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/counter/intel-qep.c b/drivers/counter/intel-qep.c
> > index ab10ba33f46a..a8d3dccecc0f 100644
> > --- a/drivers/counter/intel-qep.c
> > +++ b/drivers/counter/intel-qep.c
> > @@ -473,8 +473,7 @@ static void intel_qep_remove(struct pci_dev *pci)
> >  	intel_qep_writel(qep, INTEL_QEPCON, 0);
> >  }
> >  
> > -#ifdef CONFIG_PM
> > -static int intel_qep_suspend(struct device *dev)
> > +static int __maybe_unused intel_qep_suspend(struct device *dev)
> >  {
> >  	struct pci_dev *pdev = container_of(dev, struct pci_dev, dev);
> >  	struct intel_qep *qep = pci_get_drvdata(pdev);
> > @@ -486,7 +485,7 @@ static int intel_qep_suspend(struct device *dev)
> >  	return 0;
> >  }
> >  
> > -static int intel_qep_resume(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 intel_qep *qep = pci_get_drvdata(pdev);
> > @@ -512,7 +511,6 @@ static int intel_qep_resume(struct device *dev)
> >  
> >  	return 0;
> >  }
> > -#endif
> >  
> >  static UNIVERSAL_DEV_PM_OPS(intel_qep_pm_ops,
> >  			    intel_qep_suspend, intel_qep_resume, NULL);
> 

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