From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751380AbdGQRYE (ORCPT ); Mon, 17 Jul 2017 13:24:04 -0400 Received: from mail-qt0-f194.google.com ([209.85.216.194]:34465 "EHLO mail-qt0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751291AbdGQRYC (ORCPT ); Mon, 17 Jul 2017 13:24:02 -0400 Subject: Re: [PATCH 1/6] genirq: generic chip: add generic irq_mask_ack functions From: Doug Berger To: Thomas Gleixner Cc: Marc Zyngier , mans@mansr.com, Bartosz Golaszewski , Sebastian Frias , Boris Brezillon , open list References: <20170707192016.13001-1-opendmb@gmail.com> <20170707192016.13001-2-opendmb@gmail.com> <1ccd2ce6-ac51-afed-f8be-f453f18b5464@gmail.com> Message-ID: Date: Mon, 17 Jul 2017 10:23:59 -0700 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.1.1 MIME-Version: 1.0 In-Reply-To: <1ccd2ce6-ac51-afed-f8be-f453f18b5464@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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