Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Joerg Roedel]

Hi Marek,

On Tue, Feb 07, 2017 at 01:36:15PM +0100, Marek Szyprowski wrote:
> >+ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
> >+ "sysmmu.%pa", );
> 
> Can we stick to the common name across the /sysfs and use
> dev_name(data->sysmmu)
> or even dev_name(dev) here?
> 
> ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
> dev_name(dev));

That means that we have multiple 'struct device' with the same name,
no? I think will lead to confusion when using dev_printk, as its not
clear anymore which device is refered to in the message.


Joerg

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Joerg Roedel]

Hi Marek,

On Tue, Feb 07, 2017 at 01:36:15PM +0100, Marek Szyprowski wrote:
> >+ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
> >+ "sysmmu.%pa", );
> 
> Can we stick to the common name across the /sysfs and use
> dev_name(data->sysmmu)
> or even dev_name(dev) here?
> 
> ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
> dev_name(dev));

That means that we have multiple 'struct device' with the same name,
no? I think will lead to confusion when using dev_printk, as its not
clear anymore which device is refered to in the message.


Joerg

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Marek Szyprowski]

Hi Joerg,

On 2017-02-08 14:57, Joerg Roedel wrote:

On Tue, Feb 07, 2017 at 01:36:15PM +0100, Marek Szyprowski wrote:

+   ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
+"sysmmu.%pa", );

Can we stick to the common name across the /sysfs and use
dev_name(data->sysmmu)
or even dev_name(dev) here?

ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
dev_name(dev));

That means that we have multiple 'struct device' with the same name,
no? I think will lead to confusion when using dev_printk, as its not
clear anymore which device is refered to in the message.


Each sysmmu device has unique name, like every other platform device
instantiated from device tree. Here is an example from the OdroidXU3
(Exynos5422 based):

# ls -1 /sys/bus/platform/devices/ | grep sysmmu
10a6.sysmmu
10a7.sysmmu
1120.sysmmu
1121.sysmmu
11d4.sysmmu
11f1.sysmmu
11f2.sysmmu
1288.sysmmu
1289.sysmmu
128a.sysmmu
128c.sysmmu
128d.sysmmu
128e.sysmmu
13e8.sysmmu
13e9.sysmmu
1464.sysmmu
1465.sysmmu
1468.sysmmu

IMHO there is no need for open coding new unique names if device name,
which already contains the base address, can be used instead.

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Marek Szyprowski]

Hi Joerg,

On 2017-02-08 14:57, Joerg Roedel wrote:

On Tue, Feb 07, 2017 at 01:36:15PM +0100, Marek Szyprowski wrote:

+   ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
+"sysmmu.%pa", );

Can we stick to the common name across the /sysfs and use
dev_name(data->sysmmu)
or even dev_name(dev) here?

ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
dev_name(dev));

That means that we have multiple 'struct device' with the same name,
no? I think will lead to confusion when using dev_printk, as its not
clear anymore which device is refered to in the message.


Each sysmmu device has unique name, like every other platform device
instantiated from device tree. Here is an example from the OdroidXU3
(Exynos5422 based):

# ls -1 /sys/bus/platform/devices/ | grep sysmmu
10a6.sysmmu
10a7.sysmmu
1120.sysmmu
1121.sysmmu
11d4.sysmmu
11f1.sysmmu
11f2.sysmmu
1288.sysmmu
1289.sysmmu
128a.sysmmu
128c.sysmmu
128d.sysmmu
128e.sysmmu
13e8.sysmmu
13e9.sysmmu
1464.sysmmu
1465.sysmmu
1468.sysmmu

IMHO there is no need for open coding new unique names if device name,
which already contains the base address, can be used instead.

Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Marek Szyprowski]

Hi Joerg,

On 2017-02-06 17:10, Joerg Roedel wrote:

From: Joerg Roedel 

Register Exynos IOMMUs to the IOMMU core and make them
visible in sysfs. This patch does not add the links between
IOMMUs and translated devices yet.

Cc: Marek Szyprowski 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Signed-off-by: Joerg Roedel 
---
  drivers/iommu/exynos-iommu.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 57ba0d3..90f0f52 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -276,6 +276,8 @@ struct sysmmu_drvdata {
struct list_head owner_node;/* node for owner controllers list */
phys_addr_t pgtable;/* assigned page table structure */
unsigned int version;   /* our version */
+
+   struct iommu_device iommu;  /* IOMMU core handle */
  };
  
  static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)

@@ -556,6 +558,7 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
struct device *dev = >dev;
struct sysmmu_drvdata *data;
struct resource *res;
+   resource_size_t ioaddr;
  
  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);

if (!data)
@@ -565,6 +568,7 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
data->sfrbase = devm_ioremap_resource(dev, res);
if (IS_ERR(data->sfrbase))
return PTR_ERR(data->sfrbase);
+   ioaddr = res->start;
  
  	irq = platform_get_irq(pdev, 0);

if (irq <= 0) {
@@ -611,6 +615,18 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
data->sysmmu = dev;
spin_lock_init(>lock);
  
+	ret = iommu_device_sysfs_add(>iommu, >dev, NULL,

+"sysmmu.%pa", );


Can we stick to the common name across the /sysfs and use 
dev_name(data->sysmmu)

or even dev_name(dev) here?

ret = iommu_device_sysfs_add(>iommu, >dev, NULL, 
dev_name(dev));



+   if (ret)
+   return ret;
+
+   data->iommu.ops= _iommu_ops;
+   data->iommu.fwnode = >of_node->fwnode;
+
+   ret = iommu_device_register(>iommu);
+   if (ret)
+   return ret;
+
platform_set_drvdata(pdev, data);
  
  	__sysmmu_get_version(data);


Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[Marek Szyprowski]

Hi Joerg,

On 2017-02-06 17:10, Joerg Roedel wrote:

From: Joerg Roedel 

Register Exynos IOMMUs to the IOMMU core and make them
visible in sysfs. This patch does not add the links between
IOMMUs and translated devices yet.

Cc: Marek Szyprowski 
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-samsung-...@vger.kernel.org
Signed-off-by: Joerg Roedel 
---
  drivers/iommu/exynos-iommu.c | 16 
  1 file changed, 16 insertions(+)

diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index 57ba0d3..90f0f52 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -276,6 +276,8 @@ struct sysmmu_drvdata {
struct list_head owner_node;/* node for owner controllers list */
phys_addr_t pgtable;/* assigned page table structure */
unsigned int version;   /* our version */
+
+   struct iommu_device iommu;  /* IOMMU core handle */
  };
  
  static struct exynos_iommu_domain *to_exynos_domain(struct iommu_domain *dom)

@@ -556,6 +558,7 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
struct device *dev = >dev;
struct sysmmu_drvdata *data;
struct resource *res;
+   resource_size_t ioaddr;
  
  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);

if (!data)
@@ -565,6 +568,7 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
data->sfrbase = devm_ioremap_resource(dev, res);
if (IS_ERR(data->sfrbase))
return PTR_ERR(data->sfrbase);
+   ioaddr = res->start;
  
  	irq = platform_get_irq(pdev, 0);

if (irq <= 0) {
@@ -611,6 +615,18 @@ static int __init exynos_sysmmu_probe(struct 
platform_device *pdev)
data->sysmmu = dev;
spin_lock_init(>lock);
  
+	ret = iommu_device_sysfs_add(>iommu, >dev, NULL,

+"sysmmu.%pa", );


Can we stick to the common name across the /sysfs and use 
dev_name(data->sysmmu)

or even dev_name(dev) here?

ret = iommu_device_sysfs_add(>iommu, >dev, NULL, 
dev_name(dev));



+   if (ret)
+   return ret;
+
+   data->iommu.ops= _iommu_ops;
+   data->iommu.fwnode = >of_node->fwnode;
+
+   ret = iommu_device_register(>iommu);
+   if (ret)
+   return ret;
+
platform_set_drvdata(pdev, data);
  
  	__sysmmu_get_version(data);


Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[kbuild test robot]

Hi Joerg,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc7]
[cannot apply to iommu/next next-20170206]
[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/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170207-003931
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

Note: the 
linux-review/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170207-003931
 HEAD 0b8fd0f8bbcfb693621ee90abffb0774c143549d builds fine.
  It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/iommu/exynos-iommu.c: In function 'exynos_sysmmu_probe':
>> drivers/iommu/exynos-iommu.c:624:13: error: 'struct iommu_device' has no 
>> member named 'fwnode'
 data->iommu.fwnode = >of_node->fwnode;
^

vim +624 drivers/iommu/exynos-iommu.c

   618  ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
   619   "sysmmu.%pa", );
   620  if (ret)
   621  return ret;
   622  
   623  data->iommu.ops= _iommu_ops;
 > 624  data->iommu.fwnode = >of_node->fwnode;
   625  
   626  ret = iommu_device_register(>iommu);
   627  if (ret)

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


.config.gz
Description: application/gzip

Re: [PATCH 09/10] iommu/exynos: Make use of iommu_device_register interface

[kbuild test robot]

Hi Joerg,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.10-rc7]
[cannot apply to iommu/next next-20170206]
[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/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170207-003931
config: arm-allyesconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
 -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=arm 

Note: the 
linux-review/Joerg-Roedel/Let-IOMMU-core-know-about-individual-IOMMUs/20170207-003931
 HEAD 0b8fd0f8bbcfb693621ee90abffb0774c143549d builds fine.
  It only hurts bisectibility.

All errors (new ones prefixed by >>):

   drivers/iommu/exynos-iommu.c: In function 'exynos_sysmmu_probe':
>> drivers/iommu/exynos-iommu.c:624:13: error: 'struct iommu_device' has no 
>> member named 'fwnode'
 data->iommu.fwnode = >of_node->fwnode;
^

vim +624 drivers/iommu/exynos-iommu.c

   618  ret = iommu_device_sysfs_add(>iommu, >dev, NULL,
   619   "sysmmu.%pa", );
   620  if (ret)
   621  return ret;
   622  
   623  data->iommu.ops= _iommu_ops;
 > 624  data->iommu.fwnode = >of_node->fwnode;
   625  
   626  ret = iommu_device_register(>iommu);
   627  if (ret)

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


.config.gz
Description: application/gzip