[RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[Andrew Cooks]

Family 16h Model 30h SMBus controller needs the same port selection fix
as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
selection for AMD Family 17h chips")

commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
register") also fixed the port selection for Hudson2, but unfortunately
this is not the exact same device and the AMD naming and PCI Device IDs
aren't particularly helpful here.

The SMBus port selection register is common to the following Families
and models, as documented in AMD's publicly available BIOS and Kernel
Developer Guides:

 50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
 55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
 52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)

The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
between Bolton FCH and Family 16h Model 30h, but the location of the
SmBus0Sel port selection bits are different:

 51192 - Bolton Register Reference Guide

We distinguish between Bolton and Family 16h Model 30h using the PCI
Revision ID:

  Bolton is device 0x780b, revision 0x15
  Family 16h Model 30h is device 0x780b, revision 0x1F
  Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.

The following additional public AMD BKDG documents were checked and do
not share the same port selection register:

 42301 - Family 15h Model 00h-0Fh doesn't mention any
 42300 - Family 15h Model 10h-1Fh doesn't mention any
 49125 - Family 15h Model 30h-3Fh doesn't mention any

 48751 - Family 16h Model 00h-0Fh uses the previously supported
         index register SB800_PIIX4_PORT_IDX_ALT at 0x2e

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 174579d..5c90a44 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -99,7 +99,7 @@
 #define SB800_PIIX4_PORT_IDX_MASK	0x06
 #define SB800_PIIX4_PORT_IDX_SHIFT	1
 
-/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
+/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
 #define SB800_PIIX4_PORT_IDX_KERNCZ		0x02
 #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
 #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
@@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
 
 	/* Find which register is used for port selection */
 	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
-		switch (PIIX4_dev->device) {
-		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
+		if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
+		    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
+			   PIIX4_dev->revision >= 0x1F)) {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_KERNCZ;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK_KERNCZ;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ;
-			break;
-		case PCI_DEVICE_ID_AMD_HUDSON2_SMBUS:
-		default:
+		} else {
 			piix4_port_sel_sb800 = SB800_PIIX4_PORT_IDX_ALT;
 			piix4_port_mask_sb800 = SB800_PIIX4_PORT_IDX_MASK;
 			piix4_port_shift_sb800 = SB800_PIIX4_PORT_IDX_SHIFT;
-			break;
 		}
 	} else {
 		mutex_lock(&piix4_mutex_sb800);
-- 
2.7.4

Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[Tobin C. Harding]

On Mon, Feb 26, 2018 at 10:28:43AM +1000, Andrew Cooks wrote:
> Family 16h Model 30h SMBus controller needs the same port selection fix
> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
> selection for AMD Family 17h chips")
> 
> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
> register") also fixed the port selection for Hudson2, but unfortunately
> this is not the exact same device and the AMD naming and PCI Device IDs
> aren't particularly helpful here.
> 
> The SMBus port selection register is common to the following Families
> and models, as documented in AMD's publicly available BIOS and Kernel
> Developer Guides:
> 
>  50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>  55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>  52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
> 
> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
> between Bolton FCH and Family 16h Model 30h, but the location of the
> SmBus0Sel port selection bits are different:
> 
>  51192 - Bolton Register Reference Guide
> 
> We distinguish between Bolton and Family 16h Model 30h using the PCI
> Revision ID:
> 
>   Bolton is device 0x780b, revision 0x15
>   Family 16h Model 30h is device 0x780b, revision 0x1F
>   Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
> 
> The following additional public AMD BKDG documents were checked and do
> not share the same port selection register:
> 
>  42301 - Family 15h Model 00h-0Fh doesn't mention any
>  42300 - Family 15h Model 10h-1Fh doesn't mention any
>  49125 - Family 15h Model 30h-3Fh doesn't mention any
> 
>  48751 - Family 16h Model 00h-0Fh uses the previously supported
>          index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 174579d..5c90a44 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -99,7 +99,7 @@
>  #define SB800_PIIX4_PORT_IDX_MASK	0x06
>  #define SB800_PIIX4_PORT_IDX_SHIFT	1
>  
> -/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
> +/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
>  #define SB800_PIIX4_PORT_IDX_KERNCZ		0x02
>  #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
>  #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
> @@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>  
>  	/* Find which register is used for port selection */
>  	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
> -		switch (PIIX4_dev->device) {
> -		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
> +		if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||

nit:		if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||

> +		    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
> +			   PIIX4_dev->revision >= 0x1F)) {


Hope this helps,
Tobin.

Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[Andrew Cooks]

Hi Tobin

On 26/02/18 18:58, Tobin C. Harding wrote:
> On Mon, Feb 26, 2018 at 10:28:43AM +1000, Andrew Cooks wrote:
>> Family 16h Model 30h SMBus controller needs the same port selection fix
>> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
>> selection for AMD Family 17h chips")
>>
>> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
>> register") also fixed the port selection for Hudson2, but unfortunately
>> this is not the exact same device and the AMD naming and PCI Device IDs
>> aren't particularly helpful here.
>>
>> The SMBus port selection register is common to the following Families
>> and models, as documented in AMD's publicly available BIOS and Kernel
>> Developer Guides:
>>
>>  50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>>  55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>>  52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
>>
>> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
>> between Bolton FCH and Family 16h Model 30h, but the location of the
>> SmBus0Sel port selection bits are different:
>>
>>  51192 - Bolton Register Reference Guide
>>
>> We distinguish between Bolton and Family 16h Model 30h using the PCI
>> Revision ID:
>>
>>   Bolton is device 0x780b, revision 0x15
>>   Family 16h Model 30h is device 0x780b, revision 0x1F
>>   Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
>>
>> The following additional public AMD BKDG documents were checked and do
>> not share the same port selection register:
>>
>>  42301 - Family 15h Model 00h-0Fh doesn't mention any
>>  42300 - Family 15h Model 10h-1Fh doesn't mention any
>>  49125 - Family 15h Model 30h-3Fh doesn't mention any
>>
>>  48751 - Family 16h Model 00h-0Fh uses the previously supported
>>          index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
>>
>> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
>> ---
>>  drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
>> index 174579d..5c90a44 100644
>> --- a/drivers/i2c/busses/i2c-piix4.c
>> +++ b/drivers/i2c/busses/i2c-piix4.c
>> @@ -99,7 +99,7 @@
>>  #define SB800_PIIX4_PORT_IDX_MASK	0x06
>>  #define SB800_PIIX4_PORT_IDX_SHIFT	1
>>  
>> -/* On kerncz, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
>> +/* On kerncz and Hudson2, SmBus0Sel is at bit 20:19 of PMx00 DecodeEn */
>>  #define SB800_PIIX4_PORT_IDX_KERNCZ		0x02
>>  #define SB800_PIIX4_PORT_IDX_MASK_KERNCZ	0x18
>>  #define SB800_PIIX4_PORT_IDX_SHIFT_KERNCZ	3
>> @@ -359,18 +359,16 @@ static int piix4_setup_sb800(struct pci_dev *PIIX4_dev,
>>  
>>  	/* Find which register is used for port selection */
>>  	if (PIIX4_dev->vendor == PCI_VENDOR_ID_AMD) {
>> -		switch (PIIX4_dev->device) {
>> -		case PCI_DEVICE_ID_AMD_KERNCZ_SMBUS:
>> +		if ((PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) ||
> 
> nit:		if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
> 
>> +		    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
>> +			   PIIX4_dev->revision >= 0x1F)) {
> 
> 
> Hope this helps,

It does, thanks. I'll roll this fix in with whatever other feedback I get before sending the next patch version.

a.

Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[Jean Delvare]

Hi Andrew,

Sorry for the delay... What can I say :(

On Mon, 26 Feb 2018 10:28:43 +1000, Andrew Cooks wrote:
> Family 16h Model 30h SMBus controller needs the same port selection fix
> as described and fixed in commit 0fe16195f891 ("i2c: piix4: Fix SMBus port
> selection for AMD Family 17h chips")
> 
> commit 6befa3fde65f ("i2c: piix4: Support alternative port selection
> register") also fixed the port selection for Hudson2, but unfortunately
> this is not the exact same device and the AMD naming and PCI Device IDs
> aren't particularly helpful here.
> 
> The SMBus port selection register is common to the following Families
> and models, as documented in AMD's publicly available BIOS and Kernel
> Developer Guides:
> 
>  50742 - Family 15h Model 60h-6Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>  55072 - Family 15h Model 70h-7Fh (PCI_DEVICE_ID_AMD_KERNCZ_SMBUS)
>  52740 - Family 16h Model 30h-3Fh (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS)
> 
> The Hudson2 PCI Device ID (PCI_DEVICE_ID_AMD_HUDSON2_SMBUS) is shared
> between Bolton FCH and Family 16h Model 30h, but the location of the
> SmBus0Sel port selection bits are different:
> 
>  51192 - Bolton Register Reference Guide
> 
> We distinguish between Bolton and Family 16h Model 30h using the PCI
> Revision ID:
> 
>   Bolton is device 0x780b, revision 0x15
>   Family 16h Model 30h is device 0x780b, revision 0x1F
>   Family 15h Model 60h and 70h are both device 0x790b, revision 0x4A.
> 
> The following additional public AMD BKDG documents were checked and do
> not share the same port selection register:
> 
>  42301 - Family 15h Model 00h-0Fh doesn't mention any
>  42300 - Family 15h Model 10h-1Fh doesn't mention any
>  49125 - Family 15h Model 30h-3Fh doesn't mention any
> 
>  48751 - Family 16h Model 00h-0Fh uses the previously supported
>          index register SB800_PIIX4_PORT_IDX_ALT at 0x2e
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> (...)

Looks good to me. Unfortunately the patch no longer applies (my fault
obviously), it needs to be rebased on top of commit
24beb83ad289c68bce7c01351cb90465bbb1940a ("i2c-piix4: Add Hygon Dhyana
SMBus support").

I also agree with Tobin's suggestion to remove unneeded parentheses.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

This patch should also address Will Wagner's (Cc'd) complaint in another
thread ("[BUG] i2c_piix4: Hudson2 uses wrong port to access SMBus
controller").

I believe this is stable branch material.

-- 
Jean Delvare
SUSE L3 Support

Re: [RESEND][PATCH v4 1/3] i2c: piix4: Fix port selection for AMD Family 16h Model 30h

[Jean Delvare]

On Wed, 24 Jul 2019 10:37:48 +0200, Jean Delvare wrote:
> Hi Andrew,
> 
> Sorry for the delay... What can I say :(

Unfortunately by now Andrew is gone. So I will be the one rebasing and
resubmitting this series.

-- 
Jean Delvare
SUSE L3 Support

[RESEND][PATCH v4 2/3] i2c: piix4: fix probing of reserved ports on AMD

[Andrew Cooks]

Prevent bus timeouts and resets on Family 16h Model 30h by not probing
reserved Ports 3 and 4.

According to the AMD BIOS and Kernel Developer's Guides (BKDG), Port 3
and Port 4 are reserved on the following devices:
 - Family 15h Model 60h-6Fh
 - Family 15h Model 70h-7Fh
 - Family 16h Model 30h-3Fh

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 5c90a44..01f1610 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -80,7 +80,8 @@
 #define PIIX4_BLOCK_DATA	0x14
 
 /* Multi-port constants */
-#define PIIX4_MAX_ADAPTERS 4
+#define PIIX4_MAX_ADAPTERS	4
+#define HUDSON2_MAIN_PORTS	2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
 
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
@@ -800,6 +801,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
 
 static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
+static int piix4_adapter_count;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 			     bool sb800_main, u8 port, bool notify_imc,
@@ -849,6 +851,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	}
 
 	*padap = adap;
+	piix4_adapter_count++;
 	return 0;
 }
 
@@ -856,10 +859,17 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
 				    bool notify_imc)
 {
 	struct i2c_piix4_adapdata *adapdata;
-	int port;
+	int port, port_count;
 	int retval;
 
-	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+	if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
+	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
+		port_count = HUDSON2_MAIN_PORTS;
+	} else {
+		port_count = PIIX4_MAX_ADAPTERS;
+	}
+
+	for (port = 0; port < port_count; port++) {
 		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
@@ -889,6 +899,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
 {
 	int retval;
 	bool is_sb800 = false;
+	piix4_adapter_count = 0;
 
 	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
 	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
@@ -993,7 +1004,7 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	int port = PIIX4_MAX_ADAPTERS;
+	int port = piix4_adapter_count;
 
 	while (--port >= 0) {
 		if (piix4_main_adapters[port]) {
-- 
2.7.4

Re: [RESEND][PATCH v4 2/3] i2c: piix4: fix probing of reserved ports on AMD

[Jean Delvare]

On Mon, 26 Feb 2018 10:28:44 +1000, Andrew Cooks wrote:
> Prevent bus timeouts and resets on Family 16h Model 30h by not probing
> reserved Ports 3 and 4.
> 
> According to the AMD BIOS and Kernel Developer's Guides (BKDG), Port 3
> and Port 4 are reserved on the following devices:
>  - Family 15h Model 60h-6Fh
>  - Family 15h Model 70h-7Fh
>  - Family 16h Model 30h-3Fh
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)

I agree the change is needed but I'm not convinced about the
implementation. It seems more complex than it needs to be and not
reliable is something goes wrong.

> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 5c90a44..01f1610 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -80,7 +80,8 @@
>  #define PIIX4_BLOCK_DATA	0x14
>  
>  /* Multi-port constants */
> -#define PIIX4_MAX_ADAPTERS 4
> +#define PIIX4_MAX_ADAPTERS	4
> +#define HUDSON2_MAIN_PORTS	2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
>  
>  /* SB800 constants */
>  #define SB800_PIIX4_SMB_IDX		0xcd6
> @@ -800,6 +801,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
>  
>  static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
>  static struct i2c_adapter *piix4_aux_adapter;
> +static int piix4_adapter_count;
>  
>  static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  			     bool sb800_main, u8 port, bool notify_imc,
> @@ -849,6 +851,7 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  	}
>  
>  	*padap = adap;
> +	piix4_adapter_count++;
>  	return 0;
>  }

So piix4_adapter_count is counting the *overall* number of
*successfully* created adapters. This count could be *more* than
PIIX4_MAX_ADAPTERS/HUDSON2_MAIN_PORTS (if all main adapters plus the
aux adapter are created successfully). But this count could also be
*less* than PIIX4_MAX_ADAPTERS (on all systems where ports 3 and 4 are
reserved and thus now skipped).

>  
> @@ -856,10 +859,17 @@ static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba,
>  				    bool notify_imc)
>  {
>  	struct i2c_piix4_adapdata *adapdata;
> -	int port;
> +	int port, port_count;
>  	int retval;
>  
> -	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
> +	if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
> +	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {

(There's another potential problem here, see below.)

> +		port_count = HUDSON2_MAIN_PORTS;
> +	} else {
> +		port_count = PIIX4_MAX_ADAPTERS;
> +	}
> +

Here port_count represents the number of *possible* *main* adapter
ports. So not the same as piix4_adapter_count.

> +	for (port = 0; port < port_count; port++) {
>  		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
>  					   piix4_main_port_names_sb800[port],
>  					   &piix4_main_adapters[port]);
> @@ -889,6 +899,7 @@ static int piix4_probe(struct pci_dev *dev, const struct pci_device_id *id)
>  {
>  	int retval;
>  	bool is_sb800 = false;
> +	piix4_adapter_count = 0;
>  
>  	if ((dev->vendor == PCI_VENDOR_ID_ATI &&
>  	     dev->device == PCI_DEVICE_ID_ATI_SBX00_SMBUS &&
> @@ -993,7 +1004,7 @@ static void piix4_adap_remove(struct i2c_adapter *adap)
>  
>  static void piix4_remove(struct pci_dev *dev)
>  {
> -	int port = PIIX4_MAX_ADAPTERS;
> +	int port = piix4_adapter_count;

And here we have the problem. We are basically looping over
piix4_main_adapters[] which has exactly PIIX4_MAX_ADAPTERS elements,
some of which may be unused. If piix4_adapter_count is 5 (4 main + 1
aux port) we will overrun the array. And if one "middle" main adapter
port failed to register (unlikely event but can happen in theory, for
example on memory allocation error) then we may omit to unregister the
last main adapter.

What we really need here is port_count from function
piix4_add_adapters_sb800(), which unfortunately was a local and no
longer exists.

>  
>  	while (--port >= 0) {
>  		if (piix4_main_adapters[port]) {

I think the whole change can be simplified by introducing only 1 new
variable instead of 2, and using it where appropriate:

--- linux-5.1.orig/drivers/i2c/busses/i2c-piix4.c	2019-07-24 10:02:40.404816668 +0200
+++ linux-5.1/drivers/i2c/busses/i2c-piix4.c	2019-07-24 11:22:47.827417870 +0200
@@ -80,7 +80,8 @@
 #define PIIX4_BLOCK_DATA	0x14
 
 /* Multi-port constants */
-#define PIIX4_MAX_ADAPTERS 4
+#define PIIX4_MAX_ADAPTERS	4
+#define HUDSON2_MAIN_PORTS	2 /* HUDSON2, KERNCZ reserves ports 3, 4 */
 
 /* SB800 constants */
 #define SB800_PIIX4_SMB_IDX		0xcd6
@@ -814,6 +815,7 @@ MODULE_DEVICE_TABLE (pci, piix4_ids);
 
 static struct i2c_adapter *piix4_main_adapters[PIIX4_MAX_ADAPTERS];
 static struct i2c_adapter *piix4_aux_adapter;
+static int piix4_adapter_count;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 			     bool sb800_main, u8 port, bool notify_imc,
@@ -873,7 +875,14 @@ static int piix4_add_adapters_sb800(stru
 	int port;
 	int retval;
 
-	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
+	if (dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS ||
+	    dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS) {
+		piix4_adapter_count = HUDSON2_MAIN_PORTS;
+	} else {
+		piix4_adapter_count = PIIX4_MAX_ADAPTERS;
+	}
+
+	for (port = 0; port < piix4_adapter_count; port++) {
 		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
@@ -995,7 +1005,7 @@ static void piix4_adap_remove(struct i2c
 
 static void piix4_remove(struct pci_dev *dev)
 {
-	int port = PIIX4_MAX_ADAPTERS;
+	int port = piix4_adapter_count;
 
 	while (--port >= 0) {
 		if (piix4_main_adapters[port]) {

Anyone sees a problem with this approach?

The second issue I see is that ports 3 and 4 are skipped for all
PCI_DEVICE_ID_AMD_HUDSON2_SMBUS devices while the previous patch in
this series suggests that the Bolton chipset (device revision 0x15)
behaves differently. As a matter of fact the description of this patch
does not list it as being affected by the issue. So in effect we would
be disabling ports 3 and 4 for Bolton users, who may have valuable I2C
devices connected there. We just fixed a regression for them with
previous patch, let's not introduce a new one with this patch. I think
the device test should look like:

	if (PIIX4_dev->device == PCI_DEVICE_ID_AMD_KERNCZ_SMBUS ||
	    (PIIX4_dev->device == PCI_DEVICE_ID_AMD_HUDSON2_SMBUS &&
	     PIIX4_dev->revision >= 0x1F)) {
		piix4_adapter_count = HUDSON2_MAIN_PORTS;
	} else {
		(...)

i.e. the same as in previous patch.

By the way, I don't own compatible hardware. I will see if I can gain
access to a SUSE Labs machine for testing purposes, but it would be a
lot easier if someone with the hardware could test the patch series
once it is rebased. Volunteers?

Thanks,
-- 
Jean Delvare
SUSE L3 Support

[RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support

[Andrew Cooks]

Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
devices using ACPI. It builds on the related I2C mux device work
in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")

In the i2c-piix4 driver the adapters are enumerated as:
 Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)

However, in the AMD BKDG documentation[1], the implied order of ports is:
 Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...

This ordering difference is unfortunate. We assume that ACPI
developers will use the Linux ordering.

[1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
Models 30h-3Fh Processors

Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
---
 drivers/i2c/busses/i2c-piix4.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 01f1610..9a6cdc8 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -837,6 +837,12 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;
 
+	if (has_acpi_companion(&dev->dev)) {
+		acpi_preset_companion(&adap->dev,
+				ACPI_COMPANION(&dev->dev),
+				piix4_adapter_count);
+	}
+
 	snprintf(adap->name, sizeof(adap->name),
 		"SMBus PIIX4 adapter%s at %04x", name, smba);
 
-- 
2.7.4

Re: [RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support

[Tobin C. Harding]

On Mon, Feb 26, 2018 at 10:28:45AM +1000, Andrew Cooks wrote:
> Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
> devices using ACPI. It builds on the related I2C mux device work
> in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")
> 
> In the i2c-piix4 driver the adapters are enumerated as:
>  Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)
> 
> However, in the AMD BKDG documentation[1], the implied order of ports is:
>  Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...
> 
> This ordering difference is unfortunate. We assume that ACPI
> developers will use the Linux ordering.
> 
> [1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
> Models 30h-3Fh Processors
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 01f1610..9a6cdc8 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -837,6 +837,12 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  	/* set up the sysfs linkage to our parent device */
>  	adap->dev.parent = &dev->dev;
>  
> +	if (has_acpi_companion(&dev->dev)) {
> +		acpi_preset_companion(&adap->dev,
> +				ACPI_COMPANION(&dev->dev),
> +				piix4_adapter_count);

nit:
		acpi_preset_companion(&adap->dev,
				      ACPI_COMPANION(&dev->dev),
				      piix4_adapter_count);

> +	}
> +
>  	snprintf(adap->name, sizeof(adap->name),
>  		"SMBus PIIX4 adapter%s at %04x", name, smba);


Hope this helps,
Tobin

Re: [RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support

[Jean Delvare]

On Mon, 26 Feb 2018 10:28:45 +1000, Andrew Cooks wrote:
> Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
> devices using ACPI. It builds on the related I2C mux device work
> in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")
> 
> In the i2c-piix4 driver the adapters are enumerated as:
>  Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)
> 
> However, in the AMD BKDG documentation[1], the implied order of ports is:
>  Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...
> 
> This ordering difference is unfortunate. We assume that ACPI
> developers will use the Linux ordering.
> 
> [1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
> Models 30h-3Fh Processors
> 
> Signed-off-by: Andrew Cooks <andrew.cooks@opengear.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 01f1610..9a6cdc8 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -837,6 +837,12 @@ static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
>  	/* set up the sysfs linkage to our parent device */
>  	adap->dev.parent = &dev->dev;
>  
> +	if (has_acpi_companion(&dev->dev)) {
> +		acpi_preset_companion(&adap->dev,
> +				ACPI_COMPANION(&dev->dev),
> +				piix4_adapter_count);

After the change I proposed for the previous patch, this is no longer
going to work. But I don't think it was really working before anyway.

For one thing, for the same reason I want to change the previous patch:
in case of failure to register some of the adapters, the numbering of
later adapters would be shifted. Also giving the aux bus a different
number depending on the device (4 before Hudson2, 2 for Hudson2 and
later) is unlikely to match the BIOS expectations.

For another, the assumption that "ACPI developers will use the Linux
ordering" is unlikely to be valid. I think we are talking about BIOS
developers here, and they should be OS-agnostic. If they are not, then
most likely they would align with whatever Windows drivers expect. So
our best chance is to stick to the datasheet.

Lastly, this would be inconsistent even with our own driver. We are
indeed not instantiating the adapters in the order listed by the
datasheet, and i2c adapter numbering is dynamic, but we are *naming* the
adapters to match the datasheet. So we should really pass the same
number to the ACPI layer, for consistency if nothing else. This
probably means passing one more parameter to piix4_add_adapter() and/or
some more code to figure out the bus number to pass to
acpi_preset_companion(), but I don't think we can avoid that, so let's
just do it.

> +	}
> +
>  	snprintf(adap->name, sizeof(adap->name),
>  		"SMBus PIIX4 adapter%s at %04x", name, smba);
>  


-- 
Jean Delvare
SUSE L3 Support

Re: [RESEND][PATCH v4 3/3] i2c: piix4: add ACPI support

[Jean Delvare]

My attempt looks like that:

Based on earlier work by Andrew Cooks.

Enable the i2c-piix4 SMBus controller driver to enumerate I2C slave
devices using ACPI. It builds on the related I2C mux device work
in commit 8eb5c87a92c0 ("i2c: add ACPI support for I2C mux ports")

In the i2c-piix4 driver the adapters are enumerated as:
 Main SMBus adapter Port 0, Port 2, ..., aux port (i.e., ASF adapter)

However, in the AMD BKDG documentation[1], the implied order of ports is:
 Main SMBus adapter Port 0, ASF adapter, Port 2, Port 3, ...

This ordering difference is unfortunate. We assume that ACPI
developers will use the AMD documentation ordering, so we have to
pass an extra parameter to piix4_add_adapter().

[1] 52740 BIOS and Kernel Developer's Guide (BKDG) for AMD Family 16h
Models 30h-3Fh Processors

Signed-off-by: Jean Delvare <jdelvare@suse.de>
---
 drivers/i2c/busses/i2c-piix4.c |   18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

--- linux-5.1.orig/drivers/i2c/busses/i2c-piix4.c	2019-07-24 11:29:49.836450493 +0200
+++ linux-5.1/drivers/i2c/busses/i2c-piix4.c	2019-07-24 15:21:20.166362730 +0200
@@ -819,7 +819,8 @@ static int piix4_adapter_count;
 
 static int piix4_add_adapter(struct pci_dev *dev, unsigned short smba,
 			     bool sb800_main, u8 port, bool notify_imc,
-			     const char *name, struct i2c_adapter **padap)
+			     u8 hw_port_nr, const char *name,
+			     struct i2c_adapter **padap)
 {
 	struct i2c_adapter *adap;
 	struct i2c_piix4_adapdata *adapdata;
@@ -851,6 +852,12 @@ static int piix4_add_adapter(struct pci_
 	/* set up the sysfs linkage to our parent device */
 	adap->dev.parent = &dev->dev;
 
+	if (has_acpi_companion(&dev->dev)) {
+		acpi_preset_companion(&adap->dev,
+				ACPI_COMPANION(&dev->dev),
+				hw_port_nr);
+	}
+
 	snprintf(adap->name, sizeof(adap->name),
 		"SMBus PIIX4 adapter%s at %04x", name, smba);
 
@@ -883,7 +890,10 @@ static int piix4_add_adapters_sb800(stru
 	}
 
 	for (port = 0; port < piix4_adapter_count; port++) {
+		u8 hw_port_nr = port == 0 ? 0 : port + 1;
+
 		retval = piix4_add_adapter(dev, smba, true, port, notify_imc,
+					   hw_port_nr,
 					   piix4_main_port_names_sb800[port],
 					   &piix4_main_adapters[port]);
 		if (retval < 0)
@@ -954,8 +964,8 @@ static int piix4_probe(struct pci_dev *d
 			return retval;
 
 		/* Try to register main SMBus adapter, give up if we can't */
-		retval = piix4_add_adapter(dev, retval, false, 0, false, "",
-					   &piix4_main_adapters[0]);
+		retval = piix4_add_adapter(dev, retval, false, 0, false, 0,
+					   "", &piix4_main_adapters[0]);
 		if (retval < 0)
 			return retval;
 	}
@@ -981,7 +991,7 @@ static int piix4_probe(struct pci_dev *d
 	if (retval > 0) {
 		/* Try to add the aux adapter if it exists,
 		 * piix4_add_adapter will clean up if this fails */
-		piix4_add_adapter(dev, retval, false, 0, false,
+		piix4_add_adapter(dev, retval, false, 0, false, 1,
 				  is_sb800 ? piix4_aux_port_name_sb800 : "",
 				  &piix4_aux_adapter);
 	}


-- 
Jean Delvare
SUSE L3 Support