watchdog: simatic: add PCI dependency

  From: Arnd Bergmann <arnd@arndb.de>

The simatic-ipc driver no longer depends on PCI, but its watchdog portion
still needs it, otherwise P2SB runs into a build  failure:

WARNING: unmet direct dependencies detected for P2SB
  Depends on [n]: PCI [=n] && X86 [=y]
  Selected by [m]:
  - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]

drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
                pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);

Add back the minimum dependendency to make it build in random configurations
again.

Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/watchdog/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

Hi

On 2023/8/14 15:38, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The simatic-ipc driver no longer depends on PCI, but its watchdog portion
> still needs it, otherwise P2SB runs into a build  failure:

If the simatic-ipc driver no longer depends on PCI, you can
delete the dependency here:
https://elixir.bootlin.com/linux/v6.5-rc6/source/drivers/platform/x86/Kconfig#L1079

> 
> WARNING: unmet direct dependencies detected for P2SB
>   Depends on [n]: PCI [=n] && X86 [=y]
>   Selected by [m]:
>   - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]
> 
> drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>                 pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> 
> Add back the minimum dependendency to make it build in random configurations
> again.
> 
> Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 04e9b40cf7d53..09452384221a4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1680,7 +1680,7 @@ config NIC7018_WDT
>  
>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
> -	depends on SIEMENS_SIMATIC_IPC
> +	depends on SIEMENS_SIMATIC_IPC && PCI

The SIEMENS_SIMATIC_IPC_WDT does not depends on PCI directly,
it should depends on P2SB, then P2SB depends on PCI

>  	default y
>  	select WATCHDOG_CORE
>  	select P2SB

You can replace the "select" to "depends on" instead.

Reviewed-by: Xing Tong Wu <xingtong.wu@siemens.com>

--
Xing Tong
  

Hi,

On 8/14/23 09:38, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The simatic-ipc driver no longer depends on PCI, but its watchdog portion
> still needs it, otherwise P2SB runs into a build  failure:
> 
> WARNING: unmet direct dependencies detected for P2SB
>   Depends on [n]: PCI [=n] && X86 [=y]
>   Selected by [m]:
>   - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]
> 
> drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>                 pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> 
> Add back the minimum dependendency to make it build in random configurations
> again.
> 
> Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hdegoede@redhat.com>

Wachdog maintainers the commit causing the fixed issue is part of my
platform-drivers-x86-simatic-ipc branch:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=platform-drivers-x86-simatic-ipc

And more importantly that branch already contains a conflicting commit to the SIEMENS_SIMATIC_IPC_WDT Kconfig entry, which was merged through that branch with
Guenter's ack.

So I'll also pick up this fix and merge it through that branch to avoid conflicts.

Regards,

Hans




> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 04e9b40cf7d53..09452384221a4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1680,7 +1680,7 @@ config NIC7018_WDT
>  
>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
> -	depends on SIEMENS_SIMATIC_IPC
> +	depends on SIEMENS_SIMATIC_IPC && PCI
>  	default y
>  	select WATCHDOG_CORE
>  	select P2SB
  

Hi,

On 8/14/23 09:38, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The simatic-ipc driver no longer depends on PCI, but its watchdog portion
> still needs it, otherwise P2SB runs into a build  failure:
> 
> WARNING: unmet direct dependencies detected for P2SB
>   Depends on [n]: PCI [=n] && X86 [=y]
>   Selected by [m]:
>   - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]
> 
> drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>                 pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> 
> Add back the minimum dependendency to make it build in random configurations
> again.
> 
> Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thank you for your patch, I've applied this patch to my review-hans 
branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans



> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 04e9b40cf7d53..09452384221a4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1680,7 +1680,7 @@ config NIC7018_WDT
>  
>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
> -	depends on SIEMENS_SIMATIC_IPC
> +	depends on SIEMENS_SIMATIC_IPC && PCI
>  	default y
>  	select WATCHDOG_CORE
>  	select P2SB
  

Hi Xing Tong,

On 8/14/23 10:27, xingtong.wu wrote:
> Hi
> 
> On 2023/8/14 15:38, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@arndb.de>
>>
>> The simatic-ipc driver no longer depends on PCI, but its watchdog portion
>> still needs it, otherwise P2SB runs into a build  failure:
> 
> If the simatic-ipc driver no longer depends on PCI, you can
> delete the dependency here:
> https://elixir.bootlin.com/linux/v6.5-rc6/source/drivers/platform/x86/Kconfig#L1079

That has already been done, the latest code of the
simatic-ipc support lives here:

https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=platform-drivers-x86-simatic-ipc

and there that depends on has been dropped, which is actually what is causing the issue this patch addresses.

>> WARNING: unmet direct dependencies detected for P2SB
>>   Depends on [n]: PCI [=n] && X86 [=y]
>>   Selected by [m]:
>>   - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]
>>
>> drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>>                 pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
>>
>> Add back the minimum dependendency to make it build in random configurations
>> again.
>>
>> Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
>> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
>> ---
>>  drivers/watchdog/Kconfig | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 04e9b40cf7d53..09452384221a4 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1680,7 +1680,7 @@ config NIC7018_WDT
>>  
>>  config SIEMENS_SIMATIC_IPC_WDT
>>  	tristate "Siemens Simatic IPC Watchdog"
>> -	depends on SIEMENS_SIMATIC_IPC
>> +	depends on SIEMENS_SIMATIC_IPC && PCI
> 
> The SIEMENS_SIMATIC_IPC_WDT does not depends on PCI directly,
> it should depends on P2SB, then P2SB depends on PCI

P2SB is a helper library and as such it is selected by all code
which needs it and when selecting the Kconfig entry doing
the selecting Kconfig must either depend on or select all
the Kconfig dependencies of the Kconfig entry it is selecting.

> 
>>  	default y
>>  	select WATCHDOG_CORE
>>  	select P2SB
> 
> You can replace the "select" to "depends on" instead.

No that would mean that we start mixing select and depends on
for P2SB, where as currently all users use select.

Mixing select and depends on is know to cause dependency
cycle issues and thus should be avoided.

Arnd's patch is correct as is (and I have merged it
this morning).

Regards,

Hans
  

On Mon, Aug 14, 2023, at 10:27, xingtong.wu wrote:

>>  
>>  config SIEMENS_SIMATIC_IPC_WDT
>>  	tristate "Siemens Simatic IPC Watchdog"
>> -	depends on SIEMENS_SIMATIC_IPC
>> +	depends on SIEMENS_SIMATIC_IPC && PCI
>
> The SIEMENS_SIMATIC_IPC_WDT does not depends on PCI directly,
> it should depends on P2SB, then P2SB depends on PCI
>
>>  	default y
>>  	select WATCHDOG_CORE
>>  	select P2SB
>
> You can replace the "select" to "depends on" instead.

That would also work, but it's generally best not to mix
'depends on' and 'select' for the same symbol. Changing
all three 'select P2SB' to 'depends on P2SB', or removing
the prompt to turn P2SB itself into a hidden symbol is
probably a good follow-up patch, but I'd keep that separate
from the regression fix.

     Arnd
  

Hi,

On 8/14/23 10:48, Arnd Bergmann wrote:
> On Mon, Aug 14, 2023, at 10:27, xingtong.wu wrote:
> 
>>>  
>>>  config SIEMENS_SIMATIC_IPC_WDT
>>>  	tristate "Siemens Simatic IPC Watchdog"
>>> -	depends on SIEMENS_SIMATIC_IPC
>>> +	depends on SIEMENS_SIMATIC_IPC && PCI
>>
>> The SIEMENS_SIMATIC_IPC_WDT does not depends on PCI directly,
>> it should depends on P2SB, then P2SB depends on PCI
>>
>>>  	default y
>>>  	select WATCHDOG_CORE
>>>  	select P2SB
>>
>> You can replace the "select" to "depends on" instead.
> 
> That would also work, but it's generally best not to mix
> 'depends on' and 'select' for the same symbol. Changing
> all three 'select P2SB' to 'depends on P2SB', or removing
> the prompt to turn P2SB itself into a hidden symbol is
> probably a good follow-up patch, but I'd keep that separate
> from the regression fix.

Right P2SB would benefit from being changed into a hidden
symbol.

Andy, any comments on making P2SB a hidden symbol ?

Regards,

Hans


> 
>      Arnd
>
  

On Mon, Aug 14, 2023 at 09:38:50AM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> The simatic-ipc driver no longer depends on PCI, but its watchdog portion
> still needs it, otherwise P2SB runs into a build  failure:
> 
> WARNING: unmet direct dependencies detected for P2SB
>   Depends on [n]: PCI [=n] && X86 [=y]
>   Selected by [m]:
>   - SIEMENS_SIMATIC_IPC_WDT [=m] && WATCHDOG [=y] && SIEMENS_SIMATIC_IPC [=y]
> 
> drivers/platform/x86/p2sb.c:121:3: error: call to undeclared function 'pci_bus_write_config_dword'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
>                 pci_bus_write_config_dword(bus, devfn_p2sb, P2SBC, 0);
> 
> Add back the minimum dependendency to make it build in random configurations
> again.
> 
> Fixes: b72da71ce24b0 ("platform/x86: simatic-ipc: drop PCI runtime depends and header")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Guenter Roeck <linux@roeck-us.net>

> ---
>  drivers/watchdog/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 04e9b40cf7d53..09452384221a4 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1680,7 +1680,7 @@ config NIC7018_WDT
>  
>  config SIEMENS_SIMATIC_IPC_WDT
>  	tristate "Siemens Simatic IPC Watchdog"
> -	depends on SIEMENS_SIMATIC_IPC
> +	depends on SIEMENS_SIMATIC_IPC && PCI
>  	default y
>  	select WATCHDOG_CORE
>  	select P2SB
> -- 
> 2.39.2
>
  

On Mon, Aug 14, 2023 at 11:51 AM Hans de Goede <hdegoede@redhat.com> wrote:
> On 8/14/23 10:48, Arnd Bergmann wrote:

...

> Right P2SB would benefit from being changed into a hidden
> symbol.

Yes, I dunno why it wasn't that way from day 1. Perhaps due to the
original contribution done by somebody else...

> Andy, any comments on making P2SB a hidden symbol ?

I'm fine with that.