| Message ID | 5b43041f-4f97-41dc-87fb-c2da425e7654@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2] i2c: i801: Fix missing Kconfig dependency | expand |
Hi Heiner, first of all, thanks for the fast reaction! On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: > The original change adds usage of i2c_root_adapter(), which is > implemented in i2c-mux.c. Therefore we can't use the multiplexing > if I2C_I801=y and I2C_MUX=m. What is wrong with select I2C_MUX? And is this covering all the cases? Last thing, how have you tested and reproduced the issue? Thanks, Andi > Handling the dependencies in the code would become unnecessarily > complex, therefore create a new config symbol. > > Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
On 05.04.2024 02:54, Andi Shyti wrote: > Hi Heiner, > > first of all, thanks for the fast reaction! > > On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: >> The original change adds usage of i2c_root_adapter(), which is >> implemented in i2c-mux.c. Therefore we can't use the multiplexing >> if I2C_I801=y and I2C_MUX=m. > > What is wrong with select I2C_MUX? > It would solve the issue, but: We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled. W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter() is in a conditionally compiled code part, controlled by: #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI > And is this covering all the cases? > yes > Last thing, how have you tested and reproduced the issue? > The CI bot report included a link to the kernel config. So it was easy to understand the root cause of the issue. I could reproduce it by setting: I2C_I801=y I2C_MUX=m I2C_MUX_GPIO=m This config was also used to test the fix. Underlying reason for the issue is that i801 has a code dependency on i2c_mux, but not on i2c_mux_gpio. > Thanks, > Andi > Heiner >> Handling the dependencies in the code would become unnecessarily >> complex, therefore create a new config symbol. >> >> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") >> Reported-by: kernel test robot <lkp@intel.com> >> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Hi Heiner, On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote: > On 05.04.2024 02:54, Andi Shyti wrote: > > Hi Heiner, > > > > first of all, thanks for the fast reaction! > > > > On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: > >> The original change adds usage of i2c_root_adapter(), which is > >> implemented in i2c-mux.c. Therefore we can't use the multiplexing > >> if I2C_I801=y and I2C_MUX=m. > > > > What is wrong with select I2C_MUX? > > > It would solve the issue, but: > We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled. > W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter() > is in a conditionally compiled code part, controlled by: > > #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI > > > And is this covering all the cases? > > > yes > > > Last thing, how have you tested and reproduced the issue? > > > The CI bot report included a link to the kernel config. So it was easy > to understand the root cause of the issue. I could reproduce it by setting: > I2C_I801=y > I2C_MUX=m > I2C_MUX_GPIO=m > This config was also used to test the fix. > > Underlying reason for the issue is that i801 has a code dependency on i2c_mux, > but not on i2c_mux_gpio. Even though it might look trivial, I need to reproduce the issue first and check the various cases. Because I'm not at home and, anyway, the report has come late in the week, I can queue this up in the next week's pull request. I'm not extremely happy with the new _CONFIG but I understand the reason behind it. Thanks, Andi > > Thanks, > > Andi > > > Heiner > > >> Handling the dependencies in the code would become unnecessarily > >> complex, therefore create a new config symbol. > >> > >> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") > >> Reported-by: kernel test robot <lkp@intel.com> > >> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >
On 06.04.2024 02:29, Andi Shyti wrote: > Hi Heiner, > > On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote: >> On 05.04.2024 02:54, Andi Shyti wrote: >>> Hi Heiner, >>> >>> first of all, thanks for the fast reaction! >>> >>> On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: >>>> The original change adds usage of i2c_root_adapter(), which is >>>> implemented in i2c-mux.c. Therefore we can't use the multiplexing >>>> if I2C_I801=y and I2C_MUX=m. >>> >>> What is wrong with select I2C_MUX? >>> >> It would solve the issue, but: >> We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled. >> W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter() >> is in a conditionally compiled code part, controlled by: >> >> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI >> >>> And is this covering all the cases? >>> >> yes >> >>> Last thing, how have you tested and reproduced the issue? >>> >> The CI bot report included a link to the kernel config. So it was easy >> to understand the root cause of the issue. I could reproduce it by setting: >> I2C_I801=y >> I2C_MUX=m >> I2C_MUX_GPIO=m >> This config was also used to test the fix. >> >> Underlying reason for the issue is that i801 has a code dependency on i2c_mux, >> but not on i2c_mux_gpio. > > Even though it might look trivial, I need to reproduce the issue > first and check the various cases. > > Because I'm not at home and, anyway, the report has come late in > the week, I can queue this up in the next week's pull request. > > I'm not extremely happy with the new _CONFIG but I understand the > reason behind it. > Whether an additional config symbol or an equivalent expression for conditional compiling is better may be a matter of personal taste. I had a similar issue in r8169 and followed the suggestion to put the logic to a new config symbol. a2634a5ffcaf ("r8169: fix building with CONFIG_LEDS_CLASS=m") Therefore I went the same way here. Sometimes I encounter concerns when using macros like IS_REACHABLE(). > Thanks, > Andi > Heiner >>> Thanks, >>> Andi >>> >> Heiner >> >>>> Handling the dependencies in the code would become unnecessarily >>>> complex, therefore create a new config symbol. >>>> >>>> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>
On 06.04.2024 02:29, Andi Shyti wrote: > Hi Heiner, > > On Fri, Apr 05, 2024 at 07:57:24AM +0200, Heiner Kallweit wrote: >> On 05.04.2024 02:54, Andi Shyti wrote: >>> Hi Heiner, >>> >>> first of all, thanks for the fast reaction! >>> >>> On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: >>>> The original change adds usage of i2c_root_adapter(), which is >>>> implemented in i2c-mux.c. Therefore we can't use the multiplexing >>>> if I2C_I801=y and I2C_MUX=m. >>> >>> What is wrong with select I2C_MUX? >>> >> It would solve the issue, but: >> We would uselessly enable I2C_MUX also if I2C_MUX_GPIO or DMI are disabled. >> W/o them the mux part in i801 is a no-op. The call to i2c_root_adapter() >> is in a conditionally compiled code part, controlled by: >> >> #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI >> >>> And is this covering all the cases? >>> >> yes >> >>> Last thing, how have you tested and reproduced the issue? >>> >> The CI bot report included a link to the kernel config. So it was easy >> to understand the root cause of the issue. I could reproduce it by setting: >> I2C_I801=y >> I2C_MUX=m >> I2C_MUX_GPIO=m >> This config was also used to test the fix. >> >> Underlying reason for the issue is that i801 has a code dependency on i2c_mux, >> but not on i2c_mux_gpio. > > Even though it might look trivial, I need to reproduce the issue > first and check the various cases. > > Because I'm not at home and, anyway, the report has come late in > the week, I can queue this up in the next week's pull request. > > I'm not extremely happy with the new _CONFIG but I understand the > reason behind it. > Another simple solution would be to move the implementation of i2c_root_adapter() from i2c mux to i2c core. It just uses i2c_parent_is_i2c_adapter() which is an inline function of i2c core. What do you think? > Thanks, > Andi > Heiner >>> Thanks, >>> Andi >>> >> Heiner >> >>>> Handling the dependencies in the code would become unnecessarily >>>> complex, therefore create a new config symbol. >>>> >>>> Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") >>>> Reported-by: kernel test robot <lkp@intel.com> >>>> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ >>>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >>
> Another simple solution would be to move the implementation of > i2c_root_adapter() from i2c mux to i2c core. It just uses > i2c_parent_is_i2c_adapter() which is an inline function of > i2c core. What do you think? I have no objections putting i2c_root_adapter() into the core. I think, however, that this patch makes the code a tad more readable. What is the downside of the symbol (despite we have way too many of those in general)?
On 08.04.2024 09:42, Wolfram Sang wrote: > >> Another simple solution would be to move the implementation of >> i2c_root_adapter() from i2c mux to i2c core. It just uses >> i2c_parent_is_i2c_adapter() which is an inline function of >> i2c core. What do you think? > > I have no objections putting i2c_root_adapter() into the core. I think, > however, that this patch makes the code a tad more readable. What is the > downside of the symbol (despite we have way too many of those in > general)? > I have no strong preference here, Andi mention that a new config symbol wouldn't be his preferred approach. Therefore I brought up moving the function to i2c core as an alternative. Maybe he can elaborate on the reasoning.
Hi Wolfram, On Mon, Apr 08, 2024 at 09:42:11AM +0200, Wolfram Sang wrote: > > Another simple solution would be to move the implementation of > > i2c_root_adapter() from i2c mux to i2c core. It just uses > > i2c_parent_is_i2c_adapter() which is an inline function of > > i2c core. What do you think? > > I have no objections putting i2c_root_adapter() into the core. I think, > however, that this patch makes the code a tad more readable. What is the > downside of the symbol (despite we have way too many of those in > general)? That's the only reason: "having too many of those in general". But i2c_root_adapter(), I believe should belong to the mux, even though it makes life easier moving it to the core. Heiner, I tested your patch and it works as expected. If no objections or suggestions, I will push it tomorrow. Andi
On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: > The original change adds usage of i2c_root_adapter(), which is > implemented in i2c-mux.c. Therefore we can't use the multiplexing > if I2C_I801=y and I2C_MUX=m. > Handling the dependencies in the code would become unnecessarily > complex, therefore create a new config symbol. > > Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
Hi Heiner, On Thu, Apr 04, 2024 at 10:09:50PM +0200, Heiner Kallweit wrote: > The original change adds usage of i2c_root_adapter(), which is > implemented in i2c-mux.c. Therefore we can't use the multiplexing > if I2C_I801=y and I2C_MUX=m. > Handling the dependencies in the code would become unnecessarily > complex, therefore create a new config symbol. > > Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") > Reported-by: kernel test robot <lkp@intel.com> > Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> I forgot that the original patch was queued for the next merge window, so that this patch goes through i2c-host. Applied to i2c/i2c-host on git://git.kernel.org/pub/scm/linux/kernel/git/andi.shyti/linux.git Thank you, Andi Patches applied =============== [1/1] i2c: i801: Fix missing Kconfig dependency commit: ab9713bb88443f8791e2917833ff1ed802046b2f
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 97989c914..3eb17f43a 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -163,6 +163,14 @@ config I2C_I801 This driver can also be built as a module. If so, the module will be called i2c-i801. +config I2C_I801_MUX + def_bool I2C_I801 + depends on DMI && I2C_MUX_GPIO + depends on !(I2C_I801=y && I2C_MUX=m) + help + Optional support for multiplexed SMBUS on certain systems with + more than 8 memory slots. + config I2C_ISCH tristate "Intel SCH SMBus 1.0" depends on PCI diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 4294c0c63..92b53b480 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -120,7 +120,7 @@ #include <linux/pm_runtime.h> #include <linux/mutex.h> -#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI +#ifdef CONFIG_I2C_I801_MUX #include <linux/gpio/machine.h> #include <linux/platform_data/i2c-mux-gpio.h> #endif @@ -289,7 +289,7 @@ struct i801_priv { int len; u8 *data; -#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI +#ifdef CONFIG_I2C_I801_MUX struct platform_device *mux_pdev; struct gpiod_lookup_table *lookup; struct notifier_block mux_notifier_block; @@ -1300,7 +1300,7 @@ static void i801_probe_optional_slaves(struct i801_priv *priv) register_dell_lis3lv02d_i2c_device(priv); /* Instantiate SPD EEPROMs unless the SMBus is multiplexed */ -#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) +#ifdef CONFIG_I2C_I801_MUX if (!priv->mux_pdev) #endif i2c_register_spd(&priv->adapter); @@ -1310,7 +1310,7 @@ static void __init input_apanel_init(void) {} static void i801_probe_optional_slaves(struct i801_priv *priv) {} #endif /* CONFIG_X86 && CONFIG_DMI */ -#if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI +#ifdef CONFIG_I2C_I801_MUX static struct i801_mux_config i801_mux_config_asus_z8_d12 = { .gpio_chip = "gpio_ich", .values = { 0x02, 0x03 },
The original change adds usage of i2c_root_adapter(), which is implemented in i2c-mux.c. Therefore we can't use the multiplexing if I2C_I801=y and I2C_MUX=m. Handling the dependencies in the code would become unnecessarily complex, therefore create a new config symbol. Fixes: 71b494e043d2 ("i2c: i801: Call i2c_register_spd for muxed child segments") Reported-by: kernel test robot <lkp@intel.com> Closes: https://lore.kernel.org/oe-kbuild-all/202404042206.MjAQC32x-lkp@intel.com/ Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- - Completely rework the fix and create a new config symbol. --- drivers/i2c/busses/Kconfig | 8 ++++++++ drivers/i2c/busses/i2c-i801.c | 8 ++++---- 2 files changed, 12 insertions(+), 4 deletions(-)