[PATCH] i2c: i801: Simplify class-based client device instantiation

[PATCH] i2c: i801: Simplify class-based client device instantiation

[Heiner Kallweit]

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(-)

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));
-- 
2.42.0

Re: [PATCH] i2c: i801: Simplify class-based client device instantiation

[Andi Shyti]

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

Re: [PATCH] i2c: i801: Simplify class-based client device instantiation

[Jean Delvare]

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>

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: i801: Simplify class-based client device instantiation

[Andi Shyti]

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

Re: [PATCH] i2c: i801: Simplify class-based client device instantiation

[Andi Shyti]

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

Re: [PATCH] i2c: i801: Simplify class-based client device instantiation

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 755 bytes --]

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!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]