| Message ID | 20221207143738.67721-1-yangyingliang@huawei.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() | expand |
> -----Original Message----- > From: Yang Yingliang <yangyingliang@huawei.com> > Sent: Wednesday, December 7, 2022 10:38 PM > To: Jes.Sorensen@gmail.com; kvalo@kernel.org > Cc: linux-wireless@vger.kernel.org > Subject: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() > > It is not allowed to call consume_skb() from hardware interrupt context ^^^^^^^^^^^^^ kfree_skb()? because this patch is to replace dev_kfree_skb(). > or with interrupts being disabled. So replace dev_kfree_skb() with > dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. > > Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > --- > drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > index ac641a56efb0..d0600af5bef4 100644 > --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, > pending = priv->rx_urb_pending_count; > } else { > skb = (struct sk_buff *)rx_urb->urb.context; > - dev_kfree_skb(skb); > + dev_consume_skb_irq(skb); Why not dev_kfree_skb_irq() instead? any reason? > usb_free_urb(&rx_urb->urb); > } > > -- > 2.25.1 > > > ------Please consider the environment before printing this e-mail.
On 2022/12/8 8:38, Ping-Ke Shih wrote: >> -----Original Message----- >> From: Yang Yingliang <yangyingliang@huawei.com> >> Sent: Wednesday, December 7, 2022 10:38 PM >> To: Jes.Sorensen@gmail.com; kvalo@kernel.org >> Cc: linux-wireless@vger.kernel.org >> Subject: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() >> >> It is not allowed to call consume_skb() from hardware interrupt context > ^^^^^^^^^^^^^ kfree_skb()? > because this patch is to replace dev_kfree_skb(). > >> or with interrupts being disabled. So replace dev_kfree_skb() with >> dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. >> >> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >> --- >> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> index ac641a56efb0..d0600af5bef4 100644 >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >> @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, >> pending = priv->rx_urb_pending_count; >> } else { >> skb = (struct sk_buff *)rx_urb->urb.context; >> - dev_kfree_skb(skb); >> + dev_consume_skb_irq(skb); > Why not dev_kfree_skb_irq() instead? any reason? #define dev_kfree_skb(a) consume_skb(a) dev_kfree_skb() is consume_skb(), so use dev_consume_skb_irq() instead. static inline void dev_kfree_skb_irq(struct sk_buff *skb) { __dev_kfree_skb_irq(skb, SKB_REASON_DROPPED); } static inline void dev_consume_skb_irq(struct sk_buff *skb) { __dev_kfree_skb_irq(skb, SKB_REASON_CONSUMED); } They have different free reasons. Thanks, Yang > >> usb_free_urb(&rx_urb->urb); >> } >> >> -- >> 2.25.1 >> >> >> ------Please consider the environment before printing this e-mail. > .
> -----Original Message----- > From: Yang Yingliang <yangyingliang@huawei.com> > Sent: Thursday, December 8, 2022 9:26 AM > To: Ping-Ke Shih <pkshih@realtek.com>; Jes.Sorensen@gmail.com; kvalo@kernel.org > Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com > Subject: Re: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() > > > On 2022/12/8 8:38, Ping-Ke Shih wrote: > >> -----Original Message----- > >> From: Yang Yingliang <yangyingliang@huawei.com> > >> Sent: Wednesday, December 7, 2022 10:38 PM > >> To: Jes.Sorensen@gmail.com; kvalo@kernel.org > >> Cc: linux-wireless@vger.kernel.org > >> Subject: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() > >> > >> It is not allowed to call consume_skb() from hardware interrupt context > > ^^^^^^^^^^^^^ kfree_skb()? > > because this patch is to replace dev_kfree_skb(). > > > >> or with interrupts being disabled. So replace dev_kfree_skb() with > >> dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. > >> > >> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > >> --- > >> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > >> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > >> index ac641a56efb0..d0600af5bef4 100644 > >> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > >> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c > >> @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, > >> pending = priv->rx_urb_pending_count; > >> } else { > >> skb = (struct sk_buff *)rx_urb->urb.context; > >> - dev_kfree_skb(skb); > >> + dev_consume_skb_irq(skb); > > Why not dev_kfree_skb_irq() instead? any reason? > #define dev_kfree_skb(a) consume_skb(a) > dev_kfree_skb() is consume_skb(), so use dev_consume_skb_irq() instead. > > static inline void dev_kfree_skb_irq(struct sk_buff *skb) > { > __dev_kfree_skb_irq(skb, SKB_REASON_DROPPED); > } > > static inline void dev_consume_skb_irq(struct sk_buff *skb) > { > __dev_kfree_skb_irq(skb, SKB_REASON_CONSUMED); > } > They have different free reasons. > It falls into this case because of 'priv->shutdown', so DROPPED reason makes sense, no? Or I misunderstand the reason? -- Ping-Ke
On 2022/12/8 9:41, Ping-Ke Shih wrote: >> -----Original Message----- >> From: Yang Yingliang <yangyingliang@huawei.com> >> Sent: Thursday, December 8, 2022 9:26 AM >> To: Ping-Ke Shih <pkshih@realtek.com>; Jes.Sorensen@gmail.com; kvalo@kernel.org >> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com >> Subject: Re: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() >> >> >> On 2022/12/8 8:38, Ping-Ke Shih wrote: >>>> -----Original Message----- >>>> From: Yang Yingliang <yangyingliang@huawei.com> >>>> Sent: Wednesday, December 7, 2022 10:38 PM >>>> To: Jes.Sorensen@gmail.com; kvalo@kernel.org >>>> Cc: linux-wireless@vger.kernel.org >>>> Subject: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() >>>> >>>> It is not allowed to call consume_skb() from hardware interrupt context >>> ^^^^^^^^^^^^^ kfree_skb()? >>> because this patch is to replace dev_kfree_skb(). >>> >>>> or with interrupts being disabled. So replace dev_kfree_skb() with >>>> dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. >>>> >>>> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> index ac641a56efb0..d0600af5bef4 100644 >>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, >>>> pending = priv->rx_urb_pending_count; >>>> } else { >>>> skb = (struct sk_buff *)rx_urb->urb.context; >>>> - dev_kfree_skb(skb); >>>> + dev_consume_skb_irq(skb); >>> Why not dev_kfree_skb_irq() instead? any reason? >> #define dev_kfree_skb(a) consume_skb(a) >> dev_kfree_skb() is consume_skb(), so use dev_consume_skb_irq() instead. >> >> static inline void dev_kfree_skb_irq(struct sk_buff *skb) >> { >> __dev_kfree_skb_irq(skb, SKB_REASON_DROPPED); >> } >> >> static inline void dev_consume_skb_irq(struct sk_buff *skb) >> { >> __dev_kfree_skb_irq(skb, SKB_REASON_CONSUMED); >> } >> They have different free reasons. >> > It falls into this case because of 'priv->shutdown', so DROPPED reason makes > sense, no? Or I misunderstand the reason? Because the origin call is dev_kfree_skb() which is same as consume_skb(), I called dev_consume_skb_irq() instead here. Thanks, Yang > > -- > Ping-Ke >
On 2022/12/8 9:41, Ping-Ke Shih wrote: >> -----Original Message----- >> From: Yang Yingliang <yangyingliang@huawei.com> >> Sent: Thursday, December 8, 2022 9:26 AM >> To: Ping-Ke Shih <pkshih@realtek.com>; Jes.Sorensen@gmail.com; kvalo@kernel.org >> Cc: linux-wireless@vger.kernel.org; yangyingliang@huawei.com >> Subject: Re: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() >> >> >> On 2022/12/8 8:38, Ping-Ke Shih wrote: >>>> -----Original Message----- >>>> From: Yang Yingliang <yangyingliang@huawei.com> >>>> Sent: Wednesday, December 7, 2022 10:38 PM >>>> To: Jes.Sorensen@gmail.com; kvalo@kernel.org >>>> Cc: linux-wireless@vger.kernel.org >>>> Subject: [PATCH] wifi: rtl8xxxu: don't call dev_kfree_skb() under spin_lock_irqsave() >>>> >>>> It is not allowed to call consume_skb() from hardware interrupt context >>> ^^^^^^^^^^^^^ kfree_skb()? >>> because this patch is to replace dev_kfree_skb(). >>> >>>> or with interrupts being disabled. So replace dev_kfree_skb() with >>>> dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. >>>> >>>> Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") >>>> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> >>>> --- >>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> index ac641a56efb0..d0600af5bef4 100644 >>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c >>>> @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, >>>> pending = priv->rx_urb_pending_count; >>>> } else { >>>> skb = (struct sk_buff *)rx_urb->urb.context; >>>> - dev_kfree_skb(skb); >>>> + dev_consume_skb_irq(skb); >>> Why not dev_kfree_skb_irq() instead? any reason? >> #define dev_kfree_skb(a) consume_skb(a) >> dev_kfree_skb() is consume_skb(), so use dev_consume_skb_irq() instead. >> >> static inline void dev_kfree_skb_irq(struct sk_buff *skb) >> { >> __dev_kfree_skb_irq(skb, SKB_REASON_DROPPED); >> } >> >> static inline void dev_consume_skb_irq(struct sk_buff *skb) >> { >> __dev_kfree_skb_irq(skb, SKB_REASON_CONSUMED); >> } >> They have different free reasons. >> > It falls into this case because of 'priv->shutdown', so DROPPED reason makes > sense, no? Or I misunderstand the reason? You are right, it's better to use dev_kfree_skb_irq(), because this is called when it's stopped and need to drop the SKB, I will send a v2 to change it. Thanks, Yang > > -- > Ping-Ke >
diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c index ac641a56efb0..d0600af5bef4 100644 --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c @@ -5274,7 +5274,7 @@ static void rtl8xxxu_queue_rx_urb(struct rtl8xxxu_priv *priv, pending = priv->rx_urb_pending_count; } else { skb = (struct sk_buff *)rx_urb->urb.context; - dev_kfree_skb(skb); + dev_consume_skb_irq(skb); usb_free_urb(&rx_urb->urb); }
It is not allowed to call consume_skb() from hardware interrupt context or with interrupts being disabled. So replace dev_kfree_skb() with dev_consume_skb_irq() under spin_lock_irqsave(). Compile tested only. Fixes: 26f1fad29ad9 ("New driver: rtl8xxxu (mac80211)") Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> --- drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)