| Message ID | 20230814073924.1066390-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | watchdog: simatic: add PCI dependency | expand |
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.
Hi, On 8/14/23 17:28, Andy Shevchenko wrote: > 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. Ok, can you submit a patch for this please ? Regards, Hans
On Mon, Aug 21, 2023 at 01:11:49PM +0200, Hans de Goede wrote: > On 8/14/23 17:28, Andy Shevchenko wrote: > > 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. > > Ok, can you submit a patch for this please ? Just sent.
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