[PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Ricardo Ribalda Delgado]

SMBSLVCNT must be protected with the piix4_mutex_sb800 in order to avoid
multiple buses accessing to the semaphore at the same time.

Reported-by: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
---
 drivers/i2c/busses/i2c-piix4.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
index 73cc6799cc59..c21ca7bf2efe 100644
--- a/drivers/i2c/busses/i2c-piix4.c
+++ b/drivers/i2c/busses/i2c-piix4.c
@@ -592,6 +592,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 	u8 port;
 	int retval;
 
+	mutex_lock(&piix4_mutex_sb800);
+
 	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
 	smbslvcnt  = inb_p(SMBSLVCNT);
 	do {
@@ -605,10 +607,10 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 		usleep_range(1000, 2000);
 	} while (--retries);
 	/* SMBus is still owned by the IMC, we give up */
-	if (!retries)
+	if (!retries) {
+		mutex_unlock(&piix4_mutex_sb800);
 		return -EBUSY;
-
-	mutex_lock(&piix4_mutex_sb800);
+	}
 
 	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
 	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
@@ -623,11 +625,11 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
 
 	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
 
-	mutex_unlock(&piix4_mutex_sb800);
-
 	/* Release the semaphore */
 	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
 
+	mutex_unlock(&piix4_mutex_sb800);
+
 	return retval;
 }
 
-- 
2.11.0

Re: [PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Jean Delvare]

Hi Ricardo,

On Thu,  2 Feb 2017 20:15:16 +0100, Ricardo Ribalda Delgado wrote:
> SMBSLVCNT must be protected with the piix4_mutex_sb800 in order to avoid
> multiple buses accessing to the semaphore at the same time.
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
> ---
>  drivers/i2c/busses/i2c-piix4.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-piix4.c b/drivers/i2c/busses/i2c-piix4.c
> index 73cc6799cc59..c21ca7bf2efe 100644
> --- a/drivers/i2c/busses/i2c-piix4.c
> +++ b/drivers/i2c/busses/i2c-piix4.c
> @@ -592,6 +592,8 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  	u8 port;
>  	int retval;
>  
> +	mutex_lock(&piix4_mutex_sb800);
> +
>  	/* Request the SMBUS semaphore, avoid conflicts with the IMC */
>  	smbslvcnt  = inb_p(SMBSLVCNT);
>  	do {
> @@ -605,10 +607,10 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  		usleep_range(1000, 2000);
>  	} while (--retries);
>  	/* SMBus is still owned by the IMC, we give up */
> -	if (!retries)
> +	if (!retries) {
> +		mutex_unlock(&piix4_mutex_sb800);
>  		return -EBUSY;
> -
> -	mutex_lock(&piix4_mutex_sb800);
> +	}
>  
>  	outb_p(piix4_port_sel_sb800, SB800_PIIX4_SMB_IDX);
>  	smba_en_lo = inb_p(SB800_PIIX4_SMB_IDX + 1);
> @@ -623,11 +625,11 @@ static s32 piix4_access_sb800(struct i2c_adapter *adap, u16 addr,
>  
>  	outb_p(smba_en_lo, SB800_PIIX4_SMB_IDX + 1);
>  
> -	mutex_unlock(&piix4_mutex_sb800);
> -
>  	/* Release the semaphore */
>  	outb_p(smbslvcnt | 0x20, SMBSLVCNT);
>  
> +	mutex_unlock(&piix4_mutex_sb800);
> +
>  	return retval;
>  }
>  

Thanks for the quick fix.

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

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Wolfram Sang]

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

Jean,

> Thanks for the quick fix.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>

I wonder: why do you use Signed-off instead of Reviewed-by recently?


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

Re: [PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Wolfram Sang]

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

On Thu, Feb 02, 2017 at 08:15:16PM +0100, Ricardo Ribalda Delgado wrote:
> SMBSLVCNT must be protected with the piix4_mutex_sb800 in order to avoid
> multiple buses accessing to the semaphore at the same time.
> 
> Reported-by: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

Applied to for-current and added the same fixes tag as the patch to the
resource size, thanks!


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

Re: [PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Jean Delvare]

Hi Wolfram,

On Thu, 9 Feb 2017 17:09:26 +0100, Wolfram Sang wrote:
> > Thanks for the quick fix.
> > 
> > Signed-off-by: Jean Delvare <jdelvare@suse.de>
> 
> I wonder: why do you use Signed-off instead of Reviewed-by recently?

I use Signed-off-by for drivers I maintain, and Reviewed-by for the
rest.

But I can use whatever you prefer if that's a problem, I don't really
care.


-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH] i2c: piix4: Request the SMBUS semaphore inside the mutex

[Wolfram Sang]

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


> I use Signed-off-by for drivers I maintain, and Reviewed-by for the
> rest.

I see. I never noticed so far :)

> But I can use whatever you prefer if that's a problem, I don't really
> care.

Patchwork collects Rev-by tags from threads and shows the number of tags
in a list. This is helpful for me because I want to process already
reviewed patches earlier.

So, Rev-by would in deed be much appreciated for a better workflow.

Thanks!


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