Re: [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver

[Ray Jui]

On 1/19/2015 11:28 AM, Russell King - ARM Linux wrote:
> On Sat, Jan 17, 2015 at 04:30:33PM -0800, Ray Jui wrote:
>>
>>
>> On 1/17/2015 2:40 PM, Russell King - ARM Linux wrote:
>>> On Sat, Jan 17, 2015 at 01:26:41PM -0800, Ray Jui wrote:
>>>> 	time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
>>>>
>>>> 	/* disable all interrupts */
>>>> 	writel(0, iproc_i2c->base + IE_OFFSET);
>>>>
>>>> 	if (!time_left && !atomic_read(&iproc_i2c->transfer_is_successful)) {
>>>
>>> Why are you using atomic_read() here?
>>>
>> transfer_is_successful 1) will be reset to 0 in this function (before
>> kick start the I2C transfer), 2) will be set to 1 in the ISR (to signal
>> completion of the I2C transfer), and 3) will be checked in this function
>> here. I thought that means I should declare it volatile, because it can
>> be modified in both the process context and interrupt context (and I use
>> atomic because I remember Linux checkpatch warns against using volatile)?
> 
> You don't need volatile or atomic_t for that.
> 
> Rather than switching to atomic_t when seeing the checkpatch warning,
> you'd do better to read Documentation/volatile-considered-harmful.txt
> to understand why checkpatch issues the warning, and realise that you
> don't need it for the above.
> 
> Note that in the above code, the compiler can't make an assumption
> about iproc_i2c->transfer_is_successful because it can't tell whether
> a called function (eg, wait_for_completion_timeout()) could modify it.
> 
Got it. Thanks.

> Another possible issue with the above code are these lines:
> 
> 	/* disable all interrupts */
> 	writel(0, iproc_i2c->base + IE_OFFSET);
> 
> It would be nice to think that would hit the hardware immediately, but
> that's making assumptions about hardware which are not necessary true.
> Your interrupt handler could even be running on another CPU after you've
> asked for that register to be written.
> 
> Depending on what you're trying to achieve here, you may need:
> 
> 	/* disable all interrupts */
> 	writel(0, iproc_i2c->base + IE_OFFSET);
> 	/* read it back to ensure the write has hit */
> 	readl(iproc_i2c->base + IE_OFFSET);
> 
> 	/* make sure the interrupt handler isn't running */
> 	synchronize_irq(...->irq);
> 
> if what you're trying to do is to ensure that the interrupt handler has
> finished running.
> 
This will be the most robust way of handling this. Given that we've
added an additional flag to check to make sure there's no interrupt
missed after wait_for_completion_timeout times out, it makes sense to
ensure that by the time when we check the flag there's no pending irq.
I'll add this to the driver and make 'xfer_is_done' an 'int' instead of
'atomic_t'. I will also add the call to readl to flush the write in the
remove function after interrupts are disabled.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/