| Message ID | AC37C967-209B-4BC3-9AF2-9CC4C27391C0@lairdtech.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
On 01-09-17 15:00, Eric Bentley wrote: > Return error when failing to set power management capabilities flag. This will > cause the suspend to fail but the radio will continue to operate. Allowing this > to fail without reporting error will cause the radio to be non-functional on > resume as it will have lost power. There is more to this story to tell. Initially, the suspend callback did return an error upon failing to set the flags. This was dropped by commit 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") as it set the flags only for wowl and that wowl would only be enabled if the host supports the capability flags: +#ifdef CONFIG_PM_SLEEP + /* wowl can be supported when KEEP_POWER is true and (WAKE_SDIO_IRQ + * is true or when platform data OOB irq is true). + */ + if ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_KEEP_POWER) && + ((sdio_get_host_pm_caps(sdiodev->func[1]) & MMC_PM_WAKE_SDIO_IRQ) || + (sdiodev->pdata->oob_irq_supported))) + bus_if->wowl_supported = true; +#endif However, MMC_PM_KEEP_POWER is also needed for the non-wowl case. I restored that in commit bdf1340cf20f ("brcmfmac: fix sdio suspend and resume"), but overlooked the error code return also needed to be restored. Also note that the wifi chip (the term "radio" does not quite cover it) has not really lost power. It is quite common that it is not powered through the SDIO bus. With the power-sequence support in the MMC stack these days the suspend may result in loss of power. Otherwise, it is just the bus that loses power (and clock) and the flags affect what tricks the MMC stack tries to pull to get the device accessible again upon resume. Maybe you can incorporate some of this in the commit message, but you should at least add: Fixes: 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") Fixes: bdf1340cf20f ("brcmfmac: fix sdio suspend and resume") Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Eric Bentley eric.bentley@lairdtech.com > --- > v2: corrected errant ( with { > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-)
On Fri, Sep 1, 2017 at 1:02 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > Also note that the wifi chip (the term "radio" does not quite cover it) has > not really lost power. It is quite common that it is not powered through the > SDIO bus. With the power-sequence support in the MMC stack these days the > suspend may result in loss of power. Otherwise, it is just the bus that > loses power (and clock) and the flags affect what tricks the MMC stack tries > to pull to get the device accessible again upon resume. > Arend, Indeed - I'm familiar with the platform that Eric is using and it is exactly as you say: the chip is powered externally. There's some platform issues here that need to be resolved in addition, but the change to brcmfmac stands alone OK. > Maybe you can incorporate some of this in the commit message, but you should > at least add: > > Fixes: 330b4e4be937 ("brcmfmac: Add wowl support for SDIO devices.") > Fixes: bdf1340cf20f ("brcmfmac: fix sdio suspend and resume") > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > >> Signed-off-by: Eric Bentley eric.bentley@lairdtech.com I've tested the patch on my platforms and works as expected. Eric, please add my: Tested-by: Steve deRosier <derosier@gmail.com> Thanks, - Steve
On 04-09-17 21:33, Steve deRosier wrote: > On Fri, Sep 1, 2017 at 1:02 PM, Arend van Spriel > <arend.vanspriel@broadcom.com> wrote: >> Also note that the wifi chip (the term "radio" does not quite cover it) has >> not really lost power. It is quite common that it is not powered through the >> SDIO bus. With the power-sequence support in the MMC stack these days the >> suspend may result in loss of power. Otherwise, it is just the bus that >> loses power (and clock) and the flags affect what tricks the MMC stack tries >> to pull to get the device accessible again upon resume. >> > Arend, > > Indeed - I'm familiar with the platform that Eric is using and it is > exactly as you say: the chip is powered externally. There's some > platform issues here that need to be resolved in addition, but the > change to brcmfmac stands alone OK. Sure thing. I was not arguing the fix itself. Just wanted to clarify the terms used in the commit message. Regards, Arend
Eric Bentley <eric.bentley@lairdtech.com> wrote: > Return error when failing to set power management capabilities flag. This will > cause the suspend to fail but the radio will continue to operate. Allowing this > to fail without reporting error will cause the radio to be non-functional on > resume as it will have lost power. > > Signed-off-by: Eric Bentley eric.bentley@lairdtech.com The patch is corrupted. It seems you used outlook to submit it which is a recipe for a disaster: https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches#format_issues Also please improve the commit log like Arend mentioned. fatal: corrupt patch at line 23 error: could not build fake ancestor Applying: brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend Patch failed at 0001 brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend The copy of the patch that failed is found in: .git/rebase-apply/patch Patch set to Changes Requested.
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c index 72139b5..2f7d03f 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c @@ -1264,8 +1264,10 @@ static int brcmf_ops_sdio_suspend(struct device *dev) else sdio_flags |= MMC_PM_WAKE_SDIO_IRQ; } - if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags)) + if (sdio_set_host_pm_flags(sdiodev->func[1], sdio_flags)) { brcmf_err("Failed to set pm_flags %x\n", sdio_flags); + return -EINVAL; + } return 0; } -- 2.6.0.GIT
Return error when failing to set power management capabilities flag. This will cause the suspend to fail but the radio will continue to operate. Allowing this to fail without reporting error will cause the radio to be non-functional on resume as it will have lost power. Signed-off-by: Eric Bentley eric.bentley@lairdtech.com --- v2: corrected errant ( with { --- drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)