Messages in this thread | | | Subject | Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions | From | Doug Berger <> | Date | Mon, 10 Jul 2017 10:39:43 -0700 |
| |
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
| |