-----Original Message-----
From: Paolo Abeni <pabeni@redhat.com>
[Severity: Medium]
[Finding: main-1-1]
[Sources: opus-5]
Should the changelog mention the other behavioural changes that come with
this fix? The message only describes adding mutual exclusion for
waiting_tx_skb, but the patch also:
The exact interleaving that leaks the skb is also not spelled out. Since
this carries a Fixes: tag and will be picked up for stable, could the
message describe the leaking sequence and the return-code change? The
disable_traffic locking is only mentioned in the v3 changelog below the
cut, which is dropped when the patch is applied.
Updated commit message with more info.
In oa_tc6_exit() the store happens under the lock and disable_irq() only
runs afterwards:
spin_lock_bh(&tc6->tx_skb_lock);
tc6->disable_traffic = true;
spin_unlock_bh(&tc6->tx_skb_lock);
disable_irq(tc6->spi->irq);
Can a level triggered interrupt in that window make oa_tc6_macphy_isr()
observe a stale false, set int_flag and wake the thread, which then takes
the early return above without doing the data chunk transfer that
deasserts INT?
Moving disable_irq call before setting the disable_traffic flag.
quoted
spin_lock_bh(&tc6->tx_skb_lock);
+ if (tc6->waiting_tx_skb) {
+ netif_stop_queue(tc6->netdev);
+ spin_unlock_bh(&tc6->tx_skb_lock);
+ return NETDEV_TX_BUSY;
+ }
[Severity: Low]
[Finding: fable-5-1-5]
[Sources: fable-5, opus-5]
Does moving skb_linearize() in front of the waiting_tx_skb check change the
back-pressure policy?
Previously an xmit that hit the busy case returned NETDEV_TX_BUSY without
touching the skb. Now every xmit linearizes first, so a transient
skb_linearize() -ENOMEM during a busy period frees the frame and returns
NETDEV_TX_OK, where before it would have been requeued and retried.
The kernel-doc above oa_tc6_start_xmit() still says:
* Return: NETDEV_TX_OK if the transmit ethernet frame skb added in the tx_skb_q
* otherwise returns NETDEV_TX_BUSY.
which no longer covers the drop cases. Should that be refreshed too?
Yes. Updating the comment to cover all the return conditions.