[PATCH v2 0/4] i2c: smbus cleanups and SPD support for piix4

[PATCH v2 0/4] i2c: smbus cleanups and SPD support for piix4

[Thomas Weißschuh]

Patches 1-3 are preparation patches, with patch 1 being a fix.
Patch 4 is the actual change to piix4.

Patch 3 drops the warning about muxed busses.
I didn't feel that the warning only would warrant the additional
complexity it introduces with multiple callers of i2c_register_spd().

If other feel different, maybe a more generic warning/info or a source
code comment would suffice.

On a machine with 32 slots of which 16 are populated only the first 8
slots are addressable, half of which are empty.
Unfortunately I couldn't run a custom kernel for testing.
But manually instantiating ee1004 devices worked as expected,
so the proposed changes should also work.

Tested with spd5118 and two DIMMs.

Guenter, I dropped your Tested-by as the piix4 patch changed.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
Changes in v2:
- Add new i2c-smbus prep patches
- Only register SPD for port 0
- Link to v1: https://lore.kernel.org/r/20240530-piix4-spd-v1-1-9cbf1abebf41@weissschuh.net

---
Thomas Weißschuh (4):
      i2c: smbus: only limit max banks to eight
      i2c: smbus: probe SPDs on a best-effort basis
      i2c: smbus: drop warning about muxed segments requirement
      i2c: piix4: Register SPDs

 drivers/i2c/busses/Kconfig     |  1 +
 drivers/i2c/busses/i2c-piix4.c |  4 ++++
 drivers/i2c/i2c-smbus.c        | 15 ++++-----------
 3 files changed, 9 insertions(+), 11 deletions(-)
---
base-commit: 4e2600bca665197acb537ae63f24d3075e6bac8b
change-id: 20240530-piix4-spd-39c156b22959

Best regards,
-- 
Thomas Weißschuh <linux@weissschuh.net>

[PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Thomas Weißschuh]

If there are less than eight slots in total,
only probe those.
Now the code matches the comment "..., then limit slots to 8".

Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/i2c/i2c-smbus.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index f809f0ef2004..8f0403652606 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -356,7 +356,7 @@ void i2c_register_spd(struct i2c_adapter *adap)
 	 * as this is the max number of SPD EEPROMs that can be addressed per bus.
 	 */
 	if (i2c_parent_is_i2c_adapter(adap)) {
-		slot_count = 8;
+		slot_count = min(slot_count, 8);
 	} else {
 		if (slot_count > 8) {
 			dev_warn(&adap->dev,

-- 
2.45.2

Re: [PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Andi Shyti]

Hi Thomas,

On Thu, Jun 27, 2024 at 07:48:11PM GMT, Thomas Weißschuh wrote:
> If there are less than eight slots in total,
> only probe those.
> Now the code matches the comment "..., then limit slots to 8".
> 
> Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

I don't see the need for the Fixes here... was there a bug that
has been fixed?

Thanks,
Andi

Re: [PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Thomas Weißschuh]

Jul 4, 2024 23:57:36 Andi Shyti <andi.shyti@kernel.org>:

> Hi Thomas,
>
> On Thu, Jun 27, 2024 at 07:48:11PM GMT, Thomas Weißschuh wrote:
>> If there are less than eight slots in total,
>> only probe those.
>> Now the code matches the comment "..., then limit slots to 8".
>>
>> Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>
> I don't see the need for the Fixes here... was there a bug that
> has been fixed?

More addresses are probed than are possible.
Which is a change from the old behavior and also
contradicts the comment.
IMO it's a bug. Probably not a big one and I'm not sure if user-observable.
Surely nothing for stable.

But I'm not hung up on it and will drop that tag in the next revision.

Re: [PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Heiner Kallweit]

On 05.07.2024 07:55, Thomas Weißschuh wrote:
> Jul 4, 2024 23:57:36 Andi Shyti <andi.shyti@kernel.org>:
> 
>> Hi Thomas,
>>
>> On Thu, Jun 27, 2024 at 07:48:11PM GMT, Thomas Weißschuh wrote:
>>> If there are less than eight slots in total,
>>> only probe those.
>>> Now the code matches the comment "..., then limit slots to 8".
>>>
>>> Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>
>> I don't see the need for the Fixes here... was there a bug that
>> has been fixed?
> 
> More addresses are probed than are possible.

Later in the function there's the following:

for (n = 0; n < slot_count && dimm_count; n++) {

With dimm_count being decremented with each instantiated DIMM module.
If a system has less than 8 slots, then it also has less than 8 modules
and we finish once all modules have been instantiated.
Having said that I don't see any excess probing.

> Which is a change from the old behavior and also
> contradicts the comment.
> IMO it's a bug. Probably not a big one and I'm not sure if user-observable.
> Surely nothing for stable.
> 
> But I'm not hung up on it and will drop that tag in the next revision.

Re: [PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Andi Shyti]

Hi Thomas,

On Fri, Jul 05, 2024 at 07:55:21AM GMT, Thomas Weißschuh  wrote:
> Jul 4, 2024 23:57:36 Andi Shyti <andi.shyti@kernel.org>:
> > On Thu, Jun 27, 2024 at 07:48:11PM GMT, Thomas Weißschuh wrote:
> >> If there are less than eight slots in total,
> >> only probe those.
> >> Now the code matches the comment "..., then limit slots to 8".
> >>
> >> Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
> >> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> >
> > I don't see the need for the Fixes here... was there a bug that
> > has been fixed?
> 
> More addresses are probed than are possible.
> Which is a change from the old behavior and also
> contradicts the comment.
> IMO it's a bug. Probably not a big one and I'm not sure if user-observable.
> Surely nothing for stable.

The Fixes tag means that you want the patch to be backported to
stable kernels. Someone will take the effort of taking all the
new "Fixes:" and port them to older kernels.

We want this when patches fix crashes, deadlocks, memory leaks,
security holes, misbehaviours, earthquakes and floodings.

Andi

> But I'm not hung up on it and will drop that tag in the next revision.

Re: [PATCH v2 1/4] i2c: smbus: only limit max banks to eight

[Thomas Weißschuh]

Hi Andi,

On 2024-07-05 13:56:24+0000, Andi Shyti wrote:
> On Fri, Jul 05, 2024 at 07:55:21AM GMT, Thomas Weißschuh  wrote:
> > Jul 4, 2024 23:57:36 Andi Shyti <andi.shyti@kernel.org>:
> > > On Thu, Jun 27, 2024 at 07:48:11PM GMT, Thomas Weißschuh wrote:
> > >> If there are less than eight slots in total,
> > >> only probe those.
> > >> Now the code matches the comment "..., then limit slots to 8".
> > >>
> > >> Fixes: 8821c8376993 ("i2c: smbus: Prepare i2c_register_spd for usage on muxed segments")
> > >> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > >
> > > I don't see the need for the Fixes here... was there a bug that
> > > has been fixed?
> > 
> > More addresses are probed than are possible.
> > Which is a change from the old behavior and also
> > contradicts the comment.
> > IMO it's a bug. Probably not a big one and I'm not sure if user-observable.
> > Surely nothing for stable.
> 
> The Fixes tag means that you want the patch to be backported to
> stable kernels. Someone will take the effort of taking all the
> new "Fixes:" and port them to older kernels.

It's my understanding that a Fixes tag itself is not enough for the
stable process. For that it also needs a "Cc: stable@vger.kernel.org" in
the patch or explicit notification of the stable team.
(Or being picked up by Autosel)

Anyways, I'll squash the commits and drop the Fixes tat, as there was no
bug as pointed out by Heiner.

> We want this when patches fix crashes, deadlocks, memory leaks,
> security holes, misbehaviours, earthquakes and floodings.
> 
> Andi
> 
> > But I'm not hung up on it and will drop that tag in the next revision.

[PATCH v2 2/4] i2c: smbus: probe SPDs on a best-effort basis

[Thomas Weißschuh]

Even if it is likely that not all slots can be probed,
then at least probe the 8 slots that can.

Also adapt the comment to better fit the new logic.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/i2c/i2c-smbus.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index 8f0403652606..cdbb95fe104e 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -352,18 +352,15 @@ void i2c_register_spd(struct i2c_adapter *adap)
 		return;
 
 	/*
-	 * If we're a child adapter on a muxed segment, then limit slots to 8,
-	 * as this is the max number of SPD EEPROMs that can be addressed per bus.
+	 * The max number of SPD EEPROMs that can be addressed per bus is 8.
+	 * If more slots are present either muxed or multiple busses are
+	 * necessary or the additional slots are ignored.
 	 */
-	if (i2c_parent_is_i2c_adapter(adap)) {
-		slot_count = min(slot_count, 8);
-	} else {
-		if (slot_count > 8) {
-			dev_warn(&adap->dev,
-				 "More than 8 memory slots on a single bus, contact i801 maintainer to add missing mux config\n");
-			return;
-		}
+	if (!i2c_parent_is_i2c_adapter(adap) && slot_count > 8) {
+		dev_warn(&adap->dev,
+			 "More than 8 memory slots on a single bus, contact i801 maintainer to add missing mux config\n");
 	}
+	slot_count = min(slot_count, 8);
 
 	/*
 	 * Memory types could be found at section 7.18.2 (Memory Device — Type), table 78

-- 
2.45.2

[PATCH v2 3/4] i2c: smbus: drop warning about muxed segments requirement

[Thomas Weißschuh]

The check and warning are very specific to the SPD usage of the i801
driver. That was fine as long as i801 was the only caller of
i2c_register_spd(). Now that piix4 also wants to do the same the check
and warning are not accurate anymore.
Instead of introducing a more complicated calling protocol only to print
a warning, drop the warning.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/i2c/i2c-smbus.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
index cdbb95fe104e..f0ac35fd0c5a 100644
--- a/drivers/i2c/i2c-smbus.c
+++ b/drivers/i2c/i2c-smbus.c
@@ -356,10 +356,6 @@ void i2c_register_spd(struct i2c_adapter *adap)
 	 * If more slots are present either muxed or multiple busses are
 	 * necessary or the additional slots are ignored.
 	 */
-	if (!i2c_parent_is_i2c_adapter(adap) && slot_count > 8) {
-		dev_warn(&adap->dev,
-			 "More than 8 memory slots on a single bus, contact i801 maintainer to add missing mux config\n");
-	}
 	slot_count = min(slot_count, 8);
 
 	/*

-- 
2.45.2

[PATCH v2 4/4] i2c: piix4: Register SPDs

[Thomas Weißschuh]

The piix4 I2C bus can carry SPDs, register them if present.
Only look on bus 0, as this is where the SPDs seem to be located.

Only the first 8 slots are supported. If the system has more,
then these will not be visible.

The AUX bus can not be probed as on some platforms it reports all
devices present and all reads return "0".
This would allow the ee1004 to be probed incorrectly.

Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
---
 drivers/i2c/busses/Kconfig     | 1 +
 drivers/i2c/busses/i2c-piix4.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index fe6e8a1bb607..ff66e883b348 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -195,6 +195,7 @@ config I2C_ISMT
 config I2C_PIIX4
 	tristate "Intel PIIX4 and compatible (ATI/AMD/Serverworks/Broadcom/SMSC)"
 	depends on PCI && HAS_IOPORT
+	select I2C_SMBUS
 	help
 	  If you say yes to this option, support will be included for the Intel
 	  PIIX4 family of mainboard I2C interfaces.  Specifically, the following
diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 6a0392172b2f..14752d946f58 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -29,6 +29,7 @@
 #include <linux/stddef.h>
 #include <linux/ioport.h>
 #include <linux/i2c.h>
+#include <linux/i2c-smbus.h>
 #include <linux/slab.h>
 #include <linux/dmi.h>
 #include <linux/acpi.h>
@@ -982,6 +983,9 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 		return retval;
 	}
 
+	if (port == 0)
+		i2c_register_spd(adap);
+
 	*padap = adap;
 	return 0;
 }

-- 
2.45.2

Re: [PATCH v2 4/4] i2c: piix4: Register SPDs

[Guenter Roeck]

On 6/27/24 10:48, Thomas Weißschuh wrote:
> The piix4 I2C bus can carry SPDs, register them if present.
> Only look on bus 0, as this is where the SPDs seem to be located.
> 
> Only the first 8 slots are supported. If the system has more,
> then these will not be visible.
> 
> The AUX bus can not be probed as on some platforms it reports all
> devices present and all reads return "0".
> This would allow the ee1004 to be probed incorrectly.

Was this reported somewhere ? I don't see it happen on any of my systems
(of course that doesn't really mean anything).

> 
> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>

Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks,
Guenter

Re: [PATCH v2 4/4] i2c: piix4: Register SPDs

[Thomas Weißschuh]

On 2024-06-28 16:09:09+0000, Guenter Roeck wrote:
> On 6/27/24 10:48, Thomas Weißschuh wrote:
> > The piix4 I2C bus can carry SPDs, register them if present.
> > Only look on bus 0, as this is where the SPDs seem to be located.
> > 
> > Only the first 8 slots are supported. If the system has more,
> > then these will not be visible.
> > 
> > The AUX bus can not be probed as on some platforms it reports all
> > devices present and all reads return "0".
> > This would allow the ee1004 to be probed incorrectly.
> 
> Was this reported somewhere ? I don't see it happen on any of my systems
> (of course that doesn't really mean anything).

It happened on one of the big server systems I tested on.

> > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> 
> Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>

Thanks!

FYI, combined tags are discouraged by
Documentation/process/maintainer-tip.rst:

  Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
  they just complicate automated extraction of tags.

I'll add the tags in split form to the patch.


Thomas

Re: [PATCH v2 4/4] i2c: piix4: Register SPDs

[Guenter Roeck]

On 6/29/24 00:19, Thomas Weißschuh wrote:
> On 2024-06-28 16:09:09+0000, Guenter Roeck wrote:
>> On 6/27/24 10:48, Thomas Weißschuh wrote:
>>> The piix4 I2C bus can carry SPDs, register them if present.
>>> Only look on bus 0, as this is where the SPDs seem to be located.
>>>
>>> Only the first 8 slots are supported. If the system has more,
>>> then these will not be visible.
>>>
>>> The AUX bus can not be probed as on some platforms it reports all
>>> devices present and all reads return "0".
>>> This would allow the ee1004 to be probed incorrectly.
>>
>> Was this reported somewhere ? I don't see it happen on any of my systems
>> (of course that doesn't really mean anything).
> 
> It happened on one of the big server systems I tested on.
> 
>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>
>> Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks!
> 
> FYI, combined tags are discouraged by
> Documentation/process/maintainer-tip.rst:
> 
>    Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
>    they just complicate automated extraction of tags.
> 
> I'll add the tags in split form to the patch.
> 
> 
> Thomas

Re: [PATCH v2 4/4] i2c: piix4: Register SPDs

[Guenter Roeck]

On 6/29/24 00:19, Thomas Weißschuh wrote:
> On 2024-06-28 16:09:09+0000, Guenter Roeck wrote:
>> On 6/27/24 10:48, Thomas Weißschuh wrote:
>>> The piix4 I2C bus can carry SPDs, register them if present.
>>> Only look on bus 0, as this is where the SPDs seem to be located.
>>>
>>> Only the first 8 slots are supported. If the system has more,
>>> then these will not be visible.
>>>
>>> The AUX bus can not be probed as on some platforms it reports all
>>> devices present and all reads return "0".
>>> This would allow the ee1004 to be probed incorrectly.
>>
>> Was this reported somewhere ? I don't see it happen on any of my systems
>> (of course that doesn't really mean anything).
> 
> It happened on one of the big server systems I tested on.
> 

>>> Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
>>
>> Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks!
> 
> FYI, combined tags are discouraged by
> Documentation/process/maintainer-tip.rst:
> 
>    Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
>    they just complicate automated extraction of tags.
> 
> I'll add the tags in split form to the patch.
> 

NP with me. though strictly speaking that only applies to the tip tree.
I started using it after someone else asked for it, but I don't like it
too much anyway. Combining it was an attempt to avoid the "please
combine ..." feedback ;-).

On a side note, I also applied the other three patches, but I am not sure
if that counts as "Tested", so I didn't send a tag for those.

Thanks,
Guenter

Re: [PATCH v2 4/4] i2c: piix4: Register SPDs

[Andi Shyti]

Hi Thomas,

On Sat, Jun 29, 2024 at 09:19:48AM GMT, Thomas Weißschuh wrote:
> On 2024-06-28 16:09:09+0000, Guenter Roeck wrote:
> > On 6/27/24 10:48, Thomas Weißschuh wrote:
> > > The piix4 I2C bus can carry SPDs, register them if present.
> > > Only look on bus 0, as this is where the SPDs seem to be located.
> > > 
> > > Only the first 8 slots are supported. If the system has more,
> > > then these will not be visible.
> > > 
> > > The AUX bus can not be probed as on some platforms it reports all
> > > devices present and all reads return "0".
> > > This would allow the ee1004 to be probed incorrectly.
> > 
> > Was this reported somewhere ? I don't see it happen on any of my systems
> > (of course that doesn't really mean anything).
> 
> It happened on one of the big server systems I tested on.
> 
> > > Signed-off-by: Thomas Weißschuh <linux@weissschuh.net>
> > 
> > Reviewed-and-tested-by: Guenter Roeck <linux@roeck-us.net>
> 
> Thanks!
> 
> FYI, combined tags are discouraged by
> Documentation/process/maintainer-tip.rst:
> 
>   Please do not use combined tags, e.g. ``Reported-and-tested-by``, as
>   they just complicate automated extraction of tags.
> 
> I'll add the tags in split form to the patch.

No worries, I'll take care of it.

Thanks,
Andi

Re: [PATCH v2 0/4] i2c: smbus cleanups and SPD support for piix4

[Andi Shyti]

Hi Thomas,

...

> Thomas Weißschuh (4):
>       i2c: smbus: only limit max banks to eight
>       i2c: smbus: probe SPDs on a best-effort basis
>       i2c: smbus: drop warning about muxed segments requirement

These three patches are shuffling around the code,
adding/removing/moving the same bits. Can we squash them to a
single patch?

Thanks,
Andi

Re: [PATCH v2 0/4] i2c: smbus cleanups and SPD support for piix4

[Thomas Weißschuh]

Jul 4, 2024 23:56:45 Andi Shyti <andi.shyti@kernel.org>:

> Hi Thomas,
>
> ...
>
>> Thomas Weißschuh (4):
>>       i2c: smbus: only limit max banks to eight
>>       i2c: smbus: probe SPDs on a best-effort basis
>>       i2c: smbus: drop warning about muxed segments requirement
>
> These three patches are shuffling around the code,
> adding/removing/moving the same bits. Can we squash them to a
> single patch?

IMO they are doing different things, but I'll squash them for the next submission.