| Message ID | 20241204133518.581207-3-parthiban.veerasooran@microchip.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Fixes on the OPEN Alliance TC6 10BASE-T1x MAC-PHY support generic lib | expand |
On Wed, 4 Dec 2024 19:05:18 +0530 Parthiban Veerasooran wrote: > @@ -1210,7 +1213,9 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb) > return NETDEV_TX_OK; > } > > + mutex_lock(&tc6->tx_skb_lock); > tc6->waiting_tx_skb = skb; > + mutex_unlock(&tc6->tx_skb_lock); start_xmit runs in BH / softirq context. You can't take sleeping locks. The lock has to be a spin lock. You could possibly try to use the existing spin lock of the tx queue (__netif_tx_lock()) but that may be more challenging to do cleanly from within a library.. Please make sure you test with builds including the kernel/configs/debug.config Kconfigs.
Hi Jakub, On 10/12/24 5:41 am, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Wed, 4 Dec 2024 19:05:18 +0530 Parthiban Veerasooran wrote: >> @@ -1210,7 +1213,9 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb) >> return NETDEV_TX_OK; >> } >> >> + mutex_lock(&tc6->tx_skb_lock); >> tc6->waiting_tx_skb = skb; >> + mutex_unlock(&tc6->tx_skb_lock); > > start_xmit runs in BH / softirq context. You can't take sleeping locks. > The lock has to be a spin lock. You could possibly try to use the > existing spin lock of the tx queue (__netif_tx_lock()) but that may be > more challenging to do cleanly from within a library.. Thanks for the input. Yes, it looks like implementing a spin lock would be a right choice. I will implement it and do the testing as you suggested below and share the feedback. Best regards, Parthiban V > > Please make sure you test with builds including the > kernel/configs/debug.config Kconfigs. > -- > pw-bot: cr
Hi Jakub, On 12/12/24 6:03 pm, Parthiban.Veerasooran@microchip.com wrote: > Hi Jakub, > > On 10/12/24 5:41 am, Jakub Kicinski wrote: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> On Wed, 4 Dec 2024 19:05:18 +0530 Parthiban Veerasooran wrote: >>> @@ -1210,7 +1213,9 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb) >>> return NETDEV_TX_OK; >>> } >>> >>> + mutex_lock(&tc6->tx_skb_lock); >>> tc6->waiting_tx_skb = skb; >>> + mutex_unlock(&tc6->tx_skb_lock); >> >> start_xmit runs in BH / softirq context. You can't take sleeping locks. >> The lock has to be a spin lock. You could possibly try to use the >> existing spin lock of the tx queue (__netif_tx_lock()) but that may be >> more challenging to do cleanly from within a library.. > Thanks for the input. Yes, it looks like implementing a spin lock would > be a right choice. I will implement it and do the testing as you > suggested below and share the feedback. I tried using spin_lock_bh() variants (as the softirq involved) on both start_xmit() and spi_thread() where the critical regions need to be protected and tested by enabling the Kconfigs in the kernel/configs/debug.config. Didn't notice any warnings in the dmesg log. Note: Prior to the above test, purposefully I tried with spin_lock() variants on both the sides to check/simulate for the warnings using Kconfigs kernel/configs/debug.config. Got some warnings in the dmesg regarding deadlock which clarified the expected behavior. And then I proceeded with the above fix and it worked as expected. If you agree, I will prepare the next version with this fix and post. Best regards, Parthiban V > > Best regards, > Parthiban V >> >> Please make sure you test with builds including the >> kernel/configs/debug.config Kconfigs. >> -- >> pw-bot: cr >
On Fri, 13 Dec 2024 10:35:03 +0000 Parthiban.Veerasooran@microchip.com wrote: > >> start_xmit runs in BH / softirq context. You can't take sleeping locks. > >> The lock has to be a spin lock. You could possibly try to use the > >> existing spin lock of the tx queue (__netif_tx_lock()) but that may be > >> more challenging to do cleanly from within a library.. > > Thanks for the input. Yes, it looks like implementing a spin lock would > > be a right choice. I will implement it and do the testing as you > > suggested below and share the feedback. > I tried using spin_lock_bh() variants (as the softirq involved) on both > start_xmit() and spi_thread() where the critical regions need to be > protected and tested by enabling the Kconfigs in the > kernel/configs/debug.config. Didn't notice any warnings in the dmesg log. > > Note: Prior to the above test, purposefully I tried with spin_lock() > variants on both the sides to check/simulate for the warnings using > Kconfigs kernel/configs/debug.config. Got some warnings in the dmesg > regarding deadlock which clarified the expected behavior. And then I > proceeded with the above fix and it worked as expected. > > If you agree, I will prepare the next version with this fix and post. Go ahead.
Hi Jakub, On 14/12/24 8:12 am, Jakub Kicinski wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > On Fri, 13 Dec 2024 10:35:03 +0000 Parthiban.Veerasooran@microchip.com > wrote: >>>> start_xmit runs in BH / softirq context. You can't take sleeping locks. >>>> The lock has to be a spin lock. You could possibly try to use the >>>> existing spin lock of the tx queue (__netif_tx_lock()) but that may be >>>> more challenging to do cleanly from within a library.. >>> Thanks for the input. Yes, it looks like implementing a spin lock would >>> be a right choice. I will implement it and do the testing as you >>> suggested below and share the feedback. >> I tried using spin_lock_bh() variants (as the softirq involved) on both >> start_xmit() and spi_thread() where the critical regions need to be >> protected and tested by enabling the Kconfigs in the >> kernel/configs/debug.config. Didn't notice any warnings in the dmesg log. >> >> Note: Prior to the above test, purposefully I tried with spin_lock() >> variants on both the sides to check/simulate for the warnings using >> Kconfigs kernel/configs/debug.config. Got some warnings in the dmesg >> regarding deadlock which clarified the expected behavior. And then I >> proceeded with the above fix and it worked as expected. >> >> If you agree, I will prepare the next version with this fix and post. > > Go ahead. Thanks for your comment. It is already posted for review. https://patchwork.kernel.org/project/netdevbpf/patch/20241213123159.439739-3-parthiban.veerasooran@microchip.com/ Best regards, Parthiban V
diff --git a/drivers/net/ethernet/oa_tc6.c b/drivers/net/ethernet/oa_tc6.c index 4c8b0ca922b7..574bfd4e9356 100644 --- a/drivers/net/ethernet/oa_tc6.c +++ b/drivers/net/ethernet/oa_tc6.c @@ -113,6 +113,7 @@ struct oa_tc6 { struct mii_bus *mdiobus; struct spi_device *spi; struct mutex spi_ctrl_lock; /* Protects spi control transfer */ + struct mutex tx_skb_lock; /* Protects tx skb handling */ void *spi_ctrl_tx_buf; void *spi_ctrl_rx_buf; void *spi_data_tx_buf; @@ -1004,8 +1005,10 @@ static u16 oa_tc6_prepare_spi_tx_buf_for_tx_skbs(struct oa_tc6 *tc6) for (used_tx_credits = 0; used_tx_credits < tc6->tx_credits; used_tx_credits++) { if (!tc6->ongoing_tx_skb) { + mutex_lock(&tc6->tx_skb_lock); tc6->ongoing_tx_skb = tc6->waiting_tx_skb; tc6->waiting_tx_skb = NULL; + mutex_unlock(&tc6->tx_skb_lock); } if (!tc6->ongoing_tx_skb) break; @@ -1210,7 +1213,9 @@ netdev_tx_t oa_tc6_start_xmit(struct oa_tc6 *tc6, struct sk_buff *skb) return NETDEV_TX_OK; } + mutex_lock(&tc6->tx_skb_lock); tc6->waiting_tx_skb = skb; + mutex_unlock(&tc6->tx_skb_lock); /* Wake spi kthread to perform spi transfer */ wake_up_interruptible(&tc6->spi_wq); @@ -1240,6 +1245,7 @@ struct oa_tc6 *oa_tc6_init(struct spi_device *spi, struct net_device *netdev) tc6->netdev = netdev; SET_NETDEV_DEV(netdev, &spi->dev); mutex_init(&tc6->spi_ctrl_lock); + mutex_init(&tc6->tx_skb_lock); /* Set the SPI controller to pump at realtime priority */ tc6->spi->rt = true;
There are two skb pointers to manage tx skb's enqueued from n/w stack. waiting_tx_skb pointer points to the tx skb which needs to be processed and ongoing_tx_skb pointer points to the tx skb which is being processed. SPI thread prepares the tx data chunks from the tx skb pointed by the ongoing_tx_skb pointer. When the tx skb pointed by the ongoing_tx_skb is processed, the tx skb pointed by the waiting_tx_skb is assigned to ongoing_tx_skb and the waiting_tx_skb pointer is assigned with NULL. Whenever there is a new tx skb from n/w stack, it will be assigned to waiting_tx_skb pointer if it is NULL. Enqueuing and processing of a tx skb handled in two different threads. Consider a scenario where the SPI thread processed an ongoing_tx_skb and it moves next tx skb from waiting_tx_skb pointer to ongoing_tx_skb pointer without doing any NULL check. At this time, if the waiting_tx_skb pointer is NULL then ongoing_tx_skb pointer is also assigned with NULL. After that, if a new tx skb is assigned to waiting_tx_skb pointer by the n/w stack and there is a chance to overwrite the tx skb pointer with NULL in the SPI thread. Finally one of the tx skb will be left as unhandled, resulting packet missing and memory leak. - Consider the below scenario where the TXC reported from the previous transfer is 10 and ongoing_tx_skb holds an tx ethernet frame which can be transported in 20 TXCs and waiting_tx_skb is still NULL. tx_credits = 10; /* 21 are filled in the previous transfer */ ongoing_tx_skb = 20; waiting_tx_skb = NULL; /* Still NULL */ - So, (tc6->ongoing_tx_skb || tc6->waiting_tx_skb) becomes true. - After oa_tc6_prepare_spi_tx_buf_for_tx_skbs() ongoing_tx_skb = 10; waiting_tx_skb = NULL; /* Still NULL */ - Perform SPI transfer. - Process SPI rx buffer to get the TXC from footers. - Now let's assume previously filled 21 TXCs are freed so we are good to transport the next remaining 10 tx chunks from ongoing_tx_skb. tx_credits = 21; ongoing_tx_skb = 10; waiting_tx_skb = NULL; - So, (tc6->ongoing_tx_skb || tc6->waiting_tx_skb) becomes true again. - In the oa_tc6_prepare_spi_tx_buf_for_tx_skbs() ongoing_tx_skb = NULL; waiting_tx_skb = NULL; - Now the below bad case might happen, Thread1 (oa_tc6_start_xmit) Thread2 (oa_tc6_spi_thread_handler) --------------------------- ----------------------------------- - if waiting_tx_skb is NULL - if ongoing_tx_skb is NULL - ongoing_tx_skb = waiting_tx_skb - waiting_tx_skb = skb - waiting_tx_skb = NULL ... - ongoing_tx_skb = NULL - if waiting_tx_skb is NULL - waiting_tx_skb = skb To overcome the above issue, protect the moving of tx skb reference from waiting_tx_skb pointer to ongoing_tx_skb pointer and assigning new tx skb to waiting_tx_skb pointer, so that the other thread can't access the waiting_tx_skb pointer until the current thread completes moving the tx skb reference safely. Fixes: 53fbde8ab21e ("net: ethernet: oa_tc6: implement transmit path to transfer tx ethernet frames") Signed-off-by: Parthiban Veerasooran <parthiban.veerasooran@microchip.com> --- drivers/net/ethernet/oa_tc6.c | 6 ++++++ 1 file changed, 6 insertions(+)