* [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
@ 2023-03-20 12:22 Hans de Goede
2023-03-23 14:33 ` Ulf Hansson
2023-03-31 15:00 ` Kalle Valo
0 siblings, 2 replies; 7+ messages in thread
From: Hans de Goede @ 2023-03-20 12:22 UTC (permalink / raw)
To: Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo
Cc: Hans de Goede, linux-wireless, brcm80211-dev-list.pdl,
SHA-cyfmac-dev-list, Ulf Hansson
After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
unless WOWL is used"), the wifi adapter by default is turned off on suspend
and then re-probed on resume.
In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
the newly added re-probe on resume fails like this:
brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
ieee80211 phy1: brcmf_bus_started: failed: -110
ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
It seems this specific brcmfmac model does not like being reprobed without
it actually being turned off first.
And the adapter is not being turned off during suspend because of
commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
driver").
Now that the driver is being reprobed on resume, the disabling of ACPI
pm is no longer necessary, except when WOWL is used (in which case there
is no-reprobe).
Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
the brcmfmac43430a1 suspend/resume regression and should help save some
power when suspended.
This change means that the code now also may re-enable ACPI pm when WOWL
gets disabled. ACPI pm should only be re-enabled if it was enabled by
the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
to save the original state for this.
This has been tested on the following devices:
Asus T100TA brcmfmac43241b4-sdio
Acer Iconia One 7 B1-750 brcmfmac43340-sdio
Chuwi Hi8 brcmfmac43430a0-sdio
Chuwi Hi8 brcmfmac43430a1-sdio
(the Asus T100TA is the device for which the prohibiting of ACPI pm
was originally added)
Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop no longer used "struct device *dev" local variable from
brcmf_ops_sdio_probe() - Reported-by: kernel test robot <lkp@intel.com>
---
- Note this is a resend of v2 with Kalle Valo/s email address fixed
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 36 +++++++++++++------
.../broadcom/brcm80211/brcmfmac/sdio.h | 2 ++
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index b7c918f241c9..65d4799a5658 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -994,15 +994,34 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
MODULE_DEVICE_TABLE(sdio, brcmf_sdmmc_ids);
-static void brcmf_sdiod_acpi_set_power_manageable(struct device *dev,
- int val)
+static void brcmf_sdiod_acpi_save_power_manageable(struct brcmf_sdio_dev *sdiodev)
{
#if IS_ENABLED(CONFIG_ACPI)
struct acpi_device *adev;
- adev = ACPI_COMPANION(dev);
+ adev = ACPI_COMPANION(&sdiodev->func1->dev);
if (adev)
- adev->flags.power_manageable = 0;
+ sdiodev->func1_power_manageable = adev->flags.power_manageable;
+
+ adev = ACPI_COMPANION(&sdiodev->func2->dev);
+ if (adev)
+ sdiodev->func2_power_manageable = adev->flags.power_manageable;
+#endif
+}
+
+static void brcmf_sdiod_acpi_set_power_manageable(struct brcmf_sdio_dev *sdiodev,
+ int enable)
+{
+#if IS_ENABLED(CONFIG_ACPI)
+ struct acpi_device *adev;
+
+ adev = ACPI_COMPANION(&sdiodev->func1->dev);
+ if (adev)
+ adev->flags.power_manageable = enable ? sdiodev->func1_power_manageable : 0;
+
+ adev = ACPI_COMPANION(&sdiodev->func2->dev);
+ if (adev)
+ adev->flags.power_manageable = enable ? sdiodev->func2_power_manageable : 0;
#endif
}
@@ -1012,7 +1031,6 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
int err;
struct brcmf_sdio_dev *sdiodev;
struct brcmf_bus *bus_if;
- struct device *dev;
brcmf_dbg(SDIO, "Enter\n");
brcmf_dbg(SDIO, "Class=%x\n", func->class);
@@ -1020,14 +1038,9 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
brcmf_dbg(SDIO, "sdio device ID: 0x%04x\n", func->device);
brcmf_dbg(SDIO, "Function#: %d\n", func->num);
- dev = &func->dev;
-
/* Set MMC_QUIRK_LENIENT_FN0 for this card */
func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
- /* prohibit ACPI power management for this device */
- brcmf_sdiod_acpi_set_power_manageable(dev, 0);
-
/* Consume func num 1 but dont do anything with it. */
if (func->num == 1)
return 0;
@@ -1059,6 +1072,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
dev_set_drvdata(&sdiodev->func1->dev, bus_if);
sdiodev->dev = &sdiodev->func1->dev;
+ brcmf_sdiod_acpi_save_power_manageable(sdiodev);
brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_DOWN);
brcmf_dbg(SDIO, "F2 found, calling brcmf_sdiod_probe...\n");
@@ -1124,6 +1138,8 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
if (sdiodev->settings->bus.sdio.oob_irq_supported ||
pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
+ /* Stop ACPI from turning off the device when wowl is enabled */
+ brcmf_sdiod_acpi_set_power_manageable(sdiodev, !enabled);
sdiodev->wowl_enabled = enabled;
brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
return;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
index b76d34d36bde..0d18ed15b403 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
@@ -188,6 +188,8 @@ struct brcmf_sdio_dev {
char nvram_name[BRCMF_FW_NAME_LEN];
char clm_name[BRCMF_FW_NAME_LEN];
bool wowl_enabled;
+ bool func1_power_manageable;
+ bool func2_power_manageable;
enum brcmf_sdiod_state state;
struct brcmf_sdiod_freezer *freezer;
const struct firmware *clm_fw;
--
2.39.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
2023-03-20 12:22 [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression Hans de Goede
@ 2023-03-23 14:33 ` Ulf Hansson
2023-03-24 7:11 ` Kalle Valo
2023-03-24 8:12 ` Yann Gautier
2023-03-31 15:00 ` Kalle Valo
1 sibling, 2 replies; 7+ messages in thread
From: Ulf Hansson @ 2023-03-23 14:33 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo,
linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list,
Yann Gautier, Christophe ROULLIER-SCND-02
+ Yann, Christophe
On Mon, 20 Mar 2023 at 13:22, Hans de Goede <hdegoede@redhat.com> wrote:
>
> After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
> unless WOWL is used"), the wifi adapter by default is turned off on suspend
> and then re-probed on resume.
>
> In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
> the newly added re-probe on resume fails like this:
>
> brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
> ieee80211 phy1: brcmf_bus_started: failed: -110
> ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
> brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
>
> It seems this specific brcmfmac model does not like being reprobed without
> it actually being turned off first.
>
> And the adapter is not being turned off during suspend because of
> commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
> driver").
>
> Now that the driver is being reprobed on resume, the disabling of ACPI
> pm is no longer necessary, except when WOWL is used (in which case there
> is no-reprobe).
>
> Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
> the brcmfmac43430a1 suspend/resume regression and should help save some
> power when suspended.
>
> This change means that the code now also may re-enable ACPI pm when WOWL
> gets disabled. ACPI pm should only be re-enabled if it was enabled by
> the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
> to save the original state for this.
>
> This has been tested on the following devices:
>
> Asus T100TA brcmfmac43241b4-sdio
> Acer Iconia One 7 B1-750 brcmfmac43340-sdio
> Chuwi Hi8 brcmfmac43430a0-sdio
> Chuwi Hi8 brcmfmac43430a1-sdio
>
> (the Asus T100TA is the device for which the prohibiting of ACPI pm
> was originally added)
>
> Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Seems reasonable to me, thanks for fixing this! Feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Kind regards
Uffe
> ---
> Changes in v2:
> - Drop no longer used "struct device *dev" local variable from
> brcmf_ops_sdio_probe() - Reported-by: kernel test robot <lkp@intel.com>
> ---
> - Note this is a resend of v2 with Kalle Valo/s email address fixed
> ---
> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 36 +++++++++++++------
> .../broadcom/brcm80211/brcmfmac/sdio.h | 2 ++
> 2 files changed, 28 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index b7c918f241c9..65d4799a5658 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> @@ -994,15 +994,34 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
> MODULE_DEVICE_TABLE(sdio, brcmf_sdmmc_ids);
>
>
> -static void brcmf_sdiod_acpi_set_power_manageable(struct device *dev,
> - int val)
> +static void brcmf_sdiod_acpi_save_power_manageable(struct brcmf_sdio_dev *sdiodev)
> {
> #if IS_ENABLED(CONFIG_ACPI)
> struct acpi_device *adev;
>
> - adev = ACPI_COMPANION(dev);
> + adev = ACPI_COMPANION(&sdiodev->func1->dev);
> if (adev)
> - adev->flags.power_manageable = 0;
> + sdiodev->func1_power_manageable = adev->flags.power_manageable;
> +
> + adev = ACPI_COMPANION(&sdiodev->func2->dev);
> + if (adev)
> + sdiodev->func2_power_manageable = adev->flags.power_manageable;
> +#endif
> +}
> +
> +static void brcmf_sdiod_acpi_set_power_manageable(struct brcmf_sdio_dev *sdiodev,
> + int enable)
> +{
> +#if IS_ENABLED(CONFIG_ACPI)
> + struct acpi_device *adev;
> +
> + adev = ACPI_COMPANION(&sdiodev->func1->dev);
> + if (adev)
> + adev->flags.power_manageable = enable ? sdiodev->func1_power_manageable : 0;
> +
> + adev = ACPI_COMPANION(&sdiodev->func2->dev);
> + if (adev)
> + adev->flags.power_manageable = enable ? sdiodev->func2_power_manageable : 0;
> #endif
> }
>
> @@ -1012,7 +1031,6 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
> int err;
> struct brcmf_sdio_dev *sdiodev;
> struct brcmf_bus *bus_if;
> - struct device *dev;
>
> brcmf_dbg(SDIO, "Enter\n");
> brcmf_dbg(SDIO, "Class=%x\n", func->class);
> @@ -1020,14 +1038,9 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
> brcmf_dbg(SDIO, "sdio device ID: 0x%04x\n", func->device);
> brcmf_dbg(SDIO, "Function#: %d\n", func->num);
>
> - dev = &func->dev;
> -
> /* Set MMC_QUIRK_LENIENT_FN0 for this card */
> func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
>
> - /* prohibit ACPI power management for this device */
> - brcmf_sdiod_acpi_set_power_manageable(dev, 0);
> -
> /* Consume func num 1 but dont do anything with it. */
> if (func->num == 1)
> return 0;
> @@ -1059,6 +1072,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
> dev_set_drvdata(&sdiodev->func1->dev, bus_if);
> sdiodev->dev = &sdiodev->func1->dev;
>
> + brcmf_sdiod_acpi_save_power_manageable(sdiodev);
> brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_DOWN);
>
> brcmf_dbg(SDIO, "F2 found, calling brcmf_sdiod_probe...\n");
> @@ -1124,6 +1138,8 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
>
> if (sdiodev->settings->bus.sdio.oob_irq_supported ||
> pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
> + /* Stop ACPI from turning off the device when wowl is enabled */
> + brcmf_sdiod_acpi_set_power_manageable(sdiodev, !enabled);
> sdiodev->wowl_enabled = enabled;
> brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
> return;
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> index b76d34d36bde..0d18ed15b403 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
> @@ -188,6 +188,8 @@ struct brcmf_sdio_dev {
> char nvram_name[BRCMF_FW_NAME_LEN];
> char clm_name[BRCMF_FW_NAME_LEN];
> bool wowl_enabled;
> + bool func1_power_manageable;
> + bool func2_power_manageable;
> enum brcmf_sdiod_state state;
> struct brcmf_sdiod_freezer *freezer;
> const struct firmware *clm_fw;
> --
> 2.39.1
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
2023-03-23 14:33 ` Ulf Hansson
@ 2023-03-24 7:11 ` Kalle Valo
2023-03-24 7:43 ` Hans de Goede
2023-03-24 8:12 ` Yann Gautier
1 sibling, 1 reply; 7+ messages in thread
From: Kalle Valo @ 2023-03-24 7:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: Hans de Goede, Arend van Spriel, Franky Lin, Hante Meuleman,
linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list,
Yann Gautier, Christophe ROULLIER-SCND-02
Ulf Hansson <ulf.hansson@linaro.org> writes:
> + Yann, Christophe
>
> On Mon, 20 Mar 2023 at 13:22, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
>> unless WOWL is used"), the wifi adapter by default is turned off on suspend
>> and then re-probed on resume.
>>
>> In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
>> the newly added re-probe on resume fails like this:
>>
>> brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
>> ieee80211 phy1: brcmf_bus_started: failed: -110
>> ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
>> brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
>>
>> It seems this specific brcmfmac model does not like being reprobed without
>> it actually being turned off first.
>>
>> And the adapter is not being turned off during suspend because of
>> commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
>> driver").
>>
>> Now that the driver is being reprobed on resume, the disabling of ACPI
>> pm is no longer necessary, except when WOWL is used (in which case there
>> is no-reprobe).
>>
>> Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
>> the brcmfmac43430a1 suspend/resume regression and should help save some
>> power when suspended.
>>
>> This change means that the code now also may re-enable ACPI pm when WOWL
>> gets disabled. ACPI pm should only be re-enabled if it was enabled by
>> the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
>> to save the original state for this.
>>
>> This has been tested on the following devices:
>>
>> Asus T100TA brcmfmac43241b4-sdio
>> Acer Iconia One 7 B1-750 brcmfmac43340-sdio
>> Chuwi Hi8 brcmfmac43430a0-sdio
>> Chuwi Hi8 brcmfmac43430a1-sdio
>>
>> (the Asus T100TA is the device for which the prohibiting of ACPI pm
>> was originally added)
>>
>> Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Seems reasonable to me, thanks for fixing this! Feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
As this is an older regression from v5.19 should I take this to
wireless-next to get more testing time?
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
2023-03-24 7:11 ` Kalle Valo
@ 2023-03-24 7:43 ` Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2023-03-24 7:43 UTC (permalink / raw)
To: Kalle Valo, Ulf Hansson
Cc: Arend van Spriel, Franky Lin, Hante Meuleman, linux-wireless,
brcm80211-dev-list.pdl, SHA-cyfmac-dev-list, Yann Gautier,
Christophe ROULLIER-SCND-02
Hi Kalle,
On 3/24/23 08:11, Kalle Valo wrote:
> Ulf Hansson <ulf.hansson@linaro.org> writes:
>
>> + Yann, Christophe
>>
>> On Mon, 20 Mar 2023 at 13:22, Hans de Goede <hdegoede@redhat.com> wrote:
>>>
>>> After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
>>> unless WOWL is used"), the wifi adapter by default is turned off on suspend
>>> and then re-probed on resume.
>>>
>>> In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
>>> the newly added re-probe on resume fails like this:
>>>
>>> brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
>>> ieee80211 phy1: brcmf_bus_started: failed: -110
>>> ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
>>> brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
>>>
>>> It seems this specific brcmfmac model does not like being reprobed without
>>> it actually being turned off first.
>>>
>>> And the adapter is not being turned off during suspend because of
>>> commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
>>> driver").
>>>
>>> Now that the driver is being reprobed on resume, the disabling of ACPI
>>> pm is no longer necessary, except when WOWL is used (in which case there
>>> is no-reprobe).
>>>
>>> Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
>>> the brcmfmac43430a1 suspend/resume regression and should help save some
>>> power when suspended.
>>>
>>> This change means that the code now also may re-enable ACPI pm when WOWL
>>> gets disabled. ACPI pm should only be re-enabled if it was enabled by
>>> the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
>>> to save the original state for this.
>>>
>>> This has been tested on the following devices:
>>>
>>> Asus T100TA brcmfmac43241b4-sdio
>>> Acer Iconia One 7 B1-750 brcmfmac43340-sdio
>>> Chuwi Hi8 brcmfmac43430a0-sdio
>>> Chuwi Hi8 brcmfmac43430a1-sdio
>>>
>>> (the Asus T100TA is the device for which the prohibiting of ACPI pm
>>> was originally added)
>>>
>>> Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
>>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>>
>> Seems reasonable to me, thanks for fixing this! Feel free to add:
>>
>> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> As this is an older regression from v5.19 should I take this to
> wireless-next to get more testing time?
AFAIK the only ACPI platforms (which this patch impacts) these SDIO wifi modules are used on are x86 Bay Trail and Cherry Trail SoCs, such as the list of _4_ devices I have tested on and it does fix a real regression.
Even though this is a somewhat older regression my personal preference would be for this to get send to Linus sooner rather then later so that this regression gets fixed.
Regards,
Hans
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
2023-03-23 14:33 ` Ulf Hansson
2023-03-24 7:11 ` Kalle Valo
@ 2023-03-24 8:12 ` Yann Gautier
1 sibling, 0 replies; 7+ messages in thread
From: Yann Gautier @ 2023-03-24 8:12 UTC (permalink / raw)
To: Ulf Hansson, Hans de Goede
Cc: Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo,
linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list,
Christophe ROULLIER-SCND-02
On 3/23/23 15:33, Ulf Hansson wrote:
> + Yann, Christophe
>
Hi Ulf,
Thanks for the forward!
The patch only changes code under CONFIG_ACPI, which isn't enabled on
Arm platforms. So no impact for STM32MP1 on which we saw the first
issue, and for which you made the ("brcmfmac: Avoid keeping power to
SDIO card unless WOWL is used") patch.
Best regards,
Yann
> On Mon, 20 Mar 2023 at 13:22, Hans de Goede <hdegoede@redhat.com> wrote:
>>
>> After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
>> unless WOWL is used"), the wifi adapter by default is turned off on suspend
>> and then re-probed on resume.
>>
>> In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
>> the newly added re-probe on resume fails like this:
>>
>> brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
>> ieee80211 phy1: brcmf_bus_started: failed: -110
>> ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
>> brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
>>
>> It seems this specific brcmfmac model does not like being reprobed without
>> it actually being turned off first.
>>
>> And the adapter is not being turned off during suspend because of
>> commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
>> driver").
>>
>> Now that the driver is being reprobed on resume, the disabling of ACPI
>> pm is no longer necessary, except when WOWL is used (in which case there
>> is no-reprobe).
>>
>> Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
>> the brcmfmac43430a1 suspend/resume regression and should help save some
>> power when suspended.
>>
>> This change means that the code now also may re-enable ACPI pm when WOWL
>> gets disabled. ACPI pm should only be re-enabled if it was enabled by
>> the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
>> to save the original state for this.
>>
>> This has been tested on the following devices:
>>
>> Asus T100TA brcmfmac43241b4-sdio
>> Acer Iconia One 7 B1-750 brcmfmac43340-sdio
>> Chuwi Hi8 brcmfmac43430a0-sdio
>> Chuwi Hi8 brcmfmac43430a1-sdio
>>
>> (the Asus T100TA is the device for which the prohibiting of ACPI pm
>> was originally added)
>>
>> Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
>> Cc: Ulf Hansson <ulf.hansson@linaro.org>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>
> Seems reasonable to me, thanks for fixing this! Feel free to add:
>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
>
> Kind regards
> Uffe
>
>> ---
>> Changes in v2:
>> - Drop no longer used "struct device *dev" local variable from
>> brcmf_ops_sdio_probe() - Reported-by: kernel test robot <lkp@intel.com>
>> ---
>> - Note this is a resend of v2 with Kalle Valo/s email address fixed
>> ---
>> .../broadcom/brcm80211/brcmfmac/bcmsdh.c | 36 +++++++++++++------
>> .../broadcom/brcm80211/brcmfmac/sdio.h | 2 ++
>> 2 files changed, 28 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>> index b7c918f241c9..65d4799a5658 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
>> @@ -994,15 +994,34 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
>> MODULE_DEVICE_TABLE(sdio, brcmf_sdmmc_ids);
>>
>>
>> -static void brcmf_sdiod_acpi_set_power_manageable(struct device *dev,
>> - int val)
>> +static void brcmf_sdiod_acpi_save_power_manageable(struct brcmf_sdio_dev *sdiodev)
>> {
>> #if IS_ENABLED(CONFIG_ACPI)
>> struct acpi_device *adev;
>>
>> - adev = ACPI_COMPANION(dev);
>> + adev = ACPI_COMPANION(&sdiodev->func1->dev);
>> if (adev)
>> - adev->flags.power_manageable = 0;
>> + sdiodev->func1_power_manageable = adev->flags.power_manageable;
>> +
>> + adev = ACPI_COMPANION(&sdiodev->func2->dev);
>> + if (adev)
>> + sdiodev->func2_power_manageable = adev->flags.power_manageable;
>> +#endif
>> +}
>> +
>> +static void brcmf_sdiod_acpi_set_power_manageable(struct brcmf_sdio_dev *sdiodev,
>> + int enable)
>> +{
>> +#if IS_ENABLED(CONFIG_ACPI)
>> + struct acpi_device *adev;
>> +
>> + adev = ACPI_COMPANION(&sdiodev->func1->dev);
>> + if (adev)
>> + adev->flags.power_manageable = enable ? sdiodev->func1_power_manageable : 0;
>> +
>> + adev = ACPI_COMPANION(&sdiodev->func2->dev);
>> + if (adev)
>> + adev->flags.power_manageable = enable ? sdiodev->func2_power_manageable : 0;
>> #endif
>> }
>>
>> @@ -1012,7 +1031,6 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
>> int err;
>> struct brcmf_sdio_dev *sdiodev;
>> struct brcmf_bus *bus_if;
>> - struct device *dev;
>>
>> brcmf_dbg(SDIO, "Enter\n");
>> brcmf_dbg(SDIO, "Class=%x\n", func->class);
>> @@ -1020,14 +1038,9 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
>> brcmf_dbg(SDIO, "sdio device ID: 0x%04x\n", func->device);
>> brcmf_dbg(SDIO, "Function#: %d\n", func->num);
>>
>> - dev = &func->dev;
>> -
>> /* Set MMC_QUIRK_LENIENT_FN0 for this card */
>> func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
>>
>> - /* prohibit ACPI power management for this device */
>> - brcmf_sdiod_acpi_set_power_manageable(dev, 0);
>> -
>> /* Consume func num 1 but dont do anything with it. */
>> if (func->num == 1)
>> return 0;
>> @@ -1059,6 +1072,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
>> dev_set_drvdata(&sdiodev->func1->dev, bus_if);
>> sdiodev->dev = &sdiodev->func1->dev;
>>
>> + brcmf_sdiod_acpi_save_power_manageable(sdiodev);
>> brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_DOWN);
>>
>> brcmf_dbg(SDIO, "F2 found, calling brcmf_sdiod_probe...\n");
>> @@ -1124,6 +1138,8 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
>>
>> if (sdiodev->settings->bus.sdio.oob_irq_supported ||
>> pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
>> + /* Stop ACPI from turning off the device when wowl is enabled */
>> + brcmf_sdiod_acpi_set_power_manageable(sdiodev, !enabled);
>> sdiodev->wowl_enabled = enabled;
>> brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
>> return;
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
>> index b76d34d36bde..0d18ed15b403 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
>> @@ -188,6 +188,8 @@ struct brcmf_sdio_dev {
>> char nvram_name[BRCMF_FW_NAME_LEN];
>> char clm_name[BRCMF_FW_NAME_LEN];
>> bool wowl_enabled;
>> + bool func1_power_manageable;
>> + bool func2_power_manageable;
>> enum brcmf_sdiod_state state;
>> struct brcmf_sdiod_freezer *freezer;
>> const struct firmware *clm_fw;
>> --
>> 2.39.1
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
2023-03-20 12:22 [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression Hans de Goede
2023-03-23 14:33 ` Ulf Hansson
@ 2023-03-31 15:00 ` Kalle Valo
1 sibling, 0 replies; 7+ messages in thread
From: Kalle Valo @ 2023-03-31 15:00 UTC (permalink / raw)
To: Hans de Goede
Cc: Arend van Spriel, Franky Lin, Hante Meuleman, Hans de Goede,
linux-wireless, brcm80211-dev-list.pdl, SHA-cyfmac-dev-list,
Ulf Hansson
Hans de Goede <hdegoede@redhat.com> wrote:
> After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
> unless WOWL is used"), the wifi adapter by default is turned off on suspend
> and then re-probed on resume.
>
> In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
> the newly added re-probe on resume fails like this:
>
> brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
> ieee80211 phy1: brcmf_bus_started: failed: -110
> ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
> brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
>
> It seems this specific brcmfmac model does not like being reprobed without
> it actually being turned off first.
>
> And the adapter is not being turned off during suspend because of
> commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
> driver").
>
> Now that the driver is being reprobed on resume, the disabling of ACPI
> pm is no longer necessary, except when WOWL is used (in which case there
> is no-reprobe).
>
> Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
> the brcmfmac43430a1 suspend/resume regression and should help save some
> power when suspended.
>
> This change means that the code now also may re-enable ACPI pm when WOWL
> gets disabled. ACPI pm should only be re-enabled if it was enabled by
> the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
> to save the original state for this.
>
> This has been tested on the following devices:
>
> Asus T100TA brcmfmac43241b4-sdio
> Acer Iconia One 7 B1-750 brcmfmac43340-sdio
> Chuwi Hi8 brcmfmac43430a0-sdio
> Chuwi Hi8 brcmfmac43430a1-sdio
>
> (the Asus T100TA is the device for which the prohibiting of ACPI pm
> was originally added)
>
> Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
> Cc: Ulf Hansson <ulf.hansson@linaro.org>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> Reviewed-by: Ulf Hansson <ulf.hansson@linaro.org>
Patch applied to wireless.git, thanks.
e4efa515d58f wifi: brcmfmac: Fix SDIO suspend/resume regression
--
https://patchwork.kernel.org/project/linux-wireless/patch/20230320122252.240070-1-hdegoede@redhat.com/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression
@ 2023-03-20 12:09 Hans de Goede
0 siblings, 0 replies; 7+ messages in thread
From: Hans de Goede @ 2023-03-20 12:09 UTC (permalink / raw)
To: Arend van Spriel, Franky Lin, Hante Meuleman, Kalle Valo
Cc: Hans de Goede, linux-wireless, brcm80211-dev-list.pdl,
SHA-cyfmac-dev-list, Ulf Hansson
After commit 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card
unless WOWL is used"), the wifi adapter by default is turned off on suspend
and then re-probed on resume.
In at least 2 model x86/acpi tablets with brcmfmac43430a1 wifi adapters,
the newly added re-probe on resume fails like this:
brcmfmac: brcmf_sdio_bus_rxctl: resumed on timeout
ieee80211 phy1: brcmf_bus_started: failed: -110
ieee80211 phy1: brcmf_attach: dongle is not responding: err=-110
brcmfmac: brcmf_sdio_firmware_callback: brcmf_attach failed
It seems this specific brcmfmac model does not like being reprobed without
it actually being turned off first.
And the adapter is not being turned off during suspend because of
commit f0992ace680c ("brcmfmac: prohibit ACPI power management for brcmfmac
driver").
Now that the driver is being reprobed on resume, the disabling of ACPI
pm is no longer necessary, except when WOWL is used (in which case there
is no-reprobe).
Move the dis-/en-abling of ACPI pm to brcmf_sdio_wowl_config(), this fixes
the brcmfmac43430a1 suspend/resume regression and should help save some
power when suspended.
This change means that the code now also may re-enable ACPI pm when WOWL
gets disabled. ACPI pm should only be re-enabled if it was enabled by
the ACPI core originally. Add a brcmf_sdiod_acpi_save_power_manageable()
to save the original state for this.
This has been tested on the following devices:
Asus T100TA brcmfmac43241b4-sdio
Acer Iconia One 7 B1-750 brcmfmac43340-sdio
Chuwi Hi8 brcmfmac43430a0-sdio
Chuwi Hi8 brcmfmac43430a1-sdio
(the Asus T100TA is the device for which the prohibiting of ACPI pm
was originally added)
Fixes: 92cadedd9d5f ("brcmfmac: Avoid keeping power to SDIO card unless WOWL is used")
Cc: Ulf Hansson <ulf.hansson@linaro.org>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v2:
- Drop no longer used "struct device *dev" local variable from
brcmf_ops_sdio_probe() - Reported-by: kernel test robot <lkp@intel.com>
---
.../broadcom/brcm80211/brcmfmac/bcmsdh.c | 36 +++++++++++++------
.../broadcom/brcm80211/brcmfmac/sdio.h | 2 ++
2 files changed, 28 insertions(+), 10 deletions(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
index b7c918f241c9..65d4799a5658 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
@@ -994,15 +994,34 @@ static const struct sdio_device_id brcmf_sdmmc_ids[] = {
MODULE_DEVICE_TABLE(sdio, brcmf_sdmmc_ids);
-static void brcmf_sdiod_acpi_set_power_manageable(struct device *dev,
- int val)
+static void brcmf_sdiod_acpi_save_power_manageable(struct brcmf_sdio_dev *sdiodev)
{
#if IS_ENABLED(CONFIG_ACPI)
struct acpi_device *adev;
- adev = ACPI_COMPANION(dev);
+ adev = ACPI_COMPANION(&sdiodev->func1->dev);
if (adev)
- adev->flags.power_manageable = 0;
+ sdiodev->func1_power_manageable = adev->flags.power_manageable;
+
+ adev = ACPI_COMPANION(&sdiodev->func2->dev);
+ if (adev)
+ sdiodev->func2_power_manageable = adev->flags.power_manageable;
+#endif
+}
+
+static void brcmf_sdiod_acpi_set_power_manageable(struct brcmf_sdio_dev *sdiodev,
+ int enable)
+{
+#if IS_ENABLED(CONFIG_ACPI)
+ struct acpi_device *adev;
+
+ adev = ACPI_COMPANION(&sdiodev->func1->dev);
+ if (adev)
+ adev->flags.power_manageable = enable ? sdiodev->func1_power_manageable : 0;
+
+ adev = ACPI_COMPANION(&sdiodev->func2->dev);
+ if (adev)
+ adev->flags.power_manageable = enable ? sdiodev->func2_power_manageable : 0;
#endif
}
@@ -1012,7 +1031,6 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
int err;
struct brcmf_sdio_dev *sdiodev;
struct brcmf_bus *bus_if;
- struct device *dev;
brcmf_dbg(SDIO, "Enter\n");
brcmf_dbg(SDIO, "Class=%x\n", func->class);
@@ -1020,14 +1038,9 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
brcmf_dbg(SDIO, "sdio device ID: 0x%04x\n", func->device);
brcmf_dbg(SDIO, "Function#: %d\n", func->num);
- dev = &func->dev;
-
/* Set MMC_QUIRK_LENIENT_FN0 for this card */
func->card->quirks |= MMC_QUIRK_LENIENT_FN0;
- /* prohibit ACPI power management for this device */
- brcmf_sdiod_acpi_set_power_manageable(dev, 0);
-
/* Consume func num 1 but dont do anything with it. */
if (func->num == 1)
return 0;
@@ -1059,6 +1072,7 @@ static int brcmf_ops_sdio_probe(struct sdio_func *func,
dev_set_drvdata(&sdiodev->func1->dev, bus_if);
sdiodev->dev = &sdiodev->func1->dev;
+ brcmf_sdiod_acpi_save_power_manageable(sdiodev);
brcmf_sdiod_change_state(sdiodev, BRCMF_SDIOD_DOWN);
brcmf_dbg(SDIO, "F2 found, calling brcmf_sdiod_probe...\n");
@@ -1124,6 +1138,8 @@ void brcmf_sdio_wowl_config(struct device *dev, bool enabled)
if (sdiodev->settings->bus.sdio.oob_irq_supported ||
pm_caps & MMC_PM_WAKE_SDIO_IRQ) {
+ /* Stop ACPI from turning off the device when wowl is enabled */
+ brcmf_sdiod_acpi_set_power_manageable(sdiodev, !enabled);
sdiodev->wowl_enabled = enabled;
brcmf_dbg(SDIO, "Configuring WOWL, enabled=%d\n", enabled);
return;
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
index b76d34d36bde..0d18ed15b403 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.h
@@ -188,6 +188,8 @@ struct brcmf_sdio_dev {
char nvram_name[BRCMF_FW_NAME_LEN];
char clm_name[BRCMF_FW_NAME_LEN];
bool wowl_enabled;
+ bool func1_power_manageable;
+ bool func2_power_manageable;
enum brcmf_sdiod_state state;
struct brcmf_sdiod_freezer *freezer;
const struct firmware *clm_fw;
--
2.39.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2023-03-31 15:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 12:22 [PATCH v2] wifi: brcmfmac: Fix SDIO suspend/resume regression Hans de Goede
2023-03-23 14:33 ` Ulf Hansson
2023-03-24 7:11 ` Kalle Valo
2023-03-24 7:43 ` Hans de Goede
2023-03-24 8:12 ` Yann Gautier
2023-03-31 15:00 ` Kalle Valo
-- strict thread matches above, loose matches on Subject: below --
2023-03-20 12:09 Hans de Goede
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).