On 01/10/18 18:31, Daniel Mack wrote:
On 30/9/2018 11:10 PM, Chris Packham wrote:
With this in mind, I don't see why this
- st = readl_relaxed(nfc->regs + NDSR);
- if (st & (NDSR_RDY(0) | NDSR_RDY(1)))
complete(&nfc->complete);Yeah, me neither. Chris, are you absolutely sure this is the reason? I'm asking because it took me several tries sometimes to trigger the bug, so is there a chance that you see an error at all times, regardless of whether my patch is applied?
It seems pretty consistent. Without this patch there seems to be no problem. With this patch it triggers pretty much straight away. I can't discount that there might be something wrong with my dts (the R/B configuration was missing initially).
I've also been able to run this on the DB-88F6820-AMC board with the same result (the dts for this is in the for-next branch of git://git.infradead.org/linux-mvebu.git).
The really odd thing is the following seems to avoid the problem
st = readl_relaxed(nfc->regs + NDSR);udelay(1000);if (st & (NDSR_RDY(0) | NDSR_RDY(1)))complete(&nfc->complete);Which is weird because the st value has already been read so the udelay should have no effect.
Erm, yes. That's totally weird. Which gcc are you using for this?
arm-softfloat-linux-gnueabi-gcc (crosstool-NG crosstool-ng-1.22.0) 4.9.3
Could you please try and use readl() here instead of readl_relaxed()? That will place a memory barrier after the read to enforce ordering.
I'd previously tried readl() based on the same hunch. No change.
I think my snippet above might be misleading. While a delay between readl_relaxed() and the if should not change the outcome, this is also a delay between marvell_nfc_enable_int() and marvell_nfc_disable_int() which is probably more significant. Sure enough if I move the delay to just before the marvell_nfc_disable_int() the error is not seen.
But if this is a problem, many other parts of that driver should be equally affected.
On 28/09/18 19:43, Daniel Mack wrote:
Also, does my .EALREADY approach (v1) make any difference?
The v1 of this patch doesn't show the problem.
That's also very strange because the condition it triggers on is exactly the same.
One difference is that by calling complete() interrupts will be disabled in the spinlock.
Thanks, Daniel