Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Eric Bentley]

UmV0dXJuIGVycm9yIHdoZW4gZmFpbGluZyB0byBzZXQgcG93ZXIgbWFuYWdlbWVudCBjYXBhYmls
aXRpZXMgZmxhZy4gIFRoaXMgd2lsbA0KY2F1c2UgdGhlIHN1c3BlbmQgdG8gZmFpbCBidXQgdGhl
IHJhZGlvIHdpbGwgY29udGludWUgdG8gb3BlcmF0ZS4gIEFsbG93aW5nIHRoaXMNCnRvIGZhaWwg
d2l0aG91dCByZXBvcnRpbmcgZXJyb3Igd2lsbCBjYXVzZSB0aGUgcmFkaW8gdG8gYmUgbm9uLWZ1
bmN0aW9uYWwgb24gDQpyZXN1bWUgYXMgaXQgd2lsbCBoYXZlIGxvc3QgcG93ZXIuDQoNClNpZ25l
ZC1vZmYtYnk6IEVyaWMgQmVudGxleSBlcmljLmJlbnRsZXlAbGFpcmR0ZWNoLmNvbQ0KLS0tDQp2
MjogY29ycmVjdGVkIGVycmFudCAoIHdpdGggew0KLS0tDQpkcml2ZXJzL25ldC93aXJlbGVzcy9i
cm9hZGNvbS9icmNtODAyMTEvYnJjbWZtYWMvYmNtc2RoLmMgfCA0ICsrKy0NCjEgZmlsZSBjaGFu
Z2VkLCAzIGluc2VydGlvbnMoKyksIDEgZGVsZXRpb24oLSkNCg0KZGlmZiAtLWdpdCBhL2RyaXZl
cnMvbmV0L3dpcmVsZXNzL2Jyb2FkY29tL2JyY204MDIxMS9icmNtZm1hYy9iY21zZGguYyBiL2Ry
aXZlcnMvbmV0L3dpcmVsZXNzL2Jyb2FkY29tL2JyY204MDIxMS9icmNtZm1hYy9iY21zZGguYw0K
aW5kZXggNzIxMzliNS4uMmY3ZDAzZiAxMDA2NDQNCi0tLSBhL2RyaXZlcnMvbmV0L3dpcmVsZXNz
L2Jyb2FkY29tL2JyY204MDIxMS9icmNtZm1hYy9iY21zZGguYw0KKysrIGIvZHJpdmVycy9uZXQv
d2lyZWxlc3MvYnJvYWRjb20vYnJjbTgwMjExL2JyY21mbWFjL2JjbXNkaC5jDQpAQCAtMTI2NCw4
ICsxMjY0LDEwIEBAIHN0YXRpYyBpbnQgYnJjbWZfb3BzX3NkaW9fc3VzcGVuZChzdHJ1Y3QgZGV2
aWNlICpkZXYpDQogICAgICAgICAgICAgICAgIGVsc2UNCiAgICAgICAgICAgICAgICAgICAgICAg
ICBzZGlvX2ZsYWdzIHw9IE1NQ19QTV9XQUtFX1NESU9fSVJROw0KICAgICAgICAgfQ0KLSAgICAg
ICBpZiAoc2Rpb19zZXRfaG9zdF9wbV9mbGFncyhzZGlvZGV2LT5mdW5jWzFdLCBzZGlvX2ZsYWdz
KSkNCisgICAgICAgaWYgKHNkaW9fc2V0X2hvc3RfcG1fZmxhZ3Moc2Rpb2Rldi0+ZnVuY1sxXSwg
c2Rpb19mbGFncykpIHsNCiAgICAgICAgICAgICAgICAgYnJjbWZfZXJyKCJGYWlsZWQgdG8gc2V0
IHBtX2ZsYWdzICV4XG4iLCBzZGlvX2ZsYWdzKTsNCisgICAgICAgICAgICAgICByZXR1cm4gLUVJ
TlZBTDsNCisgICAgICAgfQ0KICAgICAgICAgcmV0dXJuIDA7DQp9DQotLQ0KMi42LjAuR0lUDQoN
Cg==

Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Arend van Spriel]

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(-)

Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Steve deRosier]

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

Re: [PATCH v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Arend van Spriel]

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

Re: [v2] brcmfmac: Correctly fail to suspend when SDIO does not support power on suspend

[Kalle Valo]

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.

-- 
https://patchwork.kernel.org/patch/9934065/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches