| Message ID | 20200609105913.163239-4-chi-hsien.lin@cypress.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | brcmfmac: flow control and p2p fix series | expand |
Chi-Hsien Lin <chi-hsien.lin@cypress.com> writes: > 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 <saint.chuang@cypress.com> > Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> > --- > .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 15 ++++++++++++++- > 1 file changed, 14 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index ce6f15284277..163bb7f41e44 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; > @@ -647,6 +649,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 (u8)(bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && > + ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; > +} Why casting to u8? Is it really necessary?
On 06/09/2020 7:51, Kalle Valo wrote: > Chi-Hsien Lin <chi-hsien.lin@cypress.com> writes: > >> 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 <saint.chuang@cypress.com> >> Signed-off-by: Chi-Hsien Lin <chi-hsien.lin@cypress.com> >> --- >> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 15 ++++++++++++++- >> 1 file changed, 14 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c >> index ce6f15284277..163bb7f41e44 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; >> @@ -647,6 +649,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 (u8)(bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && >> + ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; >> +} > > Why casting to u8? Is it really necessary? I don't think the casting is necessary since tx_max and tx_seq are both u8. The code is taken from a similar function data_ok(). I'll remove the casting from both functions in V2. >
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c index ce6f15284277..163bb7f41e44 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; @@ -647,6 +649,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 (u8)(bus->tx_max - bus->tx_seq) > TXCTL_CREDITS && + ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; +} + +/* To check if there's window offered */ +static bool txctl_ok(struct brcmf_sdio *bus) { return (u8)(bus->tx_max - bus->tx_seq) != 0 && ((u8)(bus->tx_max - bus->tx_seq) & 0x80) == 0; @@ -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);