diff mbox series

[V2,2/6] driver core: auxiliary bus: Add driver data helpers

Message ID 20211207171448.799376-3-david.e.box@linux.intel.com
State Accepted
Commit 365481e42a8a95c55e43e8cc236138718e762e7b
Headers show
Series Auxiliary bus driver support for Intel PCIe VSEC/DVSEC | expand

Commit Message

David E. Box Dec. 7, 2021, 5:14 p.m. UTC
Adds get/set driver data helpers for auxiliary devices.

Signed-off-by: David E. Box <david.e.box@linux.intel.com>
Reviewed-by: Mark Gross <markgross@kernel.org>
---
V2
  - No changes

 include/linux/auxiliary_bus.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Leon Romanovsky Dec. 8, 2021, 7:03 a.m. UTC | #1
On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> Adds get/set driver data helpers for auxiliary devices.
> 
> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> Reviewed-by: Mark Gross <markgross@kernel.org>
> ---
> V2
>   - No changes
> 
>  include/linux/auxiliary_bus.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)

I would really like to see an explanation why such obfuscation is really
needed. dev_*_drvdata() is a standard way to access driver data.

Thanks

> 
> diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
> index fc51d45f106b..a8338d456e81 100644
> --- a/include/linux/auxiliary_bus.h
> +++ b/include/linux/auxiliary_bus.h
> @@ -28,6 +28,16 @@ struct auxiliary_driver {
>  	const struct auxiliary_device_id *id_table;
>  };
>  
> +static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
> +{
> +	return dev_get_drvdata(&auxdev->dev);
> +}
> +
> +static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
> +{
> +	dev_set_drvdata(&auxdev->dev, data);
> +}
> +
>  static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
>  {
>  	return container_of(dev, struct auxiliary_device, dev);
> -- 
> 2.25.1
>
Greg Kroah-Hartman Dec. 8, 2021, 7:07 a.m. UTC | #2
On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > Adds get/set driver data helpers for auxiliary devices.
> > 
> > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > Reviewed-by: Mark Gross <markgross@kernel.org>
> > ---
> > V2
> >   - No changes
> > 
> >  include/linux/auxiliary_bus.h | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> 
> I would really like to see an explanation why such obfuscation is really
> needed. dev_*_drvdata() is a standard way to access driver data.

Lots of busses have this helper.  This is nothing new at all, and is
nice to have.  Look at all of the calls to dev_get_drvdata() in
include/linux/ for the examples.

thanks,

greg k-h
Leon Romanovsky Dec. 8, 2021, 8:32 a.m. UTC | #3
On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > ---
> > > V2
> > >   - No changes
> > > 
> > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > 
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> Lots of busses have this helper.  This is nothing new at all, and is
> nice to have.  Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.

I looked and this is why I asked. From the point of person who does
reviews and refactoring, such obfuscations are evil.

If I understand your correctly, the explanation is just copy/paste, right?

Thanks

> 
> thanks,
> 
> greg k-h
Greg Kroah-Hartman Dec. 8, 2021, 8:43 a.m. UTC | #4
On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > Lots of busses have this helper.  This is nothing new at all, and is
> > nice to have.  Look at all of the calls to dev_get_drvdata() in
> > include/linux/ for the examples.
> 
> I looked and this is why I asked. From the point of person who does
> reviews and refactoring, such obfuscations are evil.

Then you must consider about 80 busses evil :)

Again, this is a very common helper pattern in the kernel, one that we
have had for decades now.  It allows a driver writer to only worry about
the bus apis for that specific bus, and not have to dive down into the
driver core functions at all.  What is wrong with that?

> If I understand your correctly, the explanation is just copy/paste, right?

I do not understand what you mean by this, sorry.

greg k-h
Lee Jones Dec. 8, 2021, 8:43 a.m. UTC | #5
On Wed, 08 Dec 2021, Greg KH wrote:

> On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > Adds get/set driver data helpers for auxiliary devices.
> > > 
> > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > ---
> > > V2
> > >   - No changes
> > > 
> > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > 
> > I would really like to see an explanation why such obfuscation is really
> > needed. dev_*_drvdata() is a standard way to access driver data.

I wouldn't call it obfuscation, but it does looks like abstraction for
the sake of abstraction, which I usually push back on.  What are the
technical benefits over using the dev_*() variant?

> Lots of busses have this helper.  This is nothing new at all, and is
> nice to have.  Look at all of the calls to dev_get_drvdata() in
> include/linux/ for the examples.
Greg Kroah-Hartman Dec. 8, 2021, 8:47 a.m. UTC | #6
On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
> 
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on.  What are the
> technical benefits over using the dev_*() variant?

See my response at:
	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
for why it is a good thing to do.

In short, driver authors should not have to worry about mixing
bus-specific and low-level driver core functions.

thanks,

greg k-h
Leon Romanovsky Dec. 8, 2021, 9:12 a.m. UTC | #7
On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 10:32:13AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 08:07:39AM +0100, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > > 
> > > Lots of busses have this helper.  This is nothing new at all, and is
> > > nice to have.  Look at all of the calls to dev_get_drvdata() in
> > > include/linux/ for the examples.
> > 
> > I looked and this is why I asked. From the point of person who does
> > reviews and refactoring, such obfuscations are evil.
> 
> Then you must consider about 80 busses evil :)
> 
> Again, this is a very common helper pattern in the kernel, one that we
> have had for decades now.  It allows a driver writer to only worry about
> the bus apis for that specific bus, and not have to dive down into the
> driver core functions at all.  What is wrong with that?

What is wrong?

The idea that you have two APIs which do the same thing, one is
obfuscated version of another.

If you don't want from people to use driver core function and structures,
you shouldn't expose them in global headers.

Thanks
Lee Jones Dec. 8, 2021, 9:13 a.m. UTC | #8
On Wed, 08 Dec 2021, Greg KH wrote:

> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > 
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.

I saw this after I'd sent my query.

> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

Okay, that makes sense.

I guess my view abstraction for the sake of it is slightly higher
level as I vehemently dislike it when driver-set writers create their
own APIs, such as; (just off the top of my head, not a real example)
cros_get_data() or cros_write() which are usually abstractions of top
level APIs like platform_get_data() and regmap_write() respectively.

Abstracting at *real* API level does seem like the right thing to do.
Leon Romanovsky Dec. 8, 2021, 9:18 a.m. UTC | #9
On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> On Wed, 08 Dec 2021, Greg KH wrote:
> 
> > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > Adds get/set driver data helpers for auxiliary devices.
> > > > 
> > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > ---
> > > > V2
> > > >   - No changes
> > > > 
> > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > 
> > > I would really like to see an explanation why such obfuscation is really
> > > needed. dev_*_drvdata() is a standard way to access driver data.
> 
> I wouldn't call it obfuscation, but it does looks like abstraction for
> the sake of abstraction, which I usually push back on.  What are the
> technical benefits over using the dev_*() variant?

You can see it in Greg's answer, there is no technical benefits in any
variant. It is simple copy/paste pattern from other buses.

Maybe it is not clear from my response, I don't care if this patch is
going to be applied or not, but I would like to hear someone explains
to me what are the benefits of such one liners.

Thanks
Andy Shevchenko Dec. 8, 2021, 10:14 a.m. UTC | #10
On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:

...

> The idea that you have two APIs which do the same thing, one is
> obfuscated version of another.
> 
> If you don't want from people to use driver core function and structures,
> you shouldn't expose them in global headers.

For all these APIs the rationale is very simple. If you have callback that
takes a pointer to the container (*), you better use the APIs related to
this container (no need to have an explicit dereferencing). Otherwise you
use dev_*() APIs (when it's pointer to the pure struct device).

The value is to have coherent APIs around struct device containers.

*) under container here I assume the data structure that has the embedded
   struct device in it.
Andy Shevchenko Dec. 8, 2021, 10:15 a.m. UTC | #11
On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > 
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.
> 
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

Right! I just answered to Leon with the similar view behind (using
different words).
Leon Romanovsky Dec. 8, 2021, 10:48 a.m. UTC | #12
On Wed, Dec 08, 2021 at 12:14:41PM +0200, Andy Shevchenko wrote:
> On Wed, Dec 08, 2021 at 11:12:20AM +0200, Leon Romanovsky wrote:
> > On Wed, Dec 08, 2021 at 09:43:37AM +0100, Greg KH wrote:
> 
> ...
> 
> > The idea that you have two APIs which do the same thing, one is
> > obfuscated version of another.
> > 
> > If you don't want from people to use driver core function and structures,
> > you shouldn't expose them in global headers.
> 
> For all these APIs the rationale is very simple. If you have callback that
> takes a pointer to the container (*), you better use the APIs related to
> this container (no need to have an explicit dereferencing). Otherwise you
> use dev_*() APIs (when it's pointer to the pure struct device).
> 
> The value is to have coherent APIs around struct device containers.
> 
> *) under container here I assume the data structure that has the embedded
>    struct device in it.

Thanks

> 
 -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Bjorn Helgaas Dec. 9, 2021, 4:32 p.m. UTC | #13
[+cc Rafael, since I used generic PM as an example]

On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:
> On Wed, Dec 08, 2021 at 08:43:53AM +0000, Lee Jones wrote:
> > On Wed, 08 Dec 2021, Greg KH wrote:
> > > On Wed, Dec 08, 2021 at 09:03:16AM +0200, Leon Romanovsky wrote:
> > > > On Tue, Dec 07, 2021 at 09:14:44AM -0800, David E. Box wrote:
> > > > > Adds get/set driver data helpers for auxiliary devices.
> > > > > 
> > > > > Signed-off-by: David E. Box <david.e.box@linux.intel.com>
> > > > > Reviewed-by: Mark Gross <markgross@kernel.org>
> > > > > ---
> > > > > V2
> > > > >   - No changes
> > > > > 
> > > > >  include/linux/auxiliary_bus.h | 10 ++++++++++
> > > > >  1 file changed, 10 insertions(+)
> > > > 
> > > > I would really like to see an explanation why such obfuscation is really
> > > > needed. dev_*_drvdata() is a standard way to access driver data.
> > 
> > I wouldn't call it obfuscation, but it does looks like abstraction for
> > the sake of abstraction, which I usually push back on.  What are the
> > technical benefits over using the dev_*() variant?
> 
> See my response at:
> 	https://lore.kernel.org/r/YbBwOb6JvWkT3JWI@kroah.com
> for why it is a good thing to do.
> 
> In short, driver authors should not have to worry about mixing
> bus-specific and low-level driver core functions.

In the very common situation of PCI drivers that use generic power
management, authors *do* have to use both (example from [1]):

  ioh_gpio_probe(struct pci_dev *pdev)   # pci_driver.probe()
    pci_set_drvdata(pdev, chip);

  ioh_gpio_remove(struct pci_dev *pdev)  # pci_driver.remove()
    struct ioh_gpio *chip = pci_get_drvdata(pdev);

  ioh_gpio_suspend(struct device *dev)   # pci_driver.driver.pm.suspend()
    struct ioh_gpio *chip = dev_get_drvdata(dev);   <--

The pci_driver methods receive a struct pci_dev and use the
pci_get_drvdata() wrapper.

The generic power management methods receive a struct device and use
the underlying dev_get_drvdata().

It's kind of ugly that readers have to know that pci_get_drvdata()
gives you the same thing as dev_get_drvdata().

I guess the generic PM methods could do something like:

  pci_get_drvdata(to_pci_dev(dev));

but that seems a little bit circuitous.  It's slightly wordier, but I
might prefer to just use this everywhere and skip the pci_* wrappers:

  dev_get_drvdata(&pdev->dev);

Bjorn

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505
Andy Shevchenko Dec. 9, 2021, 4:54 p.m. UTC | #14
On Thu, Dec 09, 2021 at 10:32:45AM -0600, Bjorn Helgaas wrote:
> [+cc Rafael, since I used generic PM as an example]
> On Wed, Dec 08, 2021 at 09:47:56AM +0100, Greg KH wrote:

...

Okay, more bikeshedding :-)

> In the very common situation of PCI drivers that use generic power
> management, authors *do* have to use both (example from [1]):
> 
>   ioh_gpio_probe(struct pci_dev *pdev)   # pci_driver.probe()
>     pci_set_drvdata(pdev, chip);
> 
>   ioh_gpio_remove(struct pci_dev *pdev)  # pci_driver.remove()
>     struct ioh_gpio *chip = pci_get_drvdata(pdev);
> 
>   ioh_gpio_suspend(struct device *dev)   # pci_driver.driver.pm.suspend()
>     struct ioh_gpio *chip = dev_get_drvdata(dev);   <--
> 
> The pci_driver methods receive a struct pci_dev and use the
> pci_get_drvdata() wrapper.
> 
> The generic power management methods receive a struct device and use
> the underlying dev_get_drvdata().
> 
> It's kind of ugly that readers have to know that pci_get_drvdata()
> gives you the same thing as dev_get_drvdata().
> 
> I guess the generic PM methods could do something like:
> 
>   pci_get_drvdata(to_pci_dev(dev));
> 
> but that seems a little bit circuitous.  It's slightly wordier, but I
> might prefer to just use this everywhere and skip the pci_* wrappers:
> 
>   dev_get_drvdata(&pdev->dev);

Strictly speaking the

   <$BUS)_get_drvdata(<$CONTAINER>) != dev_get_drvdata(dev)

it's completely up to the container handling code what to do.
In 99% (or 100%?) cases it's equal, but it's not obliged to be so.

> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpio/gpio-ml-ioh.c?id=v5.15#n505
diff mbox series

Patch

diff --git a/include/linux/auxiliary_bus.h b/include/linux/auxiliary_bus.h
index fc51d45f106b..a8338d456e81 100644
--- a/include/linux/auxiliary_bus.h
+++ b/include/linux/auxiliary_bus.h
@@ -28,6 +28,16 @@  struct auxiliary_driver {
 	const struct auxiliary_device_id *id_table;
 };
 
+static inline void *auxiliary_get_drvdata(struct auxiliary_device *auxdev)
+{
+	return dev_get_drvdata(&auxdev->dev);
+}
+
+static inline void auxiliary_set_drvdata(struct auxiliary_device *auxdev, void *data)
+{
+	dev_set_drvdata(&auxdev->dev, data);
+}
+
 static inline struct auxiliary_device *to_auxiliary_dev(struct device *dev)
 {
 	return container_of(dev, struct auxiliary_device, dev);