2021-04-05 08:14:22

by Christophe JAILLET

[permalink] [raw]
Subject: [PATCH] rtlwifi: Simplify locking of a skb list accesses

The 'c2hcmd_lock' spinlock is only used to protect some __skb_queue_tail()
and __skb_dequeue() calls.
Use the lock provided in the skb itself and call skb_queue_tail() and
skb_dequeue(). These functions already include the correct locking.

Signed-off-by: Christophe JAILLET <[email protected]>
---
drivers/net/wireless/realtek/rtlwifi/base.c | 15 ++-------------
drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 -
2 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
index 6e8bd99e8911..2a7ee90a3f54 100644
--- a/drivers/net/wireless/realtek/rtlwifi/base.c
+++ b/drivers/net/wireless/realtek/rtlwifi/base.c
@@ -551,7 +551,6 @@ int rtl_init_core(struct ieee80211_hw *hw)
spin_lock_init(&rtlpriv->locks.rf_lock);
spin_lock_init(&rtlpriv->locks.waitq_lock);
spin_lock_init(&rtlpriv->locks.entry_list_lock);
- spin_lock_init(&rtlpriv->locks.c2hcmd_lock);
spin_lock_init(&rtlpriv->locks.scan_list_lock);
spin_lock_init(&rtlpriv->locks.cck_and_rw_pagea_lock);
spin_lock_init(&rtlpriv->locks.fw_ps_lock);
@@ -2269,7 +2268,6 @@ static bool rtl_c2h_fast_cmd(struct ieee80211_hw *hw, struct sk_buff *skb)
void rtl_c2hcmd_enqueue(struct ieee80211_hw *hw, struct sk_buff *skb)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
- unsigned long flags;

if (rtl_c2h_fast_cmd(hw, skb)) {
rtl_c2h_content_parsing(hw, skb);
@@ -2278,11 +2276,7 @@ void rtl_c2hcmd_enqueue(struct ieee80211_hw *hw, struct sk_buff *skb)
}

/* enqueue */
- spin_lock_irqsave(&rtlpriv->locks.c2hcmd_lock, flags);
-
- __skb_queue_tail(&rtlpriv->c2hcmd_queue, skb);
-
- spin_unlock_irqrestore(&rtlpriv->locks.c2hcmd_lock, flags);
+ skb_queue_tail(&rtlpriv->c2hcmd_queue, skb);

/* wake up wq */
queue_delayed_work(rtlpriv->works.rtl_wq, &rtlpriv->works.c2hcmd_wq, 0);
@@ -2340,16 +2334,11 @@ void rtl_c2hcmd_launcher(struct ieee80211_hw *hw, int exec)
{
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct sk_buff *skb;
- unsigned long flags;
int i;

for (i = 0; i < 200; i++) {
/* dequeue a task */
- spin_lock_irqsave(&rtlpriv->locks.c2hcmd_lock, flags);
-
- skb = __skb_dequeue(&rtlpriv->c2hcmd_queue);
-
- spin_unlock_irqrestore(&rtlpriv->locks.c2hcmd_lock, flags);
+ skb = skb_dequeue(&rtlpriv->c2hcmd_queue);

/* do it */
if (!skb)
diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
index 9119144bb5a3..877ed6a1589f 100644
--- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
+++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
@@ -2450,7 +2450,6 @@ struct rtl_locks {
spinlock_t waitq_lock;
spinlock_t entry_list_lock;
spinlock_t usb_lock;
- spinlock_t c2hcmd_lock;
spinlock_t scan_list_lock; /* lock for the scan list */

/*FW clock change */
--
2.27.0


2021-04-06 03:49:38

by Larry Finger

[permalink] [raw]
Subject: Re: [PATCH] rtlwifi: Simplify locking of a skb list accesses

On 4/5/21 2:57 AM, Christophe JAILLET wrote:
> The 'c2hcmd_lock' spinlock is only used to protect some __skb_queue_tail()
> and __skb_dequeue() calls.
> Use the lock provided in the skb itself and call skb_queue_tail() and
> skb_dequeue(). These functions already include the correct locking.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> ---
> drivers/net/wireless/realtek/rtlwifi/base.c | 15 ++-------------
> drivers/net/wireless/realtek/rtlwifi/wifi.h | 1 -
> 2 files changed, 2 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/base.c b/drivers/net/wireless/realtek/rtlwifi/base.c
> index 6e8bd99e8911..2a7ee90a3f54 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/base.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/base.c
> @@ -551,7 +551,6 @@ int rtl_init_core(struct ieee80211_hw *hw)
> spin_lock_init(&rtlpriv->locks.rf_lock);
> spin_lock_init(&rtlpriv->locks.waitq_lock);
> spin_lock_init(&rtlpriv->locks.entry_list_lock);
> - spin_lock_init(&rtlpriv->locks.c2hcmd_lock);
> spin_lock_init(&rtlpriv->locks.scan_list_lock);
> spin_lock_init(&rtlpriv->locks.cck_and_rw_pagea_lock);
> spin_lock_init(&rtlpriv->locks.fw_ps_lock);
> @@ -2269,7 +2268,6 @@ static bool rtl_c2h_fast_cmd(struct ieee80211_hw *hw, struct sk_buff *skb)
> void rtl_c2hcmd_enqueue(struct ieee80211_hw *hw, struct sk_buff *skb)
> {
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> - unsigned long flags;
>
> if (rtl_c2h_fast_cmd(hw, skb)) {
> rtl_c2h_content_parsing(hw, skb);
> @@ -2278,11 +2276,7 @@ void rtl_c2hcmd_enqueue(struct ieee80211_hw *hw, struct sk_buff *skb)
> }
>
> /* enqueue */
> - spin_lock_irqsave(&rtlpriv->locks.c2hcmd_lock, flags);
> -
> - __skb_queue_tail(&rtlpriv->c2hcmd_queue, skb);
> -
> - spin_unlock_irqrestore(&rtlpriv->locks.c2hcmd_lock, flags);
> + skb_queue_tail(&rtlpriv->c2hcmd_queue, skb);
>
> /* wake up wq */
> queue_delayed_work(rtlpriv->works.rtl_wq, &rtlpriv->works.c2hcmd_wq, 0);
> @@ -2340,16 +2334,11 @@ void rtl_c2hcmd_launcher(struct ieee80211_hw *hw, int exec)
> {
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct sk_buff *skb;
> - unsigned long flags;
> int i;
>
> for (i = 0; i < 200; i++) {
> /* dequeue a task */
> - spin_lock_irqsave(&rtlpriv->locks.c2hcmd_lock, flags);
> -
> - skb = __skb_dequeue(&rtlpriv->c2hcmd_queue);
> -
> - spin_unlock_irqrestore(&rtlpriv->locks.c2hcmd_lock, flags);
> + skb = skb_dequeue(&rtlpriv->c2hcmd_queue);
>
> /* do it */
> if (!skb)
> diff --git a/drivers/net/wireless/realtek/rtlwifi/wifi.h b/drivers/net/wireless/realtek/rtlwifi/wifi.h
> index 9119144bb5a3..877ed6a1589f 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/wifi.h
> +++ b/drivers/net/wireless/realtek/rtlwifi/wifi.h
> @@ -2450,7 +2450,6 @@ struct rtl_locks {
> spinlock_t waitq_lock;
> spinlock_t entry_list_lock;
> spinlock_t usb_lock;
> - spinlock_t c2hcmd_lock;
> spinlock_t scan_list_lock; /* lock for the scan list */
>
> /*FW clock change */
>

Acked-by: Larry Finger <[email protected]>

Thanks,

Larry

2021-04-17 17:33:29

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] rtlwifi: Simplify locking of a skb list accesses

Christophe JAILLET <[email protected]> wrote:

> The 'c2hcmd_lock' spinlock is only used to protect some __skb_queue_tail()
> and __skb_dequeue() calls.
> Use the lock provided in the skb itself and call skb_queue_tail() and
> skb_dequeue(). These functions already include the correct locking.
>
> Signed-off-by: Christophe JAILLET <[email protected]>
> Acked-by: Larry Finger <[email protected]>

Patch applied to wireless-drivers-next.git, thanks.

1186006adee9 rtlwifi: Simplify locking of a skb list accesses

--
https://patchwork.kernel.org/project/linux-wireless/patch/99cf8894fd52202cb7ce2ec6e3200eef400bc071.1617609346.git.christophe.jaillet@wanadoo.fr/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches