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