Messages in this thread | | | Date | Fri, 28 May 2021 09:59:37 -0700 | From | Bhaumik Bhatt <> | Subject | Re: [PATCH 5.10 002/299] bus: mhi: core: Clear configuration from channel context during reset |
| |
On 2021-05-28 01:52 AM, Pavel Machek wrote: > Hi! > >> > > > > 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? > >> > 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. > > Umm, if this is protected by locking, already, we really should not > add READ_ONCE. Code should be clear, not having "extra safety levels". > > I assumed it was running unlocked due to the way it was written. > > Best regards, > Pavel Thanks for the confirmation Pavel.
Thanks, Bhaumik --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
| |