[v4,1/6] irqdomain: rename variables in irq_domain_{push,pop}_irq()

Message ID 1504784522-26841-2-git-send-email-yamada.masahiro@socionext.com
State New
Headers show
Series
  • irqdomain, gpio: expand irq_domain_push_irq() for DT use and use it for GPIO
Related show

Commit Message

Masahiro Yamada Sept. 7, 2017, 11:41 a.m.
The meaning of "root" in irq_domain_{push,pop} is opposite to the
documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
IRQ domain as follows:

    CPU Vector irq_domain (root irq_domain to manage CPU vectors)
            ^
            |
    Interrupt Remapping irq_domain (manage irq_remapping entries)
            ^
            |
    IOAPIC irq_domain (manage IOAPIC delivery entries/pins)

From above, the inner-most domain (nearest to the CPU) is "root".

The document also says, "When building irq_domain hierarchy, the
irq_domain near to the device is child and the irq_domain near to
CPU is parent."  This is how irq_data->parent_data works.  In
contrast, these function use a variable "child_irq_data" for that.

To avoid confusion, flip the direction as follows:

  root_irq_data  -> tail_irq_data
  child_irq_data -> parent_irq_data

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

Changes in v4:
 - Newly added


 kernel/irq/irqdomain.c | 68 +++++++++++++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 34 deletions(-)

Comments

Marc Zyngier Sept. 7, 2017, 12:47 p.m. | #1
On 07/09/17 12:41, Masahiro Yamada wrote:
> The meaning of "root" in irq_domain_{push,pop} is opposite to the
> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
> IRQ domain as follows:
> 
>     CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>             ^
>             |
>     Interrupt Remapping irq_domain (manage irq_remapping entries)
>             ^
>             |
>     IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
> 
> From above, the inner-most domain (nearest to the CPU) is "root".
> 
> The document also says, "When building irq_domain hierarchy, the
> irq_domain near to the device is child and the irq_domain near to
> CPU is parent."  This is how irq_data->parent_data works.  In
> contrast, these function use a variable "child_irq_data" for that.
The exact opposite argument could be used for the data structure. The
irq_desc is the root of the list ordered with parent_data.

Yes, this is confusing, but because we're using the same English words
to describe two different things, we're bound to make one thing more
difficult. I'm unconvinced that this change helps anything (it certainly
confuses me more than anything else).

Thanks,

	M.
David Daney Sept. 7, 2017, 5:45 p.m. | #2
On 09/07/2017 05:47 AM, Marc Zyngier wrote:
> On 07/09/17 12:41, Masahiro Yamada wrote:
>> The meaning of "root" in irq_domain_{push,pop} is opposite to the
>> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
>> IRQ domain as follows:
>>
>>      CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>>              ^
>>              |
>>      Interrupt Remapping irq_domain (manage irq_remapping entries)
>>              ^
>>              |
>>      IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
>>
>>  From above, the inner-most domain (nearest to the CPU) is "root".
>>
>> The document also says, "When building irq_domain hierarchy, the
>> irq_domain near to the device is child and the irq_domain near to
>> CPU is parent."  This is how irq_data->parent_data works.  In
>> contrast, these function use a variable "child_irq_data" for that.
> The exact opposite argument could be used for the data structure. The
> irq_desc is the root of the list ordered with parent_data.
> 
> Yes, this is confusing, but because we're using the same English words
> to describe two different things, we're bound to make one thing more
> difficult. I'm unconvinced that this change helps anything (it certainly
> confuses me more than anything else).
> 

There may be room for improvement here.

Here is my recollection of how I choose the names:

"root" is the thing embedded in the struct irq_desc, if you think about 
a typical linked list structure like this, we can refer to the starting 
point as the "root".  Sometimes it might be referred to as the "head" of 
the list, but usually not the "tail"

"child" was used to indicate the thing we get to by traversing the link 
in the list.  The fact that ->parent is the name of the next pointer and 
that it points to something called "child" is confusing here.

So what do I think should be done?  This:

Either
   A) s/child_irq_data/parent_irq_data/g  As this patch does, but leave 
the root_irq_data name unchanged.

   B) Change the name of the ->parent in struct irq_data to ->next

But that is just my $0.02

I fear we risk a Bike Shedding type of discussion here.


David Daney

> Thanks,
> 
> 	M.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Masahiro Yamada Sept. 8, 2017, 3:05 p.m. | #3
Hi Marc, David,


2017-09-08 2:45 GMT+09:00 David Daney <ddaney@caviumnetworks.com>:
> On 09/07/2017 05:47 AM, Marc Zyngier wrote:
>>
>> On 07/09/17 12:41, Masahiro Yamada wrote:
>>>
>>> The meaning of "root" in irq_domain_{push,pop} is opposite to the
>>> documentation.  Documentation/IRQ-domain.txt depicts the hierarchy
>>> IRQ domain as follows:
>>>
>>>      CPU Vector irq_domain (root irq_domain to manage CPU vectors)
>>>              ^
>>>              |
>>>      Interrupt Remapping irq_domain (manage irq_remapping entries)
>>>              ^
>>>              |
>>>      IOAPIC irq_domain (manage IOAPIC delivery entries/pins)
>>>
>>>  From above, the inner-most domain (nearest to the CPU) is "root".
>>>
>>> The document also says, "When building irq_domain hierarchy, the
>>> irq_domain near to the device is child and the irq_domain near to
>>> CPU is parent."  This is how irq_data->parent_data works.  In
>>> contrast, these function use a variable "child_irq_data" for that.
>>
>> The exact opposite argument could be used for the data structure. The
>> irq_desc is the root of the list ordered with parent_data.
>>
>> Yes, this is confusing, but because we're using the same English words
>> to describe two different things, we're bound to make one thing more
>> difficult. I'm unconvinced that this change helps anything (it certainly
>> confuses me more than anything else).
>>
>
> There may be room for improvement here.
>
> Here is my recollection of how I choose the names:
>
> "root" is the thing embedded in the struct irq_desc, if you think about a
> typical linked list structure like this, we can refer to the starting point
> as the "root".  Sometimes it might be referred to as the "head" of the list,
> but usually not the "tail"
>
> "child" was used to indicate the thing we get to by traversing the link in
> the list.  The fact that ->parent is the name of the next pointer and that
> it points to something called "child" is confusing here.
> So what do I think should be done?  This:
>
> Either
>   A) s/child_irq_data/parent_irq_data/g  As this patch does, but leave the
> root_irq_data name unchanged.


Sounds better than the current situation.




>   B) Change the name of the ->parent in struct irq_data to ->next


This is a bad idea.

irq_data->parent_data corresponds to irq_domain->parent.
We should not lose consistency/symmetry.

irq_domain->parent originates in the "parent" argument
passed to irq_domain_create_hierarchy().
If we change this, it will introduce horrible confusion.

As the document says, when we talk about the hierarchy,
"the irq_domain near to the device is
child and the irq_domain near to CPU is parent"
This is the original concept, and should not be changed.


We can excuse that all the variables in these two helpers
were named from the point of linked-list view,
not talking about the hierarchy.


However, what I thought more confusing was the comment block.

/**
 * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.


This comment is talking about the "hierarchy" at first glance.
So, what is my mind is the picture in Documentation/IRQ-domain.txt

But, from the term "top", this is talking about the linked list here too.
The linked list is just implementation detail...




> But that is just my $0.02
>
> I fear we risk a Bike Shedding type of discussion here.

Patch

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index e84b705..da3e0b6 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -1467,20 +1467,20 @@  static void irq_domain_fix_revmap(struct irq_data *d)
 }
 
 /**
- * irq_domain_push_irq() - Push a domain in to the top of a hierarchy.
+ * irq_domain_push_irq() - Push a domain in to the tail of a hierarchy.
  * @domain:	Domain to push.
  * @virq:	Irq to push the domain in to.
  * @arg:	Passed to the irq_domain_ops alloc() function.
  *
  * For an already existing irqdomain hierarchy, as might be obtained
  * via a call to pci_enable_msix(), add an additional domain to the
- * head of the processing chain.  Must be called before request_irq()
+ * tail of the processing chain.  Must be called before request_irq()
  * has been called.
  */
 int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 {
-	struct irq_data *child_irq_data;
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
+	struct irq_data *tail_irq_data = irq_get_irq_data(virq);
 	struct irq_desc *desc;
 	int rv = 0;
 
@@ -1505,43 +1505,43 @@  int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 	if (WARN_ON(!irq_domain_is_hierarchy(domain)))
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!tail_irq_data)
 		return -EINVAL;
 
-	if (domain->parent != root_irq_data->domain)
+	if (domain->parent != tail_irq_data->domain)
 		return -EINVAL;
 
-	child_irq_data = kzalloc_node(sizeof(*child_irq_data), GFP_KERNEL,
-				      irq_data_get_node(root_irq_data));
-	if (!child_irq_data)
+	parent_irq_data = kzalloc_node(sizeof(*parent_irq_data), GFP_KERNEL,
+				      irq_data_get_node(tail_irq_data));
+	if (!parent_irq_data)
 		return -ENOMEM;
 
 	mutex_lock(&irq_domain_mutex);
 
 	/* Copy the original irq_data. */
-	*child_irq_data = *root_irq_data;
+	*parent_irq_data = *tail_irq_data;
 
 	/*
-	 * Overwrite the root_irq_data, which is embedded in struct
+	 * Overwrite the tail_irq_data, which is embedded in struct
 	 * irq_desc, with values for this domain.
 	 */
-	root_irq_data->parent_data = child_irq_data;
-	root_irq_data->domain = domain;
-	root_irq_data->mask = 0;
-	root_irq_data->hwirq = 0;
-	root_irq_data->chip = NULL;
-	root_irq_data->chip_data = NULL;
+	tail_irq_data->parent_data = parent_irq_data;
+	tail_irq_data->domain = domain;
+	tail_irq_data->mask = 0;
+	tail_irq_data->hwirq = 0;
+	tail_irq_data->chip = NULL;
+	tail_irq_data->chip_data = NULL;
 
 	/* May (probably does) set hwirq, chip, etc. */
 	rv = irq_domain_alloc_irqs_hierarchy(domain, virq, 1, arg);
 	if (rv) {
 		/* Restore the original irq_data. */
-		*root_irq_data = *child_irq_data;
+		*tail_irq_data = *parent_irq_data;
 		goto error;
 	}
 
-	irq_domain_fix_revmap(child_irq_data);
-	irq_domain_set_mapping(domain, root_irq_data->hwirq, root_irq_data);
+	irq_domain_fix_revmap(parent_irq_data);
+	irq_domain_set_mapping(domain, tail_irq_data->hwirq, tail_irq_data);
 
 error:
 	mutex_unlock(&irq_domain_mutex);
@@ -1551,7 +1551,7 @@  int irq_domain_push_irq(struct irq_domain *domain, int virq, void *arg)
 EXPORT_SYMBOL_GPL(irq_domain_push_irq);
 
 /**
- * irq_domain_pop_irq() - Remove a domain from the top of a hierarchy.
+ * irq_domain_pop_irq() - Remove a domain from the tail of a hierarchy.
  * @domain:	Domain to remove.
  * @virq:	Irq to remove the domain from.
  *
@@ -1560,8 +1560,8 @@  EXPORT_SYMBOL_GPL(irq_domain_push_irq);
  */
 int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 {
-	struct irq_data *root_irq_data = irq_get_irq_data(virq);
-	struct irq_data *child_irq_data;
+	struct irq_data *tail_irq_data = irq_get_irq_data(virq);
+	struct irq_data *parent_irq_data;
 	struct irq_data *tmp_irq_data;
 	struct irq_desc *desc;
 
@@ -1583,37 +1583,37 @@  int irq_domain_pop_irq(struct irq_domain *domain, int virq)
 	if (domain == NULL)
 		return -EINVAL;
 
-	if (!root_irq_data)
+	if (!tail_irq_data)
 		return -EINVAL;
 
 	tmp_irq_data = irq_domain_get_irq_data(domain, virq);
 
-	/* We can only "pop" if this domain is at the top of the list */
-	if (WARN_ON(root_irq_data != tmp_irq_data))
+	/* We can only "pop" if this domain is at the tail of the list */
+	if (WARN_ON(tail_irq_data != tmp_irq_data))
 		return -EINVAL;
 
-	if (WARN_ON(root_irq_data->domain != domain))
+	if (WARN_ON(tail_irq_data->domain != domain))
 		return -EINVAL;
 
-	child_irq_data = root_irq_data->parent_data;
-	if (WARN_ON(!child_irq_data))
+	parent_irq_data = tail_irq_data->parent_data;
+	if (WARN_ON(!parent_irq_data))
 		return -EINVAL;
 
 	mutex_lock(&irq_domain_mutex);
 
-	root_irq_data->parent_data = NULL;
+	tail_irq_data->parent_data = NULL;
 
-	irq_domain_clear_mapping(domain, root_irq_data->hwirq);
+	irq_domain_clear_mapping(domain, tail_irq_data->hwirq);
 	irq_domain_free_irqs_hierarchy(domain, virq, 1);
 
 	/* Restore the original irq_data. */
-	*root_irq_data = *child_irq_data;
+	*tail_irq_data = *parent_irq_data;
 
-	irq_domain_fix_revmap(root_irq_data);
+	irq_domain_fix_revmap(tail_irq_data);
 
 	mutex_unlock(&irq_domain_mutex);
 
-	kfree(child_irq_data);
+	kfree(parent_irq_data);
 
 	return 0;
 }