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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <54BD7631.50205@broadcom.com>
Date:	Mon, 19 Jan 2015 13:25:05 -0800
From:	Ray Jui <rjui@...adcom.com>
To:	Russell King - ARM Linux <linux@....linux.org.uk>
CC:	Uwe Kleine-König 
	<u.kleine-koenig@...gutronix.de>, Wolfram Sang <wsa@...-dreams.de>,
	Rob Herring <robh+dt@...nel.org>,
	"Pawel Moll" <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	"Grant Likely" <grant.likely@...aro.org>,
	Christian Daudt <bcm@...thebug.org>,
	Matt Porter <mporter@...aro.org>,
	Florian Fainelli <f.fainelli@...il.com>,
	Scott Branden <sbranden@...adcom.com>,
	<linux-i2c@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-arm-kernel@...ts.infradead.org>,
	<bcm-kernel-feedback-list@...adcom.com>,
	<devicetree@...r.kernel.org>
Subject: Re: [PATCH v4 2/3] i2c: iproc: Add Broadcom iProc I2C Driver



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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/