| Message ID | 20230420121615.967487-1-d-gole@ti.com |
|---|---|
| State | Accepted |
| Commit | 25f0617109496e1aff49594fbae5644286447a0f |
| Headers | show |
| Series | spi: bcm63xx: remove PM_SLEEP based conditional compilation | expand |
On 4/20/23 05:16, Dhruva Gole wrote: > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > it may introduce build issues with certain configs where it maybe disabled > This is because if above config is not enabled the suspend-resume > functions are never part of the code but the bcm63xx_spi_pm_ops struct > still inits them to non-existent suspend-resume functions. > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > --- > drivers/spi/spi-bcm63xx.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c > index 96633a0051b1..99395932074c 100644 > --- a/drivers/spi/spi-bcm63xx.c > +++ b/drivers/spi/spi-bcm63xx.c > @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) > clk_disable_unprepare(bs->clk); > } > > -#ifdef CONFIG_PM_SLEEP > static int bcm63xx_spi_suspend(struct device *dev) Don't we need a __maybe_unused here?
On Fri, 21 Apr 2023 at 19:17, Florian Fainelli <f.fainelli@gmail.com> wrote: > > On 4/20/23 05:16, Dhruva Gole wrote: > > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > > it may introduce build issues with certain configs where it maybe disabled > > This is because if above config is not enabled the suspend-resume > > functions are never part of the code but the bcm63xx_spi_pm_ops struct > > still inits them to non-existent suspend-resume functions. > > > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > > > Signed-off-by: Dhruva Gole <d-gole@ti.com> > > --- > > drivers/spi/spi-bcm63xx.c | 2 -- > > 1 file changed, 2 deletions(-) > > > > diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c > > index 96633a0051b1..99395932074c 100644 > > --- a/drivers/spi/spi-bcm63xx.c > > +++ b/drivers/spi/spi-bcm63xx.c > > @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) > > clk_disable_unprepare(bs->clk); > > } > > > > -#ifdef CONFIG_PM_SLEEP > > static int bcm63xx_spi_suspend(struct device *dev) > > Don't we need a __maybe_unused here? Actually the premise of this patch is wrong, and should rather be reverted. The bcm63xx_spi_pm_ops struct is initialized with SET_SYSTEM_SLEEP_PM_OPS(), which is defined as #ifdef CONFIG_PM_SLEEP #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) #else #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) #endif so for !CONFIG_PM_SLEEP it won't initialize the struct at all (or reference non-existing functions), and therefore there will be no build issues. Regards, Jonas
Hi Jonas, On 24/04/23 13:50, Jonas Gorski wrote: > On Fri, 21 Apr 2023 at 19:17, Florian Fainelli <f.fainelli@gmail.com> wrote: >> >> On 4/20/23 05:16, Dhruva Gole wrote: >>> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >>> it may introduce build issues with certain configs where it maybe disabled >>> This is because if above config is not enabled the suspend-resume >>> functions are never part of the code but the bcm63xx_spi_pm_ops struct >>> still inits them to non-existent suspend-resume functions. >>> >>> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >>> >>> Signed-off-by: Dhruva Gole <d-gole@ti.com> >>> --- >>> drivers/spi/spi-bcm63xx.c | 2 -- >>> 1 file changed, 2 deletions(-) >>> >>> diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c >>> index 96633a0051b1..99395932074c 100644 >>> --- a/drivers/spi/spi-bcm63xx.c >>> +++ b/drivers/spi/spi-bcm63xx.c >>> @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) >>> clk_disable_unprepare(bs->clk); >>> } >>> >>> -#ifdef CONFIG_PM_SLEEP >>> static int bcm63xx_spi_suspend(struct device *dev) >> >> Don't we need a __maybe_unused here? > > Actually the premise of this patch is wrong, and should rather be reverted. > > The bcm63xx_spi_pm_ops struct is initialized with > SET_SYSTEM_SLEEP_PM_OPS(), which is defined as > > #ifdef CONFIG_PM_SLEEP > #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) \ > SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) > #else > #define SET_SYSTEM_SLEEP_PM_OPS(suspend_fn, resume_fn) > #endif > Thanks for pointing this out, I must have missed this. Anyway, I have sent another patch to migrate to using DEFINE_SIMPLE_DEV_PM_OPS as per Mark's suggestion [0]. There I think it would be necessary to remove the CONFIG_PM_SLEEP checks in the driver. So no need to revert this patch. > so for !CONFIG_PM_SLEEP it won't initialize the struct at all (or > reference non-existing functions), and therefore there will be no > build issues. > > Regards, > Jonas [0] https://lore.kernel.org/all/e65683c1-9f1b-4cfb-8e14-087ef7d69595@sirena.org.uk/
On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: > Get rid of conditional compilation based on CONFIG_PM_SLEEP because > it may introduce build issues with certain configs where it maybe disabled > This is because if above config is not enabled the suspend-resume > functions are never part of the code but the bcm63xx_spi_pm_ops struct > still inits them to non-existent suspend-resume functions. > > Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") > > Signed-off-by: Dhruva Gole <d-gole@ti.com> This patch results in: drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] 632 | static int bcm63xx_spi_resume(struct device *dev) | ^~~~~~~~~~~~~~~~~~ drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] 620 | static int bcm63xx_spi_suspend(struct device *dev) on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, riscv, s390). Guenter
Hi, On 4/25/2023 8:38 PM, Guenter Roeck wrote: > On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: >> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >> it may introduce build issues with certain configs where it maybe disabled >> This is because if above config is not enabled the suspend-resume >> functions are never part of the code but the bcm63xx_spi_pm_ops struct >> still inits them to non-existent suspend-resume functions. >> >> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >> >> Signed-off-by: Dhruva Gole <d-gole@ti.com> > This patch results in: > > drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] > 632 | static int bcm63xx_spi_resume(struct device *dev) > | ^~~~~~~~~~~~~~~~~~ > drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] > 620 | static int bcm63xx_spi_suspend(struct device *dev) > > on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, > riscv, s390). Thanks for pointing this out. I could send a patch to address this as pointed here [0] by adding a __maybe_unused. However, do you think my other patch [1] could address this issue without the need for above? I think it would because DEFINE_SIMPLE_DEV_PM_OPS doesn't seem to be under any conditional CONFIG_PM. However, I may have missed something, please do let me know what's the best way to fix. [0] https://lore.kernel.org/all/24ec3728-9720-ae6a-9ff5-3e2e13a96f76@gmail.com/ [1] https://lore.kernel.org/all/20230424102546.1604484-1-d-gole@ti.com/ > > Guenter
On 4/25/23 10:18, Gole, Dhruva wrote: > Hi, > > On 4/25/2023 8:38 PM, Guenter Roeck wrote: >> On Thu, Apr 20, 2023 at 05:46:15PM +0530, Dhruva Gole wrote: >>> Get rid of conditional compilation based on CONFIG_PM_SLEEP because >>> it may introduce build issues with certain configs where it maybe disabled >>> This is because if above config is not enabled the suspend-resume >>> functions are never part of the code but the bcm63xx_spi_pm_ops struct >>> still inits them to non-existent suspend-resume functions. >>> >>> Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") >>> >>> Signed-off-by: Dhruva Gole <d-gole@ti.com> >> This patch results in: >> >> drivers/spi/spi-bcm63xx.c:632:12: error: 'bcm63xx_spi_resume' defined but not used [-Werror=unused-function] >> 632 | static int bcm63xx_spi_resume(struct device *dev) >> | ^~~~~~~~~~~~~~~~~~ >> drivers/spi/spi-bcm63xx.c:620:12: error: 'bcm63xx_spi_suspend' defined but not used [-Werror=unused-function] >> 620 | static int bcm63xx_spi_suspend(struct device *dev) >> >> on architectures with no PM support (alpha, csky, m68k, openrisc, parisc, >> riscv, s390). > > Thanks for pointing this out. > > I could send a patch to address this as pointed here [0] > > by adding a __maybe_unused. > > However, do you think my other patch [1] could address this issue without the need for above? > Personally I would go for [0] as the least invasive solution, but I really have no idea, sorry. I just hope that your (broken) patch doesn't make it as-is into the upstream kernel. Guenter > I think it would because DEFINE_SIMPLE_DEV_PM_OPS doesn't seem to be under any conditional CONFIG_PM. > > However, I may have missed something, please do let me know what's the best way to fix. > > [0] https://lore.kernel.org/all/24ec3728-9720-ae6a-9ff5-3e2e13a96f76@gmail.com/ > > [1] https://lore.kernel.org/all/20230424102546.1604484-1-d-gole@ti.com/ > >> >> Guenter >
On Tue, Apr 25, 2023 at 10:37:24AM -0700, Guenter Roeck wrote: > Personally I would go for [0] as the least invasive solution, but I really > have no idea, sorry. I just hope that your (broken) patch doesn't make it > as-is into the upstream kernel. I've applied the SIMPLE_DEV_PM_OPS patch which seems to fix the issue for riscv.
diff --git a/drivers/spi/spi-bcm63xx.c b/drivers/spi/spi-bcm63xx.c index 96633a0051b1..99395932074c 100644 --- a/drivers/spi/spi-bcm63xx.c +++ b/drivers/spi/spi-bcm63xx.c @@ -617,7 +617,6 @@ static void bcm63xx_spi_remove(struct platform_device *pdev) clk_disable_unprepare(bs->clk); } -#ifdef CONFIG_PM_SLEEP static int bcm63xx_spi_suspend(struct device *dev) { struct spi_master *master = dev_get_drvdata(dev); @@ -644,7 +643,6 @@ static int bcm63xx_spi_resume(struct device *dev) return 0; } -#endif static const struct dev_pm_ops bcm63xx_spi_pm_ops = { SET_SYSTEM_SLEEP_PM_OPS(bcm63xx_spi_suspend, bcm63xx_spi_resume)
Get rid of conditional compilation based on CONFIG_PM_SLEEP because it may introduce build issues with certain configs where it maybe disabled This is because if above config is not enabled the suspend-resume functions are never part of the code but the bcm63xx_spi_pm_ops struct still inits them to non-existent suspend-resume functions. Fixes: b42dfed83d95 ("spi: add Broadcom BCM63xx SPI controller driver") Signed-off-by: Dhruva Gole <d-gole@ti.com> --- drivers/spi/spi-bcm63xx.c | 2 -- 1 file changed, 2 deletions(-)