Re: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Lockless access to `tc6->waiting_tx_skb` causes a data race and SKB memory leak on the error path.

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition between `oa_tc6_start_xmit` and `oa_tc6_try_spi_transfer` permanently stalls the network TX queue under normal load.
--

commit 6fa7a5c7acb261bf354f20bcc76ccf99dc36a384
Author: Selvamani Rajagopal <Selvamani.Rajagopal@onsemi.com>

net: ethernet: oa_tc6: Interrupt is active low, level triggered.

According OPEN Alliance 10BASET1x MAC-PHY Serial Interface
specification, interrupt is active low, level triggered.

Code used edge triggered interrupt which has the risk of losing an
interrupt on instances like when interrupt is disabled. Level
triggered interrupt won't be deasserted unless handler runs and
clear the interrupting conditions.

Interrupt handler mechanism is changed to threaded irq from
interrupt handler and kernel thread waiting on work queue.

Introduced a logic to disable the device interrupt on error. Error
could be due in data chunk's header and footer or SPI interface itself.

> diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c
> index 91a906a7918a2..f3ac2875adfb7 100644
> --- a/drivers/net/ethernet/oa_tc6.c
> +++ b/drivers/net/ethernet/oa_tc6.c

[ ... ]

> @@ -669,6 +671,38 @@ static void oa_tc6_cleanup_ongoing_tx_skb(struct oa_tc6 *tc6)
>  	}
>  }
>  
> +static void oa_tc6_cleanup_waiting_tx_skb(struct oa_tc6 *tc6)
> +{
> +	if (tc6->waiting_tx_skb) {
> +		tc6->netdev->stats.tx_dropped++;
> +		kfree_skb(tc6->waiting_tx_skb);
> +		tc6->waiting_tx_skb = NULL;
> +	}
> +}
> +

[ ... ]

> @@ -1202,7 +1240,7 @@ EXPORT_SYMBOL_GPL(oa_tc6_zero_align_receive_frame_enable);
>   */
>  netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb)
>  {
> -	if (tc6->waiting_tx_skb) {
> +	if (tc6->disable_traffic || tc6->waiting_tx_skb) {

[Severity: High]
This is a pre-existing issue, but does this lockless check open a race
window that can permanently stall the network TX queue?

If oa_tc6_start_xmit() sees tc6->waiting_tx_skb as populated, it prepares
to stop the queue. Meanwhile, the SPI thread running
oa_tc6_try_spi_transfer() might transfer the SKB, clear waiting_tx_skb
under lock, and check netif_queue_stopped():

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

If the SPI thread checks netif_queue_stopped() before oa_tc6_start_xmit()
actually calls netif_stop_queue(), it reads false and skips waking the
queue. oa_tc6_start_xmit() then stops the queue, but waiting_tx_skb is
already cleared.

Could this leave the TX queue permanently deadlocked until an external RX
packet triggers an interrupt?

[Severity: Medium]
Could this also lead to a data race and an SKB memory leak on the error
path?

If oa_tc6_start_xmit() observes tc6->disable_traffic as false and proceeds,
and concurrently the IRQ thread hits a fatal SPI error, it will call
oa_tc6_disable_traffic(). This invokes the new lockless cleanup in
oa_tc6_cleanup_waiting_tx_skb().

If the cleanup happens while waiting_tx_skb is still NULL, it does nothing.
Then oa_tc6_start_xmit() continues, acquires the lock, and assigns the new
SKB to waiting_tx_skb.

Since disable_traffic is now true, the IRQ thread will skip processing, and
the SKB will be leaked indefinitely.

Additionally, oa_tc6_cleanup_waiting_tx_skb() locklessly reads, frees, and
clears tc6->waiting_tx_skb, which appears to cause a data race against
these concurrent accesses.

>  		netif_stop_queue(tc6->netdev);
>  		return NETDEV_TX_BUSY;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611-level-trigger-v5-0-4533a9e85ce2@onsemi.com?part=1