From: Gavin Li <[email protected]>
brcmf_sdio_fromevntchan() was being called on the the data frame
rather than the software header, causing some frames to be
mischaracterized as on the event channel rather than the data channel.
This fixes a major performance regression (due to dropped packets).
Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
Signed-off-by: Gavin Li <[email protected]>
Cc: <[email protected]> [4.7+]
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
index dfb0658713d9..d2219885071f 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
@@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
pfirst->len, pfirst->next,
pfirst->prev);
skb_unlink(pfirst, &bus->glom);
- if (brcmf_sdio_fromevntchan(pfirst->data))
+ if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
brcmf_rx_event(bus->sdiodev->dev, pfirst);
else
brcmf_rx_frame(bus->sdiodev->dev, pfirst,
--
2.11.0
On 18-1-2017 0:24, [email protected] wrote:
> From: Gavin Li <[email protected]>
>
> brcmf_sdio_fromevntchan() was being called on the the data frame
> rather than the software header, causing some frames to be
> mischaracterized as on the event channel rather than the data channel.
>
> This fixes a major performance regression (due to dropped packets).
>
> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
Actually would prefer Franky to chime in as well as he made the change
and confirm correctness. It was introduced a couple of kernel versions
back and only a performance regression so seems ok to let this go in
4.11 for now and backport as needed for stable later.
Acked-by: Arend van Spriel <[email protected]>
> Signed-off-by: Gavin Li <[email protected]>
> Cc: <[email protected]> [4.7+]
> ---
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> index dfb0658713d9..d2219885071f 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
> pfirst->len, pfirst->next,
> pfirst->prev);
> skb_unlink(pfirst, &bus->glom);
> - if (brcmf_sdio_fromevntchan(pfirst->data))
> + if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
> brcmf_rx_event(bus->sdiodev->dev, pfirst);
> else
> brcmf_rx_frame(bus->sdiodev->dev, pfirst,
>
On 18-1-2017 19:39, Gavin Li wrote:
> I think calling this a performance regression is a bit understated; my
> download speed jumped from 1Mbit/s back up to 40MBit/s after applying
> the patch due to the sheer amount of packets being incorrectly
> processed.
I was merely using your own words although admittedly I left out the
word "major".
> In addition, processing arbitrary data frames as firmware events might
> be a security vulnerability.
True. So I am not saying there is no need to backport it to stable
kernel. I meant there are no reported crashes related to this bug so
there is no need to push it through the current release cycle.
Regards,
Arend
> On Wed, Jan 18, 2017 at 4:38 AM, Arend Van Spriel
> <[email protected]> wrote:
>> On 18-1-2017 0:24, [email protected] wrote:
>>> From: Gavin Li <[email protected]>
>>>
>>> brcmf_sdio_fromevntchan() was being called on the the data frame
>>> rather than the software header, causing some frames to be
>>> mischaracterized as on the event channel rather than the data channel.
>>>
>>> This fixes a major performance regression (due to dropped packets).
>>>
>>> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
>>
>> Actually would prefer Franky to chime in as well as he made the change
>> and confirm correctness. It was introduced a couple of kernel versions
>> back and only a performance regression so seems ok to let this go in
>> 4.11 for now and backport as needed for stable later.
>>
>> Acked-by: Arend van Spriel <[email protected]>
>>> Signed-off-by: Gavin Li <[email protected]>
>>> Cc: <[email protected]> [4.7+]
>>> ---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> index dfb0658713d9..d2219885071f 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>>> pfirst->len, pfirst->next,
>>> pfirst->prev);
>>> skb_unlink(pfirst, &bus->glom);
>>> - if (brcmf_sdio_fromevntchan(pfirst->data))
>>> + if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
>>> brcmf_rx_event(bus->sdiodev->dev, pfirst);
>>> else
>>> brcmf_rx_frame(bus->sdiodev->dev, pfirst,
>>>
Gavin Li <[email protected]> wrote:
> From: Gavin Li <[email protected]>
>
> brcmf_sdio_fromevntchan() was being called on the the data frame
> rather than the software header, causing some frames to be
> mischaracterized as on the event channel rather than the data channel.
>
> This fixes a major performance regression (due to dropped packets). With
> this patch the download speed jumped from 1Mbit/s back up to 40MBit/s due
> to the sheer amount of packets being incorrectly processed.
>
> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
> Signed-off-by: Gavin Li <[email protected]>
> Cc: <[email protected]> # 4.7+
> Acked-by: Arend van Spriel <[email protected]>
> [[email protected]: improve commit logs based on email discussion]
Patch applied to wireless-drivers-next.git, thanks.
8e290cecdd01 brcmfmac: fix incorrect event channel deduction
--
https://patchwork.kernel.org/patch/9522185/
Documentation about submitting wireless patches and checking status
from patchwork:
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
I think calling this a performance regression is a bit understated; my
download speed jumped from 1Mbit/s back up to 40MBit/s after applying
the patch due to the sheer amount of packets being incorrectly
processed.
In addition, processing arbitrary data frames as firmware events might
be a security vulnerability.
On Wed, Jan 18, 2017 at 4:38 AM, Arend Van Spriel
<[email protected]> wrote:
> On 18-1-2017 0:24, [email protected] wrote:
>> From: Gavin Li <[email protected]>
>>
>> brcmf_sdio_fromevntchan() was being called on the the data frame
>> rather than the software header, causing some frames to be
>> mischaracterized as on the event channel rather than the data channel.
>>
>> This fixes a major performance regression (due to dropped packets).
>>
>> Fixes: c56caa9db8ab ("brcmfmac: screening firmware event packet")
>
> Actually would prefer Franky to chime in as well as he made the change
> and confirm correctness. It was introduced a couple of kernel versions
> back and only a performance regression so seems ok to let this go in
> 4.11 for now and backport as needed for stable later.
>
> Acked-by: Arend van Spriel <[email protected]>
>> Signed-off-by: Gavin Li <[email protected]>
>> Cc: <[email protected]> [4.7+]
>> ---
>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> index dfb0658713d9..d2219885071f 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>> @@ -1661,7 +1661,7 @@ static u8 brcmf_sdio_rxglom(struct brcmf_sdio *bus, u8 rxseq)
>> pfirst->len, pfirst->next,
>> pfirst->prev);
>> skb_unlink(pfirst, &bus->glom);
>> - if (brcmf_sdio_fromevntchan(pfirst->data))
>> + if (brcmf_sdio_fromevntchan(&dptr[SDPCM_HWHDR_LEN]))
>> brcmf_rx_event(bus->sdiodev->dev, pfirst);
>> else
>> brcmf_rx_frame(bus->sdiodev->dev, pfirst,
>>
Gavin Li <[email protected]> writes:
> I think calling this a performance regression is a bit understated; my
> download speed jumped from 1Mbit/s back up to 40MBit/s after applying
> the patch due to the sheer amount of packets being incorrectly
> processed.
>
> In addition, processing arbitrary data frames as firmware events might
> be a security vulnerability.
You should always mention valuable information like this in the commit
log, don't make us maintainers (and others) guessing the symptoms and
impact.
--
Kalle Valo
Gavin Li <[email protected]> writes:
> Should I make a v4 patch with an updated log?
No need, I'll just copy your description of the bug to the commit log
before I commit it.
--
Kalle Valo