diff mbox series

[net-next,v2,5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

Message ID c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Polling be gone on LAN95xx | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers warning 1 maintainers not CCed: edumazet@google.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 224 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Lukas Wunner May 3, 2022, 1:15 p.m. UTC
Link status of SMSC LAN95xx chips is polled once per second, even though
they're capable of signaling PHY interrupts through the MAC layer.

Forward those interrupts to the PHY driver to avoid polling.  Benefits
are reduced bus traffic, reduced CPU overhead and quicker interface
bringup.

Polling was introduced in 2016 by commit d69d16949346 ("usbnet:
smsc95xx: fix link detection for disabled autonegotiation").
Back then, the LAN95xx driver neglected to enable the ENERGYON interrupt,
hence couldn't detect link-up events when auto-negotiation was disabled.
The proper solution would have been to enable the ENERGYON interrupt
instead of polling.

Since then, PHY handling was moved from the LAN95xx driver to the SMSC
PHY driver with commit 05b35e7eb9a1 ("smsc95xx: add phylib support").
That PHY driver is capable of link detection with auto-negotiation
disabled because it enables the ENERGYON interrupt.

Note that signaling interrupts through the MAC layer not only works with
the integrated PHY, but also with an external PHY, provided its
interrupt pin is attached to LAN95xx's nPHY_INT pin.

In the unlikely event that the interrupt pin of an external PHY is
attached to a GPIO of the SoC (or not connected at all), the driver can
be amended to retrieve the irq from the PHY's of_node.

To forward PHY interrupts to phylib, it is not sufficient to call
phy_mac_interrupt().  Instead, the PHY's interrupt handler needs to run
so that PHY interrupts are cleared.  That's because according to page
119 of the LAN950x datasheet, "The source of this interrupt is a level.
The interrupt persists until it is cleared in the PHY."

https://www.microchip.com/content/dam/mchp/documents/UNG/ProductDocuments/DataSheets/LAN950x-Data-Sheet-DS00001875D.pdf

Therefore, create an IRQ domain with a single IRQ for the PHY.  In the
future, the IRQ domain may be extended to support the 11 GPIOs on the
LAN95xx.

Normally the PHY interrupt should be masked until the PHY driver has
cleared it.  However masking requires a (sleeping) USB transaction and
interrupts are received in (non-sleepable) softirq context.  I decided
not to mask the interrupt at all (by using the dummy_irq_chip's noop
->irq_mask() callback):  The USB interrupt endpoint is polled in 1 msec
intervals and normally that's sufficient to wake the PHY driver's IRQ
thread and have it clear the interrupt.  If it does take longer, worst
thing that can happen is the IRQ thread is woken again.  No big deal.

Because PHY interrupts are now perpetually enabled, there's no need to
selectively enable them on suspend.  So remove all invocations of
smsc95xx_enable_phy_wakeup_interrupts().

In smsc95xx_resume(), move the call of phy_init_hw() before
usbnet_resume() (which restarts the status URB) to ensure that the PHY
is fully initialized when an interrupt is handled.

Tested-by: Oleksij Rempel <o.rempel@pengutronix.de> # LAN9514/9512/9500
Tested-by: Ferry Toth <fntoth@gmail.com> # LAN9514
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Reviewed-by: Andrew Lunn <andrew@lunn.ch> # from a PHY perspective
Cc: Andre Edich <andre.edich@microchip.com>
---
Changes since v1:
 * Eliminate a checkpatch "no space before tabs" warning.
 * Explain in commit message why the call to phy_init_hw() is moved
   in smsc95xx_resume().

 drivers/net/usb/smsc95xx.c | 118 +++++++++++++++++++++----------------
 1 file changed, 66 insertions(+), 52 deletions(-)

Comments

Jakub Kicinski May 5, 2022, 6:32 p.m. UTC | #1
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);

IRQ maintainers could you cast your eyes over this?

Full patch:

https://lore.kernel.org/all/c6b7f4e4a17913d2f2bc4fe722df0804c2d6fea7.1651574194.git.lukas@wunner.de/
Lukas Wunner May 5, 2022, 6:53 p.m. UTC | #2
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);
> 
> IRQ maintainers could you cast your eyes over this?
> 
> 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().

Thanks,

Lukas
Marc Zyngier May 6, 2022, 10:58 a.m. UTC | #3
On Thu, 05 May 2022 19:53:28 +0100,
Lukas Wunner <lukas@wunner.de> 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);
> > 
> > IRQ maintainers could you cast your eyes over this?
> > 
> > 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?

Thanks,

	M.
Lukas Wunner May 6, 2022, 8:16 p.m. UTC | #4
On Fri, May 06, 2022 at 11:58:43AM +0100, Marc Zyngier wrote:
> On Thu, 05 May 2022 19:53:28 +0100, Lukas Wunner <lukas@wunner.de> 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.  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.

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.

Thanks,

Lukas
Marc Zyngier May 9, 2022, 8:47 a.m. UTC | #5
On Fri, 06 May 2022 21:16:47 +0100,
Lukas Wunner <lukas@wunner.de> 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 <lukas@wunner.de> 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.
Lukas Wunner May 10, 2022, 8:18 a.m. UTC | #6
On Mon, May 09, 2022 at 09:47:19AM +0100, Marc Zyngier wrote:
> On Fri, 06 May 2022 21:16:47 +0100, Lukas Wunner <lukas@wunner.de> 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 <lukas@wunner.de> wrote:
> > > > 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.
> > > 
> > > 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).
> > 
> > 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.

I was referring to the above-quoted sentence:

     "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..."

Never mind, let's focus on the problem at hand.
It's secondary who said what when.


> > 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.

Whether a warning is warranted is not dependent on the architecture,
but on the irqchip from which an interrupt normally originates:

* Interrupt normally originates from x86 APIC or arm GIC/GICv3,
  but is synthesized in non-hardirq context:  Warning is warranted.

* Interrupt normally originates from any other top-level irqchip,
  such as irq-bcm2836.c, but is synthesized in non-hardirq context:
  Warning is a false positive!

* Interrupt is always synthesized in non-hardirq context by a
  USB irqchip: Warning is a false positive, regardless whether
  the top-level irqchip is x86 APIC, arm GIC/GICv3 or anything else!


> > 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.

Done:

https://lore.kernel.org/lkml/c3caf60bfa78e5fdbdf483096b7174da65d1813a.1652168866.git.lukas@wunner.de/

I'm focussing on eliminating the false-positive warning for now.
Introducing a generic_handle_domain_irq_safe() wrapper which alleviates
drivers from calling local_irq_save() can be done in a separate step.

Thanks,

Lukas
Lukas Wunner May 11, 2022, 9:26 a.m. UTC | #7
Hi Jakub,

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);
> 
> IRQ maintainers could you cast your eyes over this?

Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in
generic_handle_domain_irq()") tonight:

http://git.kernel.org/tip/tip/c/792ea6a074ae

That allows me to drop the controversial __irq_enter_raw().

Jakub, do you want me to resend the full series (all 7 patches)
or should I send only patch [5/7] in-reply-to this one here?
Or do you prefer applying all patches except [5/7] and have me
resend that single patch?

Let me know what your preferred modus operandi is.

Thanks,

Lukas
Jakub Kicinski May 11, 2022, 4:15 p.m. UTC | #8
On Wed, 11 May 2022 11:26:16 +0200 Lukas Wunner wrote:
> > IRQ maintainers could you cast your eyes over this?  
> 
> Thomas applied 792ea6a074ae ("genirq: Remove WARN_ON_ONCE() in
> generic_handle_domain_irq()") tonight:
> 
> http://git.kernel.org/tip/tip/c/792ea6a074ae

Perfect!

> That allows me to drop the controversial __irq_enter_raw().
> 
> Jakub, do you want me to resend the full series (all 7 patches)
> or should I send only patch [5/7] in-reply-to this one here?
> Or do you prefer applying all patches except [5/7] and have me
> resend that single patch?
> 
> Let me know what your preferred modus operandi is.

Resending all patches would be the easiest for us and has the lowest
chance of screw up on our side, so resend all please & thanks!
diff mbox series

Patch

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 6309ff8e75de..9278d4e24d79 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -18,6 +18,8 @@ 
 #include <linux/usb/usbnet.h>
 #include <linux/slab.h>
 #include <linux/of_net.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
 #include <linux/mdio.h>
 #include <linux/phy.h>
 #include <net/selftests.h>
@@ -53,6 +55,9 @@ 
 #define SUSPEND_ALLMODES		(SUSPEND_SUSPEND0 | SUSPEND_SUSPEND1 | \
 					 SUSPEND_SUSPEND2 | SUSPEND_SUSPEND3)
 
+#define SMSC95XX_NR_IRQS		(1) /* raise to 12 for GPIOs */
+#define PHY_HWIRQ			(SMSC95XX_NR_IRQS - 1)
+
 struct smsc95xx_priv {
 	u32 mac_cr;
 	u32 hash_hi;
@@ -61,6 +66,9 @@  struct smsc95xx_priv {
 	spinlock_t mac_cr_lock;
 	u8 features;
 	u8 suspend_flags;
+	struct irq_chip irqchip;
+	struct irq_domain *irqdomain;
+	struct fwnode_handle *irqfwnode;
 	struct mii_bus *mdiobus;
 	struct phy_device *phydev;
 };
@@ -597,6 +605,8 @@  static void smsc95xx_mac_update_fullduplex(struct usbnet *dev)
 
 static void smsc95xx_status(struct usbnet *dev, struct urb *urb)
 {
+	struct smsc95xx_priv *pdata = dev->driver_priv;
+	unsigned long flags;
 	u32 intdata;
 
 	if (urb->actual_length != 4) {
@@ -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);
 }
 
 /* Enable or disable Tx & Rx checksum offload engines */
@@ -1080,8 +1099,9 @@  static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 {
 	struct smsc95xx_priv *pdata;
 	bool is_internal_phy;
+	char usb_path[64];
+	int ret, phy_irq;
 	u32 val;
-	int ret;
 
 	printk(KERN_INFO SMSC_CHIPNAME " v" SMSC_DRIVER_VERSION "\n");
 
@@ -1121,10 +1141,38 @@  static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 	if (ret)
 		goto free_pdata;
 
+	/* create irq domain for use by PHY driver and GPIO consumers */
+	usb_make_path(dev->udev, usb_path, sizeof(usb_path));
+	pdata->irqfwnode = irq_domain_alloc_named_fwnode(usb_path);
+	if (!pdata->irqfwnode) {
+		ret = -ENOMEM;
+		goto free_pdata;
+	}
+
+	pdata->irqdomain = irq_domain_create_linear(pdata->irqfwnode,
+						    SMSC95XX_NR_IRQS,
+						    &irq_domain_simple_ops,
+						    pdata);
+	if (!pdata->irqdomain) {
+		ret = -ENOMEM;
+		goto free_irqfwnode;
+	}
+
+	phy_irq = irq_create_mapping(pdata->irqdomain, PHY_HWIRQ);
+	if (!phy_irq) {
+		ret = -ENOENT;
+		goto remove_irqdomain;
+	}
+
+	pdata->irqchip = dummy_irq_chip;
+	pdata->irqchip.name = SMSC_CHIPNAME;
+	irq_set_chip_and_handler_name(phy_irq, &pdata->irqchip,
+				      handle_simple_irq, "phy");
+
 	pdata->mdiobus = mdiobus_alloc();
 	if (!pdata->mdiobus) {
 		ret = -ENOMEM;
-		goto free_pdata;
+		goto dispose_irq;
 	}
 
 	ret = smsc95xx_read_reg(dev, HW_CFG, &val);
@@ -1157,6 +1205,7 @@  static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 		goto unregister_mdio;
 	}
 
+	pdata->phydev->irq = phy_irq;
 	pdata->phydev->is_internal = is_internal_phy;
 
 	/* detect device revision as different features may be available */
@@ -1199,6 +1248,15 @@  static int smsc95xx_bind(struct usbnet *dev, struct usb_interface *intf)
 free_mdio:
 	mdiobus_free(pdata->mdiobus);
 
+dispose_irq:
+	irq_dispose_mapping(phy_irq);
+
+remove_irqdomain:
+	irq_domain_remove(pdata->irqdomain);
+
+free_irqfwnode:
+	irq_domain_free_fwnode(pdata->irqfwnode);
+
 free_pdata:
 	kfree(pdata);
 	return ret;
@@ -1211,6 +1269,9 @@  static void smsc95xx_unbind(struct usbnet *dev, struct usb_interface *intf)
 	phy_disconnect(dev->net->phydev);
 	mdiobus_unregister(pdata->mdiobus);
 	mdiobus_free(pdata->mdiobus);
+	irq_dispose_mapping(irq_find_mapping(pdata->irqdomain, PHY_HWIRQ));
+	irq_domain_remove(pdata->irqdomain);
+	irq_domain_free_fwnode(pdata->irqfwnode);
 	netif_dbg(dev, ifdown, dev->net, "free pdata\n");
 	kfree(pdata);
 }
@@ -1235,29 +1296,6 @@  static u32 smsc_crc(const u8 *buffer, size_t len, int filter)
 	return crc << ((filter % 2) * 16);
 }
 
-static int smsc95xx_enable_phy_wakeup_interrupts(struct usbnet *dev, u16 mask)
-{
-	int ret;
-
-	netdev_dbg(dev->net, "enabling PHY wakeup interrupts\n");
-
-	/* read to clear */
-	ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_SRC);
-	if (ret < 0)
-		return ret;
-
-	/* enable interrupt source */
-	ret = smsc95xx_mdio_read_nopm(dev, PHY_INT_MASK);
-	if (ret < 0)
-		return ret;
-
-	ret |= mask;
-
-	smsc95xx_mdio_write_nopm(dev, PHY_INT_MASK, ret);
-
-	return 0;
-}
-
 static int smsc95xx_link_ok_nopm(struct usbnet *dev)
 {
 	int ret;
@@ -1424,7 +1462,6 @@  static int smsc95xx_enter_suspend3(struct usbnet *dev)
 static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
 {
 	struct smsc95xx_priv *pdata = dev->driver_priv;
-	int ret;
 
 	if (!netif_running(dev->net)) {
 		/* interface is ifconfig down so fully power down hw */
@@ -1443,27 +1480,10 @@  static int smsc95xx_autosuspend(struct usbnet *dev, u32 link_up)
 		}
 
 		netdev_dbg(dev->net, "autosuspend entering SUSPEND1\n");
-
-		/* enable PHY wakeup events for if cable is attached */
-		ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-			PHY_INT_MASK_ANEG_COMP_);
-		if (ret < 0) {
-			netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-			return ret;
-		}
-
 		netdev_info(dev->net, "entering SUSPEND1 mode\n");
 		return smsc95xx_enter_suspend1(dev);
 	}
 
-	/* enable PHY wakeup events so we remote wakeup if cable is pulled */
-	ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-		PHY_INT_MASK_LINK_DOWN_);
-	if (ret < 0) {
-		netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-		return ret;
-	}
-
 	netdev_dbg(dev->net, "autosuspend entering SUSPEND3\n");
 	return smsc95xx_enter_suspend3(dev);
 }
@@ -1529,13 +1549,6 @@  static int smsc95xx_suspend(struct usb_interface *intf, pm_message_t message)
 	}
 
 	if (pdata->wolopts & WAKE_PHY) {
-		ret = smsc95xx_enable_phy_wakeup_interrupts(dev,
-			(PHY_INT_MASK_ANEG_COMP_ | PHY_INT_MASK_LINK_DOWN_));
-		if (ret < 0) {
-			netdev_warn(dev->net, "error enabling PHY wakeup ints\n");
-			goto done;
-		}
-
 		/* if link is down then configure EDPD and enter SUSPEND1,
 		 * otherwise enter SUSPEND0 below
 		 */
@@ -1769,11 +1782,12 @@  static int smsc95xx_resume(struct usb_interface *intf)
 			return ret;
 	}
 
+	phy_init_hw(pdata->phydev);
+
 	ret = usbnet_resume(intf);
 	if (ret < 0)
 		netdev_warn(dev->net, "usbnet_resume error\n");
 
-	phy_init_hw(pdata->phydev);
 	return ret;
 }