linux-kernel - Re: [PATCH] brcmfmac: Remove #ifdef guards for PM related functions

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 18 Apr 2022 09:09:46 +0200
From:   Arend van Spriel <arend.vanspriel@...adcom.com>
To:     Paul Cercueil <paul@...pouillou.net>,
        Arend van Spriel <aspriel@...il.com>,
        Franky Lin <franky.lin@...adcom.com>,
        Hante Meuleman <hante.meuleman@...adcom.com>,
        Kalle Valo <kvalo@...nel.org>
Cc:     linux-wireless@...r.kernel.org,
        brcm80211-dev-list.pdl@...adcom.com,
        SHA-cyfmac-dev-list@...ineon.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] brcmfmac: Remove #ifdef guards for PM related functions

On 4/15/2022 10:03 PM, Paul Cercueil wrote:
> Use the new DEFINE_SIMPLE_DEV_PM_OPS() and pm_sleep_ptr() macros to
> handle the .suspend/.resume callbacks.
> 
> These macros allow the suspend and resume functions to be automatically
> dropped by the compiler when CONFIG_SUSPEND is disabled, without having
> to use #ifdef guards. The advantage is then that these functions are not
> conditionally compiled.

The advantage stated here may not be obvious to everyone and that is 
because it only scratches the surface. The code is always compiled 
independent from the Kconfig options used and because of that the real 
advantage is that build regressions are easier to catch.

> Some other functions not directly called by the .suspend/.resume
> callbacks, but still related to PM were also taken outside #ifdef
> guards.

a few comments on this inline...

> Signed-off-by: Paul Cercueil <paul@...pouillou.net>
> ---
>   .../broadcom/brcm80211/brcmfmac/bcmsdh.c      | 44 +++++++------------
>   .../broadcom/brcm80211/brcmfmac/sdio.c        |  5 +--
>   .../broadcom/brcm80211/brcmfmac/sdio.h        | 16 -------
>   3 files changed, 19 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> index ac02244a6fdf..a8cf5a570101 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bcmsdh.c

[...]

> @@ -873,7 +865,8 @@ int brcmf_sdiod_remove(struct brcmf_sdio_dev *sdiodev)
>   		sdiodev->bus = NULL;
>   	}
>   
> -	brcmf_sdiod_freezer_detach(sdiodev);
> +	if (IS_ENABLED(CONFIG_PM_SLEEP))
> +		brcmf_sdiod_freezer_detach(sdiodev);

Please move the if statement inside the function to keep the code flow 
in the calling function the same as before.

>   
>   	/* Disable Function 2 */
>   	sdio_claim_host(sdiodev->func2);
> @@ -949,9 +942,11 @@ int brcmf_sdiod_probe(struct brcmf_sdio_dev *sdiodev)
>   		goto out;
>   	}
>   
> -	ret = brcmf_sdiod_freezer_attach(sdiodev);
> -	if (ret)
> -		goto out;
> +	if (IS_ENABLED(CONFIG_PM_SLEEP)) {
> +		ret = brcmf_sdiod_freezer_attach(sdiodev);
> +		if (ret)
> +			goto out;
> +	}

Dito. Move the if statement inside the function.

>   
>   	/* try to attach to the target device */
>   	sdiodev->bus = brcmf_sdio_probe(sdiodev);

Download attachment "smime.p7s" of type "application/pkcs7-signature" (4219 bytes)