| Message ID | 20170726202557.15632-8-ian@mnementh.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 26-07-17 22:25, Ian Molton wrote: > This function is obfuscating how IO works on this chip. Remove it > and push its logic into brcmf_sdiod_reg_{read,write}(). > > Handling of -ENOMEDIUM is altered, but as that's pretty much broken anyway > we can ignore that. Please explain why you think it is broken. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Ian Molton <ian@mnementh.co.uk> more comments below. > # Conflicts: > # drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > --- > .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 239 ++++++++------------- > .../wireless/broadcom/brcm80211/brcmfmac/sdio.h | 2 +- > 2 files changed, 87 insertions(+), 154 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index d217b1281e0d..f703d7be6a85 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c [...] > static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr, > u8 regsz, void *data) > { > - u8 func; > - s32 retry = 0; > int ret; > > - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) > - return -ENOMEDIUM; > - > /* > * figure out how to read the register based on address range > * 0x00 ~ 0x7FF: function 0 CCCR and FBR > * 0x10000 ~ 0x1FFFF: function 1 miscellaneous registers > * The rest: function 1 silicon backplane core registers > + * f0 writes must be bytewise > */ > - if ((addr & ~REG_F0_REG_MASK) == 0) > - func = SDIO_FUNC_0; > - else > - func = SDIO_FUNC_1; > - > - do { > - /* for retry wait for 1 ms till bus get settled down */ > - if (retry) > - usleep_range(1000, 2000); > > - ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz, > - data, true); > - > - } while (ret != 0 && ret != -ENOMEDIUM && > - retry++ < SDIOH_API_ACCESS_RETRY_LIMIT); > + if ((addr & ~REG_F0_REG_MASK) == 0) { > + if (WARN_ON(regsz > 1)) > + return -EINVAL; > + ret = brcmf_sdiod_f0_writeb(sdiodev->func[0], *(u8 *)data, addr); > + } else { > + switch (regsz) { > + case 1: > + sdio_writeb(sdiodev->func[1], *(u8 *)data, addr, &ret); > + break; > + case 4: > + ret = brcmf_sdiod_addrprep(sdiodev, &addr); > + if (ret) > + goto done; > > - if (ret == -ENOMEDIUM) > - brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM); > + sdio_writel(sdiodev->func[1], *(u32 *)data, addr, &ret); > + break; > + default: > + BUG(); Please do not use BUG() as it simply crashes the system. You may argue that we never reach this unless a coding mistake is made, but still we prefer WARN() over BUG() in such cases. > + ret = -EINVAL; > + break; > + } > + } > > +done: > return ret; > } > > static int brcmf_sdiod_reg_read(struct brcmf_sdio_dev *sdiodev, u32 addr, > u8 regsz, void *data) > { > - u8 func; > - s32 retry = 0; > int ret; > > - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) > - return -ENOMEDIUM; > - > /* > * figure out how to read the register based on address range > * 0x00 ~ 0x7FF: function 0 CCCR and FBR > * 0x10000 ~ 0x1FFFF: function 1 miscellaneous registers > * The rest: function 1 silicon backplane core registers > + * f0 reads must be bytewise > */ > - if ((addr & ~REG_F0_REG_MASK) == 0) > - func = SDIO_FUNC_0; > - else > - func = SDIO_FUNC_1; > - > - do { > - memset(data, 0, regsz); > - > - /* for retry wait for 1 ms till bus get settled down */ > - if (retry) > - usleep_range(1000, 2000); > - > - ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz, > - data, false); > - > - } while (ret != 0 && ret != -ENOMEDIUM && > - retry++ < SDIOH_API_ACCESS_RETRY_LIMIT); > - > - if (ret == -ENOMEDIUM) > - brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM); > - > - return ret; > -} > - > -static int > -brcmf_sdiod_set_sbaddr_window(struct brcmf_sdio_dev *sdiodev, u32 address) > -{ > - int err = 0, i; > - u32 addr; > - > - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) > - return -ENOMEDIUM; > - > - addr = (address & SBSDIO_SBWINDOW_MASK) >> 8; > - > - for (i = 0 ; i < 3 && !err ; i++, addr >>= 8) > - brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, > - addr & 0xff, &err); > - > - return err; > -} > - > -static int > -brcmf_sdiod_addrprep(struct brcmf_sdio_dev *sdiodev, u32 *addr) > -{ > - uint bar0 = *addr & ~SBSDIO_SB_OFT_ADDR_MASK; > - int err = 0; > - > - if (bar0 != sdiodev->sbwad) { > - err = brcmf_sdiod_set_sbaddr_window(sdiodev, bar0); > - if (err) > - return err; > + if ((addr & ~REG_F0_REG_MASK) == 0) { > + if (WARN_ON(regsz > 1)) > + return -EINVAL; > + *(u8 *)data = sdio_f0_readb(sdiodev->func[0], addr, &ret); > + } else { > + switch (regsz) { > + case 1: > + *(u8 *)data = sdio_readb(sdiodev->func[1], addr, &ret); > + break; > + case 4: > + ret = brcmf_sdiod_addrprep(sdiodev, &addr); > + if (ret) > + goto done; > > - sdiodev->sbwad = bar0; > + *(u32 *)data = sdio_readl(sdiodev->func[1], addr, &ret); > + break; > + default: > + BUG(); same here as with reg_write() function. > + ret = -EINVAL; > + break; > + } > } > > - *addr &= SBSDIO_SB_OFT_ADDR_MASK; > - *addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; > - > - return 0; > +done: > + return ret; > } > > u8 brcmf_sdiod_regrb(struct brcmf_sdio_dev *sdiodev, u32 addr, int *ret) > @@ -439,15 +386,9 @@ u32 brcmf_sdiod_regrl(struct brcmf_sdio_dev *sdiodev, u32 addr, int *ret) > int retval; > > brcmf_dbg(SDIO, "addr:0x%08x\n", addr); > - retval = brcmf_sdiod_addrprep(sdiodev, &addr); > - if (retval) > - goto done; > - > - retval = brcmf_sdiod_reg_read(sdiodev, addr, 4, &data); > - > brcmf_dbg(SDIO, "data:0x%08x\n", data); Now this debug statement move before the actual read, which makes it useless. You are going to remove it in a subsequent patch, but still. > + retval = brcmf_sdiod_reg_read(sdiodev, addr, 4, &data); > > -done: > if (ret) > *ret = retval; >
On 07/08/17 12:25, Arend van Spriel wrote: >> Handling of -ENOMEDIUM is altered, but as that's pretty much broken >> anyway >> we can ignore that. > > Please explain why you think it is broken. Not got the code to hand right now, but from memory, theres a trapdoor case where the state can wind up set to something that prevents it ever being changed again. I'll dig it up when I get back from holiday (this next few days). -Ian
On 07/08/17 18:51, Ian Molton wrote: > On 07/08/17 12:25, Arend van Spriel wrote: >>> Handling of -ENOMEDIUM is altered, but as that's pretty much broken >>> anyway >>> we can ignore that. >> >> Please explain why you think it is broken. > > Not got the code to hand right now, but from memory, theres a trapdoor > case where the state can wind up set to something that prevents it ever > being changed again. I'll dig it up when I get back from holiday (this > next few days). Hi, Here is the function I had in mind: void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, enum brcmf_sdiod_state state) { if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM || state == sdiodev->state) return; brcmf_dbg(TRACE, "%d -> %d\n", sdiodev->state, state); switch (sdiodev->state) { case BRCMF_SDIOD_DATA: /* any other state means bus interface is down */ brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_DOWN); break; case BRCMF_SDIOD_DOWN: /* transition from DOWN to DATA means bus interface is up */ if (state == BRCMF_SDIOD_DATA) brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_UP); break; default: break; } sdiodev->state = state; } If it's *ever* called with state = BRCMF_SDIOD_NOMEDIUM it will eventually (last line) set sdiodev->state to the same value. If its ever called again, the first if() statement will make it return before ever changing sdiodev->state again, no matter what value is passed for state. This has to be a bug, surely? -Ian
On 19-08-17 22:02, Ian Molton wrote: > On 07/08/17 18:51, Ian Molton wrote: >> On 07/08/17 12:25, Arend van Spriel wrote: >>>> Handling of -ENOMEDIUM is altered, but as that's pretty much broken >>>> anyway >>>> we can ignore that. >>> >>> Please explain why you think it is broken. >> >> Not got the code to hand right now, but from memory, theres a trapdoor >> case where the state can wind up set to something that prevents it ever >> being changed again. I'll dig it up when I get back from holiday (this >> next few days). > > Hi, > > Here is the function I had in mind: > > > void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, > enum brcmf_sdiod_state state) > { > if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM || > state == sdiodev->state) > return; > > brcmf_dbg(TRACE, "%d -> %d\n", sdiodev->state, state); > switch (sdiodev->state) { > case BRCMF_SDIOD_DATA: > /* any other state means bus interface is down */ > brcmf_bus_change_state(sdiodev->bus_if, BRCMF_BUS_DOWN); > break; > case BRCMF_SDIOD_DOWN: > /* transition from DOWN to DATA means bus interface is up */ > if (state == BRCMF_SDIOD_DATA) > brcmf_bus_change_state(sdiodev->bus_if, > BRCMF_BUS_UP); > break; > default: > break; > } > sdiodev->state = state; > } > > > If it's *ever* called with state = BRCMF_SDIOD_NOMEDIUM it will > eventually (last line) set sdiodev->state to the same value. > > If its ever called again, the first if() statement will make it return > before ever changing sdiodev->state again, no matter what value is > passed for state. > > This has to be a bug, surely? I thought I already responded this email. So it is not a bug. It really was made like this intentional. It is the end state of this little FSM. The thing is that there was no way to recover. Maybe nowadays with the MMC stack being able to power sequence the device (provided it is properly configured in device tree) we may get lucky using mmc_hw_reset(). But now you need to consider how many times to call that before giving up and what driver components/states need to be reset as well. So I would prefer to keep the current behavior until a more graceful approach has been designed and implemented to replace it. Regards, Arend
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index d217b1281e0d..f703d7be6a85 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -230,6 +230,43 @@ void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, sdiodev->state = state; } +static int brcmf_sdiod_set_sbaddr_window(struct brcmf_sdio_dev *sdiodev, + u32 address) +{ + int err = 0, i; + u32 addr; + + if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) + return -ENOMEDIUM; + + addr = (address & SBSDIO_SBWINDOW_MASK) >> 8; + + for (i = 0 ; i < 3 && !err ; i++, addr >>= 8) + brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, + addr & 0xff, &err); + + return err; +} + +static int brcmf_sdiod_addrprep(struct brcmf_sdio_dev *sdiodev, u32 *addr) +{ + uint bar0 = *addr & ~SBSDIO_SB_OFT_ADDR_MASK; + int err = 0; + + if (bar0 != sdiodev->sbwad) { + err = brcmf_sdiod_set_sbaddr_window(sdiodev, bar0); + if (err) + return err; + + sdiodev->sbwad = bar0; + } + + *addr &= SBSDIO_SB_OFT_ADDR_MASK; + *addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; + + return 0; +} + static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, u8 byte, uint regaddr) { @@ -249,173 +286,83 @@ static inline int brcmf_sdiod_f0_writeb(struct sdio_func *func, u8 byte, return err_ret; } -static int brcmf_sdiod_request_data(struct brcmf_sdio_dev *sdiodev, u8 fn, - u32 addr, u8 regsz, void *data, bool write) -{ - struct sdio_func *func; - int ret = -EINVAL; - - brcmf_dbg(SDIO, "rw=%d, func=%d, addr=0x%05x, nbytes=%d\n", - write, fn, addr, regsz); - - /* only allow byte access on F0 */ - if (WARN_ON(regsz > 1 && !fn)) - return -EINVAL; - func = sdiodev->func[fn]; - - switch (regsz) { - case 1: - if (write) { - if (fn) - sdio_writeb(func, *(u8 *)data, addr, &ret); - else - ret = brcmf_sdiod_f0_writeb(func, *(u8 *)data, - addr); - } else { - if (fn) - *(u8 *)data = sdio_readb(func, addr, &ret); - else - *(u8 *)data = sdio_f0_readb(func, addr, &ret); - } - break; - case 2: - if (write) - sdio_writew(func, *(u16 *)data, addr, &ret); - else - *(u16 *)data = sdio_readw(func, addr, &ret); - break; - case 4: - if (write) - sdio_writel(func, *(u32 *)data, addr, &ret); - else - *(u32 *)data = sdio_readl(func, addr, &ret); - break; - default: - brcmf_err("invalid size: %d\n", regsz); - break; - } - - if (ret) - brcmf_dbg(SDIO, "failed to %s data F%d@0x%05x, err: %d\n", - write ? "write" : "read", fn, addr, ret); - - return ret; -} - static int brcmf_sdiod_reg_write(struct brcmf_sdio_dev *sdiodev, u32 addr, u8 regsz, void *data) { - u8 func; - s32 retry = 0; int ret; - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) - return -ENOMEDIUM; - /* * figure out how to read the register based on address range * 0x00 ~ 0x7FF: function 0 CCCR and FBR * 0x10000 ~ 0x1FFFF: function 1 miscellaneous registers * The rest: function 1 silicon backplane core registers + * f0 writes must be bytewise */ - if ((addr & ~REG_F0_REG_MASK) == 0) - func = SDIO_FUNC_0; - else - func = SDIO_FUNC_1; - - do { - /* for retry wait for 1 ms till bus get settled down */ - if (retry) - usleep_range(1000, 2000); - ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz, - data, true); - - } while (ret != 0 && ret != -ENOMEDIUM && - retry++ < SDIOH_API_ACCESS_RETRY_LIMIT); + if ((addr & ~REG_F0_REG_MASK) == 0) { + if (WARN_ON(regsz > 1)) + return -EINVAL; + ret = brcmf_sdiod_f0_writeb(sdiodev->func[0], *(u8 *)data, addr); + } else { + switch (regsz) { + case 1: + sdio_writeb(sdiodev->func[1], *(u8 *)data, addr, &ret); + break; + case 4: + ret = brcmf_sdiod_addrprep(sdiodev, &addr); + if (ret) + goto done; - if (ret == -ENOMEDIUM) - brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM); + sdio_writel(sdiodev->func[1], *(u32 *)data, addr, &ret); + break; + default: + BUG(); + ret = -EINVAL; + break; + } + } +done: return ret; } static int brcmf_sdiod_reg_read(struct brcmf_sdio_dev *sdiodev, u32 addr, u8 regsz, void *data) { - u8 func; - s32 retry = 0; int ret; - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) - return -ENOMEDIUM; - /* * figure out how to read the register based on address range * 0x00 ~ 0x7FF: function 0 CCCR and FBR * 0x10000 ~ 0x1FFFF: function 1 miscellaneous registers * The rest: function 1 silicon backplane core registers + * f0 reads must be bytewise */ - if ((addr & ~REG_F0_REG_MASK) == 0) - func = SDIO_FUNC_0; - else - func = SDIO_FUNC_1; - - do { - memset(data, 0, regsz); - - /* for retry wait for 1 ms till bus get settled down */ - if (retry) - usleep_range(1000, 2000); - - ret = brcmf_sdiod_request_data(sdiodev, func, addr, regsz, - data, false); - - } while (ret != 0 && ret != -ENOMEDIUM && - retry++ < SDIOH_API_ACCESS_RETRY_LIMIT); - - if (ret == -ENOMEDIUM) - brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_NOMEDIUM); - - return ret; -} - -static int -brcmf_sdiod_set_sbaddr_window(struct brcmf_sdio_dev *sdiodev, u32 address) -{ - int err = 0, i; - u32 addr; - - if (sdiodev->state == BRCMF_SDIOD_NOMEDIUM) - return -ENOMEDIUM; - - addr = (address & SBSDIO_SBWINDOW_MASK) >> 8; - - for (i = 0 ; i < 3 && !err ; i++, addr >>= 8) - brcmf_sdiod_regwb(sdiodev, SBSDIO_FUNC1_SBADDRLOW + i, - addr & 0xff, &err); - - return err; -} - -static int -brcmf_sdiod_addrprep(struct brcmf_sdio_dev *sdiodev, u32 *addr) -{ - uint bar0 = *addr & ~SBSDIO_SB_OFT_ADDR_MASK; - int err = 0; - - if (bar0 != sdiodev->sbwad) { - err = brcmf_sdiod_set_sbaddr_window(sdiodev, bar0); - if (err) - return err; + if ((addr & ~REG_F0_REG_MASK) == 0) { + if (WARN_ON(regsz > 1)) + return -EINVAL; + *(u8 *)data = sdio_f0_readb(sdiodev->func[0], addr, &ret); + } else { + switch (regsz) { + case 1: + *(u8 *)data = sdio_readb(sdiodev->func[1], addr, &ret); + break; + case 4: + ret = brcmf_sdiod_addrprep(sdiodev, &addr); + if (ret) + goto done; - sdiodev->sbwad = bar0; + *(u32 *)data = sdio_readl(sdiodev->func[1], addr, &ret); + break; + default: + BUG(); + ret = -EINVAL; + break; + } } - *addr &= SBSDIO_SB_OFT_ADDR_MASK; - *addr |= SBSDIO_SB_ACCESS_2_4B_FLAG; - - return 0; +done: + return ret; } u8 brcmf_sdiod_regrb(struct brcmf_sdio_dev *sdiodev, u32 addr, int *ret) @@ -439,15 +386,9 @@ u32 brcmf_sdiod_regrl(struct brcmf_sdio_dev *sdiodev, u32 addr, int *ret) int retval; brcmf_dbg(SDIO, "addr:0x%08x\n", addr); - retval = brcmf_sdiod_addrprep(sdiodev, &addr); - if (retval) - goto done; - - retval = brcmf_sdiod_reg_read(sdiodev, addr, 4, &data); - brcmf_dbg(SDIO, "data:0x%08x\n", data); + retval = brcmf_sdiod_reg_read(sdiodev, addr, 4, &data); -done: if (ret) *ret = retval; @@ -472,13 +413,7 @@ void brcmf_sdiod_regwl(struct brcmf_sdio_dev *sdiodev, u32 addr, int retval; brcmf_dbg(SDIO, "addr:0x%08x, data:0x%08x\n", addr, data); - retval = brcmf_sdiod_addrprep(sdiodev, &addr); - if (retval) - goto done; - retval = brcmf_sdiod_reg_write(sdiodev, addr, 4, &data); - -done: if (ret) *ret = retval; } @@ -886,14 +821,12 @@ brcmf_sdiod_ramrw(struct brcmf_sdio_dev *sdiodev, bool write, u32 address, return bcmerror; } -int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, uint fn) +int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, u8 fn) { - char t_func = (char)fn; brcmf_dbg(SDIO, "Enter\n"); /* issue abort cmd52 command through F0 */ - brcmf_sdiod_request_data(sdiodev, SDIO_FUNC_0, SDIO_CCCR_ABORT, - 1, &t_func, true); + brcmf_sdiod_reg_write(sdiodev, SDIO_CCCR_ABORT, 1, &fn); brcmf_dbg(SDIO, "Exit\n"); return 0; diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h index e3b78db331b5..1e4b4760159f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h @@ -339,7 +339,7 @@ int brcmf_sdiod_ramrw(struct brcmf_sdio_dev *sdiodev, bool write, u32 address, u8 *data, uint size); /* Issue an abort to the specified function */ -int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, uint fn); +int brcmf_sdiod_abort(struct brcmf_sdio_dev *sdiodev, u8 fn); void brcmf_sdiod_sgtable_alloc(struct brcmf_sdio_dev *sdiodev); void brcmf_sdiod_change_state(struct brcmf_sdio_dev *sdiodev, enum brcmf_sdiod_state state);
This function is obfuscating how IO works on this chip. Remove it and push its logic into brcmf_sdiod_reg_{read,write}(). Handling of -ENOMEDIUM is altered, but as that's pretty much broken anyway we can ignore that. Signed-off-by: Ian Molton <ian@mnementh.co.uk> # Conflicts: # drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c --- .../wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 239 ++++++++------------- .../wireless/broadcom/brcm80211/brcmfmac/sdio.h | 2 +- 2 files changed, 87 insertions(+), 154 deletions(-)