spi: bcm63xx: remove PM_SLEEP based conditional compilation

[PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Dhruva Gole 2 years, 9 months ago

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)
 {
 	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)
-- 
2.25.1

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Guenter Roeck 2 years, 9 months ago

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

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Gole, Dhruva 2 years, 9 months ago

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

-- 
Regards,
Dhruva Gole <d-gole@ti.com>

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Guenter Roeck 2 years, 9 months ago

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
>

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Mark Brown 2 years, 9 months ago

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.

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Florian Fainelli 2 years, 9 months ago

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?
-- 
Florian

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Jonas Gorski 2 years, 9 months ago

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

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Dhruva Gole 2 years, 9 months ago

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/

-- 
Thanks and Regards,
Dhruva Gole

Re: [PATCH] spi: bcm63xx: remove PM_SLEEP based conditional compilation

Posted by Mark Brown 2 years, 9 months ago

On Thu, 20 Apr 2023 17:46:15 +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")
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[1/1] spi: bcm63xx: remove PM_SLEEP based conditional compilation
      commit: 25f0617109496e1aff49594fbae5644286447a0f

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark