From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 8FE48C43219 for ; Mon, 9 May 2022 09:25:57 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S236958AbiEIJ1w (ORCPT ); Mon, 9 May 2022 05:27:52 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59314 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S236492AbiEIIvS (ORCPT ); Mon, 9 May 2022 04:51:18 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 026A5240B7; Mon, 9 May 2022 01:47:24 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id AF3BBB80FEA; Mon, 9 May 2022 08:47:23 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 6063BC385A8; Mon, 9 May 2022 08:47:22 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1652086042; bh=m/b+9hVDs/NpFu20f3zMR0zeKJUF5Hwvxq1T1SnXtL8=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=KAkwORF04sxysBiseSCgjyrO0HfAfJI6X4MSl8xqczJ7FPY8SDXfAOAYRY/qivwIP A1G3s2hckFB6zCoTnLzk6ahWwtDAdf4cffN0U/vFDxCELc5yQBIy+8sllm8o8XUYV7 Us/tNd/6RsQZFxjXtwTxihanU813H+9L/a3Jane+o7dKfKUn3+KyE5S/WZYrfNyJ6Y 7YLzGrLKSVxSLXsiPcdWtvOXTpYIGAeHwoWGMF9RT2jzn8IKPoIL4nkLIhbONPOGI0 C+Mm8+cdr6jnTaBINkkUUD7dHzE4bS+TP8Wa4oKNWrSn+mkoAuvfiNtovfnvP/3i0T qbS1anpd/EU7A== Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1nnz31-009uWt-Oj; Mon, 09 May 2022 09:47:19 +0100 Date: Mon, 09 May 2022 09:47:19 +0100 Message-ID: <87ilqf6qjs.wl-maz@kernel.org> From: Marc Zyngier To: Lukas Wunner Cc: Mark Rutland , Jakub Kicinski , Thomas Gleixner , "David S. Miller" , Paolo Abeni , netdev@vger.kernel.org, linux-usb@vger.kernel.org, Steve Glendinning , UNGLinuxDriver@microchip.com, Oliver Neukum , Andre Edich , Oleksij Rempel , Martyn Welch , Gabriel Hojda , Christoph Fritz , Lino Sanfilippo , Philipp Rosenberger , Heiner Kallweit , Andrew Lunn , Russell King , Ferry Toth Subject: Re: [PATCH net-next v2 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling In-Reply-To: <20220506201647.GA30860@wunner.de> References: <20220505113207.487861b2@kernel.org> <20220505185328.GA14123@wunner.de> <87tua36i70.wl-maz@kernel.org> <20220506201647.GA30860@wunner.de> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: lukas@wunner.de, mark.rutland@arm.com, kuba@kernel.org, tglx@linutronix.de, davem@davemloft.net, pabeni@redhat.com, netdev@vger.kernel.org, linux-usb@vger.kernel.org, steve.glendinning@shawell.net, UNGLinuxDriver@microchip.com, oneukum@suse.com, andre.edich@microchip.com, linux@rempel-privat.de, martyn.welch@collabora.com, ghojda@yo2urs.ro, chf.fritz@googlemail.com, LinoSanfilippo@gmx.de, p.rosenberger@kunbus.com, hkallweit1@gmail.com, andrew@lunn.ch, linux@armlinux.org.uk, fntoth@gmail.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, 06 May 2022 21:16:47 +0100, Lukas Wunner wrote: > > On Fri, May 06, 2022 at 11:58:43AM +0100, Marc Zyngier wrote: > > On Thu, 05 May 2022 19:53:28 +0100, Lukas Wunner wrote: > > > On Thu, May 05, 2022 at 11:32:07AM -0700, Jakub Kicinski wrote: > > > > On Tue, 3 May 2022 15:15:05 +0200 Lukas Wunner wrote: > > > > > @@ -608,11 +618,20 @@ static void smsc95xx_status(struct usbnet *dev, struct urb *urb) > > > > > intdata = get_unaligned_le32(urb->transfer_buffer); > > > > > netif_dbg(dev, link, dev->net, "intdata: 0x%08X\n", intdata); > > > > > > > > > > + /* USB interrupts are received in softirq (tasklet) context. > > > > > + * Switch to hardirq context to make genirq code happy. > > > > > + */ > > > > > + local_irq_save(flags); > > > > > + __irq_enter_raw(); > > > > > + > > > > > if (intdata & INT_ENP_PHY_INT_) > > > > > - ; > > > > > + generic_handle_domain_irq(pdata->irqdomain, PHY_HWIRQ); > > > > > else > > > > > netdev_warn(dev->net, "unexpected interrupt, intdata=0x%08X\n", > > > > > intdata); > > > > > + > > > > > + __irq_exit_raw(); > > > > > + local_irq_restore(flags); > > > > > > > > Full patch: > > > > > > > > https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/ > > > > > > This is basically identical to what drivers/net/usb/lan78xx.c does > > > in lan78xx_status(), except I'm passing the hw irq instead of the > > > linux irq to genirq code, thereby avoiding the overhead of a > > > radix_tree_lookup(). > > > > > > generic_handle_domain_irq() warns unconditionally on !in_irq(), > > > unlike handle_irq_desc(), which constrains the warning to > > > handle_enforce_irqctx() (i.e. x86 APIC, arm GIC/GICv3). > > > Perhaps that's an oversight in generic_handle_domain_irq(), > > > unless __irq_resolve_mapping() becomes unsafe outside in_irq() > > > for some reason... > > > > > > In any case the unconditional in_irq() necessitates __irq_enter_raw() > > > here. > > > > > > And there's no _safe variant() of generic_handle_domain_irq() > > > (unlike generic_handle_irq_safe() which was recently added by > > > 509853f9e1e7), hence the necessity of an explicit local_irq_save(). > > > > Please don't directly use __irq_enter_raw() and similar things > > directly in driver code (it doesn't do anything related to RCU, for > > example, which could be problematic if used in arbitrary contexts). > > Given how infrequent this interrupt is, I'd rather you use something > > similar to what lan78xx is doing, and be done with it. > > > > And since this is a construct that seems to be often repeated, why > > don't you simply make the phy interrupt handling available over a > > smp_call_function() interface, which would always put you in the > > correct context and avoid faking things up? > > As I've explained in the commit message (linked above), LAN95xx chips > have 11 GPIOs which can be an interrupt source. This patch adds > PHY interrupt support in such a way that GPIO interrupts can easily > be supported by a subsequent commit. To this end, LAN95xx needs > to be represented as a proper irqchip. > > The crucial thing to understand is that a USB interrupt is not received > as a hardirq. USB gadgets are incapable of directly signaling an > interrupt because they cannot initiate a bus transaction by themselves. > All communication on the bus is initiated by the host controller, > which polls a gadget's Interrupt Endpoint in regular intervals. > If an interrupt is pending, that information is passed up the stack > in softirq context. Hence there's no other way than "faking things up", > to borrow your language. > > Another USB driver in the tree, drivers/gpio/gpio-dln2.c, likewise > represents the USB gadget as an irqchip to signal GPIO interrupts. > This shows that LAN95xx is not an isolated case. gpio-dln2.c does > not invoke __irq_enter_raw(), so I think users will now see a WARN > splat with that driver since Mark Rutland's 0953fb263714 (+cc). > > As I've pointed out above, it seems like an oversight that Mark > didn't make the WARN_ON_ONCE() conditional on handle_enforce_irqctx() > (as handle_irq_desc() does). Sadly you did not respond to that > observation. When did you make that observation? I can only see an email from you being sent *after* the one I am replying to. > Please clarify whether that is indeed erroneous. > Once handle_enforce_irqctx() is added to generic_handle_domain_irq(), > there's no need for me to call __irq_enter_raw(). Problem solved. I don't see it as an oversight. Drivers shouldn't rely on architectural quirks, and it is much clearer to simply forbid something that cannot be guaranteed across the board, specially for something that is as generic as USB. > Should there be a valid reason for the missing handle_enforce_irqctx(), > then I propose adding a generic_handle_domain_irq_safe() function which > calls __irq_enter_raw() (or probably __irq_enter() to get accounting), > thereby resolving your objection to calling __irq_enter_raw() from a > driver. Feel free to submit a patch. Thanks, M. -- Without deviation from the norm, progress is not possible.