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

[Sebastian Frias <sf84@laposte.net>]

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