linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/2] net: ethernet: mtk-star-emac: fix several issues on rx/tx poll
@ 2025-04-22 13:03 Louis-Alexis Eyraud
  2025-04-22 13:03 ` [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion " Louis-Alexis Eyraud
  2025-04-22 13:03 ` [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised Louis-Alexis Eyraud
  0 siblings, 2 replies; 7+ messages in thread
From: Louis-Alexis Eyraud @ 2025-04-22 13:03 UTC (permalink / raw)
  To: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan
  Cc: kernel, netdev, linux-kernel, linux-arm-kernel, linux-mediatek,
	Louis-Alexis Eyraud

This patchset fixes two issues with the mtk-star-emac driver.

The first patch fixes spin lock recursion issues I've observed on the
Mediatek Genio 350-EVK board using this driver when the Ethernet
functionality is enabled on the board (requires a correct jumper and
DIP switch configuration, as well as enabling the device in the
devicetree).
The issues can be easily reproduced with apt install or ssh commands
especially and with the CONFIG_DEBUG_SPINLOCK parameter, when
one occurs, there is backtrace similar to this:
```
BUG: spinlock recursion on CPU#0, swapper/0/0
 lock: 0xffff00000db9cf20, .magic: dead4ead, .owner: swapper/0/0,
	.owner_cpu: 0
CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
	6.15.0-rc2-next-20250417-00001-gf6a27738686c-dirty #28 PREEMPT
Hardware name: MediaTek MT8365 Open Platform EVK (DT)
Call trace:
 show_stack+0x18/0x24 (C)
 dump_stack_lvl+0x60/0x80
 dump_stack+0x18/0x24
 spin_dump+0x78/0x88
 do_raw_spin_lock+0x11c/0x120
 _raw_spin_lock+0x20/0x2c
 mtk_star_handle_irq+0xc0/0x22c [mtk_star_emac]
 __handle_irq_event_percpu+0x48/0x140
 handle_irq_event+0x4c/0xb0
 handle_fasteoi_irq+0xa0/0x1bc
 handle_irq_desc+0x34/0x58
 generic_handle_domain_irq+0x1c/0x28
 gic_handle_irq+0x4c/0x120
 do_interrupt_handler+0x50/0x84
 el1_interrupt+0x34/0x68
 el1h_64_irq_handler+0x18/0x24
 el1h_64_irq+0x6c/0x70
 regmap_mmio_read32le+0xc/0x20 (P)
 _regmap_bus_reg_read+0x6c/0xac
 _regmap_read+0x60/0xdc
 regmap_read+0x4c/0x80
 mtk_star_rx_poll+0x2f4/0x39c [mtk_star_emac]
 __napi_poll+0x38/0x188
 net_rx_action+0x164/0x2c0
 handle_softirqs+0x100/0x244
 __do_softirq+0x14/0x20
 ____do_softirq+0x10/0x20
 call_on_irq_stack+0x24/0x64
 do_softirq_own_stack+0x1c/0x40
 __irq_exit_rcu+0xd4/0x10c
 irq_exit_rcu+0x10/0x1c
 el1_interrupt+0x38/0x68
 el1h_64_irq_handler+0x18/0x24
 el1h_64_irq+0x6c/0x70
 cpuidle_enter_state+0xac/0x320 (P)
 cpuidle_enter+0x38/0x50
 do_idle+0x1e4/0x260
 cpu_startup_entry+0x34/0x3c
 rest_init+0xdc/0xe0
 console_on_rootfs+0x0/0x6c
 __primary_switched+0x88/0x90
```

The second patch is a cleanup patch to fix a inconsistency in the
mtk_star_rx_poll function between the napi_complete_done api usage and
its description in documentation.

I've tested this patchset on Mediatek Genio 350-EVK board with a kernel
based on linux-next (tag: next-20250422).

Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
---
Louis-Alexis Eyraud (2):
      net: ethernet: mtk-star-emac: fix spinlock recursion issues on rx/tx poll
      net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised

 drivers/net/ethernet/mediatek/mtk_star_emac.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
---
base-commit: 1d2c58af2b22324cc536113e010d1a38d443f888
change-id: 20250418-mtk_star_emac-fix-spinlock-recursion-issue-4d1c9207cb21

Best regards,
-- 
Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>



^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion issues on rx/tx poll
  2025-04-22 13:03 [PATCH 0/2] net: ethernet: mtk-star-emac: fix several issues on rx/tx poll Louis-Alexis Eyraud
@ 2025-04-22 13:03 ` Louis-Alexis Eyraud
  2025-04-22 14:00   ` Maxime Chevallier
  2025-04-22 13:03 ` [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised Louis-Alexis Eyraud
  1 sibling, 1 reply; 7+ messages in thread
From: Louis-Alexis Eyraud @ 2025-04-22 13:03 UTC (permalink / raw)
  To: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan
  Cc: kernel, netdev, linux-kernel, linux-arm-kernel, linux-mediatek,
	Louis-Alexis Eyraud

Use spin_lock_irqsave and spin_unlock_irqrestore instead of spin_lock
and spin_unlock in mtk_star_emac driver to avoid spinlock recursion
occurrence that can happen when enabling the DMA interrupts again in
rx/tx poll.

```
BUG: spinlock recursion on CPU#0, swapper/0/0
 lock: 0xffff00000db9cf20, .magic: dead4ead, .owner: swapper/0/0,
    .owner_cpu: 0
CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
    6.15.0-rc2-next-20250417-00001-gf6a27738686c-dirty #28 PREEMPT
Hardware name: MediaTek MT8365 Open Platform EVK (DT)
Call trace:
 show_stack+0x18/0x24 (C)
 dump_stack_lvl+0x60/0x80
 dump_stack+0x18/0x24
 spin_dump+0x78/0x88
 do_raw_spin_lock+0x11c/0x120
 _raw_spin_lock+0x20/0x2c
 mtk_star_handle_irq+0xc0/0x22c [mtk_star_emac]
 __handle_irq_event_percpu+0x48/0x140
 handle_irq_event+0x4c/0xb0
 handle_fasteoi_irq+0xa0/0x1bc
 handle_irq_desc+0x34/0x58
 generic_handle_domain_irq+0x1c/0x28
 gic_handle_irq+0x4c/0x120
 do_interrupt_handler+0x50/0x84
 el1_interrupt+0x34/0x68
 el1h_64_irq_handler+0x18/0x24
 el1h_64_irq+0x6c/0x70
 regmap_mmio_read32le+0xc/0x20 (P)
 _regmap_bus_reg_read+0x6c/0xac
 _regmap_read+0x60/0xdc
 regmap_read+0x4c/0x80
 mtk_star_rx_poll+0x2f4/0x39c [mtk_star_emac]
 __napi_poll+0x38/0x188
 net_rx_action+0x164/0x2c0
 handle_softirqs+0x100/0x244
 __do_softirq+0x14/0x20
 ____do_softirq+0x10/0x20
 call_on_irq_stack+0x24/0x64
 do_softirq_own_stack+0x1c/0x40
 __irq_exit_rcu+0xd4/0x10c
 irq_exit_rcu+0x10/0x1c
 el1_interrupt+0x38/0x68
 el1h_64_irq_handler+0x18/0x24
 el1h_64_irq+0x6c/0x70
 cpuidle_enter_state+0xac/0x320 (P)
 cpuidle_enter+0x38/0x50
 do_idle+0x1e4/0x260
 cpu_startup_entry+0x34/0x3c
 rest_init+0xdc/0xe0
 console_on_rootfs+0x0/0x6c
 __primary_switched+0x88/0x90
```

Fixes: 0a8bd81fd6aa ("net: ethernet: mtk-star-emac: separate tx/rx handling with two NAPIs")
Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
---
 drivers/net/ethernet/mediatek/mtk_star_emac.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c b/drivers/net/ethernet/mediatek/mtk_star_emac.c
index 76f202d7f05537642ec294811ace2ad4a7eae383..41d6af31027f4d827dbfdfecdb7de44326bb3de1 100644
--- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
+++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
@@ -1163,6 +1163,7 @@ static int mtk_star_tx_poll(struct napi_struct *napi, int budget)
 	struct net_device *ndev = priv->ndev;
 	unsigned int head = ring->head;
 	unsigned int entry = ring->tail;
+	unsigned long flags = 0;
 
 	while (entry != head && count < (MTK_STAR_RING_NUM_DESCS - 1)) {
 		ret = mtk_star_tx_complete_one(priv);
@@ -1182,9 +1183,9 @@ static int mtk_star_tx_poll(struct napi_struct *napi, int budget)
 		netif_wake_queue(ndev);
 
 	if (napi_complete(napi)) {
-		spin_lock(&priv->lock);
+		spin_lock_irqsave(&priv->lock, flags);
 		mtk_star_enable_dma_irq(priv, false, true);
-		spin_unlock(&priv->lock);
+		spin_unlock_irqrestore(&priv->lock, flags);
 	}
 
 	return 0;
@@ -1342,15 +1343,16 @@ static int mtk_star_rx_poll(struct napi_struct *napi, int budget)
 {
 	struct mtk_star_priv *priv;
 	int work_done = 0;
+	unsigned long flags = 0;
 
 	priv = container_of(napi, struct mtk_star_priv, rx_napi);
 
 	work_done = mtk_star_rx(priv, budget);
 	if (work_done < budget) {
 		napi_complete_done(napi, work_done);
-		spin_lock(&priv->lock);
+		spin_lock_irqsave(&priv->lock, flags);
 		mtk_star_enable_dma_irq(priv, true, false);
-		spin_unlock(&priv->lock);
+		spin_unlock_irqrestore(&priv->lock, flags);
 	}
 
 	return work_done;

-- 
2.49.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised
  2025-04-22 13:03 [PATCH 0/2] net: ethernet: mtk-star-emac: fix several issues on rx/tx poll Louis-Alexis Eyraud
  2025-04-22 13:03 ` [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion " Louis-Alexis Eyraud
@ 2025-04-22 13:03 ` Louis-Alexis Eyraud
  2025-04-22 14:07   ` Maxime Chevallier
  1 sibling, 1 reply; 7+ messages in thread
From: Louis-Alexis Eyraud @ 2025-04-22 13:03 UTC (permalink / raw)
  To: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan
  Cc: kernel, netdev, linux-kernel, linux-arm-kernel, linux-mediatek,
	Louis-Alexis Eyraud

In mtk_star_rx_poll function, on event processing completion, the
mtk_star_emac driver calls napi_complete_done but ignores its return
code and enable RX DMA interrupts inconditionally. This return code
gives the info if a device should avoid rearming its interrupts or not,
so fix this behaviour by taking it into account.

Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>
---
 drivers/net/ethernet/mediatek/mtk_star_emac.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c b/drivers/net/ethernet/mediatek/mtk_star_emac.c
index 41d6af31027f4d827dbfdfecdb7de44326bb3de1..b0ffe9b926c3d8dfaea3e11accae6d01e3ea2c4a 100644
--- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
+++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
@@ -1348,8 +1348,7 @@ static int mtk_star_rx_poll(struct napi_struct *napi, int budget)
 	priv = container_of(napi, struct mtk_star_priv, rx_napi);
 
 	work_done = mtk_star_rx(priv, budget);
-	if (work_done < budget) {
-		napi_complete_done(napi, work_done);
+	if (work_done < budget && napi_complete_done(napi, work_done)) {
 		spin_lock_irqsave(&priv->lock, flags);
 		mtk_star_enable_dma_irq(priv, true, false);
 		spin_unlock_irqrestore(&priv->lock, flags);

-- 
2.49.0



^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion issues on rx/tx poll
  2025-04-22 13:03 ` [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion " Louis-Alexis Eyraud
@ 2025-04-22 14:00   ` Maxime Chevallier
  2025-04-23  8:20     ` Louis-Alexis Eyraud
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Chevallier @ 2025-04-22 14:00 UTC (permalink / raw)
  To: Louis-Alexis Eyraud
  Cc: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan, kernel, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

Hi Louis-Alexis :)

On Tue, 22 Apr 2025 15:03:38 +0200
Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com> wrote:

> Use spin_lock_irqsave and spin_unlock_irqrestore instead of spin_lock
> and spin_unlock in mtk_star_emac driver to avoid spinlock recursion
> occurrence that can happen when enabling the DMA interrupts again in
> rx/tx poll.
> 
> ```
> BUG: spinlock recursion on CPU#0, swapper/0/0
>  lock: 0xffff00000db9cf20, .magic: dead4ead, .owner: swapper/0/0,
>     .owner_cpu: 0
> CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
>     6.15.0-rc2-next-20250417-00001-gf6a27738686c-dirty #28 PREEMPT
> Hardware name: MediaTek MT8365 Open Platform EVK (DT)
> Call trace:
>  show_stack+0x18/0x24 (C)
>  dump_stack_lvl+0x60/0x80
>  dump_stack+0x18/0x24
>  spin_dump+0x78/0x88
>  do_raw_spin_lock+0x11c/0x120
>  _raw_spin_lock+0x20/0x2c
>  mtk_star_handle_irq+0xc0/0x22c [mtk_star_emac]
>  __handle_irq_event_percpu+0x48/0x140
>  handle_irq_event+0x4c/0xb0
>  handle_fasteoi_irq+0xa0/0x1bc
>  handle_irq_desc+0x34/0x58
>  generic_handle_domain_irq+0x1c/0x28
>  gic_handle_irq+0x4c/0x120
>  do_interrupt_handler+0x50/0x84
>  el1_interrupt+0x34/0x68
>  el1h_64_irq_handler+0x18/0x24
>  el1h_64_irq+0x6c/0x70
>  regmap_mmio_read32le+0xc/0x20 (P)
>  _regmap_bus_reg_read+0x6c/0xac
>  _regmap_read+0x60/0xdc
>  regmap_read+0x4c/0x80
>  mtk_star_rx_poll+0x2f4/0x39c [mtk_star_emac]
>  __napi_poll+0x38/0x188
>  net_rx_action+0x164/0x2c0
>  handle_softirqs+0x100/0x244
>  __do_softirq+0x14/0x20
>  ____do_softirq+0x10/0x20
>  call_on_irq_stack+0x24/0x64
>  do_softirq_own_stack+0x1c/0x40
>  __irq_exit_rcu+0xd4/0x10c
>  irq_exit_rcu+0x10/0x1c
>  el1_interrupt+0x38/0x68
>  el1h_64_irq_handler+0x18/0x24
>  el1h_64_irq+0x6c/0x70
>  cpuidle_enter_state+0xac/0x320 (P)
>  cpuidle_enter+0x38/0x50
>  do_idle+0x1e4/0x260
>  cpu_startup_entry+0x34/0x3c
>  rest_init+0xdc/0xe0
>  console_on_rootfs+0x0/0x6c
>  __primary_switched+0x88/0x90
> ```
> 
> Fixes: 0a8bd81fd6aa ("net: ethernet: mtk-star-emac: separate tx/rx handling with two NAPIs")
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

As this is a fix, you need to indicate in your subject that you're
targetting the "net" tree, something like :

[PATCH net 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion issues on rx/tx poll

> ---
>  drivers/net/ethernet/mediatek/mtk_star_emac.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> index 76f202d7f05537642ec294811ace2ad4a7eae383..41d6af31027f4d827dbfdfecdb7de44326bb3de1 100644
> --- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
> +++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> @@ -1163,6 +1163,7 @@ static int mtk_star_tx_poll(struct napi_struct *napi, int budget)
>  	struct net_device *ndev = priv->ndev;
>  	unsigned int head = ring->head;
>  	unsigned int entry = ring->tail;
> +	unsigned long flags = 0;

You don't need to init flags to 0

>  
>  	while (entry != head && count < (MTK_STAR_RING_NUM_DESCS - 1)) {
>  		ret = mtk_star_tx_complete_one(priv);
> @@ -1182,9 +1183,9 @@ static int mtk_star_tx_poll(struct napi_struct *napi, int budget)
>  		netif_wake_queue(ndev);
>  
>  	if (napi_complete(napi)) {
> -		spin_lock(&priv->lock);
> +		spin_lock_irqsave(&priv->lock, flags);
>  		mtk_star_enable_dma_irq(priv, false, true);
> -		spin_unlock(&priv->lock);
> +		spin_unlock_irqrestore(&priv->lock, flags);
>  	}
>  
>  	return 0;
> @@ -1342,15 +1343,16 @@ static int mtk_star_rx_poll(struct napi_struct *napi, int budget)
>  {
>  	struct mtk_star_priv *priv;
>  	int work_done = 0;
> +	unsigned long flags = 0;

There's a rule in netdev that definitions must be ordered from longest
to shortest lines (reverse xmas tree, or RCT), so you should have in
the end :

struct mtk_star_priv *priv;
unsigned long flags;
int work_done = 0;

>  
>  	priv = container_of(napi, struct mtk_star_priv, rx_napi);
>  
>  	work_done = mtk_star_rx(priv, budget);
>  	if (work_done < budget) {
>  		napi_complete_done(napi, work_done);
> -		spin_lock(&priv->lock);
> +		spin_lock_irqsave(&priv->lock, flags);
>  		mtk_star_enable_dma_irq(priv, true, false);
> -		spin_unlock(&priv->lock);
> +		spin_unlock_irqrestore(&priv->lock, flags);
>  	}
>  
>  	return work_done;
> 

Besides these small comments, the patch looks correct to me :)

Regards,

Maxime


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised
  2025-04-22 13:03 ` [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised Louis-Alexis Eyraud
@ 2025-04-22 14:07   ` Maxime Chevallier
  2025-04-23  9:00     ` Louis-Alexis Eyraud
  0 siblings, 1 reply; 7+ messages in thread
From: Maxime Chevallier @ 2025-04-22 14:07 UTC (permalink / raw)
  To: Louis-Alexis Eyraud
  Cc: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan, kernel, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

Hi Louis-Alexis,

On Tue, 22 Apr 2025 15:03:39 +0200
Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com> wrote:

> In mtk_star_rx_poll function, on event processing completion, the
> mtk_star_emac driver calls napi_complete_done but ignores its return
> code and enable RX DMA interrupts inconditionally. This return code
> gives the info if a device should avoid rearming its interrupts or not,
> so fix this behaviour by taking it into account.
> 
> Signed-off-by: Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com>

Patch looks correct, however is it fixing a problematic behaviour
you've seen ? I'm asking because it lacks a Fixes: tag, as well as
targetting one of the net/net-next trees :)

Thanks,

Maxime



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion issues on rx/tx poll
  2025-04-22 14:00   ` Maxime Chevallier
@ 2025-04-23  8:20     ` Louis-Alexis Eyraud
  0 siblings, 0 replies; 7+ messages in thread
From: Louis-Alexis Eyraud @ 2025-04-23  8:20 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan, kernel, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

Hi Maxime,

On Tue, 2025-04-22 at 16:00 +0200, Maxime Chevallier wrote:
> Hi Louis-Alexis :)
> 
> On Tue, 22 Apr 2025 15:03:38 +0200
> Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com> wrote:
> 
> > Use spin_lock_irqsave and spin_unlock_irqrestore instead of
> > spin_lock
> > and spin_unlock in mtk_star_emac driver to avoid spinlock recursion
> > occurrence that can happen when enabling the DMA interrupts again
> > in
> > rx/tx poll.
> > 
> > ```
> > BUG: spinlock recursion on CPU#0, swapper/0/0
> >  lock: 0xffff00000db9cf20, .magic: dead4ead, .owner: swapper/0/0,
> >     .owner_cpu: 0
> > CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted
> >     6.15.0-rc2-next-20250417-00001-gf6a27738686c-dirty #28 PREEMPT
> > Hardware name: MediaTek MT8365 Open Platform EVK (DT)
> > Call trace:
> >  show_stack+0x18/0x24 (C)
> >  dump_stack_lvl+0x60/0x80
> >  dump_stack+0x18/0x24
> >  spin_dump+0x78/0x88
> >  do_raw_spin_lock+0x11c/0x120
> >  _raw_spin_lock+0x20/0x2c
> >  mtk_star_handle_irq+0xc0/0x22c [mtk_star_emac]
> >  __handle_irq_event_percpu+0x48/0x140
> >  handle_irq_event+0x4c/0xb0
> >  handle_fasteoi_irq+0xa0/0x1bc
> >  handle_irq_desc+0x34/0x58
> >  generic_handle_domain_irq+0x1c/0x28
> >  gic_handle_irq+0x4c/0x120
> >  do_interrupt_handler+0x50/0x84
> >  el1_interrupt+0x34/0x68
> >  el1h_64_irq_handler+0x18/0x24
> >  el1h_64_irq+0x6c/0x70
> >  regmap_mmio_read32le+0xc/0x20 (P)
> >  _regmap_bus_reg_read+0x6c/0xac
> >  _regmap_read+0x60/0xdc
> >  regmap_read+0x4c/0x80
> >  mtk_star_rx_poll+0x2f4/0x39c [mtk_star_emac]
> >  __napi_poll+0x38/0x188
> >  net_rx_action+0x164/0x2c0
> >  handle_softirqs+0x100/0x244
> >  __do_softirq+0x14/0x20
> >  ____do_softirq+0x10/0x20
> >  call_on_irq_stack+0x24/0x64
> >  do_softirq_own_stack+0x1c/0x40
> >  __irq_exit_rcu+0xd4/0x10c
> >  irq_exit_rcu+0x10/0x1c
> >  el1_interrupt+0x38/0x68
> >  el1h_64_irq_handler+0x18/0x24
> >  el1h_64_irq+0x6c/0x70
> >  cpuidle_enter_state+0xac/0x320 (P)
> >  cpuidle_enter+0x38/0x50
> >  do_idle+0x1e4/0x260
> >  cpu_startup_entry+0x34/0x3c
> >  rest_init+0xdc/0xe0
> >  console_on_rootfs+0x0/0x6c
> >  __primary_switched+0x88/0x90
> > ```
> > 
> > Fixes: 0a8bd81fd6aa ("net: ethernet: mtk-star-emac: separate tx/rx
> > handling with two NAPIs")
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud@collabora.com>
> 
> As this is a fix, you need to indicate in your subject that you're
> targetting the "net" tree, something like :
> 
> [PATCH net 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion
> issues on rx/tx poll
> 
> > ---
> >  drivers/net/ethernet/mediatek/mtk_star_emac.c | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mediatek/mtk_star_emac.c
> > b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> > index
> > 76f202d7f05537642ec294811ace2ad4a7eae383..41d6af31027f4d827dbfdfecd
> > b7de44326bb3de1 100644
> > --- a/drivers/net/ethernet/mediatek/mtk_star_emac.c
> > +++ b/drivers/net/ethernet/mediatek/mtk_star_emac.c
> > @@ -1163,6 +1163,7 @@ static int mtk_star_tx_poll(struct
> > napi_struct *napi, int budget)
> >  	struct net_device *ndev = priv->ndev;
> >  	unsigned int head = ring->head;
> >  	unsigned int entry = ring->tail;
> > +	unsigned long flags = 0;
> 
> You don't need to init flags to 0
> 
> >  
> >  	while (entry != head && count < (MTK_STAR_RING_NUM_DESCS -
> > 1)) {
> >  		ret = mtk_star_tx_complete_one(priv);
> > @@ -1182,9 +1183,9 @@ static int mtk_star_tx_poll(struct
> > napi_struct *napi, int budget)
> >  		netif_wake_queue(ndev);
> >  
> >  	if (napi_complete(napi)) {
> > -		spin_lock(&priv->lock);
> > +		spin_lock_irqsave(&priv->lock, flags);
> >  		mtk_star_enable_dma_irq(priv, false, true);
> > -		spin_unlock(&priv->lock);
> > +		spin_unlock_irqrestore(&priv->lock, flags);
> >  	}
> >  
> >  	return 0;
> > @@ -1342,15 +1343,16 @@ static int mtk_star_rx_poll(struct
> > napi_struct *napi, int budget)
> >  {
> >  	struct mtk_star_priv *priv;
> >  	int work_done = 0;
> > +	unsigned long flags = 0;
> 
> There's a rule in netdev that definitions must be ordered from
> longest
> to shortest lines (reverse xmas tree, or RCT), so you should have in
> the end :
> 
> struct mtk_star_priv *priv;
> unsigned long flags;
> int work_done = 0;
> 
> >  
> >  	priv = container_of(napi, struct mtk_star_priv, rx_napi);
> >  
> >  	work_done = mtk_star_rx(priv, budget);
> >  	if (work_done < budget) {
> >  		napi_complete_done(napi, work_done);
> > -		spin_lock(&priv->lock);
> > +		spin_lock_irqsave(&priv->lock, flags);
> >  		mtk_star_enable_dma_irq(priv, true, false);
> > -		spin_unlock(&priv->lock);
> > +		spin_unlock_irqrestore(&priv->lock, flags);
> >  	}
> >  
> >  	return work_done;
> > 
> 
> Besides these small comments, the patch looks correct to me :)
> 
> Regards,
> 
> Maxime

Thanks for the review, Maxime.
I'll add the missing subject prefix and fix the other issues in the v2
patchset.

Regards,
Louis-Alexis



^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised
  2025-04-22 14:07   ` Maxime Chevallier
@ 2025-04-23  9:00     ` Louis-Alexis Eyraud
  0 siblings, 0 replies; 7+ messages in thread
From: Louis-Alexis Eyraud @ 2025-04-23  9:00 UTC (permalink / raw)
  To: Maxime Chevallier
  Cc: Felix Fietkau, Sean Wang, Lorenzo Bianconi, Andrew Lunn,
	David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
	Matthias Brugger, AngeloGioacchino Del Regno, Biao Huang,
	Yinghua Pan, kernel, netdev, linux-kernel, linux-arm-kernel,
	linux-mediatek

Hi Maxime,

On Tue, 2025-04-22 at 16:07 +0200, Maxime Chevallier wrote:
> Hi Louis-Alexis,
> 
> On Tue, 22 Apr 2025 15:03:39 +0200
> Louis-Alexis Eyraud <louisalexis.eyraud@collabora.com> wrote:
> 
> > In mtk_star_rx_poll function, on event processing completion, the
> > mtk_star_emac driver calls napi_complete_done but ignores its
> > return
> > code and enable RX DMA interrupts inconditionally. This return code
> > gives the info if a device should avoid rearming its interrupts or
> > not,
> > so fix this behaviour by taking it into account.
> > 
> > Signed-off-by: Louis-Alexis Eyraud
> > <louisalexis.eyraud@collabora.com>
> 
> Patch looks correct, however is it fixing a problematic behaviour
> you've seen ? I'm asking because it lacks a Fixes: tag, as well as
> targetting one of the net/net-next trees :)
> 
I found the issue by code reading and checking if the sequence is
correct with my other fix. 
It seemed the right way to do in comparison to mtk_star_tx_poll
function that does check napi_complete return code before rearming
interrupts.

Regarding the Fixes tag and subject prefix, I'll also add those in the
v2.

> Thanks,
> 
> Maxime
> 
Regards,

Louis-Alexis


^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2025-04-23 10:03 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-22 13:03 [PATCH 0/2] net: ethernet: mtk-star-emac: fix several issues on rx/tx poll Louis-Alexis Eyraud
2025-04-22 13:03 ` [PATCH 1/2] net: ethernet: mtk-star-emac: fix spinlock recursion " Louis-Alexis Eyraud
2025-04-22 14:00   ` Maxime Chevallier
2025-04-23  8:20     ` Louis-Alexis Eyraud
2025-04-22 13:03 ` [PATCH 2/2] net: ethernet: mtk-star-emac: rearm interrupts in rx_poll only when advised Louis-Alexis Eyraud
2025-04-22 14:07   ` Maxime Chevallier
2025-04-23  9:00     ` Louis-Alexis Eyraud

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).