mbox series

[resend,0/3] rtlwifi: don't call kfree_skb() under spin_lock_irqsave()

Message ID 20221206131249.2181693-1-yangyingliang@huawei.com
Headers show
Series rtlwifi: don't call kfree_skb() under spin_lock_irqsave() | expand

Message

Yang Yingliang Dec. 6, 2022, 1:12 p.m. UTC 
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(-)

Comments

Ping-Ke Shih Dec. 7, 2022, 3:31 a.m. UTC | #1 
> -----Original Message-----
> From: Yang Yingliang <yangyingliang@huawei.com>
> Sent: Tuesday, December 6, 2022 9:13 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> 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 <yangyingliang@huawei.com>
> ---
>  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.
Yang Yingliang Dec. 7, 2022, 3:44 a.m. UTC | #2 
On 2022/12/7 11:31, Ping-Ke Shih wrote:
>> -----Original Message-----
>> From: Yang Yingliang <yangyingliang@huawei.com>
>> Sent: Tuesday, December 6, 2022 9:13 PM
>> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
>> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
>> 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 <yangyingliang@huawei.com>
>> ---
>>   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.
> .
Ping-Ke Shih Dec. 7, 2022, 3:52 a.m. UTC | #3 
> -----Original Message-----
> From: Yang Yingliang <yangyingliang@huawei.com>
> Sent: Wednesday, December 7, 2022 11:44 AM
> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> 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 <yangyingliang@huawei.com>
> >> Sent: Tuesday, December 6, 2022 9:13 PM
> >> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> >> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> >> 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
Yang Yingliang Dec. 7, 2022, 4:41 a.m. UTC | #4 
On 2022/12/7 11:52, Ping-Ke Shih wrote:
>> -----Original Message-----
>> From: Yang Yingliang <yangyingliang@huawei.com>
>> Sent: Wednesday, December 7, 2022 11:44 AM
>> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
>> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
>> 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 <yangyingliang@huawei.com>
>>>> Sent: Tuesday, December 6, 2022 9:13 PM
>>>> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
>>>> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
>>>> 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
>
Ping-Ke Shih Dec. 7, 2022, 5:07 a.m. UTC | #5 
> -----Original Message-----
> From: Yang Yingliang <yangyingliang@huawei.com>
> Sent: Wednesday, December 7, 2022 12:41 PM
> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> 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 <yangyingliang@huawei.com>
> >> Sent: Wednesday, December 7, 2022 11:44 AM
> >> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> >> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> >> 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 <yangyingliang@huawei.com>
> >>>> Sent: Tuesday, December 6, 2022 9:13 PM
> >>>> To: Ping-Ke Shih <pkshih@realtek.com>; kvalo@kernel.org
> >>>> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com
> >>>> 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 <pkshih@realtek.com>