[PATCH] i2c-piix4: Fix SB800 locking

[PATCH] i2c-piix4: Fix SB800 locking

[Jean Delvare]

We need a single mutex for all 4 shared SMBus ports on the SB800. A
per-port mutex doesn't protect us from concurrent access.

In theory the mutex should be per PCI device, however in practice we
know that there's only ever a single instance of the device in a given
system so we can use a global.

Also take the mutex during initialization, as first port may be already
in use when second port is initialized.

Signed-off-by: Jean Delvare <jdelvare@suse.de>
Cc: Christian Fetzer <fetzer.ch@gmail.com>
Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Wolfram Sang <wsa@the-dreams.de>
---
 drivers/i2c/busses/i2c-piix4.c |   22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

--- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-22 13:51:27.500458587 +0100
+++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-22 13:56:13.210960522 +0100
@@ -137,6 +137,7 @@ static const struct dmi_system_id piix4_
 };
 
 /* SB800 globals */
+DEFINE_MUTEX(piix4_mutex_sb800);
 static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
 	"SDA0", "SDA2", "SDA3", "SDA4"
 };
@@ -148,7 +149,6 @@ struct i2c_piix4_adapdata {
 	/* SB800 */
 	bool sb800_main;
 	unsigned short port;
-	struct mutex *mutex;
 };
 
 static int piix4_setup(struct pci_dev *PIIX4_dev,
@@ -275,10 +275,12 @@ static int piix4_setup_sb800(struct pci_
 	else
 		smb_en = (aux) ? 0x28 : 0x2c;
 
+	mutex_lock(&piix4_mutex_sb800);
 	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
 	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
 	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
+	mutex_unlock(&piix4_mutex_sb800);
 
 	if (!smb_en) {
 		smb_en_status = smba_en_lo & 0x10;
@@ -559,7 +561,7 @@ static s32 piix4_access_sb800(struct i2c
 	u8 port;
 	int retval;
 
-	mutex_lock(adapdata->mutex);
+	mutex_lock(&piix4_mutex_sb800);
 
 	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
@@ -574,7 +576,7 @@ static s32 piix4_access_sb800(struct i2c
 
 	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
 
-	mutex_unlock(adapdata->mutex);
+	mutex_unlock(&piix4_mutex_sb800);
 
 	return retval;
 }
@@ -673,17 +675,10 @@ static int piix4_add_adapter(struct pci_
 
 static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
 {
-	struct mutex *mutex;
 	struct i2c_piix4_adapdata *adapdata;
 	int port;
 	int retval;
 
-	mutex = kzalloc(sizeof(*mutex), GFP_KERNEL);
-	if (mutex == NULL)
-		return -ENOMEM;
-
-	mutex_init(mutex);
-
 	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
 		retval = piix4_add_adapter(dev, smba,
 					   piix4_main_port_names_sb800[port],
@@ -696,7 +691,6 @@ static int piix4_add_adapters_sb800(stru
 		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
 		adapdata->sb800_main = true;
 		adapdata->port = port;
-		adapdata->mutex = mutex;
 	}
 
 	return retval;
@@ -714,8 +708,6 @@ error:
 		}
 	}
 
-	kfree(mutex);
-
 	return retval;
 }
 
@@ -798,10 +790,8 @@ static void piix4_adap_remove(struct i2c
 		i2c_del_adapter(adap);
 		if (adapdata->port == 0) {
 			release_region(adapdata->smba, SMBIOSIZE);
-			if (adapdata->sb800_main) {
-				kfree(adapdata->mutex);
+			if (adapdata->sb800_main)
 				release_region(SB800_PIIX4_SMB_IDX, 2);
-			}
 		}
 		kfree(adapdata);
 		kfree(adap);


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c-piix4: Fix SB800 locking

[fetzerch]

Hi Jean,

On 22.01.2016 14:12, Jean Delvare wrote:
> We need a single mutex for all 4 shared SMBus ports on the SB800. A
> per-port mutex doesn't protect us from concurrent access.
> 
> In theory the mutex should be per PCI device, however in practice we
> know that there's only ever a single instance of the device in a given
> system so we can use a global.
> 
> Also take the mutex during initialization, as first port may be already
> in use when second port is initialized.

Thanks for following up on this. While the code obviously looks correct
the patch unfortunately does not fix the problem described in the
original patchset (i2c_piix4 has to be loaded before jc42).

> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>
> ---
>  drivers/i2c/busses/i2c-piix4.c |   22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> --- linux-4.5-rc0.orig/drivers/i2c/busses/i2c-piix4.c	2016-01-22 13:51:27.500458587 +0100
> +++ linux-4.5-rc0/drivers/i2c/busses/i2c-piix4.c	2016-01-22 13:56:13.210960522 +0100
> @@ -137,6 +137,7 @@ static const struct dmi_system_id piix4_
>  };
>  
>  /* SB800 globals */
> +DEFINE_MUTEX(piix4_mutex_sb800);
>  static const char *piix4_main_port_names_sb800[PIIX4_MAX_ADAPTERS] = {
>  	"SDA0", "SDA2", "SDA3", "SDA4"
>  };
> @@ -148,7 +149,6 @@ struct i2c_piix4_adapdata {
>  	/* SB800 */
>  	bool sb800_main;
>  	unsigned short port;
> -	struct mutex *mutex;
>  };
>  
>  static int piix4_setup(struct pci_dev *PIIX4_dev,
> @@ -275,10 +275,12 @@ static int piix4_setup_sb800(struct pci_
>  	else
>  		smb_en = (aux) ? 0x28 : 0x2c;
>  
> +	mutex_lock(&piix4_mutex_sb800);
>  	outb_p(smb_en, SB800_PIIX4_SMB_IDX);
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
>  	outb_p(smb_en + 1, SB800_PIIX4_SMB_IDX);
>  	smba_en_hi = inb_p(SB800_PIIX4_SMB_IDX + 1);
> +	mutex_unlock(&piix4_mutex_sb800);
>  
>  	if (!smb_en) {
>  		smb_en_status = smba_en_lo & 0x10;
> @@ -559,7 +561,7 @@ static s32 piix4_access_sb800(struct i2c
>  	u8 port;
>  	int retval;
>  
> -	mutex_lock(adapdata->mutex);
> +	mutex_lock(&piix4_mutex_sb800);
>  
>  	outb_p(SB800_PIIX4_PORT_IDX, SB800_PIIX4_SMB_IDX);
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> @@ -574,7 +576,7 @@ static s32 piix4_access_sb800(struct i2c
>  
>  	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
>  
> -	mutex_unlock(adapdata->mutex);
> +	mutex_unlock(&piix4_mutex_sb800);
>  
>  	return retval;
>  }
> @@ -673,17 +675,10 @@ static int piix4_add_adapter(struct pci_
>  
>  static int piix4_add_adapters_sb800(struct pci_dev *dev, unsigned short smba)
>  {
> -	struct mutex *mutex;
>  	struct i2c_piix4_adapdata *adapdata;
>  	int port;
>  	int retval;
>  
> -	mutex = kzalloc(sizeof(*mutex), GFP_KERNEL);
> -	if (mutex == NULL)
> -		return -ENOMEM;
> -
> -	mutex_init(mutex);
> -
>  	for (port = 0; port < PIIX4_MAX_ADAPTERS; port++) {
>  		retval = piix4_add_adapter(dev, smba,
>  					   piix4_main_port_names_sb800[port],
> @@ -696,7 +691,6 @@ static int piix4_add_adapters_sb800(stru
>  		adapdata = i2c_get_adapdata(piix4_main_adapters[port]);
>  		adapdata->sb800_main = true;
>  		adapdata->port = port;
> -		adapdata->mutex = mutex;
>  	}
>  
>  	return retval;
> @@ -714,8 +708,6 @@ error:
>  		}
>  	}
>  
> -	kfree(mutex);
> -
>  	return retval;
>  }
>  
> @@ -798,10 +790,8 @@ static void piix4_adap_remove(struct i2c
>  		i2c_del_adapter(adap);
>  		if (adapdata->port == 0) {
>  			release_region(adapdata->smba, SMBIOSIZE);
> -			if (adapdata->sb800_main) {
> -				kfree(adapdata->mutex);
> +			if (adapdata->sb800_main)
>  				release_region(SB800_PIIX4_SMB_IDX, 2);
> -			}
>  		}
>  		kfree(adapdata);
>  		kfree(adap);
> 
> 

Re: [PATCH] i2c-piix4: Fix SB800 locking

[Mika Westerberg]

On Fri, Jan 22, 2016 at 02:12:02PM +0100, Jean Delvare wrote:
> We need a single mutex for all 4 shared SMBus ports on the SB800. A
> per-port mutex doesn't protect us from concurrent access.
> 
> In theory the mutex should be per PCI device, however in practice we
> know that there's only ever a single instance of the device in a given
> system so we can use a global.
> 
> Also take the mutex during initialization, as first port may be already
> in use when second port is initialized.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>

The patch looks correct to me,

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks Jean for looking into this.

Re: [PATCH] i2c-piix4: Fix SB800 locking

[Wolfram Sang]

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

On Fri, Jan 22, 2016 at 02:12:02PM +0100, Jean Delvare wrote:
> We need a single mutex for all 4 shared SMBus ports on the SB800. A
> per-port mutex doesn't protect us from concurrent access.
> 
> In theory the mutex should be per PCI device, however in practice we
> know that there's only ever a single instance of the device in a given
> system so we can use a global.
> 
> Also take the mutex during initialization, as first port may be already
> in use when second port is initialized.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Cc: Christian Fetzer <fetzer.ch@gmail.com>
> Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Cc: Wolfram Sang <wsa@the-dreams.de>

Fixed a sparse warning by making the mutex static, took the freedom to
read Christian's mail as Tested-by and applied to for-current, thanks!


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] i2c-piix4: Fix SB800 locking

[Jean Delvare]

Le Tuesday 26 January 2016 à 06:42 +0100, Wolfram Sang a écrit :
> On Fri, Jan 22, 2016 at 02:12:02PM +0100, Jean Delvare wrote:
> > We need a single mutex for all 4 shared SMBus ports on the SB800. A
> > per-port mutex doesn't protect us from concurrent access.
> > 
> > In theory the mutex should be per PCI device, however in practice we
> > know that there's only ever a single instance of the device in a given
> > system so we can use a global.
> > 
> > Also take the mutex during initialization, as first port may be already
> > in use when second port is initialized.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> > Cc: Christian Fetzer <fetzer.ch@gmail.com>
> > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > Cc: Wolfram Sang <wsa@the-dreams.de>
> 
> Fixed a sparse warning by making the mutex static, took the freedom to
> read Christian's mail as Tested-by and applied to for-current, thanks!

Oh, I thought I had fixed that before submitting but clearly I forgot.
Thanks for doing it :)

I'll send a patch restoring I2C bus names for older devices later today.
I have a few other fixups in mind but the rest isn't as urgent.

-- 
Jean Delvare
SUSE L3 Support