Re: [PATCH v5 5/6] i2c: designware-baytrail: Fix race when resetting the semaphore

[Hans de Goede <hdegoede@redhat.com>]

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