+ Yann, Christophe
On Mon, 20 Mar 2023 at 13:22, Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
Seems reasonable to me, thanks for fixing this! Feel free to add:
Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
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@xxxxxxxxx>
> ---
> - 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
>
