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

[Doug Berger]

On 07/10/2017 10:39 AM, Doug Berger wrote:
> 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
> 

Seeing as Mans has acked the change to his driver should I submit a V2
with just the function he and I are using and remove the other five
permutations, or are you willing to move forward with the patch as is?

Thanks,
    Doug