From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756171AbdGKWvT (ORCPT ); Tue, 11 Jul 2017 18:51:19 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:45537 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756033AbdGKWvS (ORCPT ); Tue, 11 Jul 2017 18:51:18 -0400 Date: Wed, 12 Jul 2017 00:51:13 +0200 From: Sebastian Reichel To: Thomas Gleixner Cc: Linus Torvalds , Thomas Gleixner , Tony Lindgren , LKML , Andrew Morton , Ingo Molnar , "H. Peter Anvin" , Pavel Machek , Linus Walleij , Grygorii Strashko Subject: Re: [GIT pull] irq updates for 4.13 Message-ID: <20170711225113.2acaer6jepx2rwph@earth> References: <20170711135131.GW3730@atomide.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="ikrga55ypsvm5fip" Content-Disposition: inline In-Reply-To: User-Agent: NeoMutt/20170609 (1.8.3) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --ikrga55ypsvm5fip Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Tue, Jul 11, 2017 at 11:41:52PM +0200, Thomas Gleixner wrote: > [...] > > Here is a revised version of the previous patch with the conditional > locking removed and a bunch of comments added. That one also fixes Droid 4 boot. Tested-by: Sebastian Reichel -- Sebastian > 8<---------------------------- > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1090,6 +1090,16 @@ setup_irq_thread(struct irqaction *new, > /* > * Internal function to register an irqaction - typically used to > * allocate special interrupts that are part of the architecture. > + * > + * Locking rules: > + * > + * desc->request_mutex Provides serialization against a concurrent free_= irq() > + * chip_bus_lock Provides serialization for slow bus operations > + * desc->lock Provides serialization against hard interrupts > + * > + * chip_bus_lock and desc->lock are sufficient for all other management = and > + * interrupt related functions. desc->request_mutex solely serializes > + * request/free_irq(). > */ > static int > __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *n= ew) > @@ -1167,20 +1177,35 @@ static int > if (desc->irq_data.chip->flags & IRQCHIP_ONESHOT_SAFE) > new->flags &=3D ~IRQF_ONESHOT; > =20 > + /* > + * Protects against a concurrent __free_irq() call which might wait > + * for synchronize_irq() to complete without holding the optional > + * chip bus lock and desc->lock. > + */ > mutex_lock(&desc->request_mutex); > + > + /* > + * Acquire bus lock as the irq_request_resources() callback below > + * might rely on the serialization or the magic power management > + * functions which are abusing the irq_bus_lock() callback, > + */ > + chip_bus_lock(desc); > + > + /* First installed action requests resources. */ > if (!desc->action) { > ret =3D irq_request_resources(desc); > if (ret) { > pr_err("Failed to request resources for %s (irq %d) on irqchip %s\n", > new->name, irq, desc->irq_data.chip->name); > - goto out_mutex; > + goto out_bus_unlock; > } > } > =20 > - chip_bus_lock(desc); > - > /* > * The following block of code has to be executed atomically > + * protected against a concurrent interrupt and any of the other > + * management calls which are not serialized via > + * desc->request_mutex or the optional bus lock. > */ > raw_spin_lock_irqsave(&desc->lock, flags); > old_ptr =3D &desc->action; > @@ -1286,10 +1311,8 @@ static int > ret =3D __irq_set_trigger(desc, > new->flags & IRQF_TRIGGER_MASK); > =20 > - if (ret) { > - irq_release_resources(desc); > + if (ret) > goto out_unlock; > - } > } > =20 > desc->istate &=3D ~(IRQS_AUTODETECT | IRQS_SPURIOUS_DISABLED | \ > @@ -1385,12 +1408,10 @@ static int > out_unlock: > raw_spin_unlock_irqrestore(&desc->lock, flags); > =20 > - chip_bus_sync_unlock(desc); > - > if (!desc->action) > irq_release_resources(desc); > - > -out_mutex: > +out_bus_unlock: > + chip_bus_sync_unlock(desc); > mutex_unlock(&desc->request_mutex); > =20 > out_thread: > @@ -1472,6 +1493,7 @@ static struct irqaction *__free_irq(unsi > WARN(1, "Trying to free already-free IRQ %d\n", irq); > raw_spin_unlock_irqrestore(&desc->lock, flags); > chip_bus_sync_unlock(desc); > + mutex_unlock(&desc->request_mutex); > return NULL; > } > =20 > @@ -1498,6 +1520,20 @@ static struct irqaction *__free_irq(unsi > #endif > =20 > raw_spin_unlock_irqrestore(&desc->lock, flags); > + /* > + * Drop bus_lock here so the changes which were done in the chip > + * callbacks above are synced out to the irq chips which hang > + * behind a slow bus (I2C, SPI) before calling synchronize_irq(). > + * > + * Aside of that the bus_lock can also be taken from the threaded > + * handler in irq_finalize_oneshot() which results in a deadlock > + * because synchronize_irq() would wait forever for the thread to > + * complete, which is blocked on the bus lock. > + * > + * The still held desc->request_mutex() protects against a > + * concurrent request_irq() of this irq so the release of resources > + * and timing data is properly serialized. > + */ > chip_bus_sync_unlock(desc); > =20 > unregister_handler_proc(irq, action); > @@ -1530,8 +1566,15 @@ static struct irqaction *__free_irq(unsi > } > } > =20 > + /* Last action releases resources */ > if (!desc->action) { > + /* > + * Reaquire bus lock as irq_release_resources() might > + * require it to deallocate resources over the slow bus. > + */ > + chip_bus_lock(desc); > irq_release_resources(desc); > + chip_bus_sync_unlock(desc); > irq_remove_timings(desc); > } > =20 --ikrga55ypsvm5fip Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEE72YNB0Y/i3JqeVQT2O7X88g7+poFAlllVl0ACgkQ2O7X88g7 +ppeaQ//XgEofNwsghr4griqgxUITp5w0xeAzgMhEuVrOf+G/Tcjqyw3jpT4egVI glwWovNm5W3P8BOa/JoQZ2BEKWTxXtnTOUGsL9ZeojZSK5MdFBlxCTedexg2ojys IYaQJCKRL6ZN1c3DZt3NdHFjI2EpXeUoo+Kr7Mfr5xUyms8qx3U+0cRcsLMJUIPB JrI1L/afPMkwJ+ohdOUw0Jbw1HIWMiqlqzD4Np5qUHJ/jQ4H3qiaL5N65hGQKll0 WCPW4/qDZ20xSknSk/c/qhTdk/W/X+JY4quj8q3MlqJHoUav7u8w6NmWVW+56vNr 4vugC+xFXBKtN9k90jn+ZtnKX5xfsmn7pc9MIdUozDLwKGzY+OPURJG0dj/2e9Ix vUbQq4pYANOuwu7AlnCvpTL7qJ0cAV4V+oQg1RREvqIhOcjKEljvZ6UNcS5NXBao mcAdG8oiBkviacggckuCfZELJAhwZixUlsnEiD6sFKZAEQjq2gG3mlLbnysnbn4U vkoYnhpZSDS5HIi2UfML069l+ti2yrZqT1JaWJr3Jsz9R7tDYkpdHWGpBHTcVyZZ fHd5Y0fv7Hk0vNiID9u+0C2ibnZf8g33nkhgw01umzgNLQjmfWPD7qvp3OXkxydG ctm9uyByaEkR2a3sjgm4n+EknXmKf5Q6iI1t+1maLmslDplEXAQ= =ObXr -----END PGP SIGNATURE----- --ikrga55ypsvm5fip--