Re: [PATCH net-next v10 06/11] net: ethernet: oa_tc6: Support for vendor specific MMS

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] NULL pointer dereference on an error path in oa_tc6_init()
  (drivers/net/ethernet/oa_tc6/oa_tc6_main.c).
- [Medium] net: oa_tc6: oa_tc6_set_vend1_mms() called after PHY init in
  oa_tc6_init()
- [Medium] net: oa_tc6: NULL deref in oa_tc6_phy_exit() on capability
  read failure

--

net: ethernet: oa_tc6: Support for vendor specific MMS

This adds oa_tc6_set_vend1_mms(), which lets a vendor driver map
MDIO_MMD_VEND1 to one of the vendor specific MMS values (10 to 15)
allowed by the OPEN Alliance TC6 specification. The C45 MMS lookup now
takes the tc6 context. The new vend1_mms field defaults to -EOPNOTSUPP
in oa_tc6_init().

> diff --git a/drivers/net/ethernet/oa_tc6/oa_tc6_main.c b/drivers/net/ethernet/oa_tc6/oa_tc6_main.c
> index e793d7fb0635f..5b2454b759740 100644
> --- a/drivers/net/ethernet/oa_tc6/oa_tc6_main.c
> +++ b/drivers/net/ethernet/oa_tc6/oa_tc6_main.c

[ ... ]

> @@ -1658,6 +1672,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
>  	SET_NETDEV_DEV(netdev, &spi->dev);
>  	mutex_init(&tc6->spi_ctrl_lock);
>  	spin_lock_init(&tc6->tx_skb_lock);
> +	tc6->vend1_mms = -EOPNOTSUPP;
>  	tc6->ttsc_current_id = OA_TC6_TTSCA_REG_ID;
>  	INIT_LIST_HEAD(&tc6->tx_ts_skb_q);
>  

[Severity: Medium]
This patch doesn't touch it, but oa_tc6_init() still has a broken
unwind path. It was introduced earlier in the series by "net:
ethernet: oa_tc6: Support for hardware timestamp".

Can a failed capability read oops on a NULL phydev? In oa_tc6_init(),
oa_tc6_update_standard_capability() is called before oa_tc6_phy_init(),
and on failure it does goto phy_exit.

At that point tc6->phydev and tc6->mdiobus are still NULL from
devm_kzalloc(). oa_tc6_phy_exit() only bails out early when
OA_TC6_BROKEN_PHY is set.

For lan865x and S2500 it goes on to call phy_disconnect(tc6->phydev).
That dereferences the NULL phydev in phy_is_started() straight away.
After that, oa_tc6_mdiobus_unregister() hands a NULL tc6->mdiobus to
mdiobus_unregister() and mdiobus_free().

Any failure of the OA_TC6_REG_STDCAP read during probe gets there,
such as an SPI transfer error or a bad control reply:

lan865x_probe()/s2500_probe()
  oa_tc6_init()
    oa_tc6_update_standard_capability()
      oa_tc6_read_register() fails
    goto phy_exit
      oa_tc6_phy_exit()
        phy_disconnect(NULL)

The neighbouring failures before oa_tc6_phy_init() simply return
ERR_PTR(ret), including oa_tc6_unmask_macphy_error_interrupts().
Should this branch do the same?

[Severity: Medium]
Does this setter run too late to affect PHY probing and
initialization?

struct oa_tc6 is allocated inside oa_tc6_init(), and the caller only
gets the pointer back once oa_tc6_init() returns. By then
oa_tc6_init() has already run oa_tc6_phy_init(), which does:

oa_tc6_phy_init()
  oa_tc6_mdiobus_register()
    mdiobus_register()
      PHY driver .probe() / .get_features()
  phy_connect_direct()
    phy_attach_direct()
      phy_init_hw()
        .soft_reset(), fixups, .config_init(), .config_intr()
      phy_resume()

All of that goes through oa_tc6_mdiobus_read_c45() and
oa_tc6_mdiobus_write_c45(). At that point tc6->vend1_mms is still
the -EOPNOTSUPP default set a few lines earlier in oa_tc6_init().

So any MDIO_MMD_VEND1 access the PHY driver makes during probe, reset
or config_init fails with -EOPNOTSUPP. The mapping only takes effect
for accesses made after the MAC driver calls oa_tc6_set_vend1_mms(),
as s2500_probe() does later in this series.

The late store into tc6->vend1_mms is also done with no lock held,
while the MDIO bus is already registered and the PHY attached.

Would it be better to pass the VEND1 MMS into oa_tc6_init(), or into
some config structure handed to it? Then it would be in place before
oa_tc6_phy_init() runs.

If the S2500 PHY driver is known never to touch VEND1 before the MAC
driver gets control back, could the commit message say so?

[Severity: High]
This patch doesn't touch it, but there is a problem further down in
oa_tc6_init(). The earlier patch in this series, "net: ethernet: oa_tc6:
Support for hardware timestamp", added it, and it is still there at the
tip of the series.

Can the capability read failure path oops with a NULL phydev? In
oa_tc6_init():

	ret = oa_tc6_update_standard_capability(tc6);
	if (ret) {
		dev_err(&tc6->spi->dev, "Failed to read capability\n");
		goto phy_exit;
	}

This runs before oa_tc6_phy_init(), so tc6->phydev and tc6->mdiobus are
still NULL from devm_kzalloc(). The phy_exit label then calls:

oa_tc6_phy_exit() {
	if (tc6->quirk_flags & OA_TC6_BROKEN_PHY)
		return;

	phy_disconnect(tc6->phydev);
	oa_tc6_mdiobus_unregister(tc6);
}

Only adin1140 sets OA_TC6_BROKEN_PHY, so lan865x and S2500 both reach
phy_disconnect(NULL). phy_disconnect() calls phy_is_started(phydev) right
away, which dereferences phydev->state. After that,
oa_tc6_mdiobus_unregister() would also pass a NULL mdiobus to
mdiobus_unregister().

This can trigger whenever the OA_TC6_REG_STDCAP read fails during probe,
for example on an SPI transfer error or when the control read reply
check fails:

lan865x_probe()/s2500_probe()
  oa_tc6_init()
    oa_tc6_update_standard_capability()
      oa_tc6_read_register() fails
    goto phy_exit
      oa_tc6_phy_exit()
        phy_disconnect(NULL)

The nearby failures before oa_tc6_phy_init() (sw reset, unmask error
interrupts, phy init) all use return ERR_PTR(ret). Should this branch do
the same?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261106-s2500-mac-phy-support-v10-0-31b5a69b85b7%40onsemi.com