| Message ID | 20170822112550.60311-18-ian@mnementh.co.uk (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
Hi Ian, On Tue, Aug 22, 2017 at 9:25 PM, Ian Molton <ian@mnementh.co.uk> wrote: > All functions that might require the window address changing call > brcmf_sdiod_set_backplane_window() prior to access. Thus resetting > the window is not required. Wouldn't it be more safe to write these sorts of functions so that they set the window themselves instead of relying on the caller? > Signed-off-by: Ian Molton <ian@mnementh.co.uk> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > index 0d37c68637f2..cabfab9a02a2 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c > @@ -721,11 +721,6 @@ brcmf_sdiod_ramrw(struct brcmf_sdio_dev *sdiodev, bool write, u32 address, > > dev_kfree_skb(pkt); > > - /* Return the window to backplane enumeration space for core access */ > - if (brcmf_sdiod_set_backplane_window(sdiodev, sdiodev->sbwad)) > - brcmf_err("FAILED to set window back to 0x%x\n", > - sdiodev->sbwad); > - > sdio_release_host(sdiodev->func[1]); > > return err; > -- > 2.11.0 >
On 22-08-17 14:50, Julian Calaby wrote: > Hi Ian, > > On Tue, Aug 22, 2017 at 9:25 PM, Ian Molton <ian@mnementh.co.uk> wrote: >> All functions that might require the window address changing call >> brcmf_sdiod_set_backplane_window() prior to access. Thus resetting >> the window is not required. > > Wouldn't it be more safe to write these sorts of functions so that > they set the window themselves instead of relying on the caller? The function brcmf_sdiod_set_backplane_window() is a helper function by which these sorts of functions set the window themselves. It would be duplicating too much code as setting the window involves three register accesses (or so) on the device. Regards, Arend
On 22-08-17 21:38, Arend van Spriel wrote: > On 22-08-17 14:50, Julian Calaby wrote: >> Hi Ian, >> >> On Tue, Aug 22, 2017 at 9:25 PM, Ian Molton <ian@mnementh.co.uk> wrote: >>> All functions that might require the window address changing call >>> brcmf_sdiod_set_backplane_window() prior to access. Thus resetting >>> the window is not required. >> >> Wouldn't it be more safe to write these sorts of functions so that >> they set the window themselves instead of relying on the caller? > > The function brcmf_sdiod_set_backplane_window() is a helper function by > which these sorts of functions set the window themselves. It would be > duplicating too much code as setting the window involves three register > accesses (or so) on the device. By the way, the driver prefix in the subject is wrong, ie. brcmfamc should be brcmfmac. I think it already was wrong in the previous series, but I forgot to mention it. Regards, Arend
On 22/08/17 20:44, Arend van Spriel wrote: > By the way, the driver prefix in the subject is wrong, ie. brcmfamc > should be brcmfmac. I think it already was wrong in the previous series, > but I forgot to mention it. Drat. well spotted :) -Ian
Hi Arend, On Wed, Aug 23, 2017 at 5:38 AM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > On 22-08-17 14:50, Julian Calaby wrote: >> >> Hi Ian, >> >> On Tue, Aug 22, 2017 at 9:25 PM, Ian Molton <ian@mnementh.co.uk> wrote: >>> >>> All functions that might require the window address changing call >>> brcmf_sdiod_set_backplane_window() prior to access. Thus resetting >>> the window is not required. >> >> >> Wouldn't it be more safe to write these sorts of functions so that >> they set the window themselves instead of relying on the caller? > > > The function brcmf_sdiod_set_backplane_window() is a helper function by > which these sorts of functions set the window themselves. It would be > duplicating too much code as setting the window involves three register > accesses (or so) on the device. I don't think I explained my point well. The description of this patch implies that this method brcmf_sdiod_ramrw(), now relies on the window being set elsewhere before it's called. I'm asking why we can't move the setting of the window inside this function and remove any redundant calls to brcmf_sdiod_set_backplane_window() outside of it. Thanks,
On 23/08/17 11:09, Julian Calaby wrote: > I don't think I explained my point well. > > The description of this patch implies that this method > brcmf_sdiod_ramrw(), now relies on the window being set elsewhere > before it's called. I don't think it does. > I'm asking why we can't move the setting of the window inside this > function and remove any redundant calls to > brcmf_sdiod_set_backplane_window() outside of it. We do call brcmf_sdiod_set_backplane_window() in this function. We just dont need to call it a second time to restore the original window. It may have been required in the past because the code relied on a value not changing (swbadr) which was a poor assumption. In practice, the current code does not provoke this potential bug, which I remove all possibility of happening in patch 23/30 -Ian
Hi Ian, On Wed, Aug 23, 2017 at 10:27 PM, Ian Molton <ian@mnementh.co.uk> wrote: > On 23/08/17 11:09, Julian Calaby wrote: >> I don't think I explained my point well. >> >> The description of this patch implies that this method >> brcmf_sdiod_ramrw(), now relies on the window being set elsewhere >> before it's called. > > I don't think it does. "Resetting" can mean both changing it back after setting it to something or changing it from some other state to a known one. I read it as the latter. >> I'm asking why we can't move the setting of the window inside this >> function and remove any redundant calls to >> brcmf_sdiod_set_backplane_window() outside of it. > > We do call brcmf_sdiod_set_backplane_window() in this function. We just > dont need to call it a second time to restore the original window. Ah, I do not have the full code in front of me, so I couldn't see that, therefore my point is invalid. Thanks,
On 23/08/17 13:36, Julian Calaby wrote: > Hi Ian, > > On Wed, Aug 23, 2017 at 10:27 PM, Ian Molton <ian@mnementh.co.uk> wrote: >> On 23/08/17 11:09, Julian Calaby wrote: >>> I don't think I explained my point well. >>> >>> The description of this patch implies that this method >>> brcmf_sdiod_ramrw(), now relies on the window being set elsewhere >>> before it's called. >> >> I don't think it does. > > "Resetting" can mean both changing it back after setting it to > something or changing it from some other state to a known one. I read > it as the latter. True >>> I'm asking why we can't move the setting of the window inside this >>> function and remove any redundant calls to >>> brcmf_sdiod_set_backplane_window() outside of it. >> >> We do call brcmf_sdiod_set_backplane_window() in this function. We just >> dont need to call it a second time to restore the original window. > > Ah, I do not have the full code in front of me, so I couldn't see > that, therefore my point is invalid. No worries. -Ian
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 0d37c68637f2..cabfab9a02a2 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -721,11 +721,6 @@ brcmf_sdiod_ramrw(struct brcmf_sdio_dev *sdiodev, bool write, u32 address, dev_kfree_skb(pkt); - /* Return the window to backplane enumeration space for core access */ - if (brcmf_sdiod_set_backplane_window(sdiodev, sdiodev->sbwad)) - brcmf_err("FAILED to set window back to 0x%x\n", - sdiodev->sbwad); - sdio_release_host(sdiodev->func[1]); return err;
All functions that might require the window address changing call brcmf_sdiod_set_backplane_window() prior to access. Thus resetting the window is not required. Signed-off-by: Ian Molton <ian@mnementh.co.uk> --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 5 ----- 1 file changed, 5 deletions(-)