[OpenWrt-Devel] ipq40xx: essedma: Fix dead lock
diff mbox series

Message ID 1569923958-99413-1-git-send-email-mutsugi@allied-telesis.co.jp
State Accepted, archived
Delegated to: Christian Lamparter
Headers show
Series
  • [OpenWrt-Devel] ipq40xx: essedma: Fix dead lock
Related show

Commit Message

Masafumi UTSUGI Oct. 1, 2019, 9:59 a.m. UTC
edma_read_append_stats() is called from kernel timer
(Bottom half context) but it used spin_lock() to take a lock.
Using ethtool command rarely causes deadlock because of this.
Change lock function to spin_lock_bh() to avoid this.

Signed-off-by: Masafumi UTSUGI <mutsugi@allied-telesis.co.jp>
---
 .../patches-4.14/715-essedma-fix-dead-lock.patch     | 20 ++++++++++++++++++++
 .../patches-4.19/715-essedma-fix-dead-lock.patch     | 20 ++++++++++++++++++++
 2 files changed, 40 insertions(+)
 create mode 100644 target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
 create mode 100644 target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch

Comments

John Crispin Oct. 20, 2019, 9:39 a.m. UTC | #1
On 01/10/2019 11:59, Masafumi UTSUGI wrote:
> edma_read_append_stats() is called from kernel timer
> (Bottom half context) but it used spin_lock() to take a lock.
> Using ethtool command rarely causes deadlock because of this.
> Change lock function to spin_lock_bh() to avoid this.
> 

Hi,
patch looks good, could you please rebase it for v4.19 and merge the fix 
directly into the essedma patch ?
	John

> Signed-off-by: Masafumi UTSUGI <mutsugi@allied-telesis.co.jp>
> ---
>   .../patches-4.14/715-essedma-fix-dead-lock.patch     | 20 ++++++++++++++++++++
>   .../patches-4.19/715-essedma-fix-dead-lock.patch     | 20 ++++++++++++++++++++
>   2 files changed, 40 insertions(+)
>   create mode 100644 target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
>   create mode 100644 target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch
> 
> diff --git a/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch b/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
> new file mode 100644
> index 0000000..1c44924
> --- /dev/null
> +++ b/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
> @@ -0,0 +1,20 @@
> +--- a/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
> ++++ b/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
> +@@ -230,7 +230,7 @@
> + 	int i;
> + 	u32 stat;
> +
> +-	spin_lock(&edma_cinfo->stats_lock);
> ++	spin_lock_bh(&edma_cinfo->stats_lock);
> + 	p = (uint32_t *)&(edma_cinfo->edma_ethstats);
> +
> + 	for (i = 0; i < EDMA_MAX_TRANSMIT_QUEUE; i++) {
> +@@ -257,7 +257,7 @@
> + 		p++;
> + 	}
> +
> +-	spin_unlock(&edma_cinfo->stats_lock);
> ++	spin_unlock_bh(&edma_cinfo->stats_lock);
> + }
> +
> + static void edma_statistics_timer(unsigned long data)
> diff --git a/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch b/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch
> new file mode 100644
> index 0000000..1c44924
> --- /dev/null
> +++ b/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch
> @@ -0,0 +1,20 @@
> +--- a/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
> ++++ b/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
> +@@ -230,7 +230,7 @@
> + 	int i;
> + 	u32 stat;
> +
> +-	spin_lock(&edma_cinfo->stats_lock);
> ++	spin_lock_bh(&edma_cinfo->stats_lock);
> + 	p = (uint32_t *)&(edma_cinfo->edma_ethstats);
> +
> + 	for (i = 0; i < EDMA_MAX_TRANSMIT_QUEUE; i++) {
> +@@ -257,7 +257,7 @@
> + 		p++;
> + 	}
> +
> +-	spin_unlock(&edma_cinfo->stats_lock);
> ++	spin_unlock_bh(&edma_cinfo->stats_lock);
> + }
> +
> + static void edma_statistics_timer(unsigned long data)
>
Christian Lamparter Oct. 20, 2019, 11:07 a.m. UTC | #2
On Sunday, October 20, 2019 11:39:41 AM CEST John Crispin wrote:
> On 01/10/2019 11:59, Masafumi UTSUGI wrote:
> > edma_read_append_stats() is called from kernel timer
> > (Bottom half context) but it used spin_lock() to take a lock.
> > Using ethtool command rarely causes deadlock because of this.
> > Change lock function to spin_lock_bh() to avoid this.
> > 
> 
> Hi,
> patch looks good, could you please rebase it for v4.19 and merge the fix 
> directly into the essedma patch ?

Oh, I've already done that yesterday?

https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=commit;h=3e8fc768f78c6b7f7025dd15264d113da9ad81b2
John Crispin Oct. 20, 2019, 11:18 a.m. UTC | #3
On 20/10/2019 13:07, Christian Lamparter wrote:
> On Sunday, October 20, 2019 11:39:41 AM CEST John Crispin wrote:
>> On 01/10/2019 11:59, Masafumi UTSUGI wrote:
>>> edma_read_append_stats() is called from kernel timer
>>> (Bottom half context) but it used spin_lock() to take a lock.
>>> Using ethtool command rarely causes deadlock because of this.
>>> Change lock function to spin_lock_bh() to avoid this.
>>>
>>
>> Hi,
>> patch looks good, could you please rebase it for v4.19 and merge the fix
>> directly into the essedma patch ?
> 
> Oh, I've already done that yesterday?
> 
> https://git.openwrt.org/?p=openwrt/staging/chunkeey.git;a=commit;h=3e8fc768f78c6b7f7025dd15264d113da9ad81b2
> 
> 
> 
> 
> _______________________________________________
> openwrt-devel mailing list
> openwrt-devel@lists.openwrt.org
> https://lists.openwrt.org/mailman/listinfo/openwrt-devel
> 

Ok, the patchwork ticket was still open :-)
Dmitry Tunin Oct. 22, 2019, 7:53 a.m. UTC | #4
It seems that it is not quite proper fix.

Yesterday I flashed a Linksys EA6450v3 with 19.07 build. I couldn't
connect to the device using a 100 Mbit laptop.
I had to use another laptop with a 1000 Mbit port to reach the device.

It seems that after the patch we avoided a lock on 1000 Mbit but got a
lock on lower ones.

Patch
diff mbox series

diff --git a/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch b/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
new file mode 100644
index 0000000..1c44924
--- /dev/null
+++ b/target/linux/ipq40xx/patches-4.14/715-essedma-fix-dead-lock.patch
@@ -0,0 +1,20 @@ 
+--- a/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
++++ b/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
+@@ -230,7 +230,7 @@
+ 	int i;
+ 	u32 stat;
+ 
+-	spin_lock(&edma_cinfo->stats_lock);
++	spin_lock_bh(&edma_cinfo->stats_lock);
+ 	p = (uint32_t *)&(edma_cinfo->edma_ethstats);
+ 
+ 	for (i = 0; i < EDMA_MAX_TRANSMIT_QUEUE; i++) {
+@@ -257,7 +257,7 @@
+ 		p++;
+ 	}
+ 
+-	spin_unlock(&edma_cinfo->stats_lock);
++	spin_unlock_bh(&edma_cinfo->stats_lock);
+ }
+ 
+ static void edma_statistics_timer(unsigned long data)
diff --git a/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch b/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch
new file mode 100644
index 0000000..1c44924
--- /dev/null
+++ b/target/linux/ipq40xx/patches-4.19/715-essedma-fix-dead-lock.patch
@@ -0,0 +1,20 @@ 
+--- a/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
++++ b/drivers/net/ethernet/qualcomm/essedma/edma_axi.c
+@@ -230,7 +230,7 @@
+ 	int i;
+ 	u32 stat;
+ 
+-	spin_lock(&edma_cinfo->stats_lock);
++	spin_lock_bh(&edma_cinfo->stats_lock);
+ 	p = (uint32_t *)&(edma_cinfo->edma_ethstats);
+ 
+ 	for (i = 0; i < EDMA_MAX_TRANSMIT_QUEUE; i++) {
+@@ -257,7 +257,7 @@
+ 		p++;
+ 	}
+ 
+-	spin_unlock(&edma_cinfo->stats_lock);
++	spin_unlock_bh(&edma_cinfo->stats_lock);
+ }
+ 
+ static void edma_statistics_timer(unsigned long data)