| Message ID | 1612885989-12288-1-git-send-email-loic.poulain@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mhi: Fix double dma free | expand |
Hi Kalle, On Tue, 9 Feb 2021 at 16:45, Loic Poulain <loic.poulain@linaro.org> wrote: > > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel > resources, including unmapping coherent MHI areas, can be called > from different path in case of controller unregistering/removal: > - From a client driver remove callback, via mhi_unprepare_channel > - From mhi_driver_remove that unitialize all channels > > mhi_driver_remove() > |-> driver->remove() > | |-> mhi_unprepare_channel() > | |-> mhi_deinit_chan_ctxt() > |... > |-> mhi_deinit_chan_ctxt() > > This leads to double dma freeing... > > Fix that by preventing deinit for already uninitialized channel. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Reported-by: Kalle Valo <kvalo@codeaurora.org> This is a 'blind' fix tentative, can you please check if it resolves the issue on your side? Thanks, Loic
On 2/9/2021 8:53 AM, Loic Poulain wrote: > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel > resources, including unmapping coherent MHI areas, can be called > from different path in case of controller unregistering/removal: > - From a client driver remove callback, via mhi_unprepare_channel > - From mhi_driver_remove that unitialize all channels > > mhi_driver_remove() > |-> driver->remove() > | |-> mhi_unprepare_channel() > | |-> mhi_deinit_chan_ctxt() > |... > |-> mhi_deinit_chan_ctxt() > > This leads to double dma freeing... > > Fix that by preventing deinit for already uninitialized channel. > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > Reported-by: Kalle Valo <kvalo@codeaurora.org> > --- Seems like this should have a Fixes: tag, no? -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > > On 2/9/2021 8:53 AM, Loic Poulain wrote: > > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel > > resources, including unmapping coherent MHI areas, can be called > > from different path in case of controller unregistering/removal: > > - From a client driver remove callback, via mhi_unprepare_channel > > - From mhi_driver_remove that unitialize all channels > > > > mhi_driver_remove() > > |-> driver->remove() > > | |-> mhi_unprepare_channel() > > | |-> mhi_deinit_chan_ctxt() > > |... > > |-> mhi_deinit_chan_ctxt() > > > > This leads to double dma freeing... > > > > Fix that by preventing deinit for already uninitialized channel. > > > > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > > Reported-by: Kalle Valo <kvalo@codeaurora.org> > > --- > > Seems like this should have a Fixes: tag, no? Right, thanks, i'll add it in V2 once I get feedback.
Loic Poulain <loic.poulain@linaro.org> writes: > Hi Kalle, > > On Tue, 9 Feb 2021 at 16:45, Loic Poulain <loic.poulain@linaro.org> wrote: >> >> mhi_deinit_chan_ctxt functionthat takes care of unitializing channel >> resources, including unmapping coherent MHI areas, can be called >> from different path in case of controller unregistering/removal: >> - From a client driver remove callback, via mhi_unprepare_channel >> - From mhi_driver_remove that unitialize all channels >> >> mhi_driver_remove() >> |-> driver->remove() >> | |-> mhi_unprepare_channel() >> | |-> mhi_deinit_chan_ctxt() >> |... >> |-> mhi_deinit_chan_ctxt() >> >> This leads to double dma freeing... >> >> Fix that by preventing deinit for already uninitialized channel. >> >> Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> Reported-by: Kalle Valo <kvalo@codeaurora.org> > > This is a 'blind' fix tentative, can you please check if it resolves > the issue on your side? I did a quick test few times and I don't see any crashes anymore, thanks! Tested-by: Kalle Valo <kvalo@codeaurora.org> And like Jeffrey said: Fixes: a7f422f2f89e ("bus: mhi: Fix channel close issue on driver remove") Is it too late to push this to v5.11? But this should go to v5.12. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 2021-02-09 08:06 AM, Loic Poulain wrote: > On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote: >> >> On 2/9/2021 8:53 AM, Loic Poulain wrote: >> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel >> > resources, including unmapping coherent MHI areas, can be called >> > from different path in case of controller unregistering/removal: >> > - From a client driver remove callback, via mhi_unprepare_channel >> > - From mhi_driver_remove that unitialize all channels >> > >> > mhi_driver_remove() >> > |-> driver->remove() >> > | |-> mhi_unprepare_channel() >> > | |-> mhi_deinit_chan_ctxt() >> > |... >> > |-> mhi_deinit_chan_ctxt() >> > >> > This leads to double dma freeing... >> > >> > Fix that by preventing deinit for already uninitialized channel. >> > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> > Reported-by: Kalle Valo <kvalo@codeaurora.org> >> > --- >> >> Seems like this should have a Fixes: tag, no? > > Right, thanks, i'll add it in V2 once I get feedback. Hi Loic, Mani, I saw this same issue a while back but could not collect the logs for it. I had already pushed a patch to fix this differently [1] which was recently reviewed by Hemant. Although there wasn't a purposeful fixes tag for it. I think the culprit for this issue is [2]: As it allows the unprepare to go through on remove(), which was traditionally not allowed and ends up uncovering this issue as it fixes another. Channel updates [3] address that and provide a bunch of other improvements. Please consider them. Thanks, Bhaumik [1] https://lkml.org/lkml/2021/2/4/1161 [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/bus/mhi?h=v5.11-rc7&id=a7f422f2f89e7d48aa66e6488444a4c7f01269d5 [3] https://lkml.org/lkml/2021/2/4/1155 --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
Hi Bhaumik, On Tue, 9 Feb 2021 at 18:27, Bhaumik Bhatt <bbhatt@codeaurora.org> wrote: > > On 2021-02-09 08:06 AM, Loic Poulain wrote: > > On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote: > >> > >> On 2/9/2021 8:53 AM, Loic Poulain wrote: > >> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel > >> > resources, including unmapping coherent MHI areas, can be called > >> > from different path in case of controller unregistering/removal: > >> > - From a client driver remove callback, via mhi_unprepare_channel > >> > - From mhi_driver_remove that unitialize all channels > >> > > >> > mhi_driver_remove() > >> > |-> driver->remove() > >> > | |-> mhi_unprepare_channel() > >> > | |-> mhi_deinit_chan_ctxt() > >> > |... > >> > |-> mhi_deinit_chan_ctxt() > >> > > >> > This leads to double dma freeing... > >> > > >> > Fix that by preventing deinit for already uninitialized channel. > >> > > >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> > >> > Reported-by: Kalle Valo <kvalo@codeaurora.org> > >> > --- > >> > >> Seems like this should have a Fixes: tag, no? > > > > Right, thanks, i'll add it in V2 once I get feedback. > > Hi Loic, Mani, > > I saw this same issue a while back but could not collect the logs for > it. > > I had already pushed a patch to fix this differently [1] which was > recently reviewed by Hemant. > > Although there wasn't a purposeful fixes tag for it. I think the culprit > for this issue is [2]: > > As it allows the unprepare to go through on remove(), which was > traditionally not allowed and > ends up uncovering this issue as it fixes another. > > Channel updates [3] address that and provide a bunch of other > improvements. Please consider them. Yes, patch [2] is the culprit. I would recommend merging this tiny fix so that it can be easily grab for 5.11 or backported, and keep your series (rebased on top), for mhi-next (going to review/test it btw). Regards, Loic
Hi Loic, Mani, Hemant, On 2021-02-09 10:17 AM, Loic Poulain wrote: > Hi Bhaumik, > > On Tue, 9 Feb 2021 at 18:27, Bhaumik Bhatt <bbhatt@codeaurora.org> > wrote: >> >> On 2021-02-09 08:06 AM, Loic Poulain wrote: >> > On Tue, 9 Feb 2021 at 16:55, Jeffrey Hugo <jhugo@codeaurora.org> wrote: >> >> >> >> On 2/9/2021 8:53 AM, Loic Poulain wrote: >> >> > mhi_deinit_chan_ctxt functionthat takes care of unitializing channel >> >> > resources, including unmapping coherent MHI areas, can be called >> >> > from different path in case of controller unregistering/removal: >> >> > - From a client driver remove callback, via mhi_unprepare_channel >> >> > - From mhi_driver_remove that unitialize all channels >> >> > >> >> > mhi_driver_remove() >> >> > |-> driver->remove() >> >> > | |-> mhi_unprepare_channel() >> >> > | |-> mhi_deinit_chan_ctxt() >> >> > |... >> >> > |-> mhi_deinit_chan_ctxt() >> >> > >> >> > This leads to double dma freeing... >> >> > >> >> > Fix that by preventing deinit for already uninitialized channel. >> >> > >> >> > Signed-off-by: Loic Poulain <loic.poulain@linaro.org> >> >> > Reported-by: Kalle Valo <kvalo@codeaurora.org> >> >> > --- >> >> >> >> Seems like this should have a Fixes: tag, no? >> > >> > Right, thanks, i'll add it in V2 once I get feedback. >> >> Hi Loic, Mani, >> >> I saw this same issue a while back but could not collect the logs for >> it. >> >> I had already pushed a patch to fix this differently [1] which was >> recently reviewed by Hemant. >> >> Although there wasn't a purposeful fixes tag for it. I think the >> culprit >> for this issue is [2]: >> >> As it allows the unprepare to go through on remove(), which was >> traditionally not allowed and >> ends up uncovering this issue as it fixes another. >> >> Channel updates [3] address that and provide a bunch of other >> improvements. Please consider them. > > Yes, patch [2] is the culprit. I would recommend merging this tiny fix > so that it can be easily grab for 5.11 or backported, and keep your > series (rebased on top), for mhi-next (going to review/test it btw). > > Regards, > Loic If priority is to get this fix in ASAP, your suggestion is OK. I just see some typo fixes and a title update to "bus: mhi: core: Fix double dma free() call" or something as review comments for your patch. Another option is that I can send my patch [1] separately and remove it from my "channel updates" patch series, if that helps. I'd like to see what Mani and Hemant on what they prefer. Please advise. Thanks, Bhaumik [1] https://lkml.org/lkml/2021/2/4/1161 --- The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
On Tue, Feb 09, 2021 at 08:37:21PM -0800, Bhaumik Bhatt wrote: > Hi Loic, Mani, Hemant, > [...] > If priority is to get this fix in ASAP, your suggestion is OK. I just see > some > typo fixes and a title update to "bus: mhi: core: Fix double dma free() > call" > or something as review comments for your patch. > > Another option is that I can send my patch [1] separately and remove it from > my > "channel updates" patch series, if that helps. > > I'd like to see what Mani and Hemant on what they prefer. Please advise. > Both patches looks good to me although your patch needs to be resubmitted. But since the fix needs to go in quickly, I'm going to apply Loic's patch. Thanks, Mani > Thanks, > Bhaumik > > [1] https://lkml.org/lkml/2021/2/4/1161 > > --- > The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, > a Linux Foundation Collaborative Project
diff --git a/drivers/bus/mhi/core/init.c b/drivers/bus/mhi/core/init.c index aa575d3..be4eebb 100644 --- a/drivers/bus/mhi/core/init.c +++ b/drivers/bus/mhi/core/init.c @@ -557,6 +557,9 @@ void mhi_deinit_chan_ctxt(struct mhi_controller *mhi_cntrl, tre_ring = &mhi_chan->tre_ring; chan_ctxt = &mhi_cntrl->mhi_ctxt->chan_ctxt[mhi_chan->chan]; + if (!chan_ctxt->rbase) /* Already uninitialized */ + return; + mhi_free_coherent(mhi_cntrl, tre_ring->alloc_size, tre_ring->pre_aligned, tre_ring->dma_handle); vfree(buf_ring->base);
mhi_deinit_chan_ctxt functionthat takes care of unitializing channel resources, including unmapping coherent MHI areas, can be called from different path in case of controller unregistering/removal: - From a client driver remove callback, via mhi_unprepare_channel - From mhi_driver_remove that unitialize all channels mhi_driver_remove() |-> driver->remove() | |-> mhi_unprepare_channel() | |-> mhi_deinit_chan_ctxt() |... |-> mhi_deinit_chan_ctxt() This leads to double dma freeing... Fix that by preventing deinit for already uninitialized channel. Signed-off-by: Loic Poulain <loic.poulain@linaro.org> Reported-by: Kalle Valo <kvalo@codeaurora.org> --- drivers/bus/mhi/core/init.c | 3 +++ 1 file changed, 3 insertions(+) -- 2.7.4