[PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Manish Jaggi]

It is good for code readability as there are many structures ending with the name domain.
Also a code like this one is now easy to understand with the rename
old: dev_iommu_domain(dev) = domain;
new: dev_iommu_domain(dev) = iommu_domain;

Also in current code struct smmu_domain pointer variable name is always smmu_domain.
The change is on the same lines

Signed-off-by: Manish Jaggi <manish.jaggi@caviumnetworks.com>
---
  xen/drivers/passthrough/arm/smmu.c | 86 +++++++++++++++++++-------------------
  1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index fe0549e..1d294be 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -248,12 +248,12 @@ struct domain_iommu_info {
   * place.
   * */
  struct device_iommu_info {
-	struct iommu_domain *domain;
+	struct iommu_domain *iommu_domain;
  	struct iommu_group *group;
  };
  
  #define dev_archdata(dev) ((struct device_iommu_info *)dev->archdata.iommu)
-#define dev_iommu_domain(dev) (dev_archdata(dev)->domain)
+#define dev_iommu_domain(dev) (dev_archdata(dev)->iommu_domain)
  #define dev_iommu_group(dev) (dev_archdata(dev)->group)
  
  /* Xen: Dummy iommu_group */
@@ -916,8 +916,8 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
  	int flags, ret;
  	u32 fsr, far, fsynr, resume;
  	unsigned long iova;
-	struct iommu_domain *domain = dev;
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct iommu_domain *iommu_domain = dev;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
  	struct arm_smmu_device *smmu = smmu_domain->smmu;
  	void __iomem *cb_base;
@@ -943,7 +943,7 @@ static irqreturn_t arm_smmu_context_fault(int irq, void *dev)
  	iova |= ((unsigned long)far << 32);
  #endif
  
-	if (!report_iommu_fault(domain, smmu->dev, iova, flags)) {
+	if (!report_iommu_fault(iommu_domain, smmu->dev, iova, flags)) {
  		ret = IRQ_HANDLED;
  		resume = RESUME_RETRY;
  	} else {
@@ -1205,12 +1205,12 @@ static void arm_smmu_init_context_bank(struct arm_smmu_domain *smmu_domain)
  	writel_relaxed(reg, cb_base + ARM_SMMU_CB_SCTLR);
  }
  
-static int arm_smmu_init_domain_context(struct iommu_domain *domain,
+static int arm_smmu_init_domain_context(struct iommu_domain *iommu_domain,
  					struct arm_smmu_device *smmu)
  {
  	int irq, start, ret = 0;
  	unsigned long flags;
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
  
  	spin_lock_irqsave(&smmu_domain->lock, flags);
@@ -1278,7 +1278,7 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
  
  	irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
  	ret = request_irq(irq, arm_smmu_context_fault, IRQF_SHARED,
-			  "arm-smmu-context-fault", domain);
+			  "arm-smmu-context-fault", iommu_domain);
  	if (IS_ERR_VALUE(ret)) {
  		dev_err(smmu->dev, "failed to request context IRQ %d (%u)\n",
  			cfg->irptndx, irq);
@@ -1292,9 +1292,9 @@ out_unlock:
  	return ret;
  }
  
-static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
+static void arm_smmu_destroy_domain_context(struct iommu_domain *iommu_domain)
  {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_device *smmu = smmu_domain->smmu;
  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
  	void __iomem *cb_base;
@@ -1310,13 +1310,13 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
  
  	if (cfg->irptndx != INVALID_IRPTNDX) {
  		irq = smmu->irqs[smmu->num_global_irqs + cfg->irptndx];
-		free_irq(irq, domain);
+		free_irq(irq, iommu_domain);
  	}
  
  	__arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
  }
  
-static int arm_smmu_domain_init(struct iommu_domain *domain)
+static int arm_smmu_domain_init(struct iommu_domain *iommu_domain)
  {
  	struct arm_smmu_domain *smmu_domain;
  
@@ -1330,7 +1330,7 @@ static int arm_smmu_domain_init(struct iommu_domain *domain)
  		return -ENOMEM;
  
  	spin_lock_init(&smmu_domain->lock);
-	domain->priv = smmu_domain;
+	iommu_domain->priv = smmu_domain;
  	return 0;
  }
  
@@ -1399,15 +1399,15 @@ static void arm_smmu_free_pgtables(struct arm_smmu_domain *smmu_domain)
  }
  #endif
  
-static void arm_smmu_domain_destroy(struct iommu_domain *domain)
+static void arm_smmu_domain_destroy(struct iommu_domain *iommu_domain)
  {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  
  	/*
  	 * Free the domain resources. We assume that all devices have
  	 * already been detached.
  	 */
-	arm_smmu_destroy_domain_context(domain);
+	arm_smmu_destroy_domain_context(iommu_domain);
  	kfree(smmu_domain);
  }
  
@@ -1536,10 +1536,10 @@ static void arm_smmu_domain_remove_master(struct arm_smmu_domain *smmu_domain,
  	arm_smmu_master_free_smrs(smmu, cfg);
  }
  
-static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
+static int arm_smmu_attach_dev(struct iommu_domain *iommu_domain, struct device *dev)
  {
  	int ret;
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_device *smmu, *dom_smmu;
  	struct arm_smmu_master_cfg *cfg;
  
@@ -1561,7 +1561,7 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
  	dom_smmu = ACCESS_ONCE(smmu_domain->smmu);
  	if (!dom_smmu) {
  		/* Now that we have a master, we can finalise the domain */
-		ret = arm_smmu_init_domain_context(domain, smmu);
+		ret = arm_smmu_init_domain_context(iommu_domain, smmu);
  		if (IS_ERR_VALUE(ret))
  			return ret;
  
@@ -1583,13 +1583,13 @@ static int arm_smmu_attach_dev(struct iommu_domain *domain, struct device *dev)
  	ret = arm_smmu_domain_add_master(smmu_domain, cfg);
  
  	if (!ret)
-		dev_iommu_domain(dev) = domain;
+		dev_iommu_domain(dev) = iommu_domain;
  	return ret;
  }
  
-static void arm_smmu_detach_dev(struct iommu_domain *domain, struct device *dev)
+static void arm_smmu_detach_dev(struct iommu_domain *iommu_domain, struct device *dev)
  {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_master_cfg *cfg;
  
  	cfg = find_smmu_master_cfg(dev);
@@ -1838,10 +1838,10 @@ out_unlock:
  	return ret;
  }
  
-static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
+static int arm_smmu_map(struct iommu_domain *iommu_domain, unsigned long iova,
  			phys_addr_t paddr, size_t size, int prot)
  {
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  
  	if (!smmu_domain)
  		return -ENODEV;
@@ -1849,25 +1849,25 @@ static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
  	return arm_smmu_handle_mapping(smmu_domain, iova, paddr, size, prot);
  }
  
-static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
+static size_t arm_smmu_unmap(struct iommu_domain *iommu_domain, unsigned long iova,
  			     size_t size)
  {
  	int ret;
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  
  	ret = arm_smmu_handle_mapping(smmu_domain, iova, 0, size, 0);
  	arm_smmu_tlb_inv_context(smmu_domain);
  	return ret ? 0 : size;
  }
  
-static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *domain,
+static phys_addr_t arm_smmu_iova_to_phys(struct iommu_domain *iommu_domain,
  					 dma_addr_t iova)
  {
  	pgd_t *pgdp, pgd;
  	pud_t pud;
  	pmd_t pmd;
  	pte_t pte;
-	struct arm_smmu_domain *smmu_domain = domain->priv;
+	struct arm_smmu_domain *smmu_domain = iommu_domain->priv;
  	struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
  
  	pgdp = cfg->pgd;
@@ -2567,7 +2567,7 @@ static void arm_smmu_iotlb_flush(struct domain *d, unsigned long gfn,
  static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
  			       struct device *dev)
  {
-	struct iommu_domain *domain;
+	struct iommu_domain *iommu_domain;
  	struct domain_iommu_info *domain_iommu_info;
  	int ret;
  
@@ -2590,55 +2590,55 @@ static int arm_smmu_assign_dev(struct domain *d, u8 devfn,
  	 * under the same SMMU as another device assigned to this domain.
  	 * Would it useful for PCI
  	 */
-	domain = xzalloc(struct iommu_domain);
-	if (!domain)
+	iommu_domain = xzalloc(struct iommu_domain);
+	if (!iommu_domain)
  		return -ENOMEM;
  
-	ret = arm_smmu_domain_init(domain);
+	ret = arm_smmu_domain_init(iommu_domain);
  	if (ret)
  		goto err_dom_init;
  
-	domain->priv->cfg.domain = d;
+	iommu_domain->priv->cfg.domain = d;
  
-	ret = arm_smmu_attach_dev(domain, dev);
+	ret = arm_smmu_attach_dev(iommu_domain, dev);
  	if (ret)
  		goto err_attach_dev;
  
  	spin_lock(&domain_iommu_info->lock);
  	/* Chain the new context to the domain */
-	list_add(&domain->list, &domain_iommu_info->contexts);
+	list_add(&iommu_domain->list, &domain_iommu_info->contexts);
  	spin_unlock(&domain_iommu_info->lock);
  
  	return 0;
  
  err_attach_dev:
-	arm_smmu_domain_destroy(domain);
+	arm_smmu_domain_destroy(iommu_domain);
  err_dom_init:
-	xfree(domain);
+	xfree(iommu_domain);
  
  	return ret;
  }
  
  static int arm_smmu_deassign_dev(struct domain *d, struct device *dev)
  {
-	struct iommu_domain *domain = dev_iommu_domain(dev);
+	struct iommu_domain *iommu_domain = dev_iommu_domain(dev);
  	struct domain_iommu_info *domain_iommu_info;
  
  	domain_iommu_info = domain_hvm_iommu(d)->arch.priv;
  
-	if (!domain || domain->priv->cfg.domain != d) {
+	if (!iommu_domain || iommu_domain->priv->cfg.domain != d) {
  		dev_err(dev, " not attached to domain %d\n", d->domain_id);
  		return -ESRCH;
  	}
  
-	arm_smmu_detach_dev(domain, dev);
+	arm_smmu_detach_dev(iommu_domain, dev);
  
  	spin_lock(&domain_iommu_info->lock);
-	list_del(&domain->list);
+	list_del(&iommu_domain->list);
  	spin_unlock(&domain_iommu_info->lock);
  
-	arm_smmu_domain_destroy(domain);
-	xfree(domain);
+	arm_smmu_domain_destroy(iommu_domain);
+	xfree(iommu_domain);
  
  	return 0;
  }
-- 
1.9.1

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

Hi manish,

On 27/03/15 07:24, Manish Jaggi wrote:
> It is good for code readability as there are many structures ending with
> the name domain.
> Also a code like this one is now easy to understand with the rename
> old: dev_iommu_domain(dev) = domain;
> new: dev_iommu_domain(dev) = iommu_domain;
> 
> Also in current code struct smmu_domain pointer variable name is always
> smmu_domain.
> The change is on the same lines

You are modifying the code from Linux just for your own comprehension.
And we are trying to not diverge from a specific Linux commit in order
to easily backport patch.

NAcked-by: Julien Grall <julien.grall@linaro.org>

Regards,

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Jaggi, Manish]

Regards,
Manish Jaggi

________________________________________
From: Julien Grall <julien.grall@linaro.org>
Sent: Friday, March 27, 2015 6:34 PM
To: Jaggi, Manish; Xen Devel; Prasun.kapoor@cavium.com; Kumar, Vijaya; Ian Campbell; Stefano Stabellini
Subject: Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

Hi manish,

On 27/03/15 07:24, Manish Jaggi wrote:
> It is good for code readability as there are many structures ending with
> the name domain.
> Also a code like this one is now easy to understand with the rename
> old: dev_iommu_domain(dev) = domain;
> new: dev_iommu_domain(dev) = iommu_domain;
[manish] Did u see this line
>
> Also in current code struct smmu_domain pointer variable name is always
> smmu_domain.
> The change is on the same lines

You are modifying the code from Linux just for your own comprehension.
And we are trying to not diverge from a specific Linux commit in order
to easily backport patch.

[manish] please rethink on nack. There are so many data structures ending in _domain we need to provide proper naming.

NAcked-by: Julien Grall <julien.grall@linaro.org>

Regards,

--
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

On 27/03/15 13:26, Jaggi, Manish wrote:
> On 27/03/15 07:24, Manish Jaggi wrote:
>> It is good for code readability as there are many structures ending with
>> the name domain.
>> Also a code like this one is now easy to understand with the rename
>> old: dev_iommu_domain(dev) = domain;
>> new: dev_iommu_domain(dev) = iommu_domain;
> [manish] Did u see this line

I don't care about the new vs old stuff. What I care is keeping the code
as close as possible to the Linux code.

> You are modifying the code from Linux just for your own comprehension.
> And we are trying to not diverge from a specific Linux commit in order
> to easily backport patch.
> 
> [manish] please rethink on nack. There are so many data structures ending in _domain we need to provide proper naming.

We have 3 structure finishing data structures ending by _domain but the
all have a different prefix and described when necessary.

Only one was added for our purpose (arm_smmu_xen_domain). If you don't
like the name of the 2 others, please complain on the Linux ML.

The previous SMMU drivers was diverging from the Linux code and was hard
to backport patch. So my nack is not changed on this point.

Rgards,

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Jaggi, Manish]

From: Julien Grall <julien.grall@linaro.org>
Sent: Friday, March 27, 2015 7:11 PM
To: Jaggi, Manish; Xen Devel; Prasun.kapoor@cavium.com; Kumar, Vijaya; Ian Campbell; Stefano Stabellini
Subject: Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

On 27/03/15 13:26, Jaggi, Manish wrote:
> On 27/03/15 07:24, Manish Jaggi wrote:
>> It is good for code readability as there are many structures ending with
>> the name domain.
>> Also a code like this one is now easy to understand with the rename
>> old: dev_iommu_domain(dev) = domain;
>> new: dev_iommu_domain(dev) = iommu_domain;
> [manish] Did u see this line

I don't care about the new vs old stuff. What I care is keeping the code
as close as possible to the Linux code.

[manish] then you are missing a point my friend. The line mentioned old is not at all intuitive and is confusing. 
 I am not proposing a design change. All I am asking to add a prefix to iommu_domain pointers as was already done for smmu_domain pointers.

> You are modifying the code from Linux just for your own comprehension.
> And we are trying to not diverge from a specific Linux commit in order
> to easily backport patch.
>
> [manish] please rethink on nack. There are so many data structures ending in _domain we need to provide proper naming.

We have 3 structure finishing data structures ending by _domain but the
all have a different prefix and described when necessary.

[manish] But it is hard to follow sometimes and maintaining and debugging the code becomes a overhead 

Only one was added for our purpose (arm_smmu_xen_domain). If you don't
like the name of the 2 others, please complain on the Linux ML.

The previous SMMU drivers was diverging from the Linux code and was hard
to backport patch. So my nack is not changed on this point.

Rgards,

--
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

On 27/03/2015 18:05, Jaggi, Manish wrote:
> From: Julien Grall <julien.grall@linaro.org>
> Sent: Friday, March 27, 2015 7:11 PM
> To: Jaggi, Manish; Xen Devel; Prasun.kapoor@cavium.com; Kumar, Vijaya; Ian Campbell; Stefano Stabellini
> Subject: Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain
>
> On 27/03/15 13:26, Jaggi, Manish wrote:
>> On 27/03/15 07:24, Manish Jaggi wrote:
>>> It is good for code readability as there are many structures ending with
>>> the name domain.
>>> Also a code like this one is now easy to understand with the rename
>>> old: dev_iommu_domain(dev) = domain;
>>> new: dev_iommu_domain(dev) = iommu_domain;
>> [manish] Did u see this line
>
> I don't care about the new vs old stuff. What I care is keeping the code
> as close as possible to the Linux code.
>
> [manish] then you are missing a point my friend. The line mentioned old is not at all intuitive and is confusing.

Please avoid familiarity... I'm not my friend and a such things will do 
the inverse effect you are trying to reach.

>   I am not proposing a design change. All I am asking to add a prefix to iommu_domain pointers as was already done for smmu_domain pointers.

I didn't talk about design change but code modification. If you change 
the name of variables, it will require more work to backport a patch 
from Linux because the patch won't apply cleanly.

I've spent quite a lots of time to change as little as possible the 
Linux code and justify any change. You can see the different mail I had 
with Ian & Stefano for this purpose.

IHMO, changing the code just for your own comprehension is not a valid 
justification.

Regards,

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Stefano Stabellini]

On Fri, 27 Mar 2015, Jaggi, Manish wrote:
> From: Julien Grall <julien.grall@linaro.org>
> Sent: Friday, March 27, 2015 6:34 PM
> To: Jaggi, Manish; Xen Devel; Prasun.kapoor@cavium.com; Kumar, Vijaya; Ian Campbell; Stefano Stabellini
> Subject: Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain
> 
> Hi manish,
> 
> On 27/03/15 07:24, Manish Jaggi wrote:
> > It is good for code readability as there are many structures ending with
> > the name domain.
> > Also a code like this one is now easy to understand with the rename
> > old: dev_iommu_domain(dev) = domain;
> > new: dev_iommu_domain(dev) = iommu_domain;
> [manish] Did u see this line
> >
> > Also in current code struct smmu_domain pointer variable name is always
> > smmu_domain.
> > The change is on the same lines
> 
> You are modifying the code from Linux just for your own comprehension.
> And we are trying to not diverge from a specific Linux commit in order
> to easily backport patch.
> 
> [manish] please rethink on nack. There are so many data structures ending in _domain we need to provide proper naming.

We are trying to stay as close as possible to the Linux SMMU driver so
that we can easily import future updates of the driver into Xen. Although
it is true that the current naming is not great, changing the naming
scheme would make future driver updates much harder.

If it helps we could add a couple of comments on top of the structs in
smmu.c to explain the meaning of the fields, like:


/* iommu_domain, not to be confused with a Xen domain */

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Ian Campbell]

On Tue, 2015-03-31 at 17:48 +0100, Stefano Stabellini wrote:
> If it helps we could add a couple of comments on top of the structs in
> smmu.c to explain the meaning of the fields, like:
> 
> 
> /* iommu_domain, not to be confused with a Xen domain */

I was going to suggest something similar but more expansive, i.e. a
table of them all in one place (i.e. at the top of the file) for ease of
referencing:

Struct Name            What             Wherefrom     Normally found in
---------------------------------------------------------------------
iommu_domain           IOMMU Context    Linux         d->arch.blah
arch_smmu_xen_device   Device specific  Xen           device->arch.blurg 

Ian.

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

Hi Ian,

On 01/04/2015 10:30, Ian Campbell wrote:
> On Tue, 2015-03-31 at 17:48 +0100, Stefano Stabellini wrote:
>> If it helps we could add a couple of comments on top of the structs in
>> smmu.c to explain the meaning of the fields, like:
>>
>>
>> /* iommu_domain, not to be confused with a Xen domain */
>
> I was going to suggest something similar but more expansive, i.e. a
> table of them all in one place (i.e. at the top of the file) for ease of
> referencing:
>
> Struct Name            What             Wherefrom     Normally found in
> ---------------------------------------------------------------------
> iommu_domain           IOMMU Context    Linux         d->arch.blah
> arch_smmu_xen_device   Device specific  Xen           device->arch.blurg

The actual name of the structure is arm_smmu_xen_device not 
arch_smmu_xen_device. Did you suggest to rename the name?

Regards,

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Ian Campbell]

On Mon, 2015-04-06 at 16:15 +0200, Julien Grall wrote:
> Hi Ian,
> 
> On 01/04/2015 10:30, Ian Campbell wrote:
> > On Tue, 2015-03-31 at 17:48 +0100, Stefano Stabellini wrote:
> >> If it helps we could add a couple of comments on top of the structs in
> >> smmu.c to explain the meaning of the fields, like:
> >>
> >>
> >> /* iommu_domain, not to be confused with a Xen domain */
> >
> > I was going to suggest something similar but more expansive, i.e. a
> > table of them all in one place (i.e. at the top of the file) for ease of
> > referencing:
> >
> > Struct Name            What             Wherefrom     Normally found in
> > ---------------------------------------------------------------------
> > iommu_domain           IOMMU Context    Linux         d->arch.blah
> > arch_smmu_xen_device   Device specific  Xen           device->arch.blurg
> 
> The actual name of the structure is arm_smmu_xen_device not 
> arch_smmu_xen_device. Did you suggest to rename the name?

No, I was just suggesting someone should create such a table with actual
current information, instead of made up filler, in it. (you'll notice
that there is, hopefully, no field blah in d->arch either nor
device->arch.blurg in the tree either and please don't rename the field
to match those ;-)).


Ian.

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

Hi Ian,

On 14/04/15 12:15, Ian Campbell wrote:
> On Mon, 2015-04-06 at 16:15 +0200, Julien Grall wrote:
>> Hi Ian,
>>
>> On 01/04/2015 10:30, Ian Campbell wrote:
>>> On Tue, 2015-03-31 at 17:48 +0100, Stefano Stabellini wrote:
>>>> If it helps we could add a couple of comments on top of the structs in
>>>> smmu.c to explain the meaning of the fields, like:
>>>>
>>>>
>>>> /* iommu_domain, not to be confused with a Xen domain */
>>>
>>> I was going to suggest something similar but more expansive, i.e. a
>>> table of them all in one place (i.e. at the top of the file) for ease of
>>> referencing:
>>>
>>> Struct Name            What             Wherefrom     Normally found in
>>> ---------------------------------------------------------------------
>>> iommu_domain           IOMMU Context    Linux         d->arch.blah
>>> arch_smmu_xen_device   Device specific  Xen           device->arch.blurg
>>
>> The actual name of the structure is arm_smmu_xen_device not 
>> arch_smmu_xen_device. Did you suggest to rename the name?
> 
> No, I was just suggesting someone should create such a table with actual
> current information, instead of made up filler, in it. (you'll notice
> that there is, hopefully, no field blah in d->arch either nor
> device->arch.blurg in the tree either and please don't rename the field
> to match those ;-)).

Thanks for the clarification. I wanted a confirmation because on another
thread [1], Manish said you were suggested a new name.

I think a table describing the different structure would be nice.

Regards,

[1]
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00473.html

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Jaggi, Manish]

Hi Julien/Ian,
________________________________________
From: Julien Grall <julien.grall.oss@gmail.com>
Sent: Tuesday, April 14, 2015 5:16 PM
To: Ian Campbell; Julien Grall
Cc: Prasun.kapoor@cavium.com; Stefano Stabellini; Jaggi, Manish; Julien Grall; Xen Devel; Kumar, Vijaya
Subject: Re: [Xen-devel] [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

Hi Ian,

On 14/04/15 12:15, Ian Campbell wrote:
> On Mon, 2015-04-06 at 16:15 +0200, Julien Grall wrote:
>> Hi Ian,
>>
>> On 01/04/2015 10:30, Ian Campbell wrote:
>>> On Tue, 2015-03-31 at 17:48 +0100, Stefano Stabellini wrote:
>>>> If it helps we could add a couple of comments on top of the structs in
>>>> smmu.c to explain the meaning of the fields, like:
>>>>
>>>>
>>>> /* iommu_domain, not to be confused with a Xen domain */
>>>
>>> I was going to suggest something similar but more expansive, i.e. a
>>> table of them all in one place (i.e. at the top of the file) for ease of
>>> referencing:
>>>
>>> Struct Name            What             Wherefrom     Normally found in
>>> ---------------------------------------------------------------------
>>> iommu_domain           IOMMU Context    Linux         d->arch.blah
>>> arch_smmu_xen_device   Device specific  Xen           device->arch.blurg
>>
>> The actual name of the structure is arm_smmu_xen_device not
>> arch_smmu_xen_device. Did you suggest to rename the name?
>
> No, I was just suggesting someone should create such a table with actual
> current information, instead of made up filler, in it. (you'll notice
> that there is, hopefully, no field blah in d->arch either nor
> device->arch.blurg in the tree either and please don't rename the field
> to match those ;-)).

Thanks for the clarification. I wanted a confirmation because on another
thread [1], Manish said you were suggested a new name.

I think a table describing the different structure would be nice.
[manish] Table is indeed a good option, but I think changing the name of arm_smmu_xen_device to something like arch_smmu_device make more sense as arm_smmu_xen_device and arm_smmu_device are not differentiable just by name. 

Regards,

[1]
http://lists.xenproject.org/archives/html/xen-devel/2015-04/msg00473.html

--
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Julien Grall]

On 14/04/15 13:24, Jaggi, Manish wrote:
> [manish] Table is indeed a good option, but I think changing the name
> of arm_smmu_xen_device to something like arch_smmu_device make more
> sense as arm_smmu_xen_device and arm_smmu_device are not differentiable
> just by name.

Can you explain how arch_smmu_device and arm_smmu_device are easier to
differentiate? There is only 2 letters difference...

Also, what does "arch" stands for? The SMMU driver is already ARM
specific...

Regards,

-- 
Julien Grall

Re: [PATCH v1 3/3] xen/arm: smmu: Renaming struct iommu_domain *domain to, struct iommu_domain *iommu_domain

[Ian Campbell]

On Tue, 2015-04-14 at 12:24 +0000, Jaggi, Manish wrote:
> Hi Julien/Ian,

Please will you configure your mail client to do the chevron (">") style
of quotation favour in open source communities, picking out your replies
from the mail you are replying to is a massive chore, especially after a
couple of rounds of back and forth.

> I think a table describing the different structure would be nice.
> [manish] Table is indeed a good option, but I think changing the name
> of arm_smmu_xen_device to something like arch_smmu_device make more
> sense as arm_smmu_xen_device and arm_smmu_device are not
> differentiable just by name. 

Once we have a table describing the status quo then we can consider
separately whether a rename would be useful or if the documentation in
the table itself suffices.

I intend to use this table as a reference when evaluating any request to
change the names, hence I won't be considering such things until the
table exists.

Ian.