| Message ID | 2192294e-99ab-4c7d-86b1-edff058d82f3@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | Andi Shyti |
| Headers | show |
| Series | i2c: i801: Simplify class-based client device instantiation | expand |
Hi, On Tue, Oct 10, 2023 at 09:27:44PM +0200, Heiner Kallweit wrote: > Now that the legacy eeprom driver was removed, the only remaining i2c > client driver with class SPD autodetection is jc42, and this driver > supports also class HWMON. Therefore we can remove class SPD from the > supported classes of the i801 adapter driver. > Legacy class-based instantiation shouldn't be used in new code, so I > think we can remove also the generic logic that ensures that supported > classes of parent and muxed adapters don't overlap. > > Note: i801 parent supports just class HWMON now, and muxed childs > class SPD, so the supported classes don't overlap. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Don't see anything odd here, Jean, any idea here? Andi
Hi Heiner, Andi, On Tue, 10 Oct 2023 21:27:44 +0200, Heiner Kallweit wrote: > Now that the legacy eeprom driver was removed, the only remaining i2c > client driver with class SPD autodetection is jc42, and this driver > supports also class HWMON. Therefore we can remove class SPD from the I did not notice this change when it happened back in 2016. This broke the i2c-i801 driver as a side effect, because the Asus-specific mux code assumes that no I2C device driver can probe both the SMBus trunk and the SMBus segments behind the muxes. This is done (as you must know as this patch touches that part of the code) by ensuring that the trunk and the muxed segments do not share any class flag. This was sufficient as long as all device driver themselves only registered for one class, but this is no longer the case of the jc42 driver. So loading the jc42 driver on one of these Asus server board systems would possibly result in multiple jc42 devices being instantiated for the same underlying hardware device. The one instantiated on the trunk would return incorrect values or errors depending on the mux setting. Probably this went unnoticed because nobody was running such old server boards when the change happened, or they would stick to older kernel versions. > supported classes of the i801 adapter driver. > Legacy class-based instantiation shouldn't be used in new code, so I > think we can remove also the generic logic that ensures that supported > classes of parent and muxed adapters don't overlap. Agreed. If we were to add support for a new server board with muxed SMBus, we would disable class-based probing and instead explicitly instantiate devices. To be honest, I don't know why we didn't do that for the Asus Z8 series already, as I think it was already available, and it would have made the code a lot more simple. If anyone ever complains about the bug mentioned above, then we'll have to do it anyway. > Note: i801 parent supports just class HWMON now, and muxed childs "children" ^^ > class SPD, so the supported classes don't overlap. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 38 +++++++---------------------------- > 1 file changed, 7 insertions(+), 31 deletions(-) > (...) Fine with me. Reviewed-by: Jean Delvare <jdelvare@suse.de>
Hi Jean, On Tue, Oct 17, 2023 at 05:06:08PM +0200, Jean Delvare wrote: > Hi Heiner, Andi, > > On Tue, 10 Oct 2023 21:27:44 +0200, Heiner Kallweit wrote: > > Now that the legacy eeprom driver was removed, the only remaining i2c > > client driver with class SPD autodetection is jc42, and this driver > > supports also class HWMON. Therefore we can remove class SPD from the > > I did not notice this change when it happened back in 2016. This broke > the i2c-i801 driver as a side effect, because the Asus-specific mux > code assumes that no I2C device driver can probe both the SMBus trunk > and the SMBus segments behind the muxes. This is done (as you must know > as this patch touches that part of the code) by ensuring that the trunk > and the muxed segments do not share any class flag. This was sufficient > as long as all device driver themselves only registered for one class, > but this is no longer the case of the jc42 driver. > > So loading the jc42 driver on one of these Asus server board systems > would possibly result in multiple jc42 devices being instantiated for > the same underlying hardware device. The one instantiated on the trunk > would return incorrect values or errors depending on the mux setting. > > Probably this went unnoticed because nobody was running such old server > boards when the change happened, or they would stick to older kernel > versions. thanks for clarifying! Andi > > supported classes of the i801 adapter driver. > > Legacy class-based instantiation shouldn't be used in new code, so I > > think we can remove also the generic logic that ensures that supported > > classes of parent and muxed adapters don't overlap. > > Agreed. If we were to add support for a new server board with muxed > SMBus, we would disable class-based probing and instead explicitly > instantiate devices. To be honest, I don't know why we didn't do that > for the Asus Z8 series already, as I think it was already available, > and it would have made the code a lot more simple. > > If anyone ever complains about the bug mentioned above, then we'll have > to do it anyway. > > > Note: i801 parent supports just class HWMON now, and muxed childs > > "children" ^^ > > > class SPD, so the supported classes don't overlap. > > > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > > --- > > drivers/i2c/busses/i2c-i801.c | 38 +++++++---------------------------- > > 1 file changed, 7 insertions(+), 31 deletions(-) > > (...) > > Fine with me. > > Reviewed-by: Jean Delvare <jdelvare@suse.de> > > -- > Jean Delvare > SUSE L3 Support
Hi Heiner, On Tue, Oct 10, 2023 at 09:27:44PM +0200, Heiner Kallweit wrote: > Now that the legacy eeprom driver was removed, the only remaining i2c > client driver with class SPD autodetection is jc42, and this driver > supports also class HWMON. Therefore we can remove class SPD from the > supported classes of the i801 adapter driver. > Legacy class-based instantiation shouldn't be used in new code, so I > think we can remove also the generic logic that ensures that supported > classes of parent and muxed adapters don't overlap. > > Note: i801 parent supports just class HWMON now, and muxed childs > class SPD, so the supported classes don't overlap. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Acked-by: Andi Shyti <andi.shyti@kernel.org> Andi
On Tue, Oct 10, 2023 at 09:27:44PM +0200, Heiner Kallweit wrote: > Now that the legacy eeprom driver was removed, the only remaining i2c > client driver with class SPD autodetection is jc42, and this driver > supports also class HWMON. Therefore we can remove class SPD from the > supported classes of the i801 adapter driver. > Legacy class-based instantiation shouldn't be used in new code, so I > think we can remove also the generic logic that ensures that supported > classes of parent and muxed adapters don't overlap. > > Note: i801 parent supports just class HWMON now, and muxed childs > class SPD, so the supported classes don't overlap. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> Applied to for-next, thanks!
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index a485dc84d..a41f5349a 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -285,7 +285,6 @@ struct i801_priv { u8 *data; #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) && defined CONFIG_DMI - const struct i801_mux_config *mux_drvdata; struct platform_device *mux_pdev; struct gpiod_lookup_table *lookup; #endif @@ -1282,7 +1281,7 @@ static void i801_probe_optional_slaves(struct i801_priv *priv) /* Instantiate SPD EEPROMs unless the SMBus is multiplexed */ #if IS_ENABLED(CONFIG_I2C_MUX_GPIO) - if (!priv->mux_drvdata) + if (!priv->mux_pdev) #endif i2c_register_spd(&priv->adapter); } @@ -1384,11 +1383,14 @@ static void i801_add_mux(struct i801_priv *priv) const struct i801_mux_config *mux_config; struct i2c_mux_gpio_platform_data gpio_data; struct gpiod_lookup_table *lookup; + const struct dmi_system_id *id; int i; - if (!priv->mux_drvdata) + id = dmi_first_match(mux_dmi_table); + if (!id) return; - mux_config = priv->mux_drvdata; + + mux_config = id->driver_data; /* Prepare the platform data */ memset(&gpio_data, 0, sizeof(struct i2c_mux_gpio_platform_data)); @@ -1432,35 +1434,9 @@ static void i801_del_mux(struct i801_priv *priv) platform_device_unregister(priv->mux_pdev); gpiod_remove_lookup_table(priv->lookup); } - -static unsigned int i801_get_adapter_class(struct i801_priv *priv) -{ - const struct dmi_system_id *id; - const struct i801_mux_config *mux_config; - unsigned int class = I2C_CLASS_HWMON | I2C_CLASS_SPD; - int i; - - id = dmi_first_match(mux_dmi_table); - if (id) { - /* Remove branch classes from trunk */ - mux_config = id->driver_data; - for (i = 0; i < mux_config->n_values; i++) - class &= ~mux_config->classes[i]; - - /* Remember for later */ - priv->mux_drvdata = mux_config; - } - - return class; -} #else static inline void i801_add_mux(struct i801_priv *priv) { } static inline void i801_del_mux(struct i801_priv *priv) { } - -static inline unsigned int i801_get_adapter_class(struct i801_priv *priv) -{ - return I2C_CLASS_HWMON | I2C_CLASS_SPD; -} #endif static struct platform_device * @@ -1641,7 +1617,7 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id) i2c_set_adapdata(&priv->adapter, priv); priv->adapter.owner = THIS_MODULE; - priv->adapter.class = i801_get_adapter_class(priv); + priv->adapter.class = I2C_CLASS_HWMON; priv->adapter.algo = &smbus_algorithm; priv->adapter.dev.parent = &dev->dev; ACPI_COMPANION_SET(&priv->adapter.dev, ACPI_COMPANION(&dev->dev));
Now that the legacy eeprom driver was removed, the only remaining i2c client driver with class SPD autodetection is jc42, and this driver supports also class HWMON. Therefore we can remove class SPD from the supported classes of the i801 adapter driver. Legacy class-based instantiation shouldn't be used in new code, so I think we can remove also the generic logic that ensures that supported classes of parent and muxed adapters don't overlap. Note: i801 parent supports just class HWMON now, and muxed childs class SPD, so the supported classes don't overlap. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 38 +++++++---------------------------- 1 file changed, 7 insertions(+), 31 deletions(-)