Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions

[Doug Berger]

On 07/08/2017 05:08 AM, Thomas Gleixner wrote:
> On Fri, 7 Jul 2017, Doug Berger wrote:
> 
>> The irq_gc_mask_disable_reg_and_ack() function name implies that it
>> provides the combined functions of irq_gc_mask_disable_reg() and
>> irq_gc_ack().  However, the implementation does not actually do
>> that since it writes the mask instead of the disable register. It
>> also does not maintain the mask cache which makes it inappropriate
>> to use with other masking functions.
>>
>> In addition, commit 659fb32d1b67 ("genirq: replace irq_gc_ack() with
>> {set,clr}_bit variants (fwd)") effectively renamed irq_gc_ack() to
>> irq_gc_set_bit() so this function probably should have also been
>> renamed at that time.
>>
>> Since this generic chip code provides three mask functions and two
>> ack functions, this commit provides generic implementations for all
>> six combinations of the mask and ack functions suitable for use
>> with the irq_mask_ack member of the struct irq_chip.
> 
> We have exactly one user of irq_gc_mask_disable_reg_and_ack() and that
> needs exactly on function as replacement. Why do we need 6 variants of
> that right now?

This is merely a suggestion.

When I was originally adding support for the BCM7271 variant of the
irq-brcmstb-l2 interrupt controller I noticed that providing an
irq_mask_ack implementation would be slightly more efficient than only
providing irq_mask and irq_ack.  I assumed that it was philosophically
better to use a generic chip implementation than creating yet another
driver specific version of the method.

This task was originally done on a downstream development kernel derived
from the v4.1 kernel and I'm finally taking the opportunity to attempt
to upstream the change.  At that time, I was drawn to the
irq_gc_mask_disable_reg_and_ack() function based on the name, but I
discovered that it was actually using the mask register rather than the
disable register contrary to its name and hadn't been included in the
changes when mask caching was added and when some similar functions were
renamed.  I considered submitting a patch to correct what I perceived as
a bug, but after discovering there were no users of the function at that
time I decided that it should probably be removed and replaced with the
irq_gc_mask_disable_and_ack_set() function that I needed.

While preparing the upstream submission I discovered that the tango
interrupt controller driver had apparently started using the potentially
problematic function.  I'm not comfortable making changes to drivers for
devices that I'm not able to test (I'm still making mistakes with git
send-email --cc-cmd ;) so Florian accepted authorship of that change.

I had perhaps incorrectly assumed that the
irq_gc_mask_disable_reg_and_ack() function was originally included in
the generic chip implementation nearly 5 years before its first use was
to encourage driver developers to adopt generic chip implementations
rather than implementing unique versions in every driver.  To that end
I'm suggesting offering all currently possible generic chip
implementations of the irq_mask_ack method to encourage drivers to adopt
use of generic chip methods even though I only need one of them.

Perhaps someone bolder than I may be inspired to undertake converting
more irqchip drivers to use these methods.

If I am mistaken and this is an undesired change I am happy to implement
an irq-brcmstb-l2 driver specific implementation of the irq_mask_ack
method or just changing the single function.

> 
> Thanks,
> 
> 	tglx
> 

Thanks for your consideration and please let me know how you would like
me to proceed with the submission.
    Doug