Re: [PATCH net-next v3 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling

From: Marek Szyprowski <m.szyprowski@samsung.com>
To: Lukas Wunner <lukas@wunner.de>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
	Eric Dumazet <edumazet@google.com>
Cc: netdev@vger.kernel.org, linux-usb@vger.kernel.org,
	Steve Glendinning <steve.glendinning@shawell.net>,
	UNGLinuxDriver@microchip.com, Oliver Neukum <oneukum@suse.com>,
	Andre Edich <andre.edich@microchip.com>,
	Oleksij Rempel <linux@rempel-privat.de>,
	Martyn Welch <martyn.welch@collabora.com>,
	Gabriel Hojda <ghojda@yo2urs.ro>,
	Christoph Fritz <chf.fritz@googlemail.com>,
	Lino Sanfilippo <LinoSanfilippo@gmx.de>,
	Philipp Rosenberger <p.rosenberger@kunbus.com>,
	Heiner Kallweit <hkallweit1@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Russell King <linux@armlinux.org.uk>,
	Ferry Toth <fntoth@gmail.com>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	'Linux Samsung SOC' <linux-samsung-soc@vger.kernel.org>
Subject: Re: [PATCH net-next v3 5/7] usbnet: smsc95xx: Forward PHY interrupts to PHY driver to avoid polling
Date: Tue, 17 May 2022 12:18:45 +0200	[thread overview]
Message-ID: <a5315a8a-32c2-962f-f696-de9a26d30091@samsung.com> (raw)
In-Reply-To: <748ac44eeb97b209f66182f3788d2a49d7bc28fe.1652343655.git.lukas@wunner.de>

Hi Lukas,

On 12.05.2022 10:42, Lukas Wunner wrote:
> 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>

This patch landed in the recent linux next-20220516 as commit 
1ce8b37241ed ("usbnet: smsc95xx: Forward PHY interrupts to PHY driver to 
avoid polling"). Unfortunately it breaks smsc95xx usb ethernet operation 
after system suspend-resume cycle. On the Odroid XU3 board I got the 
following warning in the kernel log:

# time rtcwake -s10 -mmem
rtcwake: wakeup from "mem" using /dev/rtc0 at Tue May 17 09:16:07 2022
PM: suspend entry (deep)
Filesystems sync: 0.001 seconds
Freezing user space processes ... (elapsed 0.002 seconds) done.
OOM killer disabled.
Freezing remaining freezable tasks ... (elapsed 0.001 seconds) done.
printk: Suspending console(s) (use no_console_suspend to debug)
smsc95xx 4-1.1:1.0 eth0: entering SUSPEND2 mode
smsc95xx 4-1.1:1.0 eth0: Failed to read reg index 0x00000114: -113
smsc95xx 4-1.1:1.0 eth0: Error reading MII_ACCESS
smsc95xx 4-1.1:1.0 eth0: __smsc95xx_mdio_read: MII is busy
------------[ cut here ]------------
WARNING: CPU: 2 PID: 73 at drivers/net/phy/phy.c:946 
phy_state_machine+0x98/0x28c
Modules linked in: snd_soc_hdmi_codec snd_soc_odroid governor_passive 
snd_soc_i2s exynos_bus snd_soc_idma snd_soc_s3c_dma exynosdrm 
analogix_dp snd_soc_max98090 snd_soc_core ac97_bus snd_pcm_dmaengine 
snd_pcm clk_s2mps11 rtc_s5m snd_timer snd soundcore ina2xx exynos_gsc 
pwm_samsung exynos_adc s5p_jpeg ohci_exynosv4l2_mem2mem phy_exynos_usb2 
panfrost ehci_exynos s5p_mfc drm_shmem_helper videobuf2_dma_contig 
videobuf2_memops videobuf2_v4l2 videobuf2_common gpu_sched videodev mc 
exynos_ppmu exynos5422_dmc exynos_nocp s5p_sss rtc_s3c exynos_rng 
s3c2410_wdt s5p_cec pwm_fan
CPU: 2 PID: 73 Comm: kworker/2:1 Tainted: G        W 
5.18.0-rc6-01433-g1ce8b37241ed #5040
Hardware name: Samsung Exynos (Flattened Device Tree)
Workqueue: events_power_efficient phy_state_machine
  unwind_backtrace from show_stack+0x10/0x14
  show_stack from dump_stack_lvl+0x40/0x4c
  dump_stack_lvl from __warn+0xc8/0x13c
  __warn from warn_slowpath_fmt+0x5c/0xb4
  warn_slowpath_fmt from phy_state_machine+0x98/0x28c
  phy_state_machine from process_one_work+0x1ec/0x4cc
  process_one_work from worker_thread+0x58/0x54c
  worker_thread from kthread+0xd0/0xec
  kthread from ret_from_fork+0x14/0x2c
Exception stack(0xf0aa9fb0 to 0xf0aa9ff8)
...
---[ end trace 0000000000000000 ]---

It looks that the driver's suspend/resume operations might need some 
adjustments. After the system suspend/resume cycle the driver is not 
operational anymore. Reverting the $subject patch on top of linux 
next-20220516 restores ethernet operation after system suspend/resume.

> ---
> Only change since v2:
>   * Drop call to __irq_enter_raw() which worked around a warning in
>     generic_handle_domain_irq().  That warning is gone since
>     792ea6a074ae (queued on tip.git/irq/urgent).
>     (Marc Zyngier, Thomas Gleixner)
>
>   drivers/net/usb/smsc95xx.c | 113 ++++++++++++++++++++-----------------
>   1 file changed, 61 insertions(+), 52 deletions(-)

 > ...

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland