From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933904AbdGKVal (ORCPT ); Tue, 11 Jul 2017 17:30:41 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:45382 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932475AbdGKVak (ORCPT ); Tue, 11 Jul 2017 17:30:40 -0400 Date: Tue, 11 Jul 2017 23:30:36 +0200 From: Sebastian Reichel To: Linus Torvalds Cc: 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: <20170711213035.hnorr7v46zlcs4t3@earth> References: <20170711135131.GW3730@atomide.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; protocol="application/pgp-signature"; boundary="d7fafetklmqcgrao" 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 --d7fafetklmqcgrao Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable Hi, On Tue, Jul 11, 2017 at 11:16:03AM -0700, Linus Torvalds wrote: > On Tue, Jul 11, 2017 at 10:52 AM, Thomas Gleixner wr= ote: > > Not completely, because of the free path issues. See the other mail. To= ny > > confirmed that it works. I wait for Sebastian and queue it with a proper > > changelog, ok? > > Ugh, I absolutely detest your ugly "bool buslock" parameter to > irq_release_resources(). So /me will skip testing Thomas' patch for now. > And there seems to be no reason for it. >=20 > Why don't you just move the >=20 > chip_bus_sync_unlock(desc); >=20 > call in __free_irq() down to just before you release the request_mutex? >=20 > In fact, looking at __free_irq(), I note that it's locking is > completely broken shit. Look at the >=20 > if (!action) { > WARN(1, "Trying to free already-free IRQ %d\n", i= rq); >=20 > error case, and look for where it unlocks request_mutex. Yeah, it doesn't. >=20 > So honestly, I think this code is broken, and it's broken partly > because it has some really bad locking and logic rules. >=20 > Why not fix those stupid bugs and clean things up at the same time? > Make the rule be that as you take the request_mutex lock, you then > also do the chip_bus_lock(). >=20 > And when you release the request_mutex lock, you do > chip_bus_sync_unlock() just before. >=20 > And no, I have no idea what the locking rules are for > irq_finalize_oneshot() - it does that chip_bus_lock() without having > any external serialization. Is that ok? Are the chip handlers able to > deal with that? Same seems to go for free_percpu_irq(). >=20 > Anyway, patch attached (AGAIN, TOTALLY UNTESTED) showing what I mean, > and fixing (well, modulo any bugs I introduced by my untested sh*t) > that definite bug in lack of unlocking. >=20 > But that "bool buslock" thing really is too disgusting. Conditional > locking should not be done. It's a sign of serious problems, imnsho. >=20 > Comments? Even if they are "Linus, you're way out of line, and you > can't just move that chip_bus_sync_unlock() down like that because of > XYZ, you moron". >=20 > For example, it's entirely possible that we can't do the > "synchronize_irq()" waiting while we hold that chip_bus_lock(). But > the ones I looked at seemed to all take sleeping locks (or no locks at > all - doing other things), which implies that they certainly can't be > blocking irq delivery. >=20 > So I'm *not* claiming that the attached patch is necessarily right. I > just really don't like your conditional lock thing, and this would > seem to possibly be a clean way around it if it works. This fixes boot on Droid 4: Tested-by: Sebastian Reichel -- Sebastian > kernel/irq/manage.c | 14 +++++++------- > 1 file changed, 7 insertions(+), 7 deletions(-) >=20 > diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c > index 5624b2dd6b58..c4cbda784ea5 100644 > --- a/kernel/irq/manage.c > +++ b/kernel/irq/manage.c > @@ -1168,17 +1168,17 @@ __setup_irq(unsigned int irq, struct irq_desc *de= sc, struct irqaction *new) > new->flags &=3D ~IRQF_ONESHOT; > =20 > mutex_lock(&desc->request_mutex); > + chip_bus_lock(desc); > + > 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_unlock_chip_bus; > } > } > =20 > - chip_bus_lock(desc); > - > /* > * The following block of code has to be executed atomically > */ > @@ -1385,12 +1385,11 @@ __setup_irq(unsigned int irq, struct irq_desc *de= sc, struct irqaction *new) > out_unlock: > raw_spin_unlock_irqrestore(&desc->lock, flags); > =20 > - chip_bus_sync_unlock(desc); > - > if (!desc->action) > irq_release_resources(desc); > =20 > -out_mutex: > +out_unlock_chip_bus: > + chip_bus_sync_unlock(desc); > mutex_unlock(&desc->request_mutex); > =20 > out_thread: > @@ -1472,6 +1471,7 @@ static struct irqaction *__free_irq(unsigned int ir= q, void *dev_id) > 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,7 +1498,6 @@ static struct irqaction *__free_irq(unsigned int ir= q, void *dev_id) > #endif > =20 > raw_spin_unlock_irqrestore(&desc->lock, flags); > - chip_bus_sync_unlock(desc); > =20 > unregister_handler_proc(irq, action); > =20 > @@ -1535,6 +1534,7 @@ static struct irqaction *__free_irq(unsigned int ir= q, void *dev_id) > irq_remove_timings(desc); > } > =20 > + chip_bus_sync_unlock(desc); > mutex_unlock(&desc->request_mutex); > =20 > irq_chip_pm_put(&desc->irq_data); --d7fafetklmqcgrao Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCgAdFiEE72YNB0Y/i3JqeVQT2O7X88g7+poFAlllQ3YACgkQ2O7X88g7 +pq30hAAgU9R/DBPiDv0Ob+7NRzOIFZ7tWcY9gxIOC3fwP9eb/WV18RP9/wZWZs5 fBaGsgUuk98WJjrKFlO/yaFQPIHpIQgdF3t/g5uPKi0+RQYhxvYl104Z5dhdWxw9 1DBnGzD9s6iqgBEkUz6NxxLpjja1RvQWHvGC4mXTNJIE1VRRWnJrRAMDVEhPE8CR UR1rIVcjVFgzjV99F2jGmASVoH3BHdWlnEI1sqJmg2isin0U3/k1IMVvalhKAgna J8UnT9iTcsXxfwyYzX0C0HRNp3zNEl5qtbBYDk64E3S0q1pn0Je16kyFZFjeBt9+ RRBdZXQywEM+Q+K6H9qNttXFnhKzx+B3xY2caxLSDv4G5CLtexptAk4IaplIE3TU wkQ6ba3BawlB11LX9lcYCK12wyc4OHQp7UgIHVwjQMlibR+wQRp+xE54K6xqL9l/ 1wHiYQMXLFssWpoD0RWuuijKYwIKsh3Vj2kZjWedAsi3ymXx0/4dq2jV6HoQSnOU iblObDgdZP1A5tKg+DaNNnjv+PH+WFjGS2gahCODMRFpxrEjp2Mg5ErMk1/jfQYw Z3Mol/qpMXd9lgFfmAtV/vz9/VcbKxQB6OmICkZDCgztu3CLZK7L3P/ycOcoLxrG u/NEH6nH6Tmxl8eK+kpndhdgMrFm2w/kp8tci87thtsAx8takdI= =64/3 -----END PGP SIGNATURE----- --d7fafetklmqcgrao--