Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453686 > unrolled thread

[PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell()

Back to article view | Back to linux.kernel

Contents

  [PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell() Sebastian Frias <sf84@laposte.net> - 2016-08-02 11:20 +0200
    Re: [PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell() Thomas Gleixner <tglx@linutronix.de> - 2016-08-04 12:40 +0200
      Re: [PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell() Sebastian Frias <sf84@laposte.net> - 2016-08-04 12:40 +0200
        Re: [PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell() Thomas Gleixner <tglx@linutronix.de> - 2016-08-04 16:40 +0200

#1453686 — [PATCH 1/2] irqdomain: fix irq_domain_xlate_onetwocell()

Use IRQ_TYPE_SENSE_MASK on irq_domain_xlate_onetwocell()

According to the xlate() callback definition, the 'out_type' parameter
needs to be the "linux irq type". A mask for such bits exists,
IRQ_TYPE_SENSE_MASK, and as a matter of fact it is used in
irq_domain_xlate_twocell(), use it for irq_domain_xlate_onetwocell() as
well.

Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
drivers can use")

Signed-off-by: Sebastian Frias <sf84@laposte.net>
---
 kernel/irq/irqdomain.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/kernel/irq/irqdomain.c b/kernel/irq/irqdomain.c
index 8798b6c..1bdd3fe 100644
--- a/kernel/irq/irqdomain.c
+++ b/kernel/irq/irqdomain.c
@@ -824,7 +824,10 @@ 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;
+	if (intsize > 1)
+		*out_type = intspec[1] & IRQ_TYPE_SENSE_MASK;
+	else
+		*out_type = IRQ_TYPE_NONE;
 	return 0;
 }
 EXPORT_SYMBOL_GPL(irq_domain_xlate_onetwocell);
-- 
1.7.11.2

[toc] | [next] | [standalone]

#1456319

On Tue, 2 Aug 2016, Sebastian Frias wrote:

> Use IRQ_TYPE_SENSE_MASK on irq_domain_xlate_onetwocell()
> 
> According to the xlate() callback definition, the 'out_type' parameter
> needs to be the "linux irq type". A mask for such bits exists,
> IRQ_TYPE_SENSE_MASK, and as a matter of fact it is used in
> irq_domain_xlate_twocell(), use it for irq_domain_xlate_onetwocell() as
> well.
> 
> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
> drivers can use")

You seem to be obsessed by adding "Fixes" tags and 'Fix' to the $subject, but
your changelog just babbles about making the code the same as in
irq_domain_xlate_twocell() and avoids carefully to explain what is broken,
what consequences that has and what the fix is.

Now if you look at the call site which uses the xlate functions then you'll
notice that we have a sanity check there already:

        if (irq_domain_translate(domain, fwspec, &hwirq, &type))
                return 0;
        /*
         * WARN if the irqchip returns a type with bits
         * outside the sense mask set and clear these bits.
         */
        if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
                type &= IRQ_TYPE_SENSE_MASK;

So we better remove the masking in the xlate functions completely and let all
the offending device trees trip over the warning so they get eventually fixed.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1456320

Hi Thomas,

On 08/04/2016 12:05 PM, Thomas Gleixner wrote:
> On Tue, 2 Aug 2016, Sebastian Frias wrote:
> 
>> Use IRQ_TYPE_SENSE_MASK on irq_domain_xlate_onetwocell()
>>
>> According to the xlate() callback definition, the 'out_type' parameter
>> needs to be the "linux irq type". A mask for such bits exists,
>> IRQ_TYPE_SENSE_MASK, and as a matter of fact it is used in
>> irq_domain_xlate_twocell(), use it for irq_domain_xlate_onetwocell() as
>> well.
>>
>> Fixes: 16b2e6e2f31d ("irq_domain: Create common xlate functions that device
>> drivers can use")
> 
> You seem to be obsessed by adding "Fixes" tags and 'Fix' to the $subject, but
> your changelog just babbles about making the code the same as in
> irq_domain_xlate_twocell() and avoids carefully to explain what is broken,
> what consequences that has and what the fix is.

I'm sorry but I think your comment above is unfair, so you could have omitted
it entirely.

> 
> Now if you look at the call site which uses the xlate functions then you'll
> notice that we have a sanity check there already:
> 
>         if (irq_domain_translate(domain, fwspec, &hwirq, &type))
>                 return 0;
>         /*
>          * WARN if the irqchip returns a type with bits
>          * outside the sense mask set and clear these bits.
>          */
>         if (WARN_ON(type & ~IRQ_TYPE_SENSE_MASK))
>                 type &= IRQ_TYPE_SENSE_MASK;
> 

(for what is worth, my patch was based on a recent 4.7 that does not has the
code you mention above)

> So we better remove the masking in the xlate functions completely and let all
> the offending device trees trip over the warning so they get eventually fixed.

That's fine by me.
If you want I can submit a patch that does what you want (if so, should I use
"fixes" or not?), let me know.

Best regards,

Sebastian

[toc] | [prev] | [next] | [standalone]

#1456437

On Thu, 4 Aug 2016, Sebastian Frias wrote:
> On 08/04/2016 12:05 PM, Thomas Gleixner wrote:
> (for what is worth, my patch was based on a recent 4.7 that does not has the
> code you mention above)

In future, please always check against mainline or the head of the relevant
maintainer tree.
 
> > So we better remove the masking in the xlate functions completely and let all
> > the offending device trees trip over the warning so they get eventually fixed.
> 
> That's fine by me.
> If you want I can submit a patch that does what you want (if so, should I use
> "fixes" or not?), let me know.

If it is broken then it wants a fixes tag plus an explanation WHY it is
broken, what consequences the breakage has etc.

If it's just a regular code improvement then not.

Decide yourself.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web