MAC-PHY interrupt changed to level triggered interrupt
Subsystems (5)
v5 cover MAC-PHY interrupt changed to level triggered interrupt
According to OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface specification, MAC-PHY interrupt is "active low, level triggered". The specification mentions about the conditions in which the IRQ is asserted and deasserted.
Bug is inadvertently introduced by treating the IRQ in the OA TC6 framework driver and in dt-binding YAML file as edge triggered.
With the changes to use level triggered interrupt, use of threaded irq is more efficient than the current method that has interrupt hander working with work queue.
This change of interrupt handler mechanism exposed couple of race conditions due to the fact that interrupts were not masked on protocol error. And pointers were not initialized with null after skbs are freed.
Changes are done in two files
- OA TC6 framework Ethernet driver
- YAML file for the vendor that already uses OA TC6 framework.
Maintainer for this driver is already informed and aware of these changes. Testing for these changes was done in onsemi's setup and found to be working.
Changes in v5:
- Removed the extraneous FCS that came with the frame before passing to the stack
- Base commit was upadted on few patches to ensure that it is pointing to the correct commit ID.
- Commit messages have been updated to be more descriptive and gives more detail now.
- Couple of race conditions pointed out by AI review is fixed.
- Link to v4: https://lore.kernel.org/r/[email protected]
Changes in v4:
- IRQ handler is changed to interrupt handler + wake up thread to interrupt handler + threaded irq. Threaded irq mechanism is better suited for level triggered interrupt. Because it can keep the interrupt disabled until interrupting conditions are handled by a handler thread.
- SPI data handling function is called again on EAGAIN error code as it indicates RX buffer overflow error, which requires draining the bad data chunks.
- Changed wakeup thread to threaded IRQ
- RX buffer overflow is handled before threaded irq returns
- Link to v3: https://lore.kernel.org/r/[email protected]
---
Selvamani Rajagopal (4):
net: ethernet: oa_tc6: Interrupt is active low, level triggered.
net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
net: ethernet: oa_tc6: Remove FCS size in RX frame
dt-bindings: net: updated interrupt type to be active low, level triggered
.../devicetree/bindings/net/microchip,lan8650.yaml | 2 +- drivers/net/ethernet/oa_tc6.c | 140 +++++++++++++-------- 2 files changed, 89 insertions(+), 53 deletions(-)
---
base-commit: 22e2036479cb77df6281ebbd376ae6c330774790
change-id: 20260531-level-trigger-8cb1a83af034
Best regards,
Signature · 1 line
Selvamani Rajagopal <[email protected]>
PATCH v5 1/4 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. Threaded irq mechanism is best suited for level triggered interrupt as it disables the interrupt until handler is run in thread level, while giving us an ability to have interrupt context handler to signal the threaded irq handler.
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. This will avoid having repeated interrupts, in case the driver couldn't recover from the error condition with the available recovery mechanism.
---
changes in v5:
- Updated the commit message to be more descriptive
- Fixed the race condition between interrupt event and threaded irq to avoid interrupt storm and accessing skb after freed.
changes in v4:
Interrupt handling mechanism changed to threaded irq
changes in v3:
interrupt registered as level triggerred
--- drivers/net/ethernet/oa_tc6.c | 126 +++++++++++++++++++++++++----------------- 1 file changed, 76 insertions(+), 50 deletions(-)
drivers/net/ethernet/oa_tc6.c +76 -50
@@ -7,6 +7,7 @@#include <linux/bitfield.h> #include <linux/iopoll.h> +#include <linux/interrupt.h> #include <linux/mdio.h> #include <linux/phy.h> #include <linux/oa_tc6.h>
@@ -44,6 +45,8 @@#define INT_MASK0_LOSS_OF_FRAME_ERR_MASK BIT(4)#define INT_MASK0_RX_BUFFER_OVERFLOW_ERR_MASK BIT(3)#define INT_MASK0_TX_PROTOCOL_ERR_MASK BIT(0) +#define INT_MASK0_ALL_INTERRUPTS (GENMASK(5, 0) | \ + GENMASK(12, 7))/* PHY Clause 22 registers base address and mask */ #define OA_TC6_PHY_STD_REG_ADDR_BASE 0xFF00
@@ -121,14 +124,13 @@ struct oa_tc6 {struct sk_buff *ongoing_tx_skb;struct sk_buff *waiting_tx_skb;struct sk_buff *rx_skb; -struct task_struct *spi_thread; -wait_queue_head_t spi_wq;u16 tx_skb_offset;u16 spi_data_tx_buf_offset;u16 tx_credits;u8 rx_chunks_available;bool rx_buf_overflow;bool int_flag; +bool disable_traffic; };enum oa_tc6_header_type {
@@ -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 ; + } +} + +static void oa_tc6_free_pending_skbs (struct oa_tc6 *tc6) +{ +oa_tc6_cleanup_ongoing_tx_skb (tc6); +oa_tc6_cleanup_ongoing_rx_skb (tc6); +oa_tc6_cleanup_waiting_tx_skb (tc6); +} + +/* If the failure is at SPI interface level, masking and clearing +* the interrupt of the device won't work. Since SPI interrupt is +* disabled, it should stop the repeated interrupts. +*/ +static void oa_tc6_disable_traffic (struct oa_tc6 *tc6) +{ +u32 regval =INT_MASK0_ALL_INTERRUPTS ; + + tc6->disable_traffic = true; +oa_tc6_free_pending_skbs (tc6); +oa_tc6_write_register (tc6,OA_TC6_REG_INT_MASK0 , regval); +oa_tc6_read_register (tc6,OA_TC6_REG_STATUS0 , ®val); +oa_tc6_write_register (tc6,OA_TC6_REG_STATUS0 , regval); +dev_err (&tc6->spi->dev,"Device interrupt disabled to avoid interrupt storm" ); +} +static int oa_tc6_process_extended_status (struct oa_tc6 *tc6) {u32 value;
@@ -1105,29 +1139,29 @@ static int oa_tc6_try_spi_transfer(struct oa_tc6 *tc6)return 0 ; } -static int oa_tc6_spi_thread_handler (void *data) +static irqreturn_t oa_tc6_macphy_threaded_irq (int irq,void *data) {struct oa_tc6 *tc6 = data; -int ret; - -while (likely (!kthread_should_stop ())) { -/* This kthread will be waken up if there is a tx skb or mac-phy -* interrupt to perform spi transfer with tx chunks. -*/ -wait_event_interruptible (tc6->spi_wq, tc6->int_flag || - (tc6->waiting_tx_skb && - tc6->tx_credits) || -kthread_should_stop ()); - -if (kthread_should_stop ()) -break ; +int ret =0 ; - ret =oa_tc6_try_spi_transfer (tc6); -if (ret) -return ret; +/* It is possible that interrupt woke the thread before it is +* disabled. Until we come up with good recovery mechanism, +* no need to attempt spi transfer, once it fails. Pending skbs +* are already freed. +*/ +if (!tc6->disable_traffic) { +while (tc6->int_flag || + (tc6->waiting_tx_skb && tc6->tx_credits)) { + ret =oa_tc6_try_spi_transfer (tc6); +if (ret) { +disable_irq_nosync (tc6->spi->irq); +oa_tc6_disable_traffic (tc6); +break ; + } + } } -return 0 ; +return IRQ_HANDLED ; }static int oa_tc6_update_buffer_status_from_register (struct oa_tc6 *tc6)
@@ -1161,11 +1195,15 @@ static irqreturn_t oa_tc6_macphy_isr(int irq, void *data)* the previous rx footer. * - extended status event not reported in the previous rx footer. */ - tc6->int_flag = true; -/* Wake spi kthread to perform spi transfer */ -wake_up_interruptible (&tc6->spi_wq); - -return IRQ_HANDLED ; +if (tc6->disable_traffic) +disable_irq_nosync (tc6->spi->irq); +else + tc6->int_flag = true; +/* Wake IRQ thread to perform spi transfer . In case +* disable_traffic is set, threaded irq may run again +* one more time. +*/ +return IRQ_WAKE_THREAD ; }/**
@@ -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) {netif_stop_queue (tc6->netdev);return NETDEV_TX_BUSY ; }
@@ -1217,8 +1255,8 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb) tc6->waiting_tx_skb = skb;spin_unlock_bh (&tc6->tx_skb_lock); -/* Wake spi kthread to perform spi transfer */ -wake_up_interruptible (&tc6->spi_wq); +/* Wake the threaded IRQ to perform spi transfer. */ +irq_wake_thread (tc6->spi->irq, tc6);return NETDEV_TX_OK ; }
@@ -1311,24 +1349,15 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev) goto phy_exit; } -init_waitqueue_head (&tc6->spi_wq); - - tc6->spi_thread =kthread_run (oa_tc6_spi_thread_handler, tc6, -"oa-tc6-spi-thread" ); -if (IS_ERR (tc6->spi_thread)) { -dev_err (&tc6->spi->dev,"Failed to create SPI thread\n" ); - goto phy_exit; - } - -sched_set_fifo (tc6->spi_thread); - - ret =devm_request_irq (&tc6->spi->dev, tc6->spi->irq, oa_tc6_macphy_isr, -IRQF_TRIGGER_FALLING ,dev_name (&tc6->spi->dev), - tc6); + ret =devm_request_threaded_irq (&tc6->spi->dev, tc6->spi->irq, + oa_tc6_macphy_isr, + oa_tc6_macphy_threaded_irq, +IRQF_TRIGGER_LOW |IRQF_ONESHOT , +dev_name (&tc6->spi->dev), tc6);if (ret) {dev_err (&tc6->spi->dev,"Failed to request macphy isr %d\n" , ret); - goto kthread_stop; + goto phy_exit; }/* oa_tc6_sw_reset_macphy() function resets and clears the MAC-PHY reset
@@ -1338,12 +1367,10 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev)* 7.7 and 9.2.8.8 in the OPEN Alliance specification for more details. */ tc6->int_flag = true; -wake_up_interruptible (&tc6->spi_wq); +irq_wake_thread (tc6->spi->irq, tc6);return tc6; -kthread_stop: -kthread_stop (tc6->spi_thread); phy_exit:oa_tc6_phy_exit (tc6);return NULL ;
@@ -1356,11 +1383,10 @@ EXPORT_SYMBOL_GPL(oa_tc6_init);*/ void oa_tc6_exit (struct oa_tc6 *tc6) { + tc6->disable_traffic = true; +disable_irq (tc6->spi->irq);oa_tc6_phy_exit (tc6); -kthread_stop (tc6->spi_thread); -dev_kfree_skb_any (tc6->ongoing_tx_skb); -dev_kfree_skb_any (tc6->waiting_tx_skb); -dev_kfree_skb_any (tc6->rx_skb); +oa_tc6_free_pending_skbs (tc6); }EXPORT_SYMBOL_GPL (oa_tc6_exit);
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
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 <[email protected]>
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.
Quoted 4 lines
drivers/net/ethernet/oa_tc6.c +0
-0
[ ... ]
Quoted 13 lines
drivers/net/ethernet/oa_tc6.c +9
-0@@ -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 ; + } +} +
[ ... ]
drivers/net/ethernet/oa_tc6.c EXPORT_SYMBOL_GPL(oa_tc6_zero_align_receive_frame_enable); · line 1243
Quoted 6 lines
drivers/net/ethernet/oa_tc6.c +1
-1@@ -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.
Quoted 3 lines
drivers/net/ethernet/oa_tc6.c +0
-0netif_stop_queue (tc6->netdev);return NETDEV_TX_BUSY ; }
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
In reply to Selvamani RajagopalHi Selvamani,
I did a quick test by connecting Mikroe LAN8651 Click to a Raspberry Pi 4 and shared the feedback below. Please let me know if you need any further details.
Test case 1: Single LAN8651 instance on RPI4
Setup:
RPI4 #1 + LAN8651 (IP: 192.168.10.101) <--- RPI4 #2 + EVB-LAN8670-USB (IP: 192.168.10.102)
Commands:
iperf3 -s -p 5001 <--- iperf3 -c 192.168.10.101 -u -b 9.4M -i 1 -t 0 -p 5001
Result:
No issues observed.
Test case 2: Two LAN8651 instances on the same RPI4
Setup:
RPI4 #1 + LAN8651 (IP: 192.168.10.101) <--- RPI4 #2 + EVB-LAN8670-USB (IP: 192.168.10.102)
RPI4 #1 + LAN8651 (IP: 192.168.20.101) <--- RPI4 #2 + EVB-LAN8670-USB (IP: 192.168.20.102)
Result:
Initially working fine with continuous "Receive buffer overflow" errors. This is expected, as both USB devices transmit at full speed while RPI4 can handle the traffic only up to a maximum SPI frequency of 15 MHz. Eventually, the system crashed. The crash message was captured in the dmesg log,
[ 8276.676335] net_ratelimit: 2448 callbacks suppressed [ 8276.676341] eth1: Receive buffer overflow error [ 8276.676349] eth2: Receive buffer overflow error [ 8276.680025] eth2: Receive buffer overflow error [ 8276.680033] eth1: Receive buffer overflow error [ 8276.683701] eth2: Receive buffer overflow error [ 8276.683710] eth1: Receive buffer overflow error [ 8276.687378] eth2: Receive buffer overflow error [ 8276.687387] eth1: Receive buffer overflow error [ 8276.691055] eth2: Receive buffer overflow error [ 8276.691064] eth1: Receive buffer overflow error [ 8281.662600] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074 [ 8281.670936] Mem abort info: [ 8281.673747] ESR = 0x0000000096000005 [ 8281.677544] EC = 0x25: DABT (current EL), IL = 32 bits [ 8281.682917] SET = 0, FnV = 0 [ 8281.685997] EA = 0, S1PTW = 0 [ 8281.689173] FSC = 0x05: level 1 translation fault [ 8281.694109] Data abort info: [ 8281.697017] ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 [ 8281.702571] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 8281.707680] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 8281.713056] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000040a1d000 [ 8281.719578] [0000000000000074] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000 [ 8281.728391] Internal error: Oops: 0000000096000005 [#1] SMP [ 8281.734115] Modules linked in: sch_fq lan865x_t1s(O) microchip_t1s(O)
snd_seq_dummy snd_hrtimer snd_seq snd_seq_device rfcomm algif_hash aes_neon_bs algif_skcipher af_alg bnep binfmt_misc brcmfmac_cyw brcmfmac vc4 hci_uart brcmutil btbcm bluetooth v3d snd_soc_hdmi_codec bcm2835_isp cfg80211 rpi_hevc_dec bcm2835_codec(C) drm_exec bcm2835_v4l2(C) drm_display_helper ecdh_generic cec ecc bcm2835_mmal_vchiq gpu_sched videobuf2_vmalloc vc_sm_cma v4l2_mem2mem drm_dma_helper rfkill crc_ccitt drm_client_lib drm_shmem_helper videobuf2_dma_contig videobuf2_memops drm_kms_helper videobuf2_v4l2 snd_soc_core videodev raspberrypi_hwmon snd_bcm2835(C) snd_compress i2c_brcmstb snd_pcm_dmaengine snd_pcm videobuf2_common snd_timer mc raspberrypi_gpiomem spi_bcm2835 snd gpio_fan nvmem_rmem sch_fq_codel i2c_dev zram lz4_compress drm fuse drm_panel_orientation_quirks backlight nfnetlink
[ 8281.811847] CPU: 3 UID: 0 PID: 1759 Comm: irq/59-spi0.0 Tainted: G
C O 7.1.0-rc7-v8+ #1 PREEMPT
[ 8281.822067] Tainted: [C]=CRAP, [O]=OOT_MODULE
[ 8281.826473] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 8281.832377] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 8281.839427] pc : skb_put+0x14/0x80
[ 8281.842864] lr : oa_tc6_macphy_threaded_irq+0x428/0x880 [lan865x_t1s]
[ 8281.849386] sp : ffffffc083c4bd40
[ 8281.852735] x29: ffffffc083c4bd40 x28: 000000002020003e x27:
ffffffe59e5609c8
[ 8281.859962] x26: ffffff8103d55080 x25: 0000000000000001 x24:
ffffff8040566880
[ 8281.867187] x23: 0000000000000001 x22: 0000000000000000 x21:
0000000000000000
[ 8281.874414] x20: 000000003e002020 x19: ffffff80405668a0 x18:
00000000000b6748
[ 8281.881641] x17: ffffff9b64502000 x16: ffffffe59f08cef0 x15:
1ae8add2c0a08935
[ 8281.888867] x14: b154d86008ee08e7 x13: b66a1ae8add2c0a0 x12:
8935b154d86008ee
[ 8281.896093] x11: 00000000000000c0 x10: 0000000000001ae0 x9 :
ffffffe590bb7918
[ 8281.903320] x8 : ffffff80493c1b40 x7 : 0000000000000004 x6 :
ffffffffffffffff
[ 8281.910547] x5 : ffffffe59fb9d000 x4 : 0000000000000004 x3 :
0000000000000000
[ 8281.917773] x2 : 0000000000000000 x1 : 0000000000000040 x0 :
0000000000000000
[ 8281.925000] Call trace:
[ 8281.927468] skb_put+0x14/0x80 (P)
[ 8281.930905] oa_tc6_macphy_threaded_irq+0x428/0x880 [lan865x_t1s]
[ 8281.937073] irq_thread_fn+0x34/0xc0
[ 8281.940686] irq_thread+0x1a8/0x308
[ 8281.944212] kthread+0x138/0x150
[ 8281.947472] ret_from_fork+0x10/0x20
[ 8281.951089] Code: d503201f d503233f a9bf7bfd 910003fd (b9407406)
[ 8281.957258] ---[ end trace 0000000000000000 ]---
[ 8281.961969] genirq: exiting task "irq/59-spi0.0" (1759) is an active
IRQ thread (irq 59)
[ 8282.080140] irq 59: nobody cared (try booting with the "irqpoll" option)
[ 8282.086344] CPU: 0 UID: 0 PID: 15 Comm: rcu_preempt Tainted: G D
C O 7.1.0-rc7-v8+ #1 PREEMPT
[ 8282.086352] Tainted: [D]=DIE, [C]=CRAP, [O]=OOT_MODULE
[ 8282.086354] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 8282.086357] Call trace:
[ 8282.086359] show_stack+0x20/0x38 (C)
[ 8282.086372] dump_stack_lvl+0x60/0x80
[ 8282.086378] dump_stack+0x18/0x24
[ 8282.086382] __report_bad_irq+0x54/0xf0
[ 8282.086388] note_interrupt+0x344/0x398
[ 8282.086393] handle_irq_event+0xa4/0x110
[ 8282.086397] handle_level_irq+0xe0/0x178
[ 8282.086401] handle_irq_desc+0x3c/0x68
[ 8282.086407] generic_handle_domain_irq+0x20/0x40
[ 8282.086413] bcm2835_gpio_irq_handle_bank+0x180/0x1c8
[ 8282.086420] bcm2835_gpio_irq_handler+0x88/0x188
[ 8282.086424] handle_irq_desc+0x3c/0x68
[ 8282.086430] generic_handle_domain_irq+0x20/0x40
[ 8282.086435] gic_handle_irq+0x4c/0xe0
[ 8282.086438] call_on_irq_stack+0x30/0x88
[ 8282.086444] do_interrupt_handler+0x88/0x98
[ 8282.086447] el1_interrupt+0x3c/0x60
[ 8282.086452] el1h_64_irq_handler+0x18/0x30
[ 8282.086457] el1h_64_irq+0x6c/0x70
[ 8282.086460] _raw_spin_unlock_irq+0x10/0x60 (P)
[ 8282.086465] rcu_gp_kthread+0x2f0/0x310
[ 8282.086471] kthread+0x138/0x150
[ 8282.086476] ret_from_fork+0x10/0x20
[ 8282.086481] handlers:
[ 8282.170193] lan8650 spi0.1: SPI transfer timed out
[ 8282.170591] [<0000000094f492f8>] oa_tc6_macphy_isr [lan865x_t1s]
[ 8282.174845] spi_master spi0: failed to transfer one message from queue
[ 8282.178435] threaded [<000000008b769ba3>] oa_tc6_macphy_threaded_irq
[lan865x_t1s]
[ 8282.178443] spi_master spi0: noqueue transfer failed[ 8282.182578] Disabling IRQ #59 [ 8282.238458] lan8650 spi0.1 eth2: SPI data transfer failed: -110 [ 8282.244490] lan8650 spi0.1: Device interrupt disabled to avoid
interrupt storm
Best regards,
Parthiban V
Quoted 26 lines · On 12/06/26 3:25 am, Selvamani Rajagopal via B4 Relay wrote:
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
From: Selvamani Rajagopal <[email protected]>
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. Threaded irq mechanism is best suited for level triggered interrupt as it disables the interrupt until handler is run in thread level, while giving us an ability to have interrupt context handler to signal the threaded irq handler.
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. This will avoid having repeated interrupts, in case the driver couldn't recover from the error condition with the available recovery mechanism.
Fixes 2c6ce5354453 ("net: ethernet: oa_tc6: implement mac-phy interrupt")Signed-off-by Selvamani Rajagopal <[email protected]>
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Quoted 9 lines
Subject: Re: [PATCH net v5 1/4] net: ethernet:
oa_tc6: Interrupt is active low, level triggered.Hi Selvamani,
I did a quick test by connecting Mikroe LAN8651 Click to a Raspberry Pi 4 and shared the feedback below. Please let me know if you need any further details.
Parthiban,
Thanks for testing this.
Though the NULL pointer reference after skb_put is a clue, I am working with our team to see we can see this crash in our setup. Will keep you updated.
Quoted 6 lines
[ 8276.691064] eth1: Receive buffer overflow error [ 8281.662600] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074> drm_panel_orientation_quirks backlight nfnetlink [ 8281.839427] pc : skb_put+0x14/0x80 [ 8281.842864] lr : oa_tc6_macphy_threaded_irq+0x428/0x880 [lan865x_t1s]
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Hi Selvamani,
Quoted 12 lines · Selvamani Rajagopal
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Quoted 9 lines
Subject: Re: [PATCH net v5 1/4] net: ethernet:
oa_tc6: Interrupt is active low, level triggered.Hi Selvamani,
I did a quick test by connecting Mikroe LAN8651 Click to a Raspberry Pi 4 and shared the feedback below. Please let me know if you need any further details.
Quoted 6 lines
Parthiban,
Thanks for testing this.
Though the NULL pointer reference after
skb_putis a clue, I am working with our team to see we can see this crash in our setup. Will keep you updated.
Sure, thank you.
Best regards,
Parthiban V
Quoted 8 lines
Quoted 7 lines
[ 8276.691064] eth1: Receive buffer overflow error [ 8281.662600] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000074> drm_panel_orientation_quirks backlight nfnetlink [ 8281.839427] pc : skb_put+0x14/0x80 [ 8281.842864] lr : oa_tc6_macphy_threaded_irq+0x428/0x880 [lan865x_t1s]
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
In reply to [email protected]Quoted 7 lines
Subject: Re: [PATCH net v5 1/4] net: ethernet:
oa_tc6: Interrupt is active low, level triggered.Test case 2: Two LAN8651 instances on the same RPI4
Quoted 9 lines
Setup:
RPI4 #1 + LAN8651 (IP: 192.168.10.101) <--- RPI4 #2 + EVB-LAN8670-USB (IP: 192.168.10.102)
RPI4 #1 + LAN8651 (IP: 192.168.20.101) <--- RPI4 #2 + EVB-LAN8670-USB (IP: 192.168.20.102)Result:
Parthiban,
It appears that we can't reproduce the crash you saw in your setup. Code has been running all day with 5+ millions of "™Receive buffer overflow error" (Yes. I added a counter to see how many times, code returns EAGAIN error code)
One obvious reason is that our EVB has only one network interface. Just like your setup in Test case 1, where you didn't see any issue.
AI review bot Sashiko suggested one potential issue where skb pointers aren't protected. But those concerns are in transmit path. This crash seems to be in receive path. If you think that might help, I can generate a patch for that.
What do you suggest? Since you are able to see the crash, would you have time to investigate?
Sincerely
Selva
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Quoted 6 lines
AI review bot Sashiko suggested one potential issue where skb pointers aren't protected. But those
concerns are in transmit path. This crash seems to be in receive path. If you think that might help,
I can generate a patch for that.
Parthiban,
I just submitted a patch for "net" tree. I was able to see one crash though. Crash signature was different from yours. As I remember, yours is NULL pointer access. Mine was due to trying to place the data beyond the "end" point.
Anyway, if you have time to spare and want to try and see if it fixes your crash, I would appreciate the feedback..
https://patchwork.kernel.org/project/netdevbpf/list/?series=1114495
Quoted 6 lines
What do you suggest? Since you are able to see the crash, would you have time to investigate?
Sincerely
Selva
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Hi Selvamani,
Quoted 12 lines · Selvamani Rajagopal
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Quoted 6 lines
AI review bot Sashiko suggested one potential issue where skb pointers aren't protected. But those
concerns are in transmit path. This crash seems to be in receive path. If you think that might help,
I can generate a patch for that.Parthiban,
Quoted 8 lines
I just submitted a patch for "net" tree. I was able to see one crash though. Crash signature was different from yours. As I remember, yours is NULL pointer access. Mine was due to trying to place the data beyond the "end" point.
Anyway, if you have time to spare and want to try and see if it fixes your crash, I would appreciate the feedback..
https://patchwork.kernel.org/project/netdevbpf/list/?series=1114495
Thank you for the update, and I appreciate your efforts.
I will find some time this week to test and share my feedback. In the meantime, would it be possible for you to test using two instances (Test Case 2)? I did not encounter many issues when testing with a single instance.
I believe that testing with two instances increases the likelihood of reproducing the issue in your setup as well.
Best regards,
Parthiban V
Quoted 7 lines
Quoted 7 lines
What do you suggest? Since you are able to see the crash, would you have time to investigate?
Sincerely
Selva
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
In reply to Selvamani RajagopalHi Selvamani,
Quoted 12 lines · Selvamani Rajagopal
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Quoted 6 lines
AI review bot Sashiko suggested one potential issue where skb pointers aren't protected. But those
concerns are in transmit path. This crash seems to be in receive path. If you think that might help,
I can generate a patch for that.Parthiban,
Quoted 8 lines
I just submitted a patch for "net" tree. I was able to see one crash though. Crash signature was different from yours. As I remember, yours is NULL pointer access. Mine was due to trying to place the data beyond the "end" point.
Anyway, if you have time to spare and want to try and see if it fixes your crash, I would appreciate the feedback..
https://patchwork.kernel.org/project/netdevbpf/list/?series=1114495
Thank you for the update, and I appreciate your efforts.
I will find some time this week to test and share my feedback. In the meantime, would it be possible for you to test using two instances (Test Case 2)? I did not encounter many issues when testing with a single instance.
I believe that testing with two instances increases the likelihood of reproducing the issue in your setup as well.
Best regards,
Parthiban V
Quoted 7 lines
Quoted 7 lines
What do you suggest? Since you are able to see the crash, would you have time to investigate?
Sincerely
Selva
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Quoted 7 lines
-----Original Message-----
From: [email protected] <[email protected]>
Subject: Re: [PATCH net v5 1/4] net: ethernet:oa_tc6: Interrupt is active low, level triggered.
Quoted 7 lines
I will find some time this week to test and share my feedback. In the meantime, would it be possible for you to test using two instances (Test Case 2)? I did not encounter many issues when testing with a single instance.
I believe that testing with two instances increases the likelihood of reproducing the issue in your setup as well.
Parthiban,
Thanks.
Our EVB design allows only one board to be connected to one Raspberry Pi.
So, I don't think I can have a setup like yours. We did test with three Raspberry Pi boards with multi-drop connection. Couldn't see your "NULL pointer" crash. Will keep trying though.
But I could see assert in skb_put immediately quickly.
Quoted 5 lines
Best regards,
Parthiban VQuoted 2 lines
Quoted 2 lines
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Hi Selvamani,
Quoted 16 lines · Selvamani Rajagopal
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Quoted 6 lines
-----Original Message-----
From: [email protected] <[email protected]>
Subject: Re: [PATCH net v5 1/4] net: ethernet:oa_tc6: Interrupt is active low, level triggered.Quoted 7 lines
I will find some time this week to test and share my feedback. In the meantime, would it be possible for you to test using two instances (Test Case 2)? I did not encounter many issues when testing with a single instance.
I believe that testing with two instances increases the likelihood of reproducing the issue in your setup as well.
Quoted 7 lines
Parthiban,
Thanks.
Our EVB design allows only one board to be connected to one Raspberry Pi.
So, I don't think I can have a setup like yours. We did test with three Raspberry Pi boards with multi-drop connection. Couldn't see your "NULL pointer" crash. Will keep trying though.
Thank you for the update. So it seems you can't connect two of your MAC-PHYs in one RPI 4? RPI 4 can support two SPI devices (MAC-PHYs).
https://patchwork.kernel.org/project/netdevbpf/list/?series=1114495&state=%2A&archive=both
With your above patches, I did a quick test (Test case 2) with two Microchip MAC-PHYs and faced a similar issue reported before. Sharing the dmesg crash log for your reference.
[ 2863.182105] eth1: Receive buffer overflow error [ 2863.199905] eth1: Receive buffer overflow error [ 2867.669312] Unable to handle kernel NULL pointer dereference at virtual address 00000000000000b8 [ 2867.677658] Mem abort info: [ 2867.680474] ESR = 0x0000000096000005 [ 2867.684258] EC = 0x25: DABT (current EL), IL = 32 bits [ 2867.689630] SET = 0, FnV = 0 [ 2867.692717] EA = 0, S1PTW = 0 [ 2867.695888] FSC = 0x05: level 1 translation fault [ 2867.700825] Data abort info: [ 2867.703726] ISV = 0, ISS = 0x00000005, ISS2 = 0x00000000 [ 2867.709303] CM = 0, WnR = 0, TnD = 0, TagAccess = 0 [ 2867.714399] GCS = 0, Overlay = 0, DirtyBit = 0, Xs = 0 [ 2867.719773] user pgtable: 4k pages, 39-bit VAs, pgdp=0000000113c2e000 [ 2867.726296] [00000000000000b8] pgd=0000000000000000, p4d=0000000000000000, pud=0000000000000000 [ 2867.735109] Internal error: Oops: 0000000096000005 [#1] SMP [ 2867.740830] Modules linked in: lan865x_t1s(O) microchip_t1s(O) sch_fq
snd_seq_dummy snd_hrtimer snd_seq snd_seq_device rfcomm algif_hash aes_neon_bs algif_skcipher af_alg bnep binfmt_misc brcmfmac_cyw brcmfmac hci_uart brcmutil btbcm bluetooth vc4 cfg80211 snd_soc_hdmi_codec drm_exec ecdh_generic ecc drm_display_helper cec rfkill bcm2835_codec(C) drm_dma_helper v3d rpi_hevc_dec drm_client_lib bcm2835_v4l2(C) gpu_sched drm_shmem_helper crc_ccitt bcm2835_isp snd_soc_core drm_kms_helper bcm2835_mmal_vchiq v4l2_mem2mem vc_sm_cma videobuf2_vmalloc videobuf2_dma_contig raspberrypi_hwmon videobuf2_memops snd_compress snd_bcm2835(C) videobuf2_v4l2 snd_pcm_dmaengine i2c_brcmstb snd_pcm snd_timer videodev videobuf2_common snd mc raspberrypi_gpiomem spi_bcm2835 gpio_fan nvmem_rmem sch_fq_codel i2c_dev zram lz4_compress drm fuse drm_panel_orientation_quirks backlight nfnetlink [last
unloaded: microchip_t1s(O)]
[ 2867.821558] CPU: 3 UID: 0 PID: 2808 Comm: irq/59-spi0.0 Tainted: G
C O 7.1.0-rc7-v8+ #2 PREEMPT
[ 2867.831779] Tainted: [C]=CRAP, [O]=OOT_MODULE
[ 2867.836183] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 2867.842088] pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS
BTYPE=--)
[ 2867.849138] pc : oa_tc6_update_rx_skb+0x2c/0xa8 [lan865x_t1s]
[ 2867.854955] lr : oa_tc6_macphy_threaded_irq+0x430/0x870 [lan865x_t1s]
[ 2867.861476] sp : ffffffc083dbbd20
[ 2867.864825] x29: ffffffc083dbbd20 x28: 000000003e002020 x27:
ffffffed1cf609c8
[ 2867.872051] x26: 0000000000000000 x25: 0000000000000001 x24:
ffffff8040796480
[ 2867.879277] x23: 000000002020003e x22: 0000000000000000 x21:
0000000000000040
[ 2867.886504] x20: ffffff804a479080 x19: ffffff8040796480 x18:
00000000000982f8
[ 2867.893731] x17: ffffff80482d6500 x16: ffffffed1d87b6b0 x15:
ffffff8041a43c00
[ 2867.900957] x14: 0000000000000016 x13: 0000073d6a5d38dc x12:
00000000001d4ebe
[ 2867.908184] x11: 00000000000000c0 x10: 0000000000001ae0 x9 :
ffffffecc9c959e8
[ 2867.915410] x8 : ffffff804f1e5a40 x7 : 0000000000000002 x6 :
ffffffffffffffff
[ 2867.922636] x5 : ffffffed1e59d000 x4 : 0000000000000002 x3 :
0000000000000000
[ 2867.929863] x2 : 0000000000000040 x1 : ffffff804a479080 x0 :
0000000000000000
[ 2867.937090] Call trace:
[ 2867.939558] oa_tc6_update_rx_skb+0x2c/0xa8 [lan865x_t1s] (P)
[ 2867.945375] oa_tc6_macphy_threaded_irq+0x430/0x870 [lan865x_t1s]
[ 2867.951543] irq_thread_fn+0x34/0xc0
[ 2867.955156] irq_thread+0x1a8/0x308
[ 2867.958680] kthread+0x138/0x150
[ 2867.961942] ret_from_fork+0x10/0x20
[ 2867.965558] Code: aa0103f4 f90013f5 12001c55 f9403800 (29570403)
[ 2867.971727] ---[ end trace 0000000000000000 ]---
[ 2867.976443] genirq: exiting task "irq/59-spi0.0" (2808) is an active
IRQ thread (irq 59)
[ 2868.094789] irq 59: nobody cared (try booting with the "irqpoll" option)
[ 2868.101000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Tainted: G D C
O 7.1.0-rc7-v8+ #2 PREEMPT
[ 2868.101007] Tainted: [D]=DIE, [C]=CRAP, [O]=OOT_MODULE
[ 2868.101009] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[ 2868.101011] Call trace:
[ 2868.101013] show_stack+0x20/0x38 (C)
[ 2868.101027] dump_stack_lvl+0x60/0x80
[ 2868.101033] dump_stack+0x18/0x24
[ 2868.101038] __report_bad_irq+0x54/0xf0
[ 2868.101043] note_interrupt+0x344/0x398
[ 2868.101048] handle_irq_event+0xa4/0x110
[ 2868.101051] handle_level_irq+0xe0/0x178
[ 2868.101056] handle_irq_desc+0x3c/0x68
[ 2868.101061] generic_handle_domain_irq+0x20/0x40
[ 2868.101067] bcm2835_gpio_irq_handle_bank+0x180/0x1c8
[ 2868.101074] bcm2835_gpio_irq_handler+0x88/0x188
[ 2868.101080] handle_irq_desc+0x3c/0x68
[ 2868.101085] generic_handle_domain_irq+0x20/0x40
[ 2868.101091] gic_handle_irq+0x4c/0xe0
[ 2868.101094] call_on_irq_stack+0x30/0x88
[ 2868.101099] do_interrupt_handler+0x88/0x98
[ 2868.101102] el1_interrupt+0x3c/0x60
[ 2868.101108] el1h_64_irq_handler+0x18/0x30
[ 2868.101113] el1h_64_irq+0x6c/0x70
[ 2868.101116] default_idle_call+0x34/0x1a0 (P)
[ 2868.101123] do_idle+0x260/0x2a0
[ 2868.101128] cpu_startup_entry+0x3c/0x50
[ 2868.101132] rest_init+0xe8/0xf0
[ 2868.101137] start_kernel+0x7f4/0x800
[ 2868.101143] __primary_switched+0x88/0x98
[ 2868.101149] handlers:
[ 2868.207750] lan8650 spi0.1: SPI transfer timed out
[ 2868.208070] [<0000000019361c17>] oa_tc6_macphy_isr [lan865x_t1s]
[ 2868.212048] spi_master spi0: failed to transfer one message from queue
[ 2868.215296] threaded [<00000000a4e6f0fa>] oa_tc6_macphy_threaded_irq
[lan865x_t1s]
[ 2868.219005] spi_master spi0: noqueue transfer failed[ 2868.223053] Disabling IRQ #59 [ 2868.260162] lan8650 spi0.1 eth2: SPI data transfer failed: -110 [ 2868.266211] lan8650 spi0.1: Device interrupt disabled to avoid
interrupt storm
Best regards,
Parthiban V
Quoted 9 lines
But I could see assert in
skb_putimmediately quickly.Quoted 5 lines
Best regards,
Parthiban VQuoted 2 lines
Quoted 2 lines
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Quoted 8 lines
-----Original Message-----
From: [email protected] <[email protected]>
Subject: Re: [PATCH net v5 1/4] net: ethernet:oa_tc6: Interrupt is active low, level triggered.With your above patches, I did a quick test (Test case 2) with two Microchip MAC-PHYs and faced a similar issue reported before. Sharing the dmesg crash log for your reference.
Root cause seems to be same. When oa_tc6_update_rx_skb function is called, tc6->rx_skb seems to be NULL, which may mean, controller seems to be not getting start
I have a theory. Look at line #933. We have the following comment. I am sure this could be true for the call to oa_tc6_prcs_rx_frame_end at line #926 or oa_tc6_prcs_ongoing_rx_frame at line #950.
/* After rx buffer overflow error received, there might be a
* possibility of getting an end valid of a previously
* incomplete rx frame along with the new rx frame start valid.
*/
Either we change the following line in the function oa_tc6_update_rx_skb
if ((tc6->rx_skb->tail + length) > tc6->rx_skb->end) {
to
if (tc6->rx_skb == NULL || (tc6->rx_skb->tail + length) > tc6->rx_skb->end) {Or add a check
If (tc6->rx_skb) before calling above mentioned two functions from the callee function.
I could do. But I have no way of verifying this. I am sure it will fix the crash. I would like to confirm whether traffic recovers.
Quoted 5 lines
[ 2863.182105] eth1: Receive buffer overflow error [ 2863.199905] eth1: Receive buffer overflow error [ 2867.669312] Unable to handle kernel NULL pointer dereference atvirtual address
00000000000000b8
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Parthiban,
Let me know if you prefer updating the patchset. I certainly prefer adding a NULL check In oa_tc6_update_rx_skb function.
Quoted 14 lines
Root cause seems to be same. When
oa_tc6_update_rx_skbfunction is called, tc6-Quoted 1 line
rx_skbseems to be NULL, which may mean, controller seems to be not getting start
I have a theory. Look at line #933. We have the following comment. I am sure this could be true
for the call tooa_tc6_prcs_rx_frame_endat line #926 oroa_tc6_prcs_ongoing_rx_frameat line #950.
/* After rx buffer overflow error received, there might be a
* possibility of getting an end valid of a previously
* incomplete rx frame along with the new rx frame start valid. */
[email protected]
maintainerRe: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Hi Selvamani,
Quoted 6 lines · Selvamani Rajagopal
EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
Parthiban,
Let me know if you prefer updating the patchset. I certainly prefer adding a NULL check In
oa_tc6_update_rx_skbfunction.
Sorry for the delayed response. I see you already shared the patches for the fixes. Today I will test the below patch series and share the feedback ASAP.
Best regards,
Parthiban V
Quoted 17 lines
Quoted 16 lines
Root cause seems to be same. When
oa_tc6_update_rx_skbfunction is called, tc6-Quoted 1 line
rx_skbseems to be NULL, which may mean, controller seems to be not getting start
I have a theory. Look at line #933. We have the following comment. I am sure this could be true
for the call tooa_tc6_prcs_rx_frame_endat line #926 oroa_tc6_prcs_ongoing_rx_frameat line #950.
/* After rx buffer overflow error received, there might be a
* possibility of getting an end valid of a previously
* incomplete rx frame along with the new rx frame start valid. */
RE: [PATCH net v5 1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
Quoted 7 lines
-----Original Message-----
From: [email protected] <[email protected]>
Subject: Re: [PATCH net v5 1/4] net: ethernet:oa_tc6: Interrupt is active low, level triggered.
Quoted 5 lines
Sorry for the delayed response. I see you already shared the patches for the fixes. Today I will test the below patch series and share the feedback ASAP.
Parthiban.
No worries. Let us hope this address the NULL pointer reference (and traffic recovers gracefully)
Sincerely
Selva
PATCH v5 2/4 net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
In reply to Selvamani RajagopalAs "dev" pointer in oa_tc6 structure is never initialized, mbiobus->parent was initialized with NULL. This change fixes it by initializing it with device pointer of spi.
---
changes in v5:
Changed Fixes: commit ID to accurately reflect where issue originated
changes in v4:
new patch
--- drivers/net/ethernet/oa_tc6.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/net/ethernet/oa_tc6.c +1 -2
@@ -110,7 +110,6 @@/* Internal structure for MAC-PHY drivers */ struct oa_tc6 { -struct device *dev;struct net_device *netdev;struct phy_device *phydev;struct mii_bus *mdiobus;
@@ -520,7 +519,7 @@ static int oa_tc6_mdiobus_register(struct oa_tc6 *tc6) tc6->mdiobus->read_c45 = oa_tc6_mdiobus_read_c45; tc6->mdiobus->write_c45 = oa_tc6_mdiobus_write_c45; tc6->mdiobus->name ="oa-tc6-mdiobus" ; - tc6->mdiobus->parent = tc6->dev; + tc6->mdiobus->parent = &tc6->spi->dev;snprintf (tc6->mdiobus->id,ARRAY_SIZE (tc6->mdiobus->id),"%s" ,dev_name (&tc6->spi->dev));
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH net v5 2/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing bounds check before skb_put() can lead to kernel panic via skb_over_panic() if an oversized frame is received or infinite chunks are streamed.
- [High] Race condition caused by ad-hoc synchronization flag and lockless SKB access leads to leaked TX socket buffers.
- [High] MDIO read failures are incorrectly treated as successful reads of value 1.
- [High] Unexpected RX chunk from MAC-PHY causes immediate NULL pointer dereference via skb_put.
- [High] A transient -ENOMEM from SKB allocation causes the network device to permanently disable itself.
- [High] Memory leak of tc6->rx_skb when receiving multiple RX start chunks without an end chunk.
--
commit 965184b0f5c3d3772f6dd68237d5d4a9d23389f0
Author: Selvamani Rajagopal <[email protected]>
net: ethernet: oa_tc6: mdiobus->parent initialized with NULL
As dev pointer in oa_tc6 structure is never initialized, mbiobus->parent was initialized with NULL. This change fixes it by initializing it with device pointer of spi.
drivers/net/ethernet/oa_tc6.c line 115
Quoted 12 lines
drivers/net/ethernet/oa_tc6.c +0
-1@@ -110,7 +110,6 @@/* Internal structure for MAC-PHY drivers */ struct oa_tc6 { -struct device *dev;struct net_device *netdev;struct phy_device *phydev;struct mii_bus *mdiobus;
[ ... ]
While reviewing this patch, I noticed a few pre-existing issues in this file that might be worth addressing:
[Severity: High]
This isn't a bug introduced by this patch, but is it possible for skb_put() to exceed the allocated size here?
In oa_tc6_prcs_rx_chunk_payload(), when processing ongoing frames:
oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);
This eventually calls into oa_tc6_update_rx_skb():
memcpy(skb_put(tc6->rx_skb, length), payload, length);
If the MAC-PHY hardware sends continuous chunks without an end_valid flag or sends a frame larger than expected, will this trigger an skb_over_panic() since there is no bounds checking against the SKB's tailroom?
[Severity: High]
This is a pre-existing issue, but is there a race condition with tc6->disable_traffic that could leak the waiting TX SKB?
In oa_tc6_start_xmit(), disable_traffic is read locklessly:
if (tc6->disable_traffic || tc6->waiting_tx_skb) {
netif_stop_queue(tc6->netdev);
return NETDEV_TX_BUSY;
}
...
spin_lock_bh(&tc6->tx_skb_lock);
tc6->waiting_tx_skb = skb;
spin_unlock_bh(&tc6->tx_skb_lock);Concurrently, if an error calls oa_tc6_disable_traffic(), it sets disable_traffic = true and calls oa_tc6_cleanup_waiting_tx_skb() without holding tx_skb_lock:
if (tc6->waiting_tx_skb) {
tc6->netdev->stats.tx_dropped++;
kfree_skb(tc6->waiting_tx_skb);
tc6->waiting_tx_skb = NULL;
}Could the cleanup path observe waiting_tx_skb as NULL before the assignment in start_xmit() completes? If so, start_xmit() would then assign the SKB, but since traffic is disabled and the queue stopped, the SKB would never be transmitted or cleaned up.
[Severity: High]
This isn't a bug introduced by this patch, but does oa_tc6_mdiobus_read() mask read failures?
In oa_tc6_mdiobus_read():
bool ret;
ret = oa_tc6_read_register(tc6, OA_TC6_PHY_STD_REG_ADDR_BASE |
(regnum & OA_TC6_PHY_STD_REG_ADDR_MASK),
®val);
if (ret)
return ret;Since ret is declared as a bool, if oa_tc6_read_register() returns a negative error code like -ENODEV, it evaluates to true (1). Will this cause the MDIO layer to misinterpret a failed register read as a successful read returning the value 1?
[Severity: High]
This is a pre-existing issue, but what happens if a chunk arrives without start_valid set when rx_skb is NULL?
In oa_tc6_prcs_rx_chunk_payload(), if a chunk bypasses the start_valid allocation:
/* Process the chunk with ongoing rx frame data */
oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);This directly invokes oa_tc6_update_rx_skb():
memcpy(skb_put(tc6->rx_skb, length), payload, length);
If tc6->rx_skb is NULL because it was never initialized by a start_valid=1 chunk, will this cause an immediate NULL pointer dereference?
[Severity: High]
This isn't a bug introduced by this patch, but is it intended for a transient memory allocation failure to permanently disable the device?
In oa_tc6_try_spi_transfer(), if oa_tc6_process_spi_data_rx_buf() returns -ENOMEM from an SKB allocation failure:
ret = oa_tc6_process_spi_data_rx_buf(tc6, spi_len);
if (ret) {
if (ret == -EAGAIN)
continue;
oa_tc6_cleanup_ongoing_tx_skb(tc6);
oa_tc6_cleanup_ongoing_rx_skb(tc6);
netdev_err(tc6->netdev, "Device error: %d\n", ret);
return ret;
}The error bubbles up to oa_tc6_macphy_threaded_irq():
ret = oa_tc6_try_spi_transfer(tc6);
if (ret) {
disable_irq_nosync(tc6->spi->irq);
oa_tc6_disable_traffic(tc6);
break;
}Does this mean an -ENOMEM error permanently shuts down the MAC-PHY interrupt and data processing, completely breaking the network interface until the module is reloaded?
[Severity: High]
This is a pre-existing issue, but does this code leak the rx_skb if consecutive start_valid chunks arrive?
In oa_tc6_prcs_rx_chunk_payload():
/* Process the chunk with only rx frame start */
if (start_valid && !end_valid) {
size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
return oa_tc6_prcs_rx_frame_start(tc6, &data[start_byte_offset],
size);
}This function calls oa_tc6_allocate_rx_skb():
ret = oa_tc6_allocate_rx_skb(tc6);
if (ret)
return ret;If the driver is already processing a frame and tc6->rx_skb is already populated, won't receiving another start_valid=1 without an intervening end_valid=1 overwrite the tc6->rx_skb pointer with a new allocation and leak the previous one?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v5 3/4 net: ethernet: oa_tc6: Remove FCS size in RX frame
In reply to Selvamani RajagopalOA TC6 MAC-PHY appends FCS to the incoming frame. It must be removed from the frame before being passed to the stack.
With FCS in the frame, many applications, like ping or any application that uses IP layer may work as they may carry the packet size information in the protocol.
Application like ptp4l, particularly if it uses layer 2 for its communication, it will fail with "bad message" due to the extra 4 bytes added by the presence of FCS.
---
changes in v5
- Removed FCS present at the end of the MAC frame before being passed to the stack.
- new patch
--- drivers/net/ethernet/oa_tc6.c | 11 +++++++++++ 1 file changed, 11 insertions(+)
drivers/net/ethernet/oa_tc6.c +11 -0
@@ -785,6 +785,17 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)static void oa_tc6_submit_rx_skb (struct oa_tc6 *tc6) { +/* MAC-PHY delivers each frame with its Ethernet FCS attached. +* Strip it before handing over to the stack, unless the user +* has asked to keep it via NETIF_F_RXFCS. Keeping the FCS +* in the frame is harmless for IP traffic, but is parsed as +* a (malformed) suffix TLV by PTP, which makes ptp4l reject +* every message with "bad message" error. +*/ +if (!(tc6->netdev->features &NETIF_F_RXFCS ) && + tc6->rx_skb->len >ETH_FCS_LEN ) +skb_trim (tc6->rx_skb, tc6->rx_skb->len -ETH_FCS_LEN ); + tc6->rx_skb->protocol =eth_type_trans (tc6->rx_skb, tc6->netdev); tc6->netdev->stats.rx_packets++; tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH net v5 3/4] net: ethernet: oa_tc6: Remove FCS size in RX frame
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
Pre-existing issues:
- [High] Missing check for tc6->rx_skb leads to a NULL pointer dereference when processing orphaned ongoing or end chunks.
- [High] Missing size bounds check for received chunks allows the MAC-PHY to cause a kernel panic via skb_over_panic().
- [High] Unconditional SKB allocation on RX frame start overwrites existing SKBs.
--
commit 47aca7211ccbe61f3e2557dd3549950a622fcf9d
Author: Selvamani Rajagopal <[email protected]>
net: ethernet: oa_tc6: Remove FCS size in RX frame
OA TC6 MAC-PHY appends FCS to the incoming frame. It must be removed from the frame before being passed to the stack.
Quoted 12 lines
drivers/net/ethernet/oa_tc6.c +4
-0@@ -785,6 +785,17 @@ static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer)static void oa_tc6_submit_rx_skb (struct oa_tc6 *tc6) { +/* MAC-PHY delivers each frame with its Ethernet FCS attached. +* Strip it before handing over to the stack, unless the user +* has asked to keep it via NETIF_F_RXFCS. Keeping the FCS +* in the frame is harmless for IP traffic, but is parsed as
drivers/net/ethernet/oa_tc6.c static int oa_tc6_process_rx_chunk_footer(struct oa_tc6 *tc6, u32 footer) · line 801
Quoted 10 lines
drivers/net/ethernet/oa_tc6.c +7
-0+* a (malformed) suffix TLV by PTP, which makes ptp4l reject +* every message with "bad message" error. +*/ +if (!(tc6->netdev->features &NETIF_F_RXFCS ) && + tc6->rx_skb->len >ETH_FCS_LEN ) +skb_trim (tc6->rx_skb, tc6->rx_skb->len -ETH_FCS_LEN ); + tc6->rx_skb->protocol =eth_type_trans (tc6->rx_skb, tc6->netdev); tc6->netdev->stats.rx_packets++; tc6->netdev->stats.rx_bytes += tc6->rx_skb->len;
While reviewing this patch, I noticed a few pre-existing issues in the same file:
drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload() {
...
/* Process the chunk with only rx frame end */
if (end_valid && !start_valid) {
size = end_byte_offset + 1;
oa_tc6_prcs_rx_frame_end(tc6, data, size);
return 0;
}
...
/* Process the chunk with ongoing rx frame data */
oa_tc6_prcs_ongoing_rx_frame(tc6, data, footer);
}[Severity: High]
This isn't a bug introduced by this patch, but does this lead to a NULL pointer dereference?
If a MAC-PHY sends an ongoing chunk or an end chunk without a preceding start chunk, oa_tc6_prcs_rx_frame_end() or oa_tc6_prcs_ongoing_rx_frame() will be called.
Both functions unconditionally call oa_tc6_update_rx_skb(), which blindly calls skb_put(tc6->rx_skb, length). If tc6->rx_skb is NULL because no start_valid was received, skb_put() will crash the system.
drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_ongoing_rx_frame() {
oa_tc6_update_rx_skb(tc6, payload, OA_TC6_CHUNK_PAYLOAD_SIZE);
}drivers/net/ethernet/oa_tc6.c:oa_tc6_update_rx_skb() {
memcpy(skb_put(tc6->rx_skb, length), payload, length);
}[Severity: High]
This isn't a bug introduced by this patch, but can this cause a kernel panic via skb_over_panic()?
The driver allocates the SKB with a fixed tailroom
(MTU + ETH_HLEN + ETH_FCS_LEN). There is no bounds check to ensure the accumulated length from ongoing chunks does not exceed the allocated tailroom.
An endless stream of chunks without an end_valid bit might cause skb_put() to exceed the tailroom and trigger a bounds check panic.
drivers/net/ethernet/oa_tc6.c:oa_tc6_prcs_rx_chunk_payload() {
...
/* Process the chunk with only rx frame start */
if (start_valid && !end_valid) {
size = OA_TC6_CHUNK_PAYLOAD_SIZE - start_byte_offset;
return oa_tc6_prcs_rx_frame_start(tc6,
&data[start_byte_offset],
size);
}...
}
drivers/net/ethernet/oa_tc6.c:oa_tc6_allocate_rx_skb() {
tc6->rx_skb = netdev_alloc_skb_ip_align(tc6->netdev, tc6->netdev->mtu +
ETH_HLEN + ETH_FCS_LEN);...
}
[Severity: High]
This isn't a bug introduced by this patch, but could this unconditional allocation overwrite and leak existing SKBs?
When an RX chunk with start_valid=1 is received, oa_tc6_allocate_rx_skb() assigns a new SKB to tc6->rx_skb.
If the MAC-PHY sends consecutive chunks with start_valid=1 (or leaves a previous frame incomplete), the prior tc6->rx_skb pointer appears to be overwritten and permanently leaked.
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v5 4/4 dt-bindings: net: updated interrupt type to be active low, level triggered
In reply to Selvamani RajagopalAccording to OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6) specification, interrupt type is active low, level triggered interrupt.
Specification calls for when interrupt level will be asserted and what condition it is de-asserted. By using edge triggered interrupt, there is a potential chance to miss it, particularly if it is asserted when interrupt is disabled.
Level triggered interrupt can't be missed as it gets de-asserted only on interrupt handler taking actions on interrupting conditions.
---
changes in v5
Added better, descriptive commit message
changes in v4
no change
changes in v3
interrupts entry changed to level triggered from edge triggered
--- Documentation/devicetree/bindings/net/microchip,lan8650.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Documentation/devicetree/bindings/net/microchip,lan8650.yaml +1 -1
@@ -67,7 +67,7 @@ examples: pinctrl-names = "default"; pinctrl-0 = <ð0_pins>; interrupt-parent = <&gpio>; - interrupts = <6 IRQ_TYPE_EDGE_FALLING>; + interrupts = <6 IRQ_TYPE_LEVEL_LOW>; local-mac-address = [04 05 06 01 02 03]; spi-max-frequency = <15000000>; };
Signature · 1 line
2.43.0
Re: [PATCH net v5 4/4] dt-bindings: net: updated interrupt type to be active low, level triggered
Quoted 8 lines · Selvamani Rajagopal
According to OPEN Alliance 10BASE-T1x MACPHY Serial Interface (TC6) specification, interrupt type is active low, level triggered interrupt.
Specification calls for when interrupt level will be asserted and what condition it is de-asserted. By using edge triggered interrupt, there is a potential chance to miss it, particularly if it is asserted when interrupt is disabled.
Quoted 6 lines
Level triggered interrupt can't be missed as it gets de-asserted only on interrupt handler taking actions on interrupting conditions.
Fixes ac49b950bea9 ("dt-bindings: net: add Microchip's LAN865X 10BASE-T1S MACPHY")Signed-off-by Selvamani Rajagopal <[email protected]>
Best regards,
Krzysztof
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:
Hello:
This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:
Quoted 9 lines · On Thu, 11 Jun 2026 14:55:37 -0700 you wrote:
According to OPEN Alliance 10BASE-T1x MAC-PHY Serial Interface specification, MAC-PHY interrupt is "active low, level triggered". The specification mentions about the conditions in which the IRQ is asserted and deasserted.
Bug is inadvertently introduced by treating the IRQ in the OA TC6 framework driver and in dt-binding YAML file as edge triggered.
[...]
Here is the summary with links:
- [net,v5,1/4] net: ethernet: oa_tc6: Interrupt is active low, level triggered.
https://git.kernel.org/netdev/net/c/b542d13fab0f - [net,v5,2/4] net: ethernet: oa_tc6: mdiobus->parent initialized with NULL https://git.kernel.org/netdev/net/c/a221d3f7e3f3 - [net,v5,3/4] net: ethernet: oa_tc6: Remove FCS size in RX frame https://git.kernel.org/netdev/net/c/a5a1d11dd372 - [net,v5,4/4] dt-bindings: net: updated interrupt type to be active low, level triggered https://git.kernel.org/netdev/net/c/31e56112e654
You are awesome, thank you!
Signature · 2 lines
Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html