linux-i2c.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: fetzerch <fetzer.ch@gmail.com>
To: Jean Delvare <jdelvare@suse.de>, Linux I2C <linux-i2c@vger.kernel.org>
Cc: Christian Fetzer <fetzer.ch@gmail.com>,
	Mika Westerberg <mika.westerberg@linux.intel.com>,
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Wolfram Sang <wsa@the-dreams.de>
Subject: Re: [PATCH] i2c-piix4: Fix SB800 locking
Date: Sat, 23 Jan 2016 14:26:23 +0100	[thread overview]
Message-ID: <56A37F7F.8000803@googlemail.com> (raw)
In-Reply-To: <20160122141202.09c94802@endymion.delvare>

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

  reply	other threads:[~2016-01-23 13:26 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-01-22 13:12 [PATCH] i2c-piix4: Fix SB800 locking Jean Delvare
2016-01-23 13:26 ` fetzerch [this message]
2016-01-25  9:29 ` Mika Westerberg
2016-01-26  5:42 ` Wolfram Sang
2016-01-26  7:04   ` Jean Delvare

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=56A37F7F.8000803@googlemail.com \
    --to=fetzer.ch@gmail.com \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=jdelvare@suse.de \
    --cc=linux-i2c@vger.kernel.org \
    --cc=mika.westerberg@linux.intel.com \
    --cc=wsa@the-dreams.de \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).