Re: [PATCH net-next v2 2/7] net: ethernet: oa_tc6: Allow custom reset settle time values

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 1 · 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).

- [Medium] oa_tc6_init() (drivers/net/ethernet/oa_tc6.c) runs the
  hardware reset sequence without holding the SPI bus.

--

net: ethernet: oa_tc6: Allow custom reset settle time values

This commit adds hwreset_settle_us and softreset_settle_us fields to
struct oa_tc6_quirks. MAC-PHY drivers can use them to set the wait after
deasserting the reset GPIO and the wait after the software reset. The
hardware reset wait stays at 1 ms by default, and there is still no
software reset wait unless a driver sets one.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 364027c39fa4a..549997ba363ad 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -1515,12 +1524,12 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev,
>  					     "failed to get reset gpio\n"));
>  
>  	if (tc6->reset_gpio) {
> -		/* Keep the reset asserted for 10 us and then allow 1 ms of
> -		 * settle time for the crystal oscillator startup.
> +		/* Keep the reset asserted for 10 us and then allow the reset
> +		 * settle time (1 ms by default).
>  		 */
>  		fsleep(10);
>  		gpiod_set_value_cansleep(tc6->reset_gpio, 0);
> -		fsleep(1000);
> +		fsleep(tc6->hwreset_settle_us);

[Severity: Medium]
Should this reset sequence in oa_tc6_init() hold the SPI bus while
the MAC-PHY settles?

The commit message says the ADIN1110 tristates its SDO pin during
reset. For the same chip, adin1110_check_spi() in
drivers/net/ethernet/adi/adin1110.c locks the bus for the whole
reset and settle window:

	if (reset_gpio) {
		/* 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);
		...
		fsleep(90000);

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

Here, reset is asserted by devm_gpiod_get_optional(..., GPIOD_OUT_HIGH),
then deasserted and held for hwreset_settle_us. No spi_bus_lock() or
spi_bus_unlock() surrounds any of these steps.

The last patch in the series, "net: ethernet: adi: adin1110: Add OA
TC6 protocol support for ADIN1110", does this in
adin1110_oa_tc6_probe():

	quirks.hwreset_settle_us = 50000;
	quirks.softreset_settle_us = 10000;

	port_priv->tc6 = oa_tc6_init(priv->spidev, netdev, &quirks);

In that mode adin1110_check_spi() is never called. The bus-locked reset
is replaced by this unlocked sequence, and the window grows from 1 ms
to 50 ms.

Can a transfer by another device on the same spi_controller drive MISO
in that window and make the ADIN1110 latch a strap that differs from
the board configuration? The later software reset in
oa_tc6_sw_reset_macphy() does not appear to re-sample straps.

Whether this happens depends on which strap is on SDO and when the
part samples it. Those are datasheet details I couldn't verify from
the tree. If the protocol strap were flipped, the first OA TC6 transfer
would fail with -EPROTO. adin1110_oa_tc6_probe() would then fail the
probe with -ENODEV, and this would happen only sometimes, depending on
timing.

Later patches in the series don't seem to add a bus lock to this path.
Would wrapping the reset assert, deassert and settle in oa_tc6_init()
with spi_bus_lock() and spi_bus_unlock() be appropriate here?

>  	}
>  
>  	/* Check the PROTE bit status so that we can reset the device */

[ ... ]

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