irqdomain: factorise irq_domain_xlate_onetwocell()

PATCH Superseded Sebastian Frias 2 participants 5 messages Aug 1, 2016 – Aug 2, 2016

Subsystems (2)

IRQ IRQ domains (IRQ number mapping library)

lore

Share & tools

lore

Sebastian Frias

author

2016-08-01 14:27 UTC #0 lore raw

PATCH 1/1 irqdomain: factorise irq_domain_xlate_onetwocell()

Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device drivers can use") introduced three similar functions:

irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the two previous ones to avoid code duplication.

Fixes 16b2e6e2f31d ("irq_domain: Create common xlate functions that device

drivers can use")

Signed-off-by Sebastian Frias <[email protected]>

---

NOTE: the factored code is not strictly the same in the sense that 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

kernel/irq/irqdomain.c +14 -3

index bee8b02..ea2df0e 100644
@@ -839,9 +839,20 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)
+		return irq_domain_xlate_onecell(d,
+						ctrlr,
+						intspec,
+						intsize,
+						out_hwirq,
+						out_type);
+	else
+		return irq_domain_xlate_twocell(d,
+						ctrlr,
+						intspec,
+						intsize,
+						out_hwirq,
+						out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);

Signature · 1 line

1.7.11.2

Sebastian Frias

author

2016-08-01 14:58 UTC #1 lore raw

PATCH v2 1/1 irqdomain: factorise irq_domain_xlate_onetwocell()

Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device drivers can use") introduced three similar functions:

irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the two previous ones to avoid code duplication.

Fixes 16b2e6e2f31d ("irq_domain: Create common xlate functions that device

drivers can use")

Signed-off-by Sebastian Frias <[email protected]>

---

NOTE: the factored code is not strictly the same in the sense that 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

kernel/irq/irqdomain.c +6 -3

index bee8b02..125a28c 100644
@@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)
+		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
+	else
+		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);

Signature · 1 line

1.7.11.2

Thomas Gleixner

2016-08-01 17:10 UTC #2 lore raw

Quoted 7 lines · Sebastian Frias

Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device drivers can use") introduced three similar functions:
irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

Quoted 5 lines

yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the two previous ones to avoid code duplication.
Fixes 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use")

That does not fix anything. It optimizes code. We use the "Fixes" tag only when the existing code is buggy.

Quoted 6 lines

Signed-off-by Sebastian Frias <[email protected]>
---
NOTE: the factored code is not strictly the same in the sense that 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

So the proper way to do that is to split this into two patches:

#1 Change the existing code to do the masking and explain why it is correct.

#2 Refactor the code and get rid of the duplicated implementation.

Quoted 13 lines

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

kernel/irq/irqdomain.c +0 -0

index bee8b02..125a28c 100644
@@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))

kernel/irq/irqdomain.c int irq_domain_xlate_onetwocell(struct irq_domain *d, · line 847

Quoted 10 lines

kernel/irq/irqdomain.c +6 -3

 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)
+		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
+	else
+		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);

So I really wonder how much of a saving that change is. I wouldn't be surprised if it would create worse code on some architectures.

Thanks,

tglx

Sebastian Frias

author

2016-08-02 08:32 UTC #3 lore raw

Hi Thomas,

Quoted 8 lines · Thomas Gleixner

Quoted 7 lines · Sebastian Frias
Commit 16b2e6e2f31d ("irq_domain: Create common xlate functions that device drivers can use") introduced three similar functions:
irq_domain_xlate_onecell()
irq_domain_xlate_twocell()
irq_domain_xlate_onetwocell()

Quoted 8 lines

Quoted 5 lines
yet the last one, irq_domain_xlate_onetwocell(), can be factored to use the two previous ones to avoid code duplication.
Fixes 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use")
That does not fix anything. It optimizes code. We use the "Fixes" tag only when the existing code is buggy.

Ok, I will remove that.

Quoted 8 lines

Quoted 7 lines
Signed-off-by Sebastian Frias <[email protected]>
---
NOTE: the factored code is not strictly the same in the sense that 16b2e6e2f31d returns "intspec[1]" as 'out_type', while this patch would make it return "intspec[1] & IRQ_TYPE_SENSE_MASK".

Quoted 5 lines

So the proper way to do that is to split this into two patches:
#1 Change the existing code to do the masking and explain why it is correct.
#2 Refactor the code and get rid of the duplicated implementation.

Ok, I can do two patches.

Quoted 20 lines

Feel free to comment on that matter.

---
 kernel/irq/irqdomain.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

kernel/irq/irqdomain.c +1 -3

index bee8b02..125a28c 100644
@@ -839,9 +839,12 @@ int irq_domain_xlate_onetwocell(struct irq_domain *d,
 {
 	if (WARN_ON(intsize < 1))
 		return -EINVAL;
-	*out_hwirq = intspec[0];
-	*out_type = (intsize > 1) ? intspec[1] : IRQ_TYPE_NONE;
-	return 0;
+	if (intsize == 1)

Quoted 9 lines

kernel/irq/irqdomain.c int irq_domain_xlate_onetwocell(struct irq_domain *d, · line 847
Quoted 5 lines
kernel/irq/irqdomain.c +5 -0
+		return irq_domain_xlate_onecell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
+	else
+		return irq_domain_xlate_twocell(d, ctrlr, intspec, intsize,
+						out_hwirq, out_type);
So I really wonder how much of a saving that change is. I wouldn't be surprised if it would create worse code on some architectures.

Maybe it does, although I looked at this from the point of view of reducing duplicated code because of the well known issues duplicated code entails. This case is a good example, since the code was duplicated we ended up with slightly different versions of it.

Best regards,

Sebastian

Sebastian Frias

author

2016-08-02 09:30 UTC #4 lore raw

Hi Thomas,

Quoted 7 lines · Sebastian Frias

Quoted 5 lines
So the proper way to do that is to split this into two patches:
#1 Change the existing code to do the masking and explain why it is correct.
#2 Refactor the code and get rid of the duplicated implementation.
Ok, I can do two patches.

I splitted it in two patches, one to fix it, and another one to refactor. However, I sent the first one (the one for the fix) as a separate one, instead of as part of a set of two patches.

I'm resending both as a set of two patches (since the second one requires the first one), sorry for the inconvenience.

Best regards,

Sebastian