Re: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[kbuild test robot]

Hi Nipun,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v4.16-rc6]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Nipun-Gupta/dma-mapping-move-dma-configuration-to-bus-infrastructure/20180323-232307
config: score-spct6600_defconfig (attached as .config)
compiler: score-elf-gcc (GCC) 4.9.4
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=score 

All errors (new ones prefixed by >>):

   init/main.o: In function `try_to_run_init_process':
   main.c:(.text+0x40): relocation truncated to fit: R_SCORE_24 against 
`run_init_process'
   main.c:(.text+0x78): relocation truncated to fit: R_SCORE_24 against `.L36'
   init/main.o: In function `.L132':
   main.c:(.text+0x190): relocation truncated to fit: R_SCORE_24 against `.L129'
   main.c:(.text+0x200): relocation truncated to fit: R_SCORE_24 against `.L132'
   init/main.o: In function `loglevel':
   main.c:(.init.text+0xa4): relocation truncated to fit: R_SCORE_24 against 
`get_option'
   init/main.o: In function `.L15':
   main.c:(.init.text+0x110): relocation truncated to fit: R_SCORE_24 against 
`strcmp'
   main.c:(.init.text+0x124): relocation truncated to fit: R_SCORE_24 against 
`parameq'
   main.c:(.init.text+0x14c): relocation truncated to fit: R_SCORE_24 against 
`printk'
   init/main.o: In function `.L31':
   main.c:(.init.text+0x160): relocation truncated to fit: R_SCORE_24 against 
`strcmp'
   init/main.o: In function `.L21':
   main.c:(.init.text+0x170): relocation truncated to fit: R_SCORE_24 against 
`.L15'
   init/main.o: In function `initcall_blacklist':
   main.c:(.init.text+0x198): additional relocation overflows omitted from the 
output
   drivers/base/platform.o: In function `platform_dma_configure':
>> platform.c:(.text.platform_dma_configure+0x0): undefined reference to 
>> `dma_common_configure'

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

RE: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Nipun Gupta]

> -Original Message-
> From: Christoph Hellwig [mailto:h...@lst.de]
> Sent: Thursday, March 22, 2018 13:46
> To: Nipun Gupta 
> 
> > +static int amba_dma_configure(struct device *dev)
> > +{
> > +   return dma_common_configure(dev);
> > +}
> 
> So it turns out we only end with two callers of dma_common_configure
> after this series.  Based ont hat I'm tempted with the suggestion
> from Robin to just have amba call platform_dma_configure, and move
> the code from dma_common_configure to platform_dma_configure.

okay, that would be fine, trivial query - will it be okay to include
'linux/platform_device.h' in the AMBA bus? I am reluctant for this change
because of including platform file.

Thanks,
Nipun

Re: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[h...@lst.de]

> > +int dma_configure(struct device *dev)
> > +{
> > +   if (dev->bus->dma_configure)
> > +   return dev->bus->dma_configure(dev);
> 
> What if dma_common_configure() is called in case "bus->dma_configure" is not 
> defined?

Then we'd still have a dependency of common code on OF and ACPI.

On the other hand we'd get free OF and ACPI dma ranges parsing for
everyone, which might be handy.  And which would really help mitigating
the risk that we missed some bus that gets dma configuration from OF,
so maybe it actually is a good idea.  I'd just rename it to
dma_default_configure or similar in that case.

Re: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Christoph Hellwig]

> +static int amba_dma_configure(struct device *dev)
> +{
> + return dma_common_configure(dev);
> +}

So it turns out we only end with two callers of dma_common_configure
after this series.  Based ont hat I'm tempted with the suggestion
from Robin to just have amba call platform_dma_configure, and move
the code from dma_common_configure to platform_dma_configure.

> +int dma_configure(struct device *dev)
> +{
> + if (dev->bus->dma_configure)
> + return dev->bus->dma_configure(dev);
> +
> + return 0;
> +}
>  void dma_deconfigure(struct device *dev)

As grep pointed out this wants a blank line after the function, and while
in nitpicking mode I'd suggest to remove the blank line above the return
statement while at it.

> diff --git a/drivers/gpu/host1x/bus.c b/drivers/gpu/host1x/bus.c
> index 88a3558..fa9896d 100644
> --- a/drivers/gpu/host1x/bus.c
> +++ b/drivers/gpu/host1x/bus.c
> @@ -314,6 +314,14 @@ static int host1x_device_match(struct device *dev, 
> struct device_driver *drv)
>   return strcmp(dev_name(dev), drv->name) == 0;
>  }
>  
> +static int host1x_dma_configure(struct device *dev)
> +{
> + if (dev->of_node)
> + return of_dma_configure(dev, dev->of_node);
> +
> + return 0;

Same here.

> + */
> +static int pci_dma_configure(struct device *dev)
> +{
> + struct device *bridge;
> + enum dev_dma_attr attr;
> + int ret = 0;
> +
> + bridge = pci_get_host_bridge_device(to_pci_dev(dev));
> +
> + if (IS_ENABLED(CONFIG_OF) && bridge->parent &&
> + bridge->parent->of_node) {
> + ret = of_dma_configure(dev, bridge->parent->of_node);
> + } else if (has_acpi_companion(bridge)) {
> + attr = acpi_get_dma_attr(to_acpi_device_node(bridge->fwnode));
> + if (attr != DEV_DMA_NOT_SUPPORTED)
> + ret = acpi_dma_configure(dev, attr);
> + }

The attr declaration can be moved into the inner scope here.

> + pci_put_host_bridge_device(bridge);
> +
> + return ret;

Drop the blank line after the return, please.

RE: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Nipun Gupta]

> -Original Message-
> From: Greg KH [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, March 21, 2018 15:00



> > +int dma_configure(struct device *dev)
> > +{
> > +   if (dev->bus->dma_configure)
> > +   return dev->bus->dma_configure(dev);
> > +
> > +   return 0;
> > +}
> >  void dma_deconfigure(struct device *dev)
> 
> Empty line after this new function?  Sorry, couldn't help it :)
> 
> >  {
> > of_dma_deconfigure(dev);
> > diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> > index f1bf7b3..d2d5891 100644
> > --- a/drivers/base/platform.c
> > +++ b/drivers/base/platform.c
> > @@ -1130,6 +1130,11 @@ int platform_pm_restore(struct device *dev)
> >
> >  #endif /* CONFIG_HIBERNATE_CALLBACKS */
> >



> > +
> > const struct dev_pm_ops *pm;
> >
> > const struct iommu_ops *iommu_ops;
> > diff --git a/include/linux/dma-mapping.h b/include/linux/dma-mapping.h
> > index eb9eab4..c15986b 100644
> > --- a/include/linux/dma-mapping.h
> > +++ b/include/linux/dma-mapping.h
> > @@ -761,6 +761,7 @@ void *dma_mark_declared_memory_occupied(struct
> device *dev,
> >  }
> >  #endif /* CONFIG_HAVE_GENERIC_DMA_COHERENT */
> >
> > +int dma_common_configure(struct device *dev);
> >  #ifdef CONFIG_HAS_DMA
> 
> Blank line after the new function declaration?
> 
> Other than those very minor things, nice job, this looks good:
> 
> Reviewed-by: Greg Kroah-Hartman 

Thank you for the review :). I will fix your comments in next version.

Regards,
Nipun

Re: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Greg KH]

On Wed, Mar 21, 2018 at 12:25:22PM +0530, Nipun Gupta wrote:
> It's bus specific aspect to map a given device on the bus and
> relevant firmware description of its DMA configuration.
> So, this change introduces '/dma_configure/' as bus callback
> giving flexibility to busses for implementing its own dma
> configuration function.
> 
> The change eases the addition of new busses w.r.t. adding the dma
> configuration functionality.
> 
> This patch also updates the PCI, Platform, ACPI and host1x bus to
> use new introduced callbacks.
> 
> Suggested-by: Christoph Hellwig 
> Signed-off-by: Nipun Gupta 
> ---
>  - The patches are based on the comments on:
>https://patchwork.kernel.org/patch/10259087/
> 
> Changes in v2:
>   - Do not have dma_deconfigure callback
>   - Have '/dma_common_configure/' API to provide a common DMA
> configuration which can be used by busses if it suits them.
>   - Platform and ACPI bus to use '/dma_common_configure/' in
> '/dma_configure/' callback.
>   - Updated commit message
>   - Updated pci_dma_configure API with changes suggested by Robin
> 
>  drivers/amba/bus.c  |  7 +++
>  drivers/base/dma-mapping.c  | 35 +++
>  drivers/base/platform.c |  6 ++
>  drivers/gpu/host1x/bus.c|  9 +
>  drivers/pci/pci-driver.c| 32 
>  include/linux/device.h  |  4 
>  include/linux/dma-mapping.h |  1 +
>  7 files changed, 74 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
> index 594c228..2fa1e8b 100644
> --- a/drivers/amba/bus.c
> +++ b/drivers/amba/bus.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  
> @@ -171,6 +172,11 @@ static int amba_pm_runtime_resume(struct device *dev)
>  }
>  #endif /* CONFIG_PM */
>  
> +static int amba_dma_configure(struct device *dev)
> +{
> + return dma_common_configure(dev);
> +}
> +
>  static const struct dev_pm_ops amba_pm = {
>   .suspend= pm_generic_suspend,
>   .resume = pm_generic_resume,
> @@ -194,6 +200,7 @@ struct bus_type amba_bustype = {
>   .dev_groups = amba_dev_groups,
>   .match  = amba_match,
>   .uevent = amba_uevent,
> + .dma_configure  = amba_dma_configure,
>   .pm = _pm,
>   .force_dma  = true,
>  };
> diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
> index 3b11835..48f9af0 100644
> --- a/drivers/base/dma-mapping.c
> +++ b/drivers/base/dma-mapping.c
> @@ -331,38 +331,33 @@ void dma_common_free_remap(void *cpu_addr, size_t size, 
> unsigned long vm_flags)
>  #endif
>  
>  /*
> - * Common configuration to enable DMA API use for a device
> + * Common configuration to enable DMA API use for a device.
> + * A bus can use this function in its 'dma_configure' callback, if
> + * suitable for the bus.
>   */
> -#include 
> -
> -int dma_configure(struct device *dev)
> +int dma_common_configure(struct device *dev)
>  {
> - struct device *bridge = NULL, *dma_dev = dev;
>   enum dev_dma_attr attr;
>   int ret = 0;
>  
> - if (dev_is_pci(dev)) {
> - bridge = pci_get_host_bridge_device(to_pci_dev(dev));
> - dma_dev = bridge;
> - if (IS_ENABLED(CONFIG_OF) && dma_dev->parent &&
> - dma_dev->parent->of_node)
> - dma_dev = dma_dev->parent;
> - }
> -
> - if (dma_dev->of_node) {
> - ret = of_dma_configure(dev, dma_dev->of_node);
> - } else if (has_acpi_companion(dma_dev)) {
> - attr = acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
> + if (dev->of_node) {
> + ret = of_dma_configure(dev, dev->of_node);
> + } else if (has_acpi_companion(dev)) {
> + attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
>   if (attr != DEV_DMA_NOT_SUPPORTED)
>   ret = acpi_dma_configure(dev, attr);
>   }
>  
> - if (bridge)
> - pci_put_host_bridge_device(bridge);
> -
>   return ret;
>  }
>  
> +int dma_configure(struct device *dev)
> +{
> + if (dev->bus->dma_configure)
> + return dev->bus->dma_configure(dev);
> +
> + return 0;
> +}
>  void dma_deconfigure(struct device *dev)

Empty line after this new function?  Sorry, couldn't help it :)

>  {
>   of_dma_deconfigure(dev);
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index f1bf7b3..d2d5891 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1130,6 +1130,11 @@ int platform_pm_restore(struct device *dev)
>  
>  #endif /* CONFIG_HIBERNATE_CALLBACKS */
>  
> +static int platform_dma_configure(struct device *dev)
> +{
> + return dma_common_configure(dev);
> +}
> +
>  static const struct dev_pm_ops platform_dev_pm_ops = {
>   .runtime_suspend = pm_generic_runtime_suspend,
>   .runtime_resume = 

RE: [PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Nipun Gupta]

> -Original Message-
> From: Bharat Bhushan
> Sent: Wednesday, March 21, 2018 12:49

> >
> > +int dma_configure(struct device *dev)
> > +{
> > +   if (dev->bus->dma_configure)
> > +   return dev->bus->dma_configure(dev);
> 
> What if dma_common_configure() is called in case "bus->dma_configure" is
> not defined?
> 
> Thanks
> -Bharat

I think it is cleaner for bus to call '/dma_common_configure/' rather
than this been called implicitly, but Robin/Christoph can comment
better on this.

Thanks,
Nipun

[PATCH v2 1/2] dma-mapping: move dma configuration to bus infrastructure

[Nipun Gupta]

It's bus specific aspect to map a given device on the bus and
relevant firmware description of its DMA configuration.
So, this change introduces '/dma_configure/' as bus callback
giving flexibility to busses for implementing its own dma
configuration function.

The change eases the addition of new busses w.r.t. adding the dma
configuration functionality.

This patch also updates the PCI, Platform, ACPI and host1x bus to
use new introduced callbacks.

Suggested-by: Christoph Hellwig 
Signed-off-by: Nipun Gupta 
---
 - The patches are based on the comments on:
   https://patchwork.kernel.org/patch/10259087/

Changes in v2:
  - Do not have dma_deconfigure callback
  - Have '/dma_common_configure/' API to provide a common DMA
configuration which can be used by busses if it suits them.
  - Platform and ACPI bus to use '/dma_common_configure/' in
'/dma_configure/' callback.
  - Updated commit message
  - Updated pci_dma_configure API with changes suggested by Robin

 drivers/amba/bus.c  |  7 +++
 drivers/base/dma-mapping.c  | 35 +++
 drivers/base/platform.c |  6 ++
 drivers/gpu/host1x/bus.c|  9 +
 drivers/pci/pci-driver.c| 32 
 include/linux/device.h  |  4 
 include/linux/dma-mapping.h |  1 +
 7 files changed, 74 insertions(+), 20 deletions(-)

diff --git a/drivers/amba/bus.c b/drivers/amba/bus.c
index 594c228..2fa1e8b 100644
--- a/drivers/amba/bus.c
+++ b/drivers/amba/bus.c
@@ -20,6 +20,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 
@@ -171,6 +172,11 @@ static int amba_pm_runtime_resume(struct device *dev)
 }
 #endif /* CONFIG_PM */
 
+static int amba_dma_configure(struct device *dev)
+{
+   return dma_common_configure(dev);
+}
+
 static const struct dev_pm_ops amba_pm = {
.suspend= pm_generic_suspend,
.resume = pm_generic_resume,
@@ -194,6 +200,7 @@ struct bus_type amba_bustype = {
.dev_groups = amba_dev_groups,
.match  = amba_match,
.uevent = amba_uevent,
+   .dma_configure  = amba_dma_configure,
.pm = _pm,
.force_dma  = true,
 };
diff --git a/drivers/base/dma-mapping.c b/drivers/base/dma-mapping.c
index 3b11835..48f9af0 100644
--- a/drivers/base/dma-mapping.c
+++ b/drivers/base/dma-mapping.c
@@ -331,38 +331,33 @@ void dma_common_free_remap(void *cpu_addr, size_t size, 
unsigned long vm_flags)
 #endif
 
 /*
- * Common configuration to enable DMA API use for a device
+ * Common configuration to enable DMA API use for a device.
+ * A bus can use this function in its 'dma_configure' callback, if
+ * suitable for the bus.
  */
-#include 
-
-int dma_configure(struct device *dev)
+int dma_common_configure(struct device *dev)
 {
-   struct device *bridge = NULL, *dma_dev = dev;
enum dev_dma_attr attr;
int ret = 0;
 
-   if (dev_is_pci(dev)) {
-   bridge = pci_get_host_bridge_device(to_pci_dev(dev));
-   dma_dev = bridge;
-   if (IS_ENABLED(CONFIG_OF) && dma_dev->parent &&
-   dma_dev->parent->of_node)
-   dma_dev = dma_dev->parent;
-   }
-
-   if (dma_dev->of_node) {
-   ret = of_dma_configure(dev, dma_dev->of_node);
-   } else if (has_acpi_companion(dma_dev)) {
-   attr = acpi_get_dma_attr(to_acpi_device_node(dma_dev->fwnode));
+   if (dev->of_node) {
+   ret = of_dma_configure(dev, dev->of_node);
+   } else if (has_acpi_companion(dev)) {
+   attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
if (attr != DEV_DMA_NOT_SUPPORTED)
ret = acpi_dma_configure(dev, attr);
}
 
-   if (bridge)
-   pci_put_host_bridge_device(bridge);
-
return ret;
 }
 
+int dma_configure(struct device *dev)
+{
+   if (dev->bus->dma_configure)
+   return dev->bus->dma_configure(dev);
+
+   return 0;
+}
 void dma_deconfigure(struct device *dev)
 {
of_dma_deconfigure(dev);
diff --git a/drivers/base/platform.c b/drivers/base/platform.c
index f1bf7b3..d2d5891 100644
--- a/drivers/base/platform.c
+++ b/drivers/base/platform.c
@@ -1130,6 +1130,11 @@ int platform_pm_restore(struct device *dev)
 
 #endif /* CONFIG_HIBERNATE_CALLBACKS */
 
+static int platform_dma_configure(struct device *dev)
+{
+   return dma_common_configure(dev);
+}
+
 static const struct dev_pm_ops platform_dev_pm_ops = {
.runtime_suspend = pm_generic_runtime_suspend,
.runtime_resume = pm_generic_runtime_resume,
@@ -1141,6 +1146,7 @@ struct bus_type platform_bus_type = {
.dev_groups = platform_dev_groups,
.match  = platform_match,
.uevent = platform_uevent,
+   .dma_configure  = platform_dma_configure,
.pm = _dev_pm_ops,
.force_dma  =