From: Qiang Yu <quic_qianyu@quicinc.com>
To: Jeffrey Hugo <quic_jhugo@quicinc.com>, <mani@kernel.org>,
<quic_hemantk@quicinc.com>, <loic.poulain@linaro.org>
Cc: <mhi@lists.linux.dev>, <linux-arm-msm@vger.kernel.org>,
<linux-kernel@vger.kernel.org>, <quic_cang@quicinc.com>
Subject: Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down
Date: Fri, 10 Jun 2022 11:21:05 +0800 [thread overview]
Message-ID: <9659ecb9-9727-a146-e286-d28d656483c3@quicinc.com> (raw)
In-Reply-To: <62d09e6f-9898-6233-dfd6-b5ba5d837571@quicinc.com>
On 6/9/2022 9:54 PM, Jeffrey Hugo wrote:
> On 6/9/2022 7:43 AM, Qiang Yu wrote:
>> EP tends to read MSI address/data once and cache them after BME is set.
>> So host should avoid changing MSI address/data after BME is set.
>>
>> In pci reset function, host invokes free_irq(), which also clears MSI
>> address/data in EP's PCIe config space. If the invalid address/data
>> are cached and used by EP, MSI triggered by EP wouldn't be received by
>> host, because an invalid MSI data is sent to an invalid MSI address.
>>
>> To fix this issue, after host runs request_irq() successfully during
>> mhi driver probe, let's invoke enable_irq()/disable_irq() instead of
>> request_irq()/free_irq() when we want to power on and power down MHI.
>> Meanwhile, Host should invoke free_irq() when mhi host driver is
>> removed.
>
> I don't think this works for hotplug, nor cases where there are
> multiple MHI devices on the system.
>
> The EP shouldn't be caching this information for multiple reasons.
> Masking the MSIs, disabling the MSIs, changing the address when the
> affinity changes, etc.
>
> It really feels like we are solving the problem in the wrong place.
>
> Right now, this gets a NACK from me.
>
After free_irq(), MSI is still enabled but MSI address and data are
cleared. So there is a chance that device initiates MSI using zero
address. How to fix this race conditions.
Maybe EP should not cache MSI data and address. But I think this patch
is necessary and we will talk with EP POC.
>>
>> Signed-off-by: Qiang Yu <quic_qianyu@quicinc.com>
>> ---
>> drivers/bus/mhi/host/init.c | 31
>> +++++++++++++++++++++++++++++++
>> drivers/bus/mhi/host/pci_generic.c | 2 ++
>> drivers/bus/mhi/host/pm.c | 4 ++--
>> 3 files changed, 35 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/bus/mhi/host/init.c b/drivers/bus/mhi/host/init.c
>> index cbb86b2..48cb093 100644
>> --- a/drivers/bus/mhi/host/init.c
>> +++ b/drivers/bus/mhi/host/init.c
>> @@ -18,6 +18,7 @@
>> #include <linux/slab.h>
>> #include <linux/vmalloc.h>
>> #include <linux/wait.h>
>> +#include <linux/irq.h>
>
> Should be in alphabetical order
>
>> #include "internal.h"
>> static DEFINE_IDA(mhi_controller_ida);
>> @@ -168,6 +169,22 @@ int mhi_init_irq_setup(struct mhi_controller
>> *mhi_cntrl)
>> unsigned long irq_flags = IRQF_SHARED | IRQF_NO_SUSPEND;
>> int i, ret;
>> + /*
>> + * if irq[0] has action, it represents all MSI IRQs have been
>> + * requested, so we just need to enable them.
>> + */
>
> This seems like an assumption about how the interrupts are allocated
> and assigned that may not hold true for all devices.
All interrupts are allocated and assigned together in mhi_pci_get_irqs()
and mhi_init_irq_setup().
So I think if irq[0] has action, other irqs must be requested
successfully. If any other msi request fail, irq[0] should have been freed.
>> + if (irq_has_action(mhi_cntrl->irq[0])) {
>> + enable_irq(mhi_cntrl->irq[0]);
>> +
>> + for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>> + if (mhi_event->offload_ev)
>> + continue;
>> +
>> + enable_irq(mhi_cntrl->irq[mhi_event->irq]);
>> + }
>> + return 0;
>> + }
>> +
>> /* if controller driver has set irq_flags, use it */
>> if (mhi_cntrl->irq_flags)
>> irq_flags = mhi_cntrl->irq_flags;
>> @@ -179,6 +196,11 @@ int mhi_init_irq_setup(struct mhi_controller
>> *mhi_cntrl)
>> "bhi", mhi_cntrl);
>> if (ret)
>> return ret;
>> + /*
>> + * IRQ marked IRQF_SHARED isn't recommended to use IRQ_NOAUTOEN,
>> + * so disable it explicitly.
>> + */
>> + disable_irq(mhi_cntrl->irq[0]);
>> for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>> if (mhi_event->offload_ev)
>> @@ -200,6 +222,8 @@ int mhi_init_irq_setup(struct mhi_controller
>> *mhi_cntrl)
>> mhi_cntrl->irq[mhi_event->irq], i);
>> goto error_request;
>> }
>> +
>> + disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>> }
>> return 0;
>> @@ -1003,8 +1027,14 @@ int mhi_register_controller(struct
>> mhi_controller *mhi_cntrl,
>> mhi_create_debugfs(mhi_cntrl);
>> + ret = mhi_init_irq_setup(mhi_cntrl);
>> + if (ret)
>> + goto error_setup_irq;
>> +
>> return 0;
>> +error_setup_irq:
>> + mhi_destroy_debugfs(mhi_cntrl);
>> err_release_dev:
>> put_device(&mhi_dev->dev);
>> err_ida_free:
>> @@ -1027,6 +1057,7 @@ void mhi_unregister_controller(struct
>> mhi_controller *mhi_cntrl)
>> struct mhi_chan *mhi_chan = mhi_cntrl->mhi_chan;
>> unsigned int i;
>> + mhi_deinit_free_irq(mhi_cntrl);
>> mhi_destroy_debugfs(mhi_cntrl);
>> destroy_workqueue(mhi_cntrl->hiprio_wq);
>> diff --git a/drivers/bus/mhi/host/pci_generic.c
>> b/drivers/bus/mhi/host/pci_generic.c
>> index 6fbc591..60020d0 100644
>> --- a/drivers/bus/mhi/host/pci_generic.c
>> +++ b/drivers/bus/mhi/host/pci_generic.c
>> @@ -945,6 +945,8 @@ static void mhi_pci_remove(struct pci_dev *pdev)
>> mhi_unregister_controller(mhi_cntrl);
>> pci_disable_pcie_error_reporting(pdev);
>> +
>> + pci_free_irq_vectors(pdev);
>> }
>> static void mhi_pci_shutdown(struct pci_dev *pdev)
>> diff --git a/drivers/bus/mhi/host/pm.c b/drivers/bus/mhi/host/pm.c
>> index dc2e8ff..190231c 100644
>> --- a/drivers/bus/mhi/host/pm.c
>> +++ b/drivers/bus/mhi/host/pm.c
>> @@ -500,7 +500,7 @@ static void mhi_pm_disable_transition(struct
>> mhi_controller *mhi_cntrl)
>> for (i = 0; i < mhi_cntrl->total_ev_rings; i++, mhi_event++) {
>> if (mhi_event->offload_ev)
>> continue;
>> - free_irq(mhi_cntrl->irq[mhi_event->irq], mhi_event);
>> + disable_irq(mhi_cntrl->irq[mhi_event->irq]);
>> tasklet_kill(&mhi_event->task);
>> }
>> @@ -1182,7 +1182,7 @@ void mhi_power_down(struct mhi_controller
>> *mhi_cntrl, bool graceful)
>> /* Wait for shutdown to complete */
>> flush_work(&mhi_cntrl->st_worker);
>> - free_irq(mhi_cntrl->irq[0], mhi_cntrl);
>> + disable_irq(mhi_cntrl->irq[0]);
>> }
>> EXPORT_SYMBOL_GPL(mhi_power_down);
>
next prev parent reply other threads:[~2022-06-10 3:23 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-09 13:43 [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down Qiang Yu
2022-06-09 13:54 ` Jeffrey Hugo
2022-06-10 3:21 ` Qiang Yu [this message]
2022-06-10 14:00 ` Jeffrey Hugo
2022-06-13 1:48 ` Qiang Yu
2022-06-13 13:07 ` Jeffrey Hugo
2022-06-15 21:16 ` Manivannan Sadhasivam
2022-06-16 15:53 ` Jeffrey Hugo
2022-06-16 20:59 ` Manivannan Sadhasivam
2022-06-20 19:59 ` Jeffrey Hugo
2022-06-20 19:57 ` Jeffrey Hugo
2022-06-21 9:24 ` Qiang Yu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9659ecb9-9727-a146-e286-d28d656483c3@quicinc.com \
--to=quic_qianyu@quicinc.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=loic.poulain@linaro.org \
--cc=mani@kernel.org \
--cc=mhi@lists.linux.dev \
--cc=quic_cang@quicinc.com \
--cc=quic_hemantk@quicinc.com \
--cc=quic_jhugo@quicinc.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).