Re: [PATCH v2 2/2] drivers: remove force dma flag from buses
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 next-20180323]
[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: i386-randconfig-x014-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers//bcma/main.c: In function 'bcma_of_fill_device':
>> drivers//bcma/main.c:210:2: error: too many arguments to function
>> 'of_dma_configure'
of_dma_configure(&core->dev, node, false);
^~~~
In file included from include/linux/of_platform.h:12:0,
from drivers//bcma/main.c:17:
include/linux/of_device.h:110:19: note: declared here
static inline int of_dma_configure(struct device *dev, struct device_node
*np)
^~~~
vim +/of_dma_configure +210 drivers//bcma/main.c
198
199 static void bcma_of_fill_device(struct device *parent,
200 struct bcma_device *core)
201 {
202 struct device_node *node;
203
204 node = bcma_of_find_child_device(parent, core);
205 if (node)
206 core->dev.of_node = node;
207
208 core->irq = bcma_of_get_irq(parent, core, 0);
209
> 210 of_dma_configure(&core->dev, node, false);
211 }
212
---
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 2/2] drivers: remove force dma flag from buses
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 next-20180323]
[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: i386-randconfig-x013-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/base/dma-mapping.c: In function 'dma_common_configure':
>> drivers/base/dma-mapping.c:344:9: error: too many arguments to function
>> 'of_dma_configure'
ret = of_dma_configure(dev, dev->of_node, force_dma);
^~~~
In file included from drivers/base/dma-mapping.c:13:0:
include/linux/of_device.h:110:19: note: declared here
static inline int of_dma_configure(struct device *dev, struct device_node
*np)
^~~~
--
drivers/pci/pci-driver.c: In function 'pci_dma_configure':
>> drivers/pci/pci-driver.c:1544:9: error: too many arguments to function
>> 'of_dma_configure'
ret = of_dma_configure(dev, bridge->parent->of_node, true);
^~~~
In file included from drivers/pci/pci-driver.c:21:0:
include/linux/of_device.h:110:19: note: declared here
static inline int of_dma_configure(struct device *dev, struct device_node
*np)
^~~~
vim +/of_dma_configure +344 drivers/base/dma-mapping.c
332
333 /*
334 * Common configuration to enable DMA API use for a device.
335 * A bus can use this function in its 'dma_configure' callback, if
336 * suitable for the bus.
337 */
338 int dma_common_configure(struct device *dev, bool force_dma)
339 {
340 enum dev_dma_attr attr;
341 int ret = 0;
342
343 if (dev->of_node) {
> 344 ret = of_dma_configure(dev, dev->of_node, force_dma);
345 } else if (has_acpi_companion(dev)) {
346 attr =
acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
347 if (attr != DEV_DMA_NOT_SUPPORTED)
348 ret = acpi_dma_configure(dev, attr);
349 }
350
351 return ret;
352 }
353
---
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 2/2] drivers: remove force dma flag from buses
> -Original Message- > From: Christoph Hellwig [mailto:h...@lst.de] > Sent: Thursday, March 22, 2018 13:49 > To: Nipun Gupta > > > --- a/drivers/dma/qcom/hidma_mgmt.c > > +++ b/drivers/dma/qcom/hidma_mgmt.c > > @@ -398,7 +398,7 @@ static int __init > hidma_mgmt_of_populate_channels(struct device_node *np) > > } > > of_node_get(child); > > new_pdev->dev.of_node = child; > > - of_dma_configure(&new_pdev->dev, child); > > + of_dma_configure(&new_pdev->dev, child, true); > > /* > > * It is assumed that calling of_msi_configure is safe on > > * platforms with or without MSI support. > > Where did we mark this bus as force_dma before? I thought these devices to be on the platform bus as the device is of type 'struct platform_device', though I am not sure then why 'of_dma_configure()' is called here. Is this not on platform bus? > > > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > > index 9a4f4246..895c83e 100644 > > --- a/drivers/of/of_reserved_mem.c > > +++ b/drivers/of/of_reserved_mem.c > > @@ -353,7 +353,7 @@ int of_reserved_mem_device_init_by_idx(struct device > *dev, > > /* ensure that dma_ops is set for virtual devices > > * using reserved memory > > */ > > - of_dma_configure(dev, np); > > + of_dma_configure(dev, np, true); > > Did all the callers of this one really force dma? I have a hard time > untangling the call stacks unfortunately. I see this API being called indirectly from NXP DPAA device driver which is for platform bus devices. So I marked 'true' out here. There are more places from where it is being called. Thanks, Nipun
Re: [PATCH v2 2/2] drivers: remove force dma flag from buses
> --- a/drivers/dma/qcom/hidma_mgmt.c > +++ b/drivers/dma/qcom/hidma_mgmt.c > @@ -398,7 +398,7 @@ static int __init hidma_mgmt_of_populate_channels(struct > device_node *np) > } > of_node_get(child); > new_pdev->dev.of_node = child; > - of_dma_configure(&new_pdev->dev, child); > + of_dma_configure(&new_pdev->dev, child, true); > /* >* It is assumed that calling of_msi_configure is safe on >* platforms with or without MSI support. Where did we mark this bus as force_dma before? > diff --git a/drivers/of/of_reserved_mem.c b/drivers/of/of_reserved_mem.c > index 9a4f4246..895c83e 100644 > --- a/drivers/of/of_reserved_mem.c > +++ b/drivers/of/of_reserved_mem.c > @@ -353,7 +353,7 @@ int of_reserved_mem_device_init_by_idx(struct device *dev, > /* ensure that dma_ops is set for virtual devices >* using reserved memory >*/ > - of_dma_configure(dev, np); > + of_dma_configure(dev, np, true); Did all the callers of this one really force dma? I have a hard time untangling the call stacks unfortunately.
Re: [PATCH v2 2/2] drivers: remove force dma flag from buses
On Wed, Mar 21, 2018 at 04:28:46PM +, Nipun Gupta wrote: > > > > -Original Message- > > From: Greg KH [mailto:gre...@linuxfoundation.org] > > Sent: Wednesday, March 21, 2018 15:05 > > To: Nipun Gupta > > Cc: robin.mur...@arm.com; h...@lst.de; li...@armlinux.org.uk; > > m.szyprow...@samsung.com; bhelg...@google.com; zaj...@gmail.com; > > andy.gr...@linaro.org; david.br...@linaro.org; dan.j.willi...@intel.com; > > vinod.k...@intel.com; thierry.red...@gmail.com; robh...@kernel.org; > > frowand.l...@gmail.com; jarkko.sakki...@linux.intel.com; > > rafael.j.wyso...@intel.com; dmitry.torok...@gmail.com; jo...@kernel.org; > > msucha...@suse.de; linux-ker...@vger.kernel.org; iommu@lists.linux- > > foundation.org; linux-wireless@vger.kernel.org; linux-arm- > > m...@vger.kernel.org; linux-...@vger.kernel.org; dmaeng...@vger.kernel.org; > > dri-de...@lists.freedesktop.org; linux-te...@vger.kernel.org; > > devicet...@vger.kernel.org; linux-...@vger.kernel.org; Bharat Bhushan > > ; Leo Li > > Subject: Re: [PATCH v2 2/2] drivers: remove force dma flag from buses > > > > On Wed, Mar 21, 2018 at 12:25:23PM +0530, Nipun Gupta wrote: > > > With each bus implementing its own DMA configuration callback, > > > there is no need for bus to explicitly have force_dma in its > > > global structure. This patch modifies of_dma_configure API to > > > accept an input parameter which specifies if implicit DMA > > > configuration is required even when it is not described by the > > > firmware. > > > > Having to "remember" what that bool variable means on the end of the > > function call is a royal pain over time, right? > > > > Why not just create a new function: > > dma_common_configure_force(dma) > > that always does this? Leave "dma_common_configure()" alone, and then > > wrap the old code with these two helper functions that call the 'core' > > code with the bool set properly? > > > > That way you do not have to "know" what that parameter is, the function > > name just documents it automatically, so when you see it in the > > bus-specific code, no need to go and have to hunt for anything. And if > > you are reading the dma core code, it's obvious what is happening as the > > functions are all right there. > > How about we do not pass any flag in 'dma_common_configure()', and inside this > API we pass "true" to 'of_dma_configure()'? I am saying this because currently > both the busses (platform and AMBA) which uses 'dma_common_configure()' passes > "true" value. If we create additional 'dma_common_configure_force()', then > 'dma_common_configure()' will not be used anytime and will become redundant. > > If someday new busses come and they needs to use similar functionality which > 'dma_common_configure()' provides, but with passing "false" to > 'of_dma_configure()', > then what you suggests of having two separate such API's will be more > reasonable > and can be implemented? If that makes things "simple", yes, sounds good. greg k-h
RE: [PATCH v2 2/2] drivers: remove force dma flag from buses
> -Original Message- > From: Greg KH [mailto:gre...@linuxfoundation.org] > Sent: Wednesday, March 21, 2018 15:05 > To: Nipun Gupta > Cc: robin.mur...@arm.com; h...@lst.de; li...@armlinux.org.uk; > m.szyprow...@samsung.com; bhelg...@google.com; zaj...@gmail.com; > andy.gr...@linaro.org; david.br...@linaro.org; dan.j.willi...@intel.com; > vinod.k...@intel.com; thierry.red...@gmail.com; robh...@kernel.org; > frowand.l...@gmail.com; jarkko.sakki...@linux.intel.com; > rafael.j.wyso...@intel.com; dmitry.torok...@gmail.com; jo...@kernel.org; > msucha...@suse.de; linux-ker...@vger.kernel.org; iommu@lists.linux- > foundation.org; linux-wireless@vger.kernel.org; linux-arm- > m...@vger.kernel.org; linux-...@vger.kernel.org; dmaeng...@vger.kernel.org; > dri-de...@lists.freedesktop.org; linux-te...@vger.kernel.org; > devicet...@vger.kernel.org; linux-...@vger.kernel.org; Bharat Bhushan > ; Leo Li > Subject: Re: [PATCH v2 2/2] drivers: remove force dma flag from buses > > On Wed, Mar 21, 2018 at 12:25:23PM +0530, Nipun Gupta wrote: > > With each bus implementing its own DMA configuration callback, > > there is no need for bus to explicitly have force_dma in its > > global structure. This patch modifies of_dma_configure API to > > accept an input parameter which specifies if implicit DMA > > configuration is required even when it is not described by the > > firmware. > > Having to "remember" what that bool variable means on the end of the > function call is a royal pain over time, right? > > Why not just create a new function: > dma_common_configure_force(dma) > that always does this? Leave "dma_common_configure()" alone, and then > wrap the old code with these two helper functions that call the 'core' > code with the bool set properly? > > That way you do not have to "know" what that parameter is, the function > name just documents it automatically, so when you see it in the > bus-specific code, no need to go and have to hunt for anything. And if > you are reading the dma core code, it's obvious what is happening as the > functions are all right there. How about we do not pass any flag in 'dma_common_configure()', and inside this API we pass "true" to 'of_dma_configure()'? I am saying this because currently both the busses (platform and AMBA) which uses 'dma_common_configure()' passes "true" value. If we create additional 'dma_common_configure_force()', then 'dma_common_configure()' will not be used anytime and will become redundant. If someday new busses come and they needs to use similar functionality which 'dma_common_configure()' provides, but with passing "false" to 'of_dma_configure()', then what you suggests of having two separate such API's will be more reasonable and can be implemented? Thanks, Nipun > > thanks, > > greg k-h
Re: [PATCH v2 2/2] drivers: remove force dma flag from buses
On Wed, Mar 21, 2018 at 12:25:23PM +0530, Nipun Gupta wrote: > With each bus implementing its own DMA configuration callback, > there is no need for bus to explicitly have force_dma in its > global structure. This patch modifies of_dma_configure API to > accept an input parameter which specifies if implicit DMA > configuration is required even when it is not described by the > firmware. Having to "remember" what that bool variable means on the end of the function call is a royal pain over time, right? Why not just create a new function: dma_common_configure_force(dma) that always does this? Leave "dma_common_configure()" alone, and then wrap the old code with these two helper functions that call the 'core' code with the bool set properly? That way you do not have to "know" what that parameter is, the function name just documents it automatically, so when you see it in the bus-specific code, no need to go and have to hunt for anything. And if you are reading the dma core code, it's obvious what is happening as the functions are all right there. thanks, greg k-h
