It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. This patchset is
trying to add all skb to a free list, then free them after
spin_unlock_irqrestore() at once.
Yang Yingliang (3):
rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
rtlwifi: rtl8188ee: don't call kfree_skb() under spin_lock_irqsave()
rtlwifi: rtl8723be: don't call kfree_skb() under spin_lock_irqsave()
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 6 +++++-
drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 6 +++++-
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 6 +++++-
3 files changed, 15 insertions(+), 3 deletions(-)
--
2.25.1
It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. So add all skb to
a free list, then free them after spin_unlock_irqrestore() at
once.
Fixes: 5c99f04fec93 ("rtlwifi: rtl8723be: Update driver to match Realtek release of 06/28/14")
Signed-off-by: Yang Yingliang <[email protected]>
---
drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
index 189cc6437600..0ba3bbed6ed3 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8723be/hw.c
@@ -30,8 +30,10 @@ static void _rtl8723be_return_beacon_queue_skb(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
struct rtl8192_tx_ring *ring = &rtlpci->tx_ring[BEACON_QUEUE];
+ struct sk_buff_head free_list;
unsigned long flags;
+ skb_queue_head_init(&free_list);
spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
while (skb_queue_len(&ring->queue)) {
struct rtl_tx_desc *entry = &ring->desc[ring->idx];
@@ -41,10 +43,12 @@ static void _rtl8723be_return_beacon_queue_skb(struct ieee80211_hw *hw)
rtlpriv->cfg->ops->get_desc(hw, (u8 *)entry,
true, HW_DESC_TXBUFF_ADDR),
skb->len, DMA_TO_DEVICE);
- kfree_skb(skb);
+ __skb_queue_tail(&free_list, skb);
ring->idx = (ring->idx + 1) % ring->entries;
}
spin_unlock_irqrestore(&rtlpriv->locks.irq_th_lock, flags);
+
+ __skb_queue_purge(&free_list);
}
static void _rtl8723be_set_bcn_ctrl_reg(struct ieee80211_hw *hw,
--
2.25.1
It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. So add all skb to
a free list, then free them after spin_unlock_irqrestore() at
once.
Fixes: 7fe3b3abb5da ("rtlwifi: rtl8188ee: rtl8821ae: Fix a queue locking problem")
Signed-off-by: Yang Yingliang <[email protected]>
---
drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
index 58c2ab3d44be..de61c9c0ddec 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8188ee/hw.c
@@ -68,8 +68,10 @@ static void _rtl88ee_return_beacon_queue_skb(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
struct rtl8192_tx_ring *ring = &rtlpci->tx_ring[BEACON_QUEUE];
+ struct sk_buff_head free_list;
unsigned long flags;
+ skb_queue_head_init(&free_list);
spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
while (skb_queue_len(&ring->queue)) {
struct rtl_tx_desc *entry = &ring->desc[ring->idx];
@@ -79,10 +81,12 @@ static void _rtl88ee_return_beacon_queue_skb(struct ieee80211_hw *hw)
rtlpriv->cfg->ops->get_desc(hw, (u8 *)entry,
true, HW_DESC_TXBUFF_ADDR),
skb->len, DMA_TO_DEVICE);
- kfree_skb(skb);
+ __skb_queue_tail(&free_list, skb);
ring->idx = (ring->idx + 1) % ring->entries;
}
spin_unlock_irqrestore(&rtlpriv->locks.irq_th_lock, flags);
+
+ __skb_queue_purge(&free_list);
}
static void _rtl88ee_disable_bcn_sub_func(struct ieee80211_hw *hw)
--
2.25.1
It is not allowed to call kfree_skb() from hardware interrupt
context or with interrupts being disabled. So add all skb to
a free list, then free them after spin_unlock_irqrestore() at
once.
Fixes: 5c99f04fec93 ("rtlwifi: rtl8723be: Update driver to match Realtek release of 06/28/14")
Signed-off-by: Yang Yingliang <[email protected]>
---
drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
index 7e0f62d59fe1..a7e3250957dc 100644
--- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
@@ -26,8 +26,10 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
struct rtl_priv *rtlpriv = rtl_priv(hw);
struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
struct rtl8192_tx_ring *ring = &rtlpci->tx_ring[BEACON_QUEUE];
+ struct sk_buff_head free_list;
unsigned long flags;
+ skb_queue_head_init(&free_list);
spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
while (skb_queue_len(&ring->queue)) {
struct rtl_tx_desc *entry = &ring->desc[ring->idx];
@@ -37,10 +39,12 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
rtlpriv->cfg->ops->get_desc(hw, (u8 *)entry,
true, HW_DESC_TXBUFF_ADDR),
skb->len, DMA_TO_DEVICE);
- kfree_skb(skb);
+ __skb_queue_tail(&free_list, skb);
ring->idx = (ring->idx + 1) % ring->entries;
}
spin_unlock_irqrestore(&rtlpriv->locks.irq_th_lock, flags);
+
+ __skb_queue_purge(&free_list);
}
static void _rtl8821ae_set_bcn_ctrl_reg(struct ieee80211_hw *hw,
--
2.25.1
> -----Original Message-----
> From: Yang Yingliang <[email protected]>
> Sent: Tuesday, December 6, 2022 9:13 PM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]
> Subject: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>
> It is not allowed to call kfree_skb() from hardware interrupt
> context or with interrupts being disabled. So add all skb to
> a free list, then free them after spin_unlock_irqrestore() at
> once.
The patch doesn't change logic, so it should work. But, I would like to know
if there is a comment about this in kernel code. Could you point it out?
>
> Fixes: 5c99f04fec93 ("rtlwifi: rtl8723be: Update driver to match Realtek release of 06/28/14")
> Signed-off-by: Yang Yingliang <[email protected]>
> ---
> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> index 7e0f62d59fe1..a7e3250957dc 100644
> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
> @@ -26,8 +26,10 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
> struct rtl_priv *rtlpriv = rtl_priv(hw);
> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
> struct rtl8192_tx_ring *ring = &rtlpci->tx_ring[BEACON_QUEUE];
> + struct sk_buff_head free_list;
> unsigned long flags;
>
> + skb_queue_head_init(&free_list);
> spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
> while (skb_queue_len(&ring->queue)) {
> struct rtl_tx_desc *entry = &ring->desc[ring->idx];
> @@ -37,10 +39,12 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
> rtlpriv->cfg->ops->get_desc(hw, (u8 *)entry,
> true, HW_DESC_TXBUFF_ADDR),
> skb->len, DMA_TO_DEVICE);
> - kfree_skb(skb);
> + __skb_queue_tail(&free_list, skb);
> ring->idx = (ring->idx + 1) % ring->entries;
> }
> spin_unlock_irqrestore(&rtlpriv->locks.irq_th_lock, flags);
> +
> + __skb_queue_purge(&free_list);
> }
>
> static void _rtl8821ae_set_bcn_ctrl_reg(struct ieee80211_hw *hw,
> --
> 2.25.1
>
>
> ------Please consider the environment before printing this e-mail.
On 2022/12/7 11:31, Ping-Ke Shih wrote:
>> -----Original Message-----
>> From: Yang Yingliang <[email protected]>
>> Sent: Tuesday, December 6, 2022 9:13 PM
>> To: Ping-Ke Shih <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>>
>> It is not allowed to call kfree_skb() from hardware interrupt
>> context or with interrupts being disabled. So add all skb to
>> a free list, then free them after spin_unlock_irqrestore() at
>> once.
> The patch doesn't change logic, so it should work. But, I would like to know
> if there is a comment about this in kernel code. Could you point it out?
You can see comment of dev_kfree_skb_irq() in include/linux/netdevice.h
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/netdevice.h?h=v6.1-rc8
Thanks,
Yang
>
>> Fixes: 5c99f04fec93 ("rtlwifi: rtl8723be: Update driver to match Realtek release of 06/28/14")
>> Signed-off-by: Yang Yingliang <[email protected]>
>> ---
>> drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c | 6 +++++-
>> 1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
>> b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
>> index 7e0f62d59fe1..a7e3250957dc 100644
>> --- a/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
>> +++ b/drivers/net/wireless/realtek/rtlwifi/rtl8821ae/hw.c
>> @@ -26,8 +26,10 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
>> struct rtl_priv *rtlpriv = rtl_priv(hw);
>> struct rtl_pci *rtlpci = rtl_pcidev(rtl_pcipriv(hw));
>> struct rtl8192_tx_ring *ring = &rtlpci->tx_ring[BEACON_QUEUE];
>> + struct sk_buff_head free_list;
>> unsigned long flags;
>>
>> + skb_queue_head_init(&free_list);
>> spin_lock_irqsave(&rtlpriv->locks.irq_th_lock, flags);
>> while (skb_queue_len(&ring->queue)) {
>> struct rtl_tx_desc *entry = &ring->desc[ring->idx];
>> @@ -37,10 +39,12 @@ static void _rtl8821ae_return_beacon_queue_skb(struct ieee80211_hw *hw)
>> rtlpriv->cfg->ops->get_desc(hw, (u8 *)entry,
>> true, HW_DESC_TXBUFF_ADDR),
>> skb->len, DMA_TO_DEVICE);
>> - kfree_skb(skb);
>> + __skb_queue_tail(&free_list, skb);
>> ring->idx = (ring->idx + 1) % ring->entries;
>> }
>> spin_unlock_irqrestore(&rtlpriv->locks.irq_th_lock, flags);
>> +
>> + __skb_queue_purge(&free_list);
>> }
>>
>> static void _rtl8821ae_set_bcn_ctrl_reg(struct ieee80211_hw *hw,
>> --
>> 2.25.1
>>
>>
>> ------Please consider the environment before printing this e-mail.
> .
> -----Original Message-----
> From: Yang Yingliang <[email protected]>
> Sent: Wednesday, December 7, 2022 11:44 AM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>
>
> On 2022/12/7 11:31, Ping-Ke Shih wrote:
> >> -----Original Message-----
> >> From: Yang Yingliang <[email protected]>
> >> Sent: Tuesday, December 6, 2022 9:13 PM
> >> To: Ping-Ke Shih <[email protected]>; [email protected]
> >> Cc: [email protected]; [email protected]
> >> Subject: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
> >>
> >> It is not allowed to call kfree_skb() from hardware interrupt
> >> context or with interrupts being disabled. So add all skb to
> >> a free list, then free them after spin_unlock_irqrestore() at
> >> once.
> > The patch doesn't change logic, so it should work. But, I would like to know
> > if there is a comment about this in kernel code. Could you point it out?
> You can see comment of dev_kfree_skb_irq() in include/linux/netdevice.h
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/netdevice.h?h=v6
> .1-rc8
>
It seems like we can replace kfree_skb() by dev_kfree_skb_irq(), right?
But your method is more efficient. Is that your point?
Ping-Ke
On 2022/12/7 11:52, Ping-Ke Shih wrote:
>> -----Original Message-----
>> From: Yang Yingliang <[email protected]>
>> Sent: Wednesday, December 7, 2022 11:44 AM
>> To: Ping-Ke Shih <[email protected]>; [email protected]
>> Cc: [email protected]; [email protected]
>> Subject: Re: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>>
>>
>> On 2022/12/7 11:31, Ping-Ke Shih wrote:
>>>> -----Original Message-----
>>>> From: Yang Yingliang <[email protected]>
>>>> Sent: Tuesday, December 6, 2022 9:13 PM
>>>> To: Ping-Ke Shih <[email protected]>; [email protected]
>>>> Cc: [email protected]; [email protected]
>>>> Subject: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>>>>
>>>> It is not allowed to call kfree_skb() from hardware interrupt
>>>> context or with interrupts being disabled. So add all skb to
>>>> a free list, then free them after spin_unlock_irqrestore() at
>>>> once.
>>> The patch doesn't change logic, so it should work. But, I would like to know
>>> if there is a comment about this in kernel code. Could you point it out?
>> You can see comment of dev_kfree_skb_irq() in include/linux/netdevice.h
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/netdevice.h?h=v6
>> .1-rc8
>>
> It seems like we can replace kfree_skb() by dev_kfree_skb_irq(), right?
> But your method is more efficient. Is that your point?
Yes, the SKBs have already been dequeued from the queue, so they can be
freed together at once.
Thanks,
Yang
>
> Ping-Ke
>
> -----Original Message-----
> From: Yang Yingliang <[email protected]>
> Sent: Wednesday, December 7, 2022 12:41 PM
> To: Ping-Ke Shih <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
>
> On 2022/12/7 11:52, Ping-Ke Shih wrote:
> >> -----Original Message-----
> >> From: Yang Yingliang <[email protected]>
> >> Sent: Wednesday, December 7, 2022 11:44 AM
> >> To: Ping-Ke Shih <[email protected]>; [email protected]
> >> Cc: [email protected]; [email protected]
> >> Subject: Re: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
> >>
> >>
> >> On 2022/12/7 11:31, Ping-Ke Shih wrote:
> >>>> -----Original Message-----
> >>>> From: Yang Yingliang <[email protected]>
> >>>> Sent: Tuesday, December 6, 2022 9:13 PM
> >>>> To: Ping-Ke Shih <[email protected]>; [email protected]
> >>>> Cc: [email protected]; [email protected]
> >>>> Subject: [PATCH resend 1/3] rtlwifi: rtl8821ae: don't call kfree_skb() under spin_lock_irqsave()
> >>>>
> >>>> It is not allowed to call kfree_skb() from hardware interrupt
> >>>> context or with interrupts being disabled. So add all skb to
> >>>> a free list, then free them after spin_unlock_irqrestore() at
> >>>> once.
> >>> The patch doesn't change logic, so it should work. But, I would like to know
> >>> if there is a comment about this in kernel code. Could you point it out?
> >> You can see comment of dev_kfree_skb_irq() in include/linux/netdevice.h
> >>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/netdevice.h?h=v6
> >> .1-rc8
> >>
> > It seems like we can replace kfree_skb() by dev_kfree_skb_irq(), right?
> > But your method is more efficient. Is that your point?
> Yes, the SKBs have already been dequeued from the queue, so they can be
> freed together at once.
>
Thanks. This patchset is good to me. But, subject prefix should be "wifi: rtlwifi: ..."
if v2 is not a hard work to you. I suppose v2 can change nothing, so add my Acked-by:
Acked-by: Ping-Ke Shih <[email protected]>