Re: [PATCH net-next v3 3/4] microchip: lan865x: enable PHY interrupt via virtual IRQ

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message's final paragraph is factually wrong about how
  collision detection is initialized: "lan865x_revb_fixup_registers[]…
- [Low] Reported concern: enabling OA_TC6_PHY_INT routes PHY IRQ
  mask/unmask through oa_tc6_phy_irq_mask()/unmask()…

Pre-existing issues:
- [High] lan865x_remove()
  (drivers/net/ethernet/microchip/lan865x/lan865x.c:420) calls…

--

microchip: lan865x: enable PHY interrupt via virtual IRQ

This passes the OA_TC6_PHY_INT quirk flag to oa_tc6_init() so the LAN865X
internal PHY interrupt, delivered as STATUS0.PHYINT, is exposed to phylib
as a virtual IRQ.  That lets phylib run the PHY driver's config_intr and
handle_interrupt callbacks instead of polling.

A few questions below, one about the commit message and two about the
code paths this flag now activates.

> lan865x_revb_fixup_registers[] does not program COL_DET_CTRL0
> (0x0087), so CDEN correctness relies on that bit's hardware reset
> default together with lan86xx_handle_interrupt() tracking every
> subsequent PLCA transition via the PSTC interrupt this patch enables.

[Severity: Low]
Is the reliance on the reset default accurate here?  Once this patch makes
the PHY IRQ valid, phy_request_interrupt() calls phy_enable_interrupts(),
which reaches lan86xx_config_intr() in drivers/net/phy/microchip_t1s.c,
and that callback programs COL_DET_CTRL0 from the live PLCA status before
unmasking PSTC:

drivers/net/phy/microchip_t1s.c:lan86xx_config_intr() {
	...
	ret = genphy_c45_plca_get_status(phydev, &plca_st);
	...
	ret = phy_modify_mmd(phydev, MDIO_MMD_VEND2,
			     LAN86XX_REG_COL_DET_CTRL0,
			     COL_DET_CTRL0_ENABLE_BIT_MASK,
			     plca_st.pst ? COL_DET_DISABLE : COL_DET_ENABLE);
	...
}

Would it be more precise to say CDEN is explicitly synchronized by
config_intr when interrupts are enabled, and then tracked by
lan86xx_handle_interrupt() on each PSTC event?  As written the paragraph
may leave readers thinking the absence of 0x0087 from the fixup table is
an open gap.

> diff --git a/drivers/net/ethernet/microchip/lan865x/lan865x.c b/drivers/net/ethernet/microchip/lan865x/lan865x.c
> index 26a2761332a5..043a3dc3f965 100644
> --- a/drivers/net/ethernet/microchip/lan865x/lan865x.c
> +++ b/drivers/net/ethernet/microchip/lan865x/lan865x.c

[ ... ]

> @@ -346,7 +347,8 @@ static int lan865x_probe(struct spi_device *spi)
>  	spi_set_drvdata(spi, priv);
>  	INIT_WORK(&priv->multicast_work, lan865x_multicast_work_handler);
>  
> -	priv->tc6 = oa_tc6_init(spi, netdev, NULL);
> +	tc6_quirks.quirk_flags = OA_TC6_PHY_INT;
> +	priv->tc6 = oa_tc6_init(spi, netdev, &tc6_quirks);

[Severity: Low]
Setting OA_TC6_PHY_INT routes the PHY mask/unmask through the virtual
irq_chip in drivers/net/ethernet/oa_tc6.c.  oa_tc6_phy_irq_unmask() sets
tc6->phy_irq_masked = false immediately, and the actual INT_MASK0
read-modify-write happens later in oa_tc6_phy_irq_bus_sync_unlock(),
where a failure is only logged:

drivers/net/ethernet/oa_tc6.c:oa_tc6_phy_irq_bus_sync_unlock() {
	...
	ret = oa_tc6_write_register(tc6, OA_TC6_REG_INT_MASK0, regval);
	if (ret)
		dev_err(&tc6->spi->dev, "Failed to write INT_MASK0: %d\n", ret);
	...
}

If a transient SPI control transfer failure hits just that access, can
STATUS0.PHYINT stay masked in hardware while the software shadow says
unmasked, so later autonomous PLCA transitions never reach
lan86xx_handle_interrupt() and CDEN goes stale?

For context, irq_bus_sync_unlock() returns void so there is no way to
propagate the error to the IRQ core, and regmap_irq_sync_unlock() logs and
continues in the same way, so this may simply be the accepted behaviour
for slow-bus irq_chips.  Also, if the config_intr sequence itself fails,
phy_request_interrupt() falls back to PHY_POLL and the static CDEN write
in lan86xx_plca_set_cfg() is used again.  Is a retry or a warning on the
shadow/hardware divergence worth it here, or is log-and-continue fine?

>  	if (!priv->tc6) {
>  		ret = -ENODEV;
>  		goto free_netdev;

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the probe
and teardown paths in this file: lan865x_remove() cancels the multicast
work before unregistering the netdev:

drivers/net/ethernet/microchip/lan865x/lan865x.c:lan865x_remove() {
	cancel_work_sync(&priv->multicast_work);
	unregister_netdev(priv->netdev);
	oa_tc6_exit(priv->tc6);
	free_netdev(priv->netdev);
}

ndo_set_rx_mode for this driver does nothing but re-queue that work:

static void lan865x_set_multicast_list(struct net_device *netdev)
{
	struct lan865x_priv *priv = netdev_priv(netdev);

	schedule_work(&priv->multicast_work);
}

Can an rx-mode change (allmulti/promisc change, multicast join) that
already holds rtnl_lock re-queue the work after cancel_work_sync() and
before unregister_netdev() takes rtnl?  There is no second cancel or flush
afterwards, so lan865x_multicast_work_handler() would either issue
oa_tc6_write_register() after oa_tc6_exit(), or still be queued when
free_netdev() frees the netdev private area that priv->multicast_work
lives in.

Would moving unregister_netdev() ahead of cancel_work_sync() close that
window?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260918143302.76909-1-parthiban.veerasooran%40microchip.com