Re: [PATCH net-next v4 04/13] net: ethernet: oa_tc6: Export the C45 access functions

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Missing forward declaration of `struct mii_bus` in `include/linux/oa_tc6.h`.

Pre-existing issues:
- [High] A Time-Of-Check to Time-Of-Use race in `oa_tc6_start_xmit` can result in a permanently stopped TX queue.
--

commit 8341f219f15c736a6e2b1b018d43e766db074905
Author: Ciprian Regus <ciprian.regus@analog.com>

net: ethernet: oa_tc6: Export the C45 access functions
    
The C45 access functions can still be used by some Ethernet drivers
which set the OA_TC6_BROKEN_PHY flag. Export them.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Ciprian Regus <ciprian.regus@analog.com>

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 2a72f0c4b0095..b37e398e30e37 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a race condition in
oa_tc6_start_xmit() that can result in a permanently stopped TX queue?

In drivers/net/ethernet/oa_tc6.c:oa_tc6_start_xmit(), tc6->waiting_tx_skb
is checked without holding tx_skb_lock:

    if (tc6->waiting_tx_skb) {
        netif_stop_queue(tc6->netdev);
        return NETDEV_TX_BUSY;
    }

Concurrently, the SPI thread can acquire the lock, move waiting_tx_skb
to ongoing_tx_skb, and set waiting_tx_skb to NULL.

After processing, the SPI thread checks if the queue is stopped to wake
it up in oa_tc6_try_spi_transfer():

    if (!tc6->waiting_tx_skb && netif_queue_stopped(tc6->netdev))
        netif_wake_queue(tc6->netdev);

If the SPI thread evaluates netif_queue_stopped() before oa_tc6_start_xmit()
executes netif_stop_queue(), it won't wake it. oa_tc6_start_xmit() will
then stop the queue and return NETDEV_TX_BUSY. 

With waiting_tx_skb being NULL and the queue stopped, will the SPI thread
fail to wake up to process new TX frames, stalling the interface?

> diff --git a/include/linux/oa_tc6.h b/include/linux/oa_tc6.h
> index 62e3d89f80ed0..2660eefa35043 100644
> --- a/include/linux/oa_tc6.h
> +++ b/include/linux/oa_tc6.h
> @@ -31,3 +31,7 @@ int oa_tc6_read_registers(struct oa_tc6 *tc6, u32 address, u32 value[],
>  			  u8 length);
>  netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb);
>  int oa_tc6_zero_align_receive_frame_enable(struct oa_tc6 *tc6);
> +int oa_tc6_mdiobus_read_c45(struct mii_bus *bus, int addr, int devnum,
> +			    int regnum);
> +int oa_tc6_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
> +			     int regnum, u16 val);

[Severity: Low]
Could this cause a compiler warning due to a missing forward declaration
of struct mii_bus?

Including this header without prior MDIO/PHY declarations being in scope
will generate a warning about the struct being declared inside a parameter
list.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609-adin1140-driver-v4-0-0753e28ee004@analog.com?part=4