diff mbox series

i2c: i801: add I2C_MUX dependency

Message ID 20240405142823.615609-1-arnd@kernel.org (mailing list archive)
State New
Headers show
Series i2c: i801: add I2C_MUX dependency | expand

Commit Message

Arnd Bergmann April 5, 2024, 2:27 p.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

When I2C_MUX is a loadable module but I2C_I801 is built-in, the newly
added notifier function causes a link error:

x86_64-linux-ld: drivers/i2c/busses/i2c-i801.o: in function `i801_notifier_call':
i2c-i801.c:(.text+0x1f5): undefined reference to `i2c_root_adapter'

This code is only built if I2C_MUX_GPIO is also enabled, so add a
conditional dependency that allows building the driver as before if the
GPIO part is disabled, but otherwise require the linker dependency at
Kconfig level.

With the added dependency, the driver cannot be selected by a builtin
ITCO_WDT driver when I2C_MUX_GPIO is a loadable module, so remove
the 'select' statement in that driver as well. This was apparently
never needed at compile-time, and the watchdog driver just needs either
the LPC or the I2C drivers, but never both.

Configurations that rely on the implied 'select' from the watchdog
driver now need to enable all three.

Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/i2c/busses/Kconfig | 1 +
 drivers/watchdog/Kconfig   | 2 --
 2 files changed, 1 insertion(+), 2 deletions(-)

Comments

Heiner Kallweit April 5, 2024, 7:18 p.m. UTC | #1 
On 05.04.2024 16:27, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When I2C_MUX is a loadable module but I2C_I801 is built-in, the newly
> added notifier function causes a link error:
> 
> x86_64-linux-ld: drivers/i2c/busses/i2c-i801.o: in function `i801_notifier_call':
> i2c-i801.c:(.text+0x1f5): undefined reference to `i2c_root_adapter'
> 
> This code is only built if I2C_MUX_GPIO is also enabled, so add a
> conditional dependency that allows building the driver as before if the
> GPIO part is disabled, but otherwise require the linker dependency at
> Kconfig level.
> 
> With the added dependency, the driver cannot be selected by a builtin
> ITCO_WDT driver when I2C_MUX_GPIO is a loadable module, so remove
> the 'select' statement in that driver as well. This was apparently
> never needed at compile-time, and the watchdog driver just needs either
> the LPC or the I2C drivers, but never both.
> 
> Configurations that rely on the implied 'select' from the watchdog
> driver now need to enable all three.
> 

Question is whether we really want that I2C_MUX restricts the choice for
I2C_I801. Alternatively we can skip building the mux part in the problem case.
The mux part can be used on very few old Asus systems with > 8 memory slots only.
Proposal I sent:
https://lore.kernel.org/all/4dhfyaefnw2rtx5q7aaum6pfwha5o3vs65iqcrj2ghps34ubtw@b3bw3gggudjs/T/

Note also the CI bot tags, as the problem was reported by a CI bot before.

> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/i2c/busses/Kconfig | 1 +
>  drivers/watchdog/Kconfig   | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 1872f1995c77..2619018dd756 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -108,6 +108,7 @@ config I2C_HIX5HD2
>  config I2C_I801
>  	tristate "Intel 82801 (ICH/PCH)"
>  	depends on PCI
> +	depends on I2C_MUX || I2C_MUX_GPIO=n
>  	select P2SB if X86
>  	select CHECK_SIGNATURE if X86 && DMI
>  	select I2C_SMBUS
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0b0df3fe1efd..4dfb3773e6e2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1301,8 +1301,6 @@ config ITCO_WDT
>  	select WATCHDOG_CORE
>  	depends on I2C || I2C=n
>  	depends on MFD_INTEL_PMC_BXT || !MFD_INTEL_PMC_BXT
> -	select LPC_ICH if !EXPERT
> -	select I2C_I801 if !EXPERT && I2C
>  	help
>  	  Hardware driver for the intel TCO timer based watchdog devices.
>  	  These drivers are included in the Intel 82801 I/O Controller
Arnd Bergmann April 5, 2024, 8:16 p.m. UTC | #2 
On Fri, Apr 5, 2024, at 21:18, Heiner Kallweit wrote:
> On 05.04.2024 16:27, Arnd Bergmann wrote:
>
> Question is whether we really want that I2C_MUX restricts the choice for
> I2C_I801. Alternatively we can skip building the mux part in the 
> problem case.
> The mux part can be used on very few old Asus systems with > 8 memory 
> slots only.
> Proposal I sent:
> https://lore.kernel.org/all/4dhfyaefnw2rtx5q7aaum6pfwha5o3vs65iqcrj2ghps34ubtw@b3bw3gggudjs/T/
>
> Note also the CI bot tags, as the problem was reported by a CI bot before.

That seems fine to me as well, and it avoids having to mess with
the watchdog driver. We may want to change that bit anyway, but
then it can be done independently.

     Arnd
Andi Shyti April 6, 2024, 12:31 a.m. UTC | #3 
Hi Arnd,

On Fri, Apr 05, 2024 at 04:27:43PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When I2C_MUX is a loadable module but I2C_I801 is built-in, the newly
> added notifier function causes a link error:
> 
> x86_64-linux-ld: drivers/i2c/busses/i2c-i801.o: in function `i801_notifier_call':
> i2c-i801.c:(.text+0x1f5): undefined reference to `i2c_root_adapter'
> 
> This code is only built if I2C_MUX_GPIO is also enabled, so add a
> conditional dependency that allows building the driver as before if the
> GPIO part is disabled, but otherwise require the linker dependency at
> Kconfig level.
> 
> With the added dependency, the driver cannot be selected by a builtin
> ITCO_WDT driver when I2C_MUX_GPIO is a loadable module, so remove
> the 'select' statement in that driver as well. This was apparently
> never needed at compile-time, and the watchdog driver just needs either
> the LPC or the I2C drivers, but never both.
> 
> Configurations that rely on the implied 'select' from the watchdog
> driver now need to enable all three.
> 
> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

thanks for the proposed fix, but Heiner has already submitted a
fix for this issue. I'm going to mark this patch as superseeded,
if that's OK with you.

Thanks,
Andi
Guenter Roeck April 6, 2024, 1:08 p.m. UTC | #4 
On Fri, Apr 05, 2024 at 04:27:43PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> When I2C_MUX is a loadable module but I2C_I801 is built-in, the newly
> added notifier function causes a link error:
> 
> x86_64-linux-ld: drivers/i2c/busses/i2c-i801.o: in function `i801_notifier_call':
> i2c-i801.c:(.text+0x1f5): undefined reference to `i2c_root_adapter'
> 
> This code is only built if I2C_MUX_GPIO is also enabled, so add a
> conditional dependency that allows building the driver as before if the
> GPIO part is disabled, but otherwise require the linker dependency at
> Kconfig level.
> 
> With the added dependency, the driver cannot be selected by a builtin
> ITCO_WDT driver when I2C_MUX_GPIO is a loadable module, so remove
> the 'select' statement in that driver as well. This was apparently
> never needed at compile-time, and the watchdog driver just needs either
> the LPC or the I2C drivers, but never both.
> 
> Configurations that rely on the implied 'select' from the watchdog
> driver now need to enable all three.
> 
> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/i2c/busses/Kconfig | 1 +
>  drivers/watchdog/Kconfig   | 2 --
>  2 files changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index 1872f1995c77..2619018dd756 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -108,6 +108,7 @@ config I2C_HIX5HD2
>  config I2C_I801
>  	tristate "Intel 82801 (ICH/PCH)"
>  	depends on PCI
> +	depends on I2C_MUX || I2C_MUX_GPIO=n
>  	select P2SB if X86
>  	select CHECK_SIGNATURE if X86 && DMI
>  	select I2C_SMBUS
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0b0df3fe1efd..4dfb3773e6e2 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1301,8 +1301,6 @@ config ITCO_WDT
>  	select WATCHDOG_CORE
>  	depends on I2C || I2C=n
>  	depends on MFD_INTEL_PMC_BXT || !MFD_INTEL_PMC_BXT
> -	select LPC_ICH if !EXPERT
> -	select I2C_I801 if !EXPERT && I2C

Sorry, I don't understand why LPC_ICH and I2C_I801 are neither a dependency
nor need to be selected. What if both LPC_ICH=n and I2C_I801=n, or if one is
selected but the other is needed to connect to the watchdog ?

Guenter

>  	help
>  	  Hardware driver for the intel TCO timer based watchdog devices.
>  	  These drivers are included in the Intel 82801 I/O Controller
> -- 
> 2.39.2
>
Arnd Bergmann April 6, 2024, 3:45 p.m. UTC | #5 
On Sat, Apr 6, 2024, at 15:08, Guenter Roeck wrote:
> On Fri, Apr 05, 2024 at 04:27:43PM +0200, Arnd Bergmann wrote:
>> 
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index 1872f1995c77..2619018dd756 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -108,6 +108,7 @@ config I2C_HIX5HD2
>>  config I2C_I801
>>  	tristate "Intel 82801 (ICH/PCH)"
>>  	depends on PCI
>> +	depends on I2C_MUX || I2C_MUX_GPIO=n
>>  	select P2SB if X86
>>  	select CHECK_SIGNATURE if X86 && DMI
>>  	select I2C_SMBUS
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 0b0df3fe1efd..4dfb3773e6e2 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1301,8 +1301,6 @@ config ITCO_WDT
>>  	select WATCHDOG_CORE
>>  	depends on I2C || I2C=n
>>  	depends on MFD_INTEL_PMC_BXT || !MFD_INTEL_PMC_BXT
>> -	select LPC_ICH if !EXPERT
>> -	select I2C_I801 if !EXPERT && I2C
>
> Sorry, I don't understand why LPC_ICH and I2C_I801 are neither a dependency
> nor need to be selected. What if both LPC_ICH=n and I2C_I801=n, or if one is
> selected but the other is needed to connect to the watchdog ?

The Kconfig dependencies are only required if there is a compile-time
dependencies. In this case, both LPC_ICH and I2C_I801 create a
platform device that is consumed by ITCO_WDT, but it could in
theory work with any other such driver providing the device.

It would be fine to make this explicit by adding
'depends on LPC_ICH || I2C_I801' to enforce that the watchdog
driver can only be selected on if at least one of these
is present, but we have a lot of examples where we don't
spell out this type of dependency.

The two 'select' statements in comparison a really bad idea
because a driver should never need to force-enable a user
visible symbol in another subsystem, and because no single
machine would actually require both the i810 and the ich driver.

   Arnd
Guenter Roeck April 8, 2024, 11:42 a.m. UTC | #6 
On Sat, Apr 06, 2024 at 05:45:57PM +0200, Arnd Bergmann wrote:
> On Sat, Apr 6, 2024, at 15:08, Guenter Roeck wrote:
> > On Fri, Apr 05, 2024 at 04:27:43PM +0200, Arnd Bergmann wrote:
> >> 
> >> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> >> index 1872f1995c77..2619018dd756 100644
> >> --- a/drivers/i2c/busses/Kconfig
> >> +++ b/drivers/i2c/busses/Kconfig
> >> @@ -108,6 +108,7 @@ config I2C_HIX5HD2
> >>  config I2C_I801
> >>  	tristate "Intel 82801 (ICH/PCH)"
> >>  	depends on PCI
> >> +	depends on I2C_MUX || I2C_MUX_GPIO=n
> >>  	select P2SB if X86
> >>  	select CHECK_SIGNATURE if X86 && DMI
> >>  	select I2C_SMBUS
> >> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> >> index 0b0df3fe1efd..4dfb3773e6e2 100644
> >> --- a/drivers/watchdog/Kconfig
> >> +++ b/drivers/watchdog/Kconfig
> >> @@ -1301,8 +1301,6 @@ config ITCO_WDT
> >>  	select WATCHDOG_CORE
> >>  	depends on I2C || I2C=n
> >>  	depends on MFD_INTEL_PMC_BXT || !MFD_INTEL_PMC_BXT
> >> -	select LPC_ICH if !EXPERT
> >> -	select I2C_I801 if !EXPERT && I2C
> >
> > Sorry, I don't understand why LPC_ICH and I2C_I801 are neither a dependency
> > nor need to be selected. What if both LPC_ICH=n and I2C_I801=n, or if one is
> > selected but the other is needed to connect to the watchdog ?
> 
> The Kconfig dependencies are only required if there is a compile-time
> dependencies. In this case, both LPC_ICH and I2C_I801 create a
> platform device that is consumed by ITCO_WDT, but it could in
> theory work with any other such driver providing the device.
> 
> It would be fine to make this explicit by adding
> 'depends on LPC_ICH || I2C_I801' to enforce that the watchdog
> driver can only be selected on if at least one of these
> is present, but we have a lot of examples where we don't
> spell out this type of dependency.
> 

Yes, I know, there are lots of inconsistencies in the kernel and its
configuration. That should not be an excuse to making it worse.

Guenter
diff mbox series

Patch

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 1872f1995c77..2619018dd756 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -108,6 +108,7 @@  config I2C_HIX5HD2
 config I2C_I801
 	tristate "Intel 82801 (ICH/PCH)"
 	depends on PCI
+	depends on I2C_MUX || I2C_MUX_GPIO=n
 	select P2SB if X86
 	select CHECK_SIGNATURE if X86 && DMI
 	select I2C_SMBUS
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0b0df3fe1efd..4dfb3773e6e2 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1301,8 +1301,6 @@  config ITCO_WDT
 	select WATCHDOG_CORE
 	depends on I2C || I2C=n
 	depends on MFD_INTEL_PMC_BXT || !MFD_INTEL_PMC_BXT
-	select LPC_ICH if !EXPERT
-	select I2C_I801 if !EXPERT && I2C
 	help
 	  Hardware driver for the intel TCO timer based watchdog devices.
 	  These drivers are included in the Intel 82801 I/O Controller