From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH 1/9] irq: add irq_set_chained_handler_and_data() Date: Wed, 17 Jun 2015 20:38:03 +0100 Message-ID: <20150617193803.GY7557@n2100.arm.linux.org.uk> References: Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from pandora.arm.linux.org.uk ([78.32.30.218]:38142 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751821AbbFQTiR (ORCPT ); Wed, 17 Jun 2015 15:38:17 -0400 Content-Disposition: inline In-Reply-To: Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Thomas Gleixner Cc: LAK , linux-gpio@vger.kernel.org, linux-tegra@vger.kernel.org, Alexandre Courbot , Hans Ulli Kroll , Jason Cooper , Lee Jones , Linus Walleij , Thierry Reding On Wed, Jun 17, 2015 at 04:10:02PM +0200, Thomas Gleixner wrote: > On Tue, 16 Jun 2015, Russell King wrote: > > > Driver authors seem to get the ordering of irq_set_chained_handler() > > and irq_set_handler_data() wrong - ordering the former before the > > latter. This opens a race window where, if there is an interrupt > > pending, the handler will be called between these two calls, > > potentially resulting in an oops. > > > > Provide a single interface to set both of these together, especially > > as that's commonly what is required. > > > > Signed-off-by: Russell King > > --- > > It probably makes sense to convert everything over to this new > > registration function, and kill all users of irq_set_chained_handler() > > to prevent this problem recurring. Thoughts? > > Yes. Classic coccinelle problem. Here is the semantic patch: > > @@ > expression E1, E2, E3; > @@ > -irq_set_chained_handler(E1, E3); > ... > -irq_set_handler_data(E1, E2); > +irq_set_chained_handler_and_data(E1, E3, E2); > > This finds and corrects all instances which get it wrong: > > arch: > arm/common/locomo.c | 3 +-- > arm/common/sa1111.c | 3 +-- > arm/mach-gemini/gpio.c | 4 ++-- > avr32/mach-at32ap/extint.c | 3 +-- > m68k/mac/psc.c | 12 ++++-------- > mips/ath25/ar2315.c | 4 ++-- > mips/ath25/ar5312.c | 4 ++-- > mips/pci/pci-ar2315.c | 4 ++-- > mips/ralink/irq.c | 3 +-- > sh/intc/core.c | 5 +++-- > > drivers: > dma/ipu/ipu_irq.c | 6 ++---- > gpio/gpio-bcm-kona.c | 5 +++-- > gpio/gpio-davinci.c | 10 ++-------- > gpio/gpio-dwapb.c | 4 ++-- > gpio/gpio-msic.c | 3 +-- > gpio/gpio-mxc.c | 10 +++++----- > gpio/gpio-mxs.c | 4 ++-- > gpio/gpio-tegra.c | 4 ++-- > gpu/ipu-v3/ipu-common.c | 13 +++++-------- > irqchip/irq-keystone.c | 4 ++-- > irqchip/spear-shirq.c | 3 +-- > mfd/pm8921-core.c | 6 ++---- > mfd/t7l66xb.c | 3 +-- > mfd/tc6393xb.c | 3 +-- > pci/host/pci-keystone.c | 7 +++---- > pinctrl/mediatek/pinctrl-mtk-common.c | 3 +-- > pinctrl/pinctrl-adi2.c | 4 ++-- > pinctrl/pinctrl-st.c | 4 ++-- > pinctrl/samsung/pinctrl-exynos.c | 4 ++-- > pinctrl/samsung/pinctrl-s3c24xx.c | 3 +-- > pinctrl/samsung/pinctrl-s3c64xx.c | 8 ++++---- > pinctrl/sunxi/pinctrl-sunxi.c | 6 +++--- > spmi/spmi-pmic-arb.c | 6 ++---- > 33 files changed, 70 insertions(+), 98 deletions(-) > > If we revert the search pattern we get the ones which got it right: > > @@ > expression E1, E2, E3; > @@ > -irq_set_handler_data(E1, E2); > ... > -irq_set_chained_handler(E1, E3); > +irq_set_chained_handler_and_data(E1, E3, E2); > > That handles another bunch and leaves us with 135 instances of > irq_set_chained_handler() which do not set handler data. So its > trivial to change them to > > irq_set_chained_handler_and_data(irq, handler, NULL); > > and then remove irq_set_chained_handler() > > If thats ok for you, then i apply the lot you sent and run the cocci > scripts right at rc1. I have another set of transformations in that > area pending. Totally fine with that. -- FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up according to speedtest.net.