| 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 |
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 >
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
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
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
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.
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
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
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.
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
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.
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).
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 > >
[+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
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 --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);