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
| ||
|
Message-Id: <X8KLAR.4CU0LCMZIMJH@crapouillou.net> Date: Tue, 19 Apr 2022 18:22:09 +0100 From: Paul Cercueil <paul@...pouillou.net> To: Arend van Spriel <arend.vanspriel@...adcom.com> Cc: Arend van Spriel <aspriel@...il.com>, Franky Lin <franky.lin@...adcom.com>, Hante Meuleman <hante.meuleman@...adcom.com>, Kalle Valo <kvalo@...nel.org>, 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 Hi Arend, Le lun., avril 18 2022 at 09:09:46 +0200, Arend van Spriel <arend.vanspriel@...adcom.com> a écrit : > 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. Exactly. I will improve the commit message to make this a bit more explicit. >> 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. Sure. Cheers, -Paul > >> /* try to attach to the target device */ >> sdiodev->bus = brcmf_sdio_probe(sdiodev);
Powered by blists - more mailing lists