| Message ID | 20160927091428.19223-1-zajec5@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kalle Valo |
| Headers | show |
On 27-9-2016 11:14, Rafał Miłecki wrote: > From: Rafał Miłecki <rafal@milecki.pl> > > Flowrings contain skbs waiting for transmission that were passed to us > by netif. It means we checked every one of them looking for 802.1x > Ethernet type. When deleting flowring we have to use freeing function > that will check for 802.1x type as well. > > Freeing skbs without a proper check was leading to counter not being > properly decreased. This was triggering a WARNING every time > brcmf_netdev_wait_pend8021x was called. Acked-by: Arend van Spriel <arend@broadcom.com> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl> > --- > Kalle: this isn't important enough for 4.8 as it's too late for that. > > I'd like to get it for 4.9 however, as this fixes bug that could lead > to WARNING on every add_key/del_key call. We was struggling with these > WARNINGs for some time and this fixes one of two problems causing them. Please mark it for stable as well. > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c | 9 ++++++++- > 1 file changed, 8 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > index b16b367..d0b738d 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c > @@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid, > > void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid) > { > + struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev); > struct brcmf_flowring_ring *ring; > + struct brcmf_if *ifp; > u16 hash_idx; > + u8 ifidx; > struct sk_buff *skb; > > ring = flow->rings[flowid]; > if (!ring) > return; > + > + ifidx = brcmf_flowring_ifidx_get(flow, flowid); > + ifp = brcmf_get_ifp(bus_if->drvr, ifidx); > + > brcmf_flowring_block(flow, flowid, false); > hash_idx = ring->hash_id; > flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX; I am not very familiar with flowring code, but I suppose this is just initializing the entry for later use, right? > @@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid) > > skb = skb_dequeue(&ring->skblist); > while (skb) { > - brcmu_pkt_buf_free_skb(skb); > + brcmf_txfinalize(ifp, skb, false); > skb = skb_dequeue(&ring->skblist); > } > >
Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > On 27-9-2016 11:14, Rafał Miłecki wrote: >> From: Rafał Miłecki <rafal@milecki.pl> >> >> Flowrings contain skbs waiting for transmission that were passed to us >> by netif. It means we checked every one of them looking for 802.1x >> Ethernet type. When deleting flowring we have to use freeing function >> that will check for 802.1x type as well. >> >> Freeing skbs without a proper check was leading to counter not being >> properly decreased. This was triggering a WARNING every time >> brcmf_netdev_wait_pend8021x was called. > > Acked-by: Arend van Spriel <arend@broadcom.com> >> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >> --- >> Kalle: this isn't important enough for 4.8 as it's too late for that. >> >> I'd like to get it for 4.9 however, as this fixes bug that could lead >> to WARNING on every add_key/del_key call. We was struggling with these >> WARNINGs for some time and this fixes one of two problems causing them. Ok, I'll queue this for 4.9. > Please mark it for stable as well. I can add that. Any ideas how old releases stable releases should this go to?
On 27-9-2016 13:27, Kalle Valo wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 27-9-2016 11:14, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> Flowrings contain skbs waiting for transmission that were passed to us >>> by netif. It means we checked every one of them looking for 802.1x >>> Ethernet type. When deleting flowring we have to use freeing function >>> that will check for 802.1x type as well. >>> >>> Freeing skbs without a proper check was leading to counter not being >>> properly decreased. This was triggering a WARNING every time >>> brcmf_netdev_wait_pend8021x was called. >> >> Acked-by: Arend van Spriel <arend@broadcom.com> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> --- >>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>> >>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>> to WARNING on every add_key/del_key call. We was struggling with these >>> WARNINGs for some time and this fixes one of two problems causing them. > > Ok, I'll queue this for 4.9. > >> Please mark it for stable as well. > > I can add that. Any ideas how old releases stable releases should this > go to? Not sure if the vendor directory move causes issues as stable can not fallback to three-way merge. I assumed it would so my last stable tag was only for 4.7 and I took care of older kernels at later time with backported patch. I can do that for this one as well. Regards, Arend
On 27 September 2016 at 13:27, Kalle Valo <kvalo@codeaurora.org> wrote: > Arend Van Spriel <arend.vanspriel@broadcom.com> writes: > >> On 27-9-2016 11:14, Rafał Miłecki wrote: >>> From: Rafał Miłecki <rafal@milecki.pl> >>> >>> Flowrings contain skbs waiting for transmission that were passed to us >>> by netif. It means we checked every one of them looking for 802.1x >>> Ethernet type. When deleting flowring we have to use freeing function >>> that will check for 802.1x type as well. >>> >>> Freeing skbs without a proper check was leading to counter not being >>> properly decreased. This was triggering a WARNING every time >>> brcmf_netdev_wait_pend8021x was called. >> >> Acked-by: Arend van Spriel <arend@broadcom.com> >>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>> --- >>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>> >>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>> to WARNING on every add_key/del_key call. We was struggling with these >>> WARNINGs for some time and this fixes one of two problems causing them. > > Ok, I'll queue this for 4.9. > >> Please mark it for stable as well. > > I can add that. Any ideas how old releases stable releases should this > go to? I was analyzing this. 1) This patch uses brcmf_get_ifp which is available in 4.4+ only. 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac: Increase nr of supported flowrings.") 3) 4.4 would also require applying to the patch without broadcom/ subdir That said I suggest 4.5+. Any objections?
On 27 September 2016 at 13:44, Rafał Miłecki <zajec5@gmail.com> wrote: > On 27 September 2016 at 13:27, Kalle Valo <kvalo@codeaurora.org> wrote: >> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >> >>> On 27-9-2016 11:14, Rafał Miłecki wrote: >>>> From: Rafał Miłecki <rafal@milecki.pl> >>>> >>>> Flowrings contain skbs waiting for transmission that were passed to us >>>> by netif. It means we checked every one of them looking for 802.1x >>>> Ethernet type. When deleting flowring we have to use freeing function >>>> that will check for 802.1x type as well. >>>> >>>> Freeing skbs without a proper check was leading to counter not being >>>> properly decreased. This was triggering a WARNING every time >>>> brcmf_netdev_wait_pend8021x was called. >>> >>> Acked-by: Arend van Spriel <arend@broadcom.com> >>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>> --- >>>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>>> >>>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>>> to WARNING on every add_key/del_key call. We was struggling with these >>>> WARNINGs for some time and this fixes one of two problems causing them. >> >> Ok, I'll queue this for 4.9. >> >>> Please mark it for stable as well. >> >> I can add that. Any ideas how old releases stable releases should this >> go to? > > I was analyzing this. > 1) This patch uses brcmf_get_ifp which is available in 4.4+ only. > 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac: > Increase nr of supported flowrings.") > 3) 4.4 would also require applying to the patch without broadcom/ subdir > > That said I suggest 4.5+. Any objections? Let me see if patchwork with pick Cc tag as it does for others. Cc: stable@vger.kernel.org # 4.5+ This may be worth backporting to 4.4 as well (as it's longterm), but I'll do it separately due to patch not applying cleanly.
On 27-9-2016 13:58, Rafał Miłecki wrote: > On 27 September 2016 at 13:44, Rafał Miłecki <zajec5@gmail.com> wrote: >> On 27 September 2016 at 13:27, Kalle Valo <kvalo@codeaurora.org> wrote: >>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >>> >>>> On 27-9-2016 11:14, Rafał Miłecki wrote: >>>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>> >>>>> Flowrings contain skbs waiting for transmission that were passed to us >>>>> by netif. It means we checked every one of them looking for 802.1x >>>>> Ethernet type. When deleting flowring we have to use freeing function >>>>> that will check for 802.1x type as well. >>>>> >>>>> Freeing skbs without a proper check was leading to counter not being >>>>> properly decreased. This was triggering a WARNING every time >>>>> brcmf_netdev_wait_pend8021x was called. >>>> >>>> Acked-by: Arend van Spriel <arend@broadcom.com> >>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>>> --- >>>>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>>>> >>>>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>>>> to WARNING on every add_key/del_key call. We was struggling with these >>>>> WARNINGs for some time and this fixes one of two problems causing them. >>> >>> Ok, I'll queue this for 4.9. >>> >>>> Please mark it for stable as well. >>> >>> I can add that. Any ideas how old releases stable releases should this >>> go to? >> >> I was analyzing this. >> 1) This patch uses brcmf_get_ifp which is available in 4.4+ only. >> 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac: >> Increase nr of supported flowrings.") >> 3) 4.4 would also require applying to the patch without broadcom/ subdir >> >> That said I suggest 4.5+. Any objections? No objections. Just a tip. I tend to look at kernel.org main page to see the stable and long-term kernel listed. So 4.7+ and 4.5+ have same meaning as 4.5 and 4.6 are not stable/long-term kernels. Regards, Arend > Let me see if patchwork with pick Cc tag as it does for others. > > Cc: stable@vger.kernel.org # 4.5+ > > This may be worth backporting to 4.4 as well (as it's longterm), but > I'll do it separately due to patch not applying cleanly.
On 27 September 2016 at 14:04, Arend Van Spriel <arend.vanspriel@broadcom.com> wrote: > On 27-9-2016 13:58, Rafał Miłecki wrote: >> On 27 September 2016 at 13:44, Rafał Miłecki <zajec5@gmail.com> wrote: >>> On 27 September 2016 at 13:27, Kalle Valo <kvalo@codeaurora.org> wrote: >>>> Arend Van Spriel <arend.vanspriel@broadcom.com> writes: >>>> >>>>> On 27-9-2016 11:14, Rafał Miłecki wrote: >>>>>> From: Rafał Miłecki <rafal@milecki.pl> >>>>>> >>>>>> Flowrings contain skbs waiting for transmission that were passed to us >>>>>> by netif. It means we checked every one of them looking for 802.1x >>>>>> Ethernet type. When deleting flowring we have to use freeing function >>>>>> that will check for 802.1x type as well. >>>>>> >>>>>> Freeing skbs without a proper check was leading to counter not being >>>>>> properly decreased. This was triggering a WARNING every time >>>>>> brcmf_netdev_wait_pend8021x was called. >>>>> >>>>> Acked-by: Arend van Spriel <arend@broadcom.com> >>>>>> Signed-off-by: Rafał Miłecki <rafal@milecki.pl> >>>>>> --- >>>>>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>>>>> >>>>>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>>>>> to WARNING on every add_key/del_key call. We was struggling with these >>>>>> WARNINGs for some time and this fixes one of two problems causing them. >>>> >>>> Ok, I'll queue this for 4.9. >>>> >>>>> Please mark it for stable as well. >>>> >>>> I can add that. Any ideas how old releases stable releases should this >>>> go to? >>> >>> I was analyzing this. >>> 1) This patch uses brcmf_get_ifp which is available in 4.4+ only. >>> 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac: >>> Increase nr of supported flowrings.") >>> 3) 4.4 would also require applying to the patch without broadcom/ subdir >>> >>> That said I suggest 4.5+. Any objections? > > No objections. Just a tip. I tend to look at kernel.org main page to see > the stable and long-term kernel listed. So 4.7+ and 4.5+ have same > meaning as 4.5 and 4.6 are not stable/long-term kernels. Some projects may work on their own stable kernels, e.g. Ubuntu, see: https://wiki.ubuntu.com/Kernel/Dev/ExtendedStable That's why I don't always look strictly at upstream stable releases only.
Rafał Miłecki <zajec5@gmail.com> writes: >>>>> Kalle: this isn't important enough for 4.8 as it's too late for that. >>>>> >>>>> I'd like to get it for 4.9 however, as this fixes bug that could lead >>>>> to WARNING on every add_key/del_key call. We was struggling with these >>>>> WARNINGs for some time and this fixes one of two problems causing them. >>> >>> Ok, I'll queue this for 4.9. >>> >>>> Please mark it for stable as well. >>> >>> I can add that. Any ideas how old releases stable releases should this >>> go to? >> >> I was analyzing this. >> 1) This patch uses brcmf_get_ifp which is available in 4.4+ only. >> 2) It applies cleanly to 4.5+ only due to 32f90caa7debd ("brcmfmac: >> Increase nr of supported flowrings.") >> 3) 4.4 would also require applying to the patch without broadcom/ subdir >> >> That said I suggest 4.5+. Any objections? > > Let me see if patchwork with pick Cc tag as it does for others. > > Cc: stable@vger.kernel.org # 4.5+ An excellent idea but no luck: Signed-off-by: Rafa? Mi?ecki <rafal@milecki.pl> Acked-by: Arend van Spriel <arend@broadcom.com> I'll add this to my patchwork wishlist though, I think it would be a really useful feature to have. (The question marks are because of my buggy copy paste, ignore those)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c index b16b367..d0b738d 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/flowring.c @@ -234,13 +234,20 @@ static void brcmf_flowring_block(struct brcmf_flowring *flow, u16 flowid, void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid) { + struct brcmf_bus *bus_if = dev_get_drvdata(flow->dev); struct brcmf_flowring_ring *ring; + struct brcmf_if *ifp; u16 hash_idx; + u8 ifidx; struct sk_buff *skb; ring = flow->rings[flowid]; if (!ring) return; + + ifidx = brcmf_flowring_ifidx_get(flow, flowid); + ifp = brcmf_get_ifp(bus_if->drvr, ifidx); + brcmf_flowring_block(flow, flowid, false); hash_idx = ring->hash_id; flow->hash[hash_idx].ifidx = BRCMF_FLOWRING_INVALID_IFIDX; @@ -249,7 +256,7 @@ void brcmf_flowring_delete(struct brcmf_flowring *flow, u16 flowid) skb = skb_dequeue(&ring->skblist); while (skb) { - brcmu_pkt_buf_free_skb(skb); + brcmf_txfinalize(ifp, skb, false); skb = skb_dequeue(&ring->skblist); }