irqdomain: protect macro variable in domain iterators
PATCH 1/1 irqdomain: protect macro variable in domain iterators
---
Error found while using those iterators in an irq controller initialization function.
May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done.
include/linux/irqdomain.h | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
include/linux/irqdomain.h +4 -4
@@ -82,12 +82,12 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \ - for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for (hw = (d)->hwirq_base; hw < (d)->hwirq_base + (d)->nr_irq; hw++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for (hw = (d)->hwirq_base, irq = irq_domain_to_irq((d), hw); \ + hw < (d)->hwirq_base + (d)->nr_irq; \ + hw++, irq = irq_domain_to_irq((d), hw))extern void irq_domain_add (struct irq_domain *domain);extern void irq_domain_del (struct irq_domain *domain);
Signature · 1 line
1.7.5.4
PATCH 1/1 irqdomain: protect macro variable in domain iterators
Quoted 20 lines · Nicolas Ferre
Signed-off-by Nicolas Ferre <[email protected]>---
Error found while using those iterators in an irq controller initialization function.May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done.
include/linux/irqdomain.h | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)include/linux/irqdomain.h +0
-0@@ -82,12 +82,12 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \
include/linux/irqdomain.h static inline unsigned int irq_domain_to_irq(struct irq_domain *d, · line 90
Quoted 10 lines
include/linux/irqdomain.h +4
-4- for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for (hw = (d)->hwirq_base; hw < (d)->hwirq_base + (d)->nr_irq; hw++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for (hw = (d)->hwirq_base, irq = irq_domain_to_irq((d), hw); \ + hw < (d)->hwirq_base + (d)->nr_irq; \ + hw++, irq = irq_domain_to_irq((d), hw))
I suggest just putting all the brackets in -- if having spotted this problem you only half-fix the macros, an opportunity is being missed; someone have to come and fix it again later:
#define irq_domain_for_each_hwirq(d, hw) \ for ((hw) = (d)->hwirq_base; (hw) < (d)->hwirq_base + (d)->nr_irq; (hw)++)
#define irq_domain_for_each_irq(d, hw, irq) \ for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq(d, hw); \ (hw) < (d)->hwirq_base + (d)->nr_irq; \ (hw)++, (irq) = irq_domain_to_irq(d, hw))
If you feel happier though, you can harmlessly add the extra brackets round the arguments to irq_domain_to_irq(), without changing the behaviour. Arguably the "always add brackets" rule is simpler to understand.
In fact, where a macro argument is not part of a larger expression, or is an operand to a comma-expression, there's no need for extra brackets -- all possible operators parse at higher priority than commas. A macro argument which itself is a comma-expression whould have to be explicitly bracketed in the macro invocation anyway, so there is no extra risk of the macro expansion being parsed in an unexpected way in that case.
Cheers
---Dave
PATCH 1/1 irqdomain: protect macro variable in domain iterators
On 12/02/2011 01:59 PM, Dave Martin :
Quoted 24 lines · Nicolas Ferre
Quoted 23 lines
Signed-off-by Nicolas Ferre<[email protected]>---
Error found while using those iterators in an irq controller initialization function.May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done.
include/linux/irqdomain.h | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)include/linux/irqdomain.h +1
-1@@ -82,12 +82,12 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \ - for (hw = d->hwirq_base; hw< d->hwirq_base + d->nr_irq; hw++) + for (hw = (d)->hwirq_base; hw< (d)->hwirq_base + (d)->nr_irq; hw++)
Quoted 11 lines
include/linux/irqdomain.h static inline unsigned int irq_domain_to_irq(struct irq_domain *d, · line 90
Quoted 7 lines
include/linux/irqdomain.h +3
-3#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw< d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for (hw = (d)->hwirq_base, irq = irq_domain_to_irq((d), hw); \ + hw< (d)->hwirq_base + (d)->nr_irq; \ + hw++, irq = irq_domain_to_irq((d), hw))I suggest just putting all the brackets in -- if having spotted this problem you only half-fix the macros, an opportunity is being missed; someone have to come and fix it again later:
Exactly, this is why I added a little comment to the patch:
"May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done."
Quoted 13 lines
#define irq_domain_for_each_hwirq(d, hw) \ for ((hw) = (d)->hwirq_base; (hw)< (d)->hwirq_base + (d)->nr_irq; (hw)++)#define irq_domain_for_each_irq(d, hw, irq) \ for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq(d, hw); \ (hw)< (d)->hwirq_base + (d)->nr_irq; \ (hw)++, (irq) = irq_domain_to_irq(d, hw))If you feel happier though, you can harmlessly add the extra brackets round the arguments to
irq_domain_to_irq(), without changing the behaviour. Arguably the "always add brackets" rule is simpler to understand.
Quoted 6 lines
In fact, where a macro argument is not part of a larger expression, or is an operand to a comma-expression, there's no need for extra brackets -- all possible operators parse at higher priority than commas. A macro argument which itself is a comma-expression whould have to be explicitly bracketed in the macro invocation anyway, so there is no extra risk of the macro expansion being parsed in an unexpected way in that case.
Thanks for the detailed explanation. I resend the patch with the "always add brackets" corrections.
Best regards,
Signature · 1 line
Nicolas Ferre
PATCH 1/1 irqdomain: protect macro variable in domain iterators
In reply to Dave MartinQuoted 37 lines · Dave Martin
Quoted 20 lines · Nicolas Ferre
Signed-off-by Nicolas Ferre <[email protected]>---
Error found while using those iterators in an irq controller initialization function.May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done.
include/linux/irqdomain.h | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)include/linux/irqdomain.h +0
-0@@ -82,12 +82,12 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \include/linux/irqdomain.h static inline unsigned int irq_domain_to_irq(struct irq_domain *d, · line 90
Quoted 10 lines
include/linux/irqdomain.h +4
-4- for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for (hw = (d)->hwirq_base; hw < (d)->hwirq_base + (d)->nr_irq; hw++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for (hw = (d)->hwirq_base, irq = irq_domain_to_irq((d), hw); \ + hw < (d)->hwirq_base + (d)->nr_irq; \ + hw++, irq = irq_domain_to_irq((d), hw))I suggest just putting all the brackets in -- if having spotted this problem you only half-fix the macros, an opportunity is being missed; someone have to come and fix it again later:
Quoted 8 lines
#define irq_domain_for_each_hwirq(d, hw) \ for ((hw) = (d)->hwirq_base; (hw) < (d)->hwirq_base + (d)->nr_irq; (hw)++)#define irq_domain_for_each_irq(d, hw, irq) \ for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq(d, hw); \ (hw) < (d)->hwirq_base + (d)->nr_irq; \ (hw)++, (irq) = irq_domain_to_irq(d, hw))
Parameters on the left side of an '=' can't be a complex expression. Look at other iterator macros.
Rob
Quoted 14 lines
If you feel happier though, you can harmlessly add the extra brackets round the arguments to
irq_domain_to_irq(), without changing the behaviour. Arguably the "always add brackets" rule is simpler to understand.In fact, where a macro argument is not part of a larger expression, or is an operand to a comma-expression, there's no need for extra brackets -- all possible operators parse at higher priority than commas. A macro argument which itself is a comma-expression whould have to be explicitly bracketed in the macro invocation anyway, so there is no extra risk of the macro expansion being parsed in an unexpected way in that case.
Cheers
---Dave
PATCH 1/1 irqdomain: protect macro variable in domain iterators
Quoted 41 lines · Rob Herring
Quoted 40 lines · Dave Martin
Quoted 20 lines · Nicolas Ferre
Signed-off-by Nicolas Ferre <[email protected]>---
Error found while using those iterators in an irq controller initialization function.May also need protection around irq and hwirq macro variables but those values are usually plain "int" anyway... Tell me if you feel that it should be done.
include/linux/irqdomain.h | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)include/linux/irqdomain.h +0
-0@@ -82,12 +82,12 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \include/linux/irqdomain.h static inline unsigned int irq_domain_to_irq(struct irq_domain *d, · line 90
Quoted 10 lines
include/linux/irqdomain.h +4
-4- for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for (hw = (d)->hwirq_base; hw < (d)->hwirq_base + (d)->nr_irq; hw++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for (hw = (d)->hwirq_base, irq = irq_domain_to_irq((d), hw); \ + hw < (d)->hwirq_base + (d)->nr_irq; \ + hw++, irq = irq_domain_to_irq((d), hw))I suggest just putting all the brackets in -- if having spotted this problem you only half-fix the macros, an opportunity is being missed; someone have to come and fix it again later:
#define irq_domain_for_each_hwirq(d, hw) \ for ((hw) = (d)->hwirq_base; (hw) < (d)->hwirq_base + (d)->nr_irq; (hw)++)
Quoted 8 lines
Quoted 5 lines
#define irq_domain_for_each_irq(d, hw, irq) \ for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq(d, hw); \ (hw) < (d)->hwirq_base + (d)->nr_irq; \ (hw)++, (irq) = irq_domain_to_irq(d, hw))Parameters on the left side of an '=' can't be a complex expression. Look at other iterator macros.
Do you mean "can't" or "shouldn't, by policy"? I don't see a statement of the policy, but feel free to point me at it if it exists.
An arbitrarily complex expression can appear on the left size of a C assignment, providing that it is an lvalue of an appropriate type; though if it involves things like casts or ?: we would rapidly get into ill- advised obfuscated code territory.
The most plausible use I can think of it something like:
int do_something(type *result, args)
{
widget w;
/* ... */
widget_for_each_whatever(*result, w) {
/* do stuff */
}
/* ... */
}I don't comment on whether this is a good idea, but from the language point of view it is perfectly reasonable.
(Note that *result = something parses how we want, but *result++, if generated in the macro expansion, will not)
You're right that this won't work with at least some of the existing macros, but my view is if that if a macro can be made trivially correct, without pitfalls, that you should do it. As a general principle, this helps to avoid latent bugs in the code. I don't think we should have a special-case rule because this is a certain special flavour of macro, unless implementing the macro robustly becomes impossible.
Just my opinion, though -- if people want it the other way, then I don't have a serious problem with that.
Cheers
---Dave
PATCH v2 1/1 irqdomain: protect macro variable in domain iterators
In reply to Nicolas Ferre---
v2: add brackets to each macro variable.
include/linux/irqdomain.h | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)
include/linux/irqdomain.h +6 -4
@@ -82,12 +82,14 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \ - for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for ((hw) = (d)->hwirq_base; \ + (hw) < (d)->hwirq_base + (d)->nr_irq; \ + (hw)++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq((d), (hw)); \ + (hw) < (d)->hwirq_base + (d)->nr_irq; \ + (hw)++, (irq) = irq_domain_to_irq((d), (hw)))extern void irq_domain_add (struct irq_domain *domain);extern void irq_domain_del (struct irq_domain *domain);
Signature · 1 line
1.7.5.4
Quoted 1 line · Nicolas Ferre
Signed-off-by Nicolas Ferre <[email protected]>
Looks OK to me:
Quoted 32 lines
---
v2: add brackets to each macro variable.include/linux/irqdomain.h | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)include/linux/irqdomain.h +6
-4@@ -82,12 +82,14 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \ - for (hw = d->hwirq_base; hw < d->hwirq_base + d->nr_irq; hw++) + for ((hw) = (d)->hwirq_base; \ + (hw) < (d)->hwirq_base + (d)->nr_irq; \ + (hw)++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw < d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq((d), (hw)); \ + (hw) < (d)->hwirq_base + (d)->nr_irq; \ + (hw)++, (irq) = irq_domain_to_irq((d), (hw)))extern void irq_domain_add (struct irq_domain *domain);extern void irq_domain_del (struct irq_domain *domain);Signature · 1 line
1.7.5.4
On 12/02/2011 02:44 PM, Dave Martin :
Quoted 6 lines · Nicolas Ferre
Quoted 1 line
Signed-off-by Nicolas Ferre<[email protected]>Looks OK to me:
Acked-by Dave Martin<[email protected]>
Rob,
Do you want a different implementation for this. I think it is the simplest way to write this macro...
If it is ok for you, it will be interesting to include this patch quickly so that I will be able to queue my own patches on top of it. (Cc: stable can be a good idea also).
Best regards,
Quoted 33 lines
Quoted 32 lines
---
v2: add brackets to each macro variable.include/linux/irqdomain.h | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-)include/linux/irqdomain.h +6
-5@@ -82,12 +82,14 @@ static inline unsigned int irq_domain_to_irq(struct irq_domain *d, }#define irq_domain_for_each_hwirq (d, hw) \ - for (hw = d->hwirq_base; hw< d->hwirq_base + d->nr_irq; hw++) + for ((hw) = (d)->hwirq_base; \ + (hw)< (d)->hwirq_base + (d)->nr_irq; \ + (hw)++)#define irq_domain_for_each_irq (d, hw, irq) \ - for (hw = d->hwirq_base, irq = irq_domain_to_irq(d, hw); \ - hw< d->hwirq_base + d->nr_irq; \ - hw++, irq = irq_domain_to_irq(d, hw)) + for ((hw) = (d)->hwirq_base, (irq) = irq_domain_to_irq((d), (hw)); \ + (hw)< (d)->hwirq_base + (d)->nr_irq; \ + (hw)++, (irq) = irq_domain_to_irq((d), (hw)))extern void irq_domain_add (struct irq_domain *domain);extern void irq_domain_del (struct irq_domain *domain); --1.7.5.4
Signature · 1 line
Nicolas Ferre
Quoted 8 lines · Nicolas Ferre
On 12/02/2011 02:44 PM, Dave Martin :
Quoted 6 lines · Nicolas Ferre
Quoted 1 line
Signed-off-by Nicolas Ferre<[email protected]>Looks OK to me:
Acked-by Dave Martin<[email protected]>
Quoted 9 lines
Rob,
Do you want a different implementation for this. I think it is the simplest way to write this macro...
If it is ok for you, it will be interesting to include this patch quickly so that I will be able to queue my own patches on top of it. (Cc: stable can be a good idea also).
Yes, it's fine. The irqdomain patches need to be taken by tglx or acked by him. I don't think this is needed for stable. The existing users are fine and new ones aren't going to be added to stable kernels.
Rob