[v5,5/6] i2c: designware-baytrail: Fix race when resetting the semaphore
diff mbox

Message ID 20170101201521.12364-5-hdegoede@redhat.com
State Superseded
Headers show

Commit Message

Hans de Goede Jan. 1, 2017, 8:15 p.m. UTC
Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
we keep the iosf_mbi_lock locked during the read-modify-write done to
reset the semaphore.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
Changes in v5:
-New patch in v5 of this patch-set
---
 drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

Comments

Andy Shevchenko Jan. 2, 2017, 9:36 a.m. UTC | #1
On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
> we keep the iosf_mbi_lock locked during the read-modify-write done to
> reset the semaphore.
> 

While patch itself looks good to me, I think it reduces a probability to
 race and doesn't eliminate an issue completely. Can you check i915 code
how it's done there?

> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
> Changes in v5:
> -New patch in v5 of this patch-set
> ---
>  drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
>  1 file changed, 2 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> b/drivers/i2c/busses/i2c-designware-baytrail.c
> index 650a700..8df529c 100644
> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> *sem)
>  
>  static void reset_semaphore(struct dw_i2c_dev *dev)
>  {
> -	u32 data;
> -
> -	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE, &data)) {
> -		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during read\n");
> -		return;
> -	}
> -
> -	data &= ~PUNIT_SEMAPHORE_BIT;
> -	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> PUNIT_SEMAPHORE, data))
> +	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> PUNIT_SEMAPHORE,
> +			    0, PUNIT_SEMAPHORE_BIT))
>  		dev_err(dev->dev, "iosf failed to reset punit
> semaphore during write\n");
>  
>  	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
Hans de Goede Jan. 2, 2017, 9:55 a.m. UTC | #2
Hi,

On 02-01-17 10:36, Andy Shevchenko wrote:
> On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
>> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so that
>> we keep the iosf_mbi_lock locked during the read-modify-write done to
>> reset the semaphore.
>>
>
> While patch itself looks good to me, I think it reduces a probability to
>  race and doesn't eliminate an issue completely.

All accesses through this register are done through the
iosf_mbi_ helpers, and the modify helper holds the
iosf_mbi_lock during the entire read-modify-wrtie cycle,
so I don't see how there can still be any issue left.

 > Can you check i915 code
> how it's done there?

AFAIK the i915 code never takes the pmic i2c bus semaphore.

Regards,

Hans

p.s.

Since you're interested in this set, you may also be interested
in this RFC patch-set:

https://www.spinics.net/lists/intel-gfx/msg115833.html


>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>> Changes in v5:
>> -New patch in v5 of this patch-set
>> ---
>>  drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
>>  1 file changed, 2 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
>> b/drivers/i2c/busses/i2c-designware-baytrail.c
>> index 650a700..8df529c 100644
>> --- a/drivers/i2c/busses/i2c-designware-baytrail.c
>> +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
>> @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
>> *sem)
>>
>>  static void reset_semaphore(struct dw_i2c_dev *dev)
>>  {
>> -	u32 data;
>> -
>> -	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> PUNIT_SEMAPHORE, &data)) {
>> -		dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during read\n");
>> -		return;
>> -	}
>> -
>> -	data &= ~PUNIT_SEMAPHORE_BIT;
>> -	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
>> PUNIT_SEMAPHORE, data))
>> +	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
>> PUNIT_SEMAPHORE,
>> +			    0, PUNIT_SEMAPHORE_BIT))
>>  		dev_err(dev->dev, "iosf failed to reset punit
>> semaphore during write\n");
>>
>>  	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);
>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Jan. 2, 2017, 10:06 a.m. UTC | #3
On Mon, 2017-01-02 at 10:55 +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-01-17 10:36, Andy Shevchenko wrote:
> > On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
> > > Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so
> > > that
> > > we keep the iosf_mbi_lock locked during the read-modify-write done
> > > to
> > > reset the semaphore.
> > > 
> > 
> > While patch itself looks good to me, I think it reduces a
> > probability to
> >  race and doesn't eliminate an issue completely.
> 
> All accesses through this register are done through the
> iosf_mbi_ helpers, and the modify helper holds the
> iosf_mbi_lock during the entire read-modify-wrtie cycle,
> so I don't see how there can still be any issue left.

Ah, seems you are right.

Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> 
>  > Can you check i915 code
> > how it's done there?
> 
> AFAIK the i915 code never takes the pmic i2c bus semaphore.
> 
> Regards,
> 
> Hans
> 
> p.s.
> 
> Since you're interested in this set, you may also be interested
> in this RFC patch-set:
> 
> https://www.spinics.net/lists/intel-gfx/msg115833.html
> 
> 
> > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > > Changes in v5:
> > > -New patch in v5 of this patch-set
> > > ---
> > >  drivers/i2c/busses/i2c-designware-baytrail.c | 11 ++---------
> > >  1 file changed, 2 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c
> > > b/drivers/i2c/busses/i2c-designware-baytrail.c
> > > index 650a700..8df529c 100644
> > > --- a/drivers/i2c/busses/i2c-designware-baytrail.c
> > > +++ b/drivers/i2c/busses/i2c-designware-baytrail.c
> > > @@ -47,15 +47,8 @@ static int get_sem(struct dw_i2c_dev *dev, u32
> > > *sem)
> > > 
> > >  static void reset_semaphore(struct dw_i2c_dev *dev)
> > >  {
> > > -	u32 data;
> > > -
> > > -	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > > PUNIT_SEMAPHORE, &data)) {
> > > -		dev_err(dev->dev, "iosf failed to reset punit
> > > semaphore during read\n");
> > > -		return;
> > > -	}
> > > -
> > > -	data &= ~PUNIT_SEMAPHORE_BIT;
> > > -	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE,
> > > PUNIT_SEMAPHORE, data))
> > > +	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ,
> > > PUNIT_SEMAPHORE,
> > > +			    0, PUNIT_SEMAPHORE_BIT))
> > >  		dev_err(dev->dev, "iosf failed to reset punit
> > > semaphore during write\n");
> > > 
> > >  	pm_qos_update_request(&dev->pm_qos,
> > > PM_QOS_DEFAULT_VALUE);
Jarkko Nikula Jan. 10, 2017, 1:16 p.m. UTC | #4
On 01/02/2017 12:06 PM, Andy Shevchenko wrote:
> On Mon, 2017-01-02 at 10:55 +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 02-01-17 10:36, Andy Shevchenko wrote:
>>> On Sun, 2017-01-01 at 21:15 +0100, Hans de Goede wrote:
>>>> Use iosf_mbi_modify instead of iosf_mbi_read + iosf_mbi_write so
>>>> that
>>>> we keep the iosf_mbi_lock locked during the read-modify-write done
>>>> to
>>>> reset the semaphore.
>>>>
>>>
>>> While patch itself looks good to me, I think it reduces a
>>> probability to
>>>  race and doesn't eliminate an issue completely.
>>
>> All accesses through this register are done through the
>> iosf_mbi_ helpers, and the modify helper holds the
>> iosf_mbi_lock during the entire read-modify-wrtie cycle,
>> so I don't see how there can still be any issue left.
>
> Ah, seems you are right.
>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>
Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/i2c/busses/i2c-designware-baytrail.c b/drivers/i2c/busses/i2c-designware-baytrail.c
index 650a700..8df529c 100644
--- a/drivers/i2c/busses/i2c-designware-baytrail.c
+++ b/drivers/i2c/busses/i2c-designware-baytrail.c
@@ -47,15 +47,8 @@  static int get_sem(struct dw_i2c_dev *dev, u32 *sem)
 
 static void reset_semaphore(struct dw_i2c_dev *dev)
 {
-	u32 data;
-
-	if (iosf_mbi_read(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE, &data)) {
-		dev_err(dev->dev, "iosf failed to reset punit semaphore during read\n");
-		return;
-	}
-
-	data &= ~PUNIT_SEMAPHORE_BIT;
-	if (iosf_mbi_write(BT_MBI_UNIT_PMC, MBI_REG_WRITE, PUNIT_SEMAPHORE, data))
+	if (iosf_mbi_modify(BT_MBI_UNIT_PMC, MBI_REG_READ, PUNIT_SEMAPHORE,
+			    0, PUNIT_SEMAPHORE_BIT))
 		dev_err(dev->dev, "iosf failed to reset punit semaphore during write\n");
 
 	pm_qos_update_request(&dev->pm_qos, PM_QOS_DEFAULT_VALUE);