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>
Subject: Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down
Date: Tue, 21 Jun 2022 17:24:45 +0800 [thread overview]
Message-ID: <7f72b026-5808-ee0d-894d-bf50bf162023@quicinc.com> (raw)
In-Reply-To: <c93aa921-b327-fdf4-adae-f0dd77e8789e@quicinc.com>
On 6/21/2022 3:57 AM, Jeffrey Hugo wrote:
> On 6/9/2022 7:54 AM, 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.
>>
>>>
>>> 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.
>
> Ah, I see. This goes to the assumption that the BHI interrupt is
> always line 0, even though as far as I am aware, the spec doesn't
> require that. The comment could be clearer I think.
>
>>
>>> + 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;
>>> + }
>
> Please no. This overloads the function to have two different
> behaviors, and it doesn't match the inline disables.
>
> Since you have inline disables, I would prefer inline enables so that
> the code is "balanced".
I remove this in v2, directly invoke enables during power up. It
becomes like this:
mhi power up ->enable_irq, mhi power down ->disable irq
pci_probe->mhi_register_controller->request_irq,
pci_remove->mhi_unregister_controller->free irq
>
>>> +
>>> /* 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);
>
> This seems like a random change that should be in a different patch.
> Why is it included here?
During probe, host alloc irq venctor, so I add this change to free
vector. I remove it in v2.
>
>>> }
>>> 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);
>>
>>
>
prev parent reply other threads:[~2022-06-21 9:25 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
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 [this message]
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=7f72b026-5808-ee0d-894d-bf50bf162023@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_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).