On 5/23/2021 10:19 PM, Manivannan Sadhasivam wrote:
On Fri, May 21, 2021 at 10:50:33AM -0700, Bhaumik Bhatt wrote:
On 2021-05-10 11:17 PM, Manivannan Sadhasivam wrote:
Hi Pavel,
On Mon, May 10, 2021 at 10:56:50PM +0200, Pavel Machek wrote:
Hi!
From: Bhaumik Bhatt bbhatt@codeaurora.org
commit 47705c08465931923e2f2b506986ca0bdf80380d upstream.
When clearing up the channel context after client drivers are done using channels, the configuration is currently not being reset entirely. Ensure this is done to appropriately handle issues where clients unaware of the context state end up calling functions which expect a context.
+++ b/drivers/bus/mhi/core/init.c @@ -544,6 +544,7 @@ void mhi_deinit_chan_ctxt(struct mhi_con
- u32 tmp;
@@ -554,7 +555,19 @@ void mhi_deinit_chan_ctxt(struct mhi_con
...
- tmp = chan_ctxt->chcfg;
- tmp &= ~CHAN_CTX_CHSTATE_MASK;
- tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
- chan_ctxt->chcfg = tmp;
- /* Update to all cores */
- smp_wmb(); }
This is really interesting code; author was careful to make sure chcfg is updated atomically, but C compiler is free to undo that. Plus, this will make all kinds of checkers angry.
Does the file need to use READ_ONCE and WRITE_ONCE?
Thanks for looking into this.
I agree that the order could be mangled between chcfg read & write and using READ_ONCE & WRITE_ONCE seems to be a good option.
Bhaumik, can you please submit a patch and tag stable?
Thanks, Mani
Best regards, Pavel -- DENX Software Engineering GmbH, Managing Director: Wolfgang Denk HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Hi Pavel/Mani,
Hemant and I went over this patch and we noticed this particular function is already being called with the channel mutex lock held. This would take care of the atomicity and we also probably don't need the smp_wmb() barrier as the mutex unlock will implicitly take care of it.
okay
To the point of compiler re-ordering, we would need some help to understand the purpose of READ_ONCE()/WRITE_ONCE() for these dependent statements:
- tmp = chan_ctxt->chcfg;
- tmp &= ~CHAN_CTX_CHSTATE_MASK;
- tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
- chan_ctxt->chcfg = tmp;
Since RMW operation means that the chan_ctxt->chcfg is copied to a local variable (tmp) and the _same_ is being written back to chan_ctxt->chcfg, can compiler reorder these dependent statements and cause a different result?
Well, I agree that there is a minimal guarantee with modern day CPUs on not breaking the order of dependent memory accesses (like here tmp variable is the one which gets read and written) but we want to make sure that this won't break on future CPUs as well. So IMO using READ_ONCE and WRITE_ONCE adds extra level of safety.
?
I'm sorry, but this argument is non-sense to me, and so I want to understand more.
I've talked to our CPU designers from time to time, but cannot speak for other vendors. A modern CPU can easily reorder accesses all it wants, so long as it does not change the end result. This is typically identified via "data dependencies", where the CPU identifies that the result of a previous instruction is required to be known before processing the current instruction (or any instructions in flight in the pipeline, the instructions don't need to be adjacent). These data dependencies can be "read" or "write".
The typical reason barriers are needed is because the CPU cannot detect these dependencies when we are talking about different "memory". For example, a write to a register in some hardware block to program some mode, and then a write to another register to activate the hardware block based on that mode. In this example, there is no data dependency that the CPU can detect, although you and I as the software writer knows there is a specific order to these operations. Thus, a barrier is required.
Your argument is that we need to protect against some hypothetical future CPU where these data dependencies are ignored, and so the CPU reorders things. Except that means that the end result is (possibly) changed, meaning the contract between software and hardware is no longer valid. It breaks the entire memory model for the C language.
In the above code snippet, you are saying this is valid for some future CPU to do:
tmp = chan_ctxt->chcfg; chan_ctxt->chcfg = tmp; //probably optimized out because this now obviously has no effect tmp &= ~CHAN_CTX_CHSTATE_MASK; tmp |= (MHI_CH_STATE_DISABLED << CHAN_CTX_CHSTATE_SHIFT);
That is clearly wrong (I seriously hope you agree), and while I've seen hardware designers do some boneheaded things to the point where I don't trust them a lot of the time, I have a hard time believing they would think that is acceptable.
That fundamentally breaks all of software to the point where the only recourse is to have a literal barrier between every line of code. That doubles the line count of Linux and kills all performance. Its plainly not tenable.
So, seriously, please explain your view in great detail because it feels like we are talking past each-other and not coming to common ground. As I understand it, adding an explicit barrier in a patch cannot be done "just because" and requires a good documented reason (in a comment next to the barrier) for why the barrier is required. It seems like the same level of scrutiny should be applied for READ_ONCE/WRITE_ONCE, but your reason for adding them, "using READ_ONCE and WRITE_ONCE adds extra level of safety", reads like the reason to use them is "just because" to me.