| Message ID | 20200610152106.175257-4-chi-hsien.lin@cypress.com |
|---|---|
| State | New |
| Headers | show |
| Series | [V2,1/6] brcmfmac: allow credit borrowing for all access categories | expand |
10.06.2020 18:21, Chi-Hsien Lin пишет: > From: Amar Shankar <amsr@cypress.com> > > It is observed that sometimes when sdiod is low in tx credits in low > rssi scenarios, the data path consumes all sdiod rx all credits and > there is no sdiod rx credit available for control path causing host > and card to go out of sync resulting in link loss between host and > card. So in order to prevent it some credits are reserved for control > path. > > Note that TXCTL_CREDITS can't be larger than the firmware default > credit update threshold 2; otherwise there will be a deadlock for both > side waiting for each other. > > Signed-off-by: Amar Shankar <amsr@cypress.com> > Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> > Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> > --- > .../broadcom/brcm80211/brcmfmac/sdio.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index ce6f15284277..4da40436b4ab 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -635,6 +635,8 @@ static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { > BRCMF_FW_ENTRY(CY_CC_43012_CHIP_ID, 0xFFFFFFFF, 43012) > }; > > +#define TXCTL_CREDITS 2 > + > static void pkt_align(struct sk_buff *p, int len, int align) > { > uint datalign; > @@ -648,8 +650,16 @@ static void pkt_align(struct sk_buff *p, int len, int align) > /* To check if there's window offered */ > static bool data_ok(struct brcmf_sdio *bus) > { > - return (u8)(bus->tx_max - bus->tx_seq) != 0 && > - ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; > + /* Reserve TXCTL_CREDITS credits for txctl */ > + return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && > + ((bus->tx_max - bus->tx_seq) & 0x80) == 0; > +} > + > +/* To check if there's window offered */ > +static bool txctl_ok(struct brcmf_sdio *bus) > +{ > + return (bus->tx_max - bus->tx_seq) != 0 && > + ((bus->tx_max - bus->tx_seq) & 0x80) == 0; > } > > static int > @@ -2655,7 +2665,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus) > brcmf_sdio_clrintr(bus); > > if (bus->ctrl_frame_stat && (bus->clkstate == CLK_AVAIL) && > - data_ok(bus)) { > + txctl_ok(bus)) { > sdio_claim_host(bus->sdiodev->func1); > if (bus->ctrl_frame_stat) { > err = brcmf_sdio_tx_ctrlframe(bus, bus->ctrl_frame_buf, > @@ -2663,6 +2673,9 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus) > bus->ctrl_frame_err = err; > wmb(); > bus->ctrl_frame_stat = false; > + if (err) > + brcmf_err("sdio ctrlframe tx failed err=%d\n", > + err); > } > sdio_release_host(bus->sdiodev->func1); > brcmf_sdio_wait_event_wakeup(bus); > Hello, This patch causes a severe WiFi performance regression on BCM4329. Please fix or revert this patch, thanks in advance. Before this patch: - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bitrate Retr [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec 0 sender [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec receiver After this patch: - - - - - - - - - - - - - - - - - - - - - - - - - [ ID] Interval Transfer Bitrate Retr [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec 3 sender [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec receiver
Dmitry Osipenko <digetx@gmail.com> writes: > 10.06.2020 18:21, Chi-Hsien Lin пишет: >> From: Amar Shankar <amsr@cypress.com> >> >> It is observed that sometimes when sdiod is low in tx credits in low >> rssi scenarios, the data path consumes all sdiod rx all credits and >> there is no sdiod rx credit available for control path causing host >> and card to go out of sync resulting in link loss between host and >> card. So in order to prevent it some credits are reserved for control >> path. >> >> Note that TXCTL_CREDITS can't be larger than the firmware default >> credit update threshold 2; otherwise there will be a deadlock for both >> side waiting for each other. >> >> Signed-off-by: Amar Shankar <amsr@cypress.com> >> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> [...] > This patch causes a severe WiFi performance regression on BCM4329. > Please fix or revert this patch, thanks in advance. > > Before this patch: > - - - - - - - - - - - - - - - - - - - - - - - - - > [ ID] Interval Transfer Bitrate Retr > [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec 0 sender > [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec > receiver > > > After this patch: > - - - - - - - - - - - - - - - - - - - - - - - - - > [ ID] Interval Transfer Bitrate Retr > [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec 3 sender > [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec > receiver Can someone please send a revert patch (with the explanation above) if a fix is not quickly found? The commit id is: commit b41c232d33666191a1db11befc0f040fcbe664e9 Author: Amar Shankar <amsr@cypress.com> AuthorDate: Wed Jun 10 10:21:03 2020 -0500 Commit: Kalle Valo <kvalo@codeaurora.org> CommitDate: Tue Jul 14 12:46:43 2020 +0300 brcmfmac: reserve 2 credits for host tx control path -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
04.08.2020 14:08, Kalle Valo пишет: > Dmitry Osipenko <digetx@gmail.com> writes: > >> 10.06.2020 18:21, Chi-Hsien Lin пишет: >>> From: Amar Shankar <amsr@cypress.com> >>> >>> It is observed that sometimes when sdiod is low in tx credits in low >>> rssi scenarios, the data path consumes all sdiod rx all credits and >>> there is no sdiod rx credit available for control path causing host >>> and card to go out of sync resulting in link loss between host and >>> card. So in order to prevent it some credits are reserved for control >>> path. >>> >>> Note that TXCTL_CREDITS can't be larger than the firmware default >>> credit update threshold 2; otherwise there will be a deadlock for both >>> side waiting for each other. >>> >>> Signed-off-by: Amar Shankar <amsr@cypress.com> >>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> > > [...] > >> This patch causes a severe WiFi performance regression on BCM4329. >> Please fix or revert this patch, thanks in advance. >> >> Before this patch: >> - - - - - - - - - - - - - - - - - - - - - - - - - >> [ ID] Interval Transfer Bitrate Retr >> [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec 0 sender >> [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec >> receiver >> >> >> After this patch: >> - - - - - - - - - - - - - - - - - - - - - - - - - >> [ ID] Interval Transfer Bitrate Retr >> [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec 3 sender >> [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec >> receiver > > Can someone please send a revert patch (with the explanation above) if a > fix is not quickly found? The commit id is: > > commit b41c232d33666191a1db11befc0f040fcbe664e9 > Author: Amar Shankar <amsr@cypress.com> > AuthorDate: Wed Jun 10 10:21:03 2020 -0500 > Commit: Kalle Valo <kvalo@codeaurora.org> > CommitDate: Tue Jul 14 12:46:43 2020 +0300 > > brcmfmac: reserve 2 credits for host tx control path > Hello Kalle, I'll send the revert if nobody will stand up to address the problem in a two weeks, thanks.
Dmitry Osipenko <digetx@gmail.com> writes: > 04.08.2020 14:08, Kalle Valo пишет: >> Dmitry Osipenko <digetx@gmail.com> writes: >> >>> 10.06.2020 18:21, Chi-Hsien Lin пишет: >>>> From: Amar Shankar <amsr@cypress.com> >>>> >>>> It is observed that sometimes when sdiod is low in tx credits in low >>>> rssi scenarios, the data path consumes all sdiod rx all credits and >>>> there is no sdiod rx credit available for control path causing host >>>> and card to go out of sync resulting in link loss between host and >>>> card. So in order to prevent it some credits are reserved for control >>>> path. >>>> >>>> Note that TXCTL_CREDITS can't be larger than the firmware default >>>> credit update threshold 2; otherwise there will be a deadlock for both >>>> side waiting for each other. >>>> >>>> Signed-off-by: Amar Shankar <amsr@cypress.com> >>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> >> >> [...] >> >>> This patch causes a severe WiFi performance regression on BCM4329. >>> Please fix or revert this patch, thanks in advance. >>> >>> Before this patch: >>> - - - - - - - - - - - - - - - - - - - - - - - - - >>> [ ID] Interval Transfer Bitrate Retr >>> [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec 0 sender >>> [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec >>> receiver >>> >>> >>> After this patch: >>> - - - - - - - - - - - - - - - - - - - - - - - - - >>> [ ID] Interval Transfer Bitrate Retr >>> [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec 3 sender >>> [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec >>> receiver >> >> Can someone please send a revert patch (with the explanation above) if a >> fix is not quickly found? The commit id is: >> >> commit b41c232d33666191a1db11befc0f040fcbe664e9 >> Author: Amar Shankar <amsr@cypress.com> >> AuthorDate: Wed Jun 10 10:21:03 2020 -0500 >> Commit: Kalle Valo <kvalo@codeaurora.org> >> CommitDate: Tue Jul 14 12:46:43 2020 +0300 >> >> brcmfmac: reserve 2 credits for host tx control path >> > > Hello Kalle, > > I'll send the revert if nobody will stand up to address the problem in a > two weeks, thanks. Thanks. Then I should be able to get the revert to v5.9 so that the release won't be broken. (v5.8 is unaffected) -- https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 8/5/2020 1:22 AM, Kalle Valo wrote: > Dmitry Osipenko <digetx@gmail.com> writes: > >> 04.08.2020 14:08, Kalle Valo пишет: >>> Dmitry Osipenko <digetx@gmail.com> writes: >>> >>>> 10.06.2020 18:21, Chi-Hsien Lin пишет: >>>>> From: Amar Shankar <amsr@cypress.com> >>>>> >>>>> It is observed that sometimes when sdiod is low in tx credits in low >>>>> rssi scenarios, the data path consumes all sdiod rx all credits and >>>>> there is no sdiod rx credit available for control path causing host >>>>> and card to go out of sync resulting in link loss between host and >>>>> card. So in order to prevent it some credits are reserved for control >>>>> path. >>>>> >>>>> Note that TXCTL_CREDITS can't be larger than the firmware default >>>>> credit update threshold 2; otherwise there will be a deadlock for both >>>>> side waiting for each other. >>>>> >>>>> Signed-off-by: Amar Shankar <amsr@cypress.com> >>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> >>> [...] >>> >>>> This patch causes a severe WiFi performance regression on BCM4329. >>>> Please fix or revert this patch, thanks in advance. >>>> >>>> Before this patch: >>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>> [ ID] Interval Transfer Bitrate Retr >>>> [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec 0 sender >>>> [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec >>>> receiver >>>> >>>> >>>> After this patch: >>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>> [ ID] Interval Transfer Bitrate Retr >>>> [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec 3 sender >>>> [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec >>>> receiver >>> Can someone please send a revert patch (with the explanation above) if a >>> fix is not quickly found? The commit id is: >>> >>> commit b41c232d33666191a1db11befc0f040fcbe664e9 >>> Author: Amar Shankar <amsr@cypress.com> >>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500 >>> Commit: Kalle Valo <kvalo@codeaurora.org> >>> CommitDate: Tue Jul 14 12:46:43 2020 +0300 >>> >>> brcmfmac: reserve 2 credits for host tx control path >>> >> Hello Kalle, >> >> I'll send the revert if nobody will stand up to address the problem in a >> two weeks, thanks. > Thanks. Then I should be able to get the revert to v5.9 so that the > release won't be broken. (v5.8 is unaffected) Dmitry/Kalle, We'll take a look and revert/fix it in a few days.
05.08.2020 11:24, Chi-Hsien Lin пишет: > > On 8/5/2020 1:22 AM, Kalle Valo wrote: >> Dmitry Osipenko <digetx@gmail.com> writes: >> >>> 04.08.2020 14:08, Kalle Valo пишет: >>>> Dmitry Osipenko <digetx@gmail.com> writes: >>>> >>>>> 10.06.2020 18:21, Chi-Hsien Lin пишет: >>>>>> From: Amar Shankar <amsr@cypress.com> >>>>>> >>>>>> It is observed that sometimes when sdiod is low in tx credits in low >>>>>> rssi scenarios, the data path consumes all sdiod rx all credits and >>>>>> there is no sdiod rx credit available for control path causing host >>>>>> and card to go out of sync resulting in link loss between host and >>>>>> card. So in order to prevent it some credits are reserved for control >>>>>> path. >>>>>> >>>>>> Note that TXCTL_CREDITS can't be larger than the firmware default >>>>>> credit update threshold 2; otherwise there will be a deadlock for >>>>>> both >>>>>> side waiting for each other. >>>>>> >>>>>> Signed-off-by: Amar Shankar <amsr@cypress.com> >>>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >>>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> >>>> [...] >>>> >>>>> This patch causes a severe WiFi performance regression on BCM4329. >>>>> Please fix or revert this patch, thanks in advance. >>>>> >>>>> Before this patch: >>>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>>> [ ID] Interval Transfer Bitrate Retr >>>>> [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec >>>>> 0 sender >>>>> [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec >>>>> receiver >>>>> >>>>> >>>>> After this patch: >>>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>>> [ ID] Interval Transfer Bitrate Retr >>>>> [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec >>>>> 3 sender >>>>> [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec >>>>> receiver >>>> Can someone please send a revert patch (with the explanation above) >>>> if a >>>> fix is not quickly found? The commit id is: >>>> >>>> commit b41c232d33666191a1db11befc0f040fcbe664e9 >>>> Author: Amar Shankar <amsr@cypress.com> >>>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500 >>>> Commit: Kalle Valo <kvalo@codeaurora.org> >>>> CommitDate: Tue Jul 14 12:46:43 2020 +0300 >>>> >>>> brcmfmac: reserve 2 credits for host tx control path >>>> >>> Hello Kalle, >>> >>> I'll send the revert if nobody will stand up to address the problem in a >>> two weeks, thanks. >> Thanks. Then I should be able to get the revert to v5.9 so that the >> release won't be broken. (v5.8 is unaffected) > > Dmitry/Kalle, > > We'll take a look and revert/fix it in a few days. Good to know, thank you.
Dmitry Osipenko 於 8/5/2020 10:01 PM 寫道: > 05.08.2020 11:24, Chi-Hsien Lin пишет: >> >> On 8/5/2020 1:22 AM, Kalle Valo wrote: >>> Dmitry Osipenko <digetx@gmail.com> writes: >>> >>>> 04.08.2020 14:08, Kalle Valo пишет: >>>>> Dmitry Osipenko <digetx@gmail.com> writes: >>>>> >>>>>> 10.06.2020 18:21, Chi-Hsien Lin пишет: >>>>>>> From: Amar Shankar <amsr@cypress.com> >>>>>>> >>>>>>> It is observed that sometimes when sdiod is low in tx credits in low >>>>>>> rssi scenarios, the data path consumes all sdiod rx all credits and >>>>>>> there is no sdiod rx credit available for control path causing host >>>>>>> and card to go out of sync resulting in link loss between host and >>>>>>> card. So in order to prevent it some credits are reserved for control >>>>>>> path. >>>>>>> >>>>>>> Note that TXCTL_CREDITS can't be larger than the firmware default >>>>>>> credit update threshold 2; otherwise there will be a deadlock for >>>>>>> both >>>>>>> side waiting for each other. >>>>>>> >>>>>>> Signed-off-by: Amar Shankar <amsr@cypress.com> >>>>>>> Signed-off-by: Jia-Shyr Chuang <joseph.chuang@cypress.com> >>>>>>> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> >>>>> [...] >>>>> >>>>>> This patch causes a severe WiFi performance regression on BCM4329. >>>>>> Please fix or revert this patch, thanks in advance. >>>>>> >>>>>> Before this patch: >>>>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>>>> [ ID] Interval Transfer Bitrate Retr >>>>>> [ 5] 0.00-10.00 sec 17.2 MBytes 14.4 Mbits/sec >>>>>> 0 sender >>>>>> [ 5] 0.00-10.04 sec 16.9 MBytes 14.1 Mbits/sec >>>>>> receiver >>>>>> >>>>>> >>>>>> After this patch: >>>>>> - - - - - - - - - - - - - - - - - - - - - - - - - >>>>>> [ ID] Interval Transfer Bitrate Retr >>>>>> [ 5] 0.00-10.00 sec 1.05 MBytes 881 Kbits/sec >>>>>> 3 sender >>>>>> [ 5] 0.00-14.01 sec 959 KBytes 561 Kbits/sec >>>>>> receiver >>>>> Can someone please send a revert patch (with the explanation above) >>>>> if a >>>>> fix is not quickly found? The commit id is: >>>>> >>>>> commit b41c232d33666191a1db11befc0f040fcbe664e9 >>>>> Author: Amar Shankar <amsr@cypress.com> >>>>> AuthorDate: Wed Jun 10 10:21:03 2020 -0500 >>>>> Commit: Kalle Valo <kvalo@codeaurora.org> >>>>> CommitDate: Tue Jul 14 12:46:43 2020 +0300 >>>>> >>>>> brcmfmac: reserve 2 credits for host tx control path >>>>> >>>> Hello Kalle, >>>> >>>> I'll send the revert if nobody will stand up to address the problem in a >>>> two weeks, thanks. >>> Thanks. Then I should be able to get the revert to v5.9 so that the >>> release won't be broken. (v5.8 is unaffected) >> >> Dmitry/Kalle, >> >> We'll take a look and revert/fix it in a few days. > > Good to know, thank you. > Hi Dmitry, We have a fix for this issue. But we got the different test result numbers from yours, so would you help us verify the same with following patch in your setup? With your confirmation, I can make sure we see the same issue. --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index e8712ad3ac45..ea07bb1bfe27 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len, int align) /* To check if there's window offered */ static bool data_ok(struct brcmf_sdio *bus) { - /* Reserve TXCTL_CREDITS credits for txctl */ - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && + u8 tx_rsv = 0; + + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ + if (bus->ctrl_frame_stat) + tx_rsv = TXCTL_CREDITS; + + return (bus->tx_max - bus->tx_seq) > tx_rsv && ((bus->tx_max - bus->tx_seq) & 0x80) == 0; } -- Here is our 4329 TX throughput result. Without the fix: Interval Transfer Bandwidth Retr 0.00-10.00 sec 36.4 MBytes 30.6 Mbits/sec receiver With the fix: Interval Transfer Bandwidth Retr 0.00-10.00 sec 47.9 MBytes 40.2 Mbits/sec receiver
11.08.2020 11:35, Wright Feng пишет: .. > Hi Dmitry, > We have a fix for this issue. But we got the different test result > numbers from yours, so would you help us verify the same with > following patch in your setup? > With your confirmation, I can make sure we see the same issue. > > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index e8712ad3ac45..ea07bb1bfe27 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len, > int align) > /* To check if there's window offered */ > static bool data_ok(struct brcmf_sdio *bus) > { > - /* Reserve TXCTL_CREDITS credits for txctl */ > - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && > + u8 tx_rsv = 0; > + > + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ > + if (bus->ctrl_frame_stat) > + tx_rsv = TXCTL_CREDITS; > + > + return (bus->tx_max - bus->tx_seq) > tx_rsv && > ((bus->tx_max - bus->tx_seq) & 0x80) == 0; > } Hello, Wright! I tried this patch and it doesn't fix the problem. Could you please tell what firmware you're using? I'm using stock firmware that comes from the linux-firmware package and this NVRAM [1]. [1] https://github.com/grate-driver/linux-firmware/blob/master/brcm/brcmfmac4329-sdio.acer%2Cpicasso.txt brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip BCM4329/3 brcmfmac: brcmf_c_process_clm_blob: no clm_blob available (err=-2), device may have limited channels available brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4329/3 wl0: Sep 2 2011 14:48:19 version 4.220.48 It also interesting that you're getting 40Mbit/s. I never managed to get that speed on 4329 using upstream brcmfmac driver, but was getting it using downstream BCMDHD driver. At some point I even tried to figure out what's the difference which makes the upstream driver to have significantly lower throughput than downstream and had older BCMDHD ported to a recent upstream kernel, unfortunately I couldn't spot anything obvious. Having BCMDHD and brcmfmac back-to-back on the same hardware, the same kernel and using same firmware (IIRC), the BCMDHD was more than x2 faster. I also remember that I had to enforce HT mode on AP in order to get speeds over 15Mbit/s using BCMDHD, which made me to conclude that upstream brcmfmac driver just doesn't support that HT mode for 4329, but now I'm seeing yours iperf results and it makes me wonder..
Dmitry Osipenko 於 8/11/2020 10:54 PM 寫道: > 11.08.2020 11:35, Wright Feng пишет: > .. >> Hi Dmitry, >> We have a fix for this issue. But we got the different test result >> numbers from yours, so would you help us verify the same with >> following patch in your setup? >> With your confirmation, I can make sure we see the same issue. >> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> index e8712ad3ac45..ea07bb1bfe27 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> @@ -664,8 +664,13 @@ static void pkt_align(struct sk_buff *p, int len, >> int align) >> /* To check if there's window offered */ >> static bool data_ok(struct brcmf_sdio *bus) >> { >> - /* Reserve TXCTL_CREDITS credits for txctl */ >> - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && >> + u8 tx_rsv = 0; >> + >> + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ >> + if (bus->ctrl_frame_stat) >> + tx_rsv = TXCTL_CREDITS; >> + >> + return (bus->tx_max - bus->tx_seq) > tx_rsv && >> ((bus->tx_max - bus->tx_seq) & 0x80) == 0; >> } > > Hello, Wright! > > I tried this patch and it doesn't fix the problem. > > Could you please tell what firmware you're using? > > I'm using stock firmware that comes from the linux-firmware package and > this NVRAM [1]. > > [1] > https://github.com/grate-driver/linux-firmware/blob/master/brcm/brcmfmac4329-sdio.acer%2Cpicasso.txt > > brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip > BCM4329/3 > brcmfmac: brcmf_c_process_clm_blob: no clm_blob available (err=-2), > device may have limited channels available > brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4329/3 wl0: Sep 2 2011 > 14:48:19 version 4.220.48 > > It also interesting that you're getting 40Mbit/s. I never managed to get > that speed on 4329 using upstream brcmfmac driver, but was getting it > using downstream BCMDHD driver. At some point I even tried to figure out > what's the difference which makes the upstream driver to have > significantly lower throughput than downstream and had older BCMDHD > ported to a recent upstream kernel, unfortunately I couldn't spot > anything obvious. Having BCMDHD and brcmfmac back-to-back on the same > hardware, the same kernel and using same firmware (IIRC), the BCMDHD was > more than x2 faster. I also remember that I had to enforce HT mode on AP > in order to get speeds over 15Mbit/s using BCMDHD, which made me to > conclude that upstream brcmfmac driver just doesn't support that HT mode > for 4329, but now I'm seeing yours iperf results and it makes me wonder.. > Hi Dmitry, The last time the drivers I used is v5.4 and I was not able to see low throughput issue as you saw. At this time, I changed the base to tag wt-2020-0727 and I am able to reproduce the issue as you did. TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec The root cause should be that tx_max and tx_seq are circle positive numbers, it should not use ">" to check if it still exists available TX credit. With the solution below, I am able to get the normal throughput. TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to verify the throughput because we are not able to find 4329b1(4329/3) which is very old product around 10 years ago. The firmware I am using is the same version but the build is for 4329b0. (private internal build - 4.220.48). My host platform is x86_64 with 4 cores on Linux kernel 4.12. I will try your NVRAM when I have time to see if I can find anything. --- diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index e8712ad..50c8107 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len, int align) /* To check if there's window offered */ static bool data_ok(struct brcmf_sdio *bus) { - /* Reserve TXCTL_CREDITS credits for txctl */ - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && - ((bus->tx_max - bus->tx_seq) & 0x80) == 0; + u8 tx_rsv = 0; + + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ + if (bus->ctrl_frame_stat) + tx_rsv = TXCTL_CREDITS; + + return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 && + ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0; } /* To check if there's window offered */ ---
12.08.2020 07:03, Wright Feng пишет: ... > Hi Dmitry, > > The last time the drivers I used is v5.4 and I was not able to see low > throughput issue as you saw. > At this time, I changed the base to tag wt-2020-0727 and I am able to > reproduce the issue as you did. > TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec > > > The root cause should be that tx_max and tx_seq are circle positive > numbers, it should not use ">" to check if it still exists available TX > credit. > With the solution below, I am able to get the normal throughput. > TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec > > Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to > verify the throughput because we are not able to find 4329b1(4329/3) > which is very old product around 10 years ago. > The firmware I am using is the same version but the build is for 4329b0. > (private internal build - 4.220.48). My host platform is x86_64 with 4 > cores on Linux kernel 4.12. I will try your NVRAM when I have time to > see if I can find anything. > > --- > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index e8712ad..50c8107 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len, > int align) > /* To check if there's window offered */ > static bool data_ok(struct brcmf_sdio *bus) > { > - /* Reserve TXCTL_CREDITS credits for txctl */ > - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && > - ((bus->tx_max - bus->tx_seq) & 0x80) == 0; > + u8 tx_rsv = 0; > + > + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ > + if (bus->ctrl_frame_stat) > + tx_rsv = TXCTL_CREDITS; > + > + return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 && > + ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0; > } > > /* To check if there's window offered */ > --- Wright, thank you very much for the patch! It fixes the problem! Tested-by: Dmitry Osipenko <digetx@gmail.com> The 4329/3 is indeed an older chip, but it's also an "old" device (Acer A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got support for the A500. There are quite a lot of other older devices with 4329/3 in a wild that are still very usable if user can wipe off ancient Android and put a modern Linux distro on them. Today that A500 tablet is still rocking hard running a modern upstream kernel, opensource drivers and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things, but of course 40Mbit will be better. Would be great if you could try to help with improving the speed :) Please feel free to contact me at any time if you'll have patches to try!
Dmitry Osipenko 於 8/12/2020 10:22 PM 寫道: > 12.08.2020 07:03, Wright Feng пишет: > ... >> Hi Dmitry, >> >> The last time the drivers I used is v5.4 and I was not able to see low >> throughput issue as you saw. >> At this time, I changed the base to tag wt-2020-0727 and I am able to >> reproduce the issue as you did. >> TX throughput with wt-2020-07-27 FMAC is 190 Kbits/sec >> >> >> The root cause should be that tx_max and tx_seq are circle positive >> numbers, it should not use ">" to check if it still exists available TX >> credit. >> With the solution below, I am able to get the normal throughput. >> TX throughput with wt-2020-07-27+patch FMAC is 40.0 Mbits/sec >> >> Regarding another case about 40Mbit/s, I am using 4329b0(4329/2) chip to >> verify the throughput because we are not able to find 4329b1(4329/3) >> which is very old product around 10 years ago. >> The firmware I am using is the same version but the build is for 4329b0. >> (private internal build - 4.220.48). My host platform is x86_64 with 4 >> cores on Linux kernel 4.12. I will try your NVRAM when I have time to >> see if I can find anything. >> >> --- >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> index e8712ad..50c8107 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> @@ -664,9 +664,14 @@ static void pkt_align(struct sk_buff *p, int len, >> int align) >> /* To check if there's window offered */ >> static bool data_ok(struct brcmf_sdio *bus) >> { >> - /* Reserve TXCTL_CREDITS credits for txctl */ >> - return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && >> - ((bus->tx_max - bus->tx_seq) & 0x80) == 0; >> + u8 tx_rsv = 0; >> + >> + /* Reserve TXCTL_CREDITS credits for txctl when it is ready to send */ >> + if (bus->ctrl_frame_stat) >> + tx_rsv = TXCTL_CREDITS; >> + >> + return (bus->tx_max - bus->tx_seq - tx_rsv) != 0 && >> + ((bus->tx_max - bus->tx_seq - tx_rsv) & 0x80) == 0; >> } >> >> /* To check if there's window offered */ >> --- > > Wright, thank you very much for the patch! It fixes the problem! > > Tested-by: Dmitry Osipenko <digetx@gmail.com> > > The 4329/3 is indeed an older chip, but it's also an "old" device (Acer > A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got > support for the A500. There are quite a lot of other older devices with > 4329/3 in a wild that are still very usable if user can wipe off ancient > Android and put a modern Linux distro on them. Today that A500 tablet is > still rocking hard running a modern upstream kernel, opensource drivers > and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things, > but of course 40Mbit will be better. Would be great if you could try to > help with improving the speed :) Please feel free to contact me at any > time if you'll have patches to try! > Dmitry, Thanks for the verification. The patch will be submitted to upstream within two days. And with regards to low THP problem on Acer A500 tablet, I will create another mail thread to discuss with you.
13.08.2020 06:03, Wright Feng пишет: .. >> Wright, thank you very much for the patch! It fixes the problem! >> >> Tested-by: Dmitry Osipenko <digetx@gmail.com> >> >> The 4329/3 is indeed an older chip, but it's also an "old" device (Acer >> A500 tablet from 2011/12) that I'm using. Upstream v5.9 kernel just got >> support for the A500. There are quite a lot of other older devices with >> 4329/3 in a wild that are still very usable if user can wipe off ancient >> Android and put a modern Linux distro on them. Today that A500 tablet is >> still rocking hard running a modern upstream kernel, opensource drivers >> and KDE Plasma 5. The 15Mbit is a good enough speed for a lot of things, >> but of course 40Mbit will be better. Would be great if you could try to >> help with improving the speed :) Please feel free to contact me at any >> time if you'll have patches to try! >> > Dmitry, > Thanks for the verification. The patch will be submitted to upstream > within two days. > And with regards to low THP problem on Acer A500 tablet, I will create > another mail thread to discuss with you. Thank you!
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index ce6f15284277..4da40436b4ab 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c @@ -635,6 +635,8 @@ static const struct brcmf_firmware_mapping brcmf_sdio_fwnames[] = { BRCMF_FW_ENTRY(CY_CC_43012_CHIP_ID, 0xFFFFFFFF, 43012) }; +#define TXCTL_CREDITS 2 + static void pkt_align(struct sk_buff *p, int len, int align) { uint datalign; @@ -648,8 +650,16 @@ static void pkt_align(struct sk_buff *p, int len, int align) /* To check if there's window offered */ static bool data_ok(struct brcmf_sdio *bus) { - return (u8)(bus->tx_max - bus->tx_seq) != 0 && - ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; + /* Reserve TXCTL_CREDITS credits for txctl */ + return (bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && + ((bus->tx_max - bus->tx_seq) & 0x80) == 0; +} + +/* To check if there's window offered */ +static bool txctl_ok(struct brcmf_sdio *bus) +{ + return (bus->tx_max - bus->tx_seq) != 0 && + ((bus->tx_max - bus->tx_seq) & 0x80) == 0; } static int @@ -2655,7 +2665,7 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus) brcmf_sdio_clrintr(bus); if (bus->ctrl_frame_stat && (bus->clkstate == CLK_AVAIL) && - data_ok(bus)) { + txctl_ok(bus)) { sdio_claim_host(bus->sdiodev->func1); if (bus->ctrl_frame_stat) { err = brcmf_sdio_tx_ctrlframe(bus, bus->ctrl_frame_buf, @@ -2663,6 +2673,9 @@ static void brcmf_sdio_dpc(struct brcmf_sdio *bus) bus->ctrl_frame_err = err; wmb(); bus->ctrl_frame_stat = false; + if (err) + brcmf_err("sdio ctrlframe tx failed err=%d\n", + err); } sdio_release_host(bus->sdiodev->func1); brcmf_sdio_wait_event_wakeup(bus);