Re: [PATCH net-next v2 7/7] net: ethernet: adi: adin1110: Add OA TC6 protocol support for ADIN1110

[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: 1 · Low: 1

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] net: adin1110: NULL netdev_ops dereference in
  adin1110_oa_tc6_probe()
- [Medium] In OA TC6 mode, the ADIN1110 hardware reset no longer goes
  through the driver's own reset sequence in adin1110_check_spi().
- [Low] In adin1110_oa_tc6_probe() (drivers/net/ethernet/adi/adin1110.c),
  every -EPROTO returned by oa_tc6_init() is turned into -ENODEV, and…

--

net: ethernet: adi: adin1110: Add OA TC6 protocol support for ADIN1110

This adds OPEN Alliance TC6 SPI protocol support for the ADIN1110 using
the oa_tc6 framework, selected by the adi,spi-oa-tc6 property. Register
accesses go through new read_reg/write_reg callbacks, so both protocols
share the MAC configuration code. ADIN2111 is rejected in TC6 mode, and
strap/property mismatches are reported with -ENODEV.

> diff --git a/drivers/net/ethernet/adi/adin1110.c b/drivers/net/ethernet/adi/adin1110.c
> index 53fac3dc8be62..3f054565acb0e 100644
> --- a/drivers/net/ethernet/adi/adin1110.c
> +++ b/drivers/net/ethernet/adi/adin1110.c

[ ... ]

> @@ -1115,14 +1230,89 @@ static int adin1110_check_spi(struct adin1110_priv *priv)

[ ... ]

> +static int adin1110_oa_tc6_probe(struct adin1110_priv *priv)
> +{

[ ... ]

> +	quirks.hwreset_settle_us = 50000;
> +	quirks.softreset_settle_us = 10000;
> +
> +	port_priv->tc6 = oa_tc6_init(priv->spidev, netdev, &quirks);

[Severity: High]
Isn't netdev->netdev_ops still NULL at this point?
devm_alloc_etherdev() leaves it unset. It is only assigned to
&adin1110_tc6_netdev_ops near the end of adin1110_oa_tc6_probe(),
after oa_tc6_init() has already returned.

quirks.quirk_flags does not set OA_TC6_BROKEN_PHY, so oa_tc6_init()
attaches the PHY to this netdev while it runs:

  oa_tc6_init()
    oa_tc6_phy_init()
      phy_connect_direct()
        phy_attach_direct()
          phy_link_topo_add_phy()
            netdev_need_ops_lock(dev)

With CONFIG_NET_SHAPER enabled, netdev_need_ops_lock() reads
dev->netdev_ops->net_shaper_ops. That would dereference the NULL
netdev_ops and oops on every probe in TC6 mode.

As far as I can tell, lan865x_probe() had the same ordering problem
with oa_tc6_init(). It now sets netdev_ops before calling into the
framework.

Can the netdev->netdev_ops assignment, and probably ethtool_ops with
it, be moved above oa_tc6_init()? None of the ndo callbacks should
be reachable before devm_register_netdev(), so moving the assignment
up looks safe.

[Severity: Medium]
In TC6 mode, adin1110_probe() branches to adin1110_oa_tc6_probe() before
adin1110_check_spi() runs, so oa_tc6_init() now does the hardware reset.
The existing reset in adin1110_check_spi() holds the SPI bus lock while
the configuration pins are latched:

drivers/net/ethernet/adi/adin1110.c:adin1110_check_spi() {
    ...
		/* MISO pin is used for internal configuration, can't have
		 * anyone else disturbing the SDO line.
		 */
		spi_bus_lock(priv->spidev->controller);

		gpiod_set_value(reset_gpio, 1);
		fsleep(10000);
		gpiod_set_value(reset_gpio, 0);

		/* Need to wait 90 ms before interacting with
		 * the MAC after a HW reset.
		 */
		fsleep(90000);

		spi_bus_unlock(priv->spidev->controller);
    ...
}

The reset in oa_tc6_init() requests the GPIO with GPIOD_OUT_HIGH and
does not lock the bus:

drivers/net/ethernet/oa_tc6.c:oa_tc6_init() {
    ...
	if (tc6->reset_gpio) {
		...
		fsleep(10);
		gpiod_set_value_cansleep(tc6->reset_gpio, 0);
		fsleep(tc6->hwreset_settle_us);
	}
    ...
}

On a shared SPI bus, can a transfer to another device drive MISO/SDO
while the ADIN1110 latches its configuration as it leaves reset? That
configuration includes the SPI_CFG1 protocol selection this path relies
on. If it latches wrong, the first control transfer would fail and probe
would report a strap/DT mismatch on a board that is strapped correctly.

The timings also differ from the generic SPI path. The reset is asserted
for 10 us instead of 10 ms, and the settle time is 50 ms where the
comment in adin1110_check_spi() asks for 90 ms. The oa_tc6 patch earlier
in this series cites a 50 ms datasheet limit. Which value is correct?
Should the comment in adin1110_check_spi() be updated, or should
hwreset_settle_us be 90 ms?

> +	if (PTR_ERR_OR_ZERO(port_priv->tc6) == -EPROTO)
> +		return dev_err_probe(dev, -ENODEV,
> +				     "no OA TC6 reply, check the SPI_CFG1 strap and adi,spi-oa-tc6\n");

[Severity: Low]
The commit message says:

  If the first OA TC6 control transfer gets no valid reply, or the PHY ID
  does not match, probe fails with -ENODEV and a message pointing at the
  strap and the property.

Is this check limited to the first control transfer? In oa_tc6.c,
-EPROTO comes from the reply checkers that every control access uses:

drivers/net/ethernet/oa_tc6.c:oa_tc6_check_ctrl_write_reply() {
    ...
	if (memcmp(tx_buf, rx_buf, size - OA_TC6_CTRL_IGNORED_SIZE))
		return -EPROTO;
    ...
}

drivers/net/ethernet/oa_tc6.c:oa_tc6_check_ctrl_read_reply() {
    ...
	if (*tx_buf != *rx_buf)
		return -EPROTO;
    ...
			if (complement != ~reply)
				return -EPROTO;
    ...
}

oa_tc6_check_ctrl_protection() only succeeds if the chip answered in TC6
mode. After that, oa_tc6_init() still runs these steps:

  oa_tc6_sw_reset_macphy()
  oa_tc6_unmask_macphy_error_interrupts()
  oa_tc6_phy_init()
  oa_tc6_enable_data_transfer()
  oa_tc6_update_buffer_status_from_register()

Each one returns its error unchanged as ERR_PTR(ret). A reply mismatch
in any of them would still be logged here as a strap/property problem,
with -ENODEV.

> +	if (IS_ERR(port_priv->tc6))
> +		return PTR_ERR(port_priv->tc6);
> +
> +	priv->read_reg = adin1110_read_reg_tc6;
> +	priv->write_reg = adin1110_write_reg_tc6;
> +
> +	ret = devm_add_action_or_reset(dev, adin1110_oa_tc6_remove, port_priv->tc6);
> +	if (ret)
> +		return ret;
> +
> +	ret = adin1110_read_reg(priv, ADIN1110_PHY_ID, &val);
> +	if (ret < 0)
> +		return ret;

The PHY ID read is handled differently. An -EPROTO from
adin1110_read_reg_tc6()->oa_tc6_read_register() is returned here as
-EPROTO rather than -ENODEV. Is that difference intended?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261109085952.105331-1-iustin.stolniceanu%40analog.com