* [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down @ 2022-06-09 13:43 Qiang Yu 2022-06-09 13:54 ` Jeffrey Hugo 0 siblings, 1 reply; 12+ messages in thread From: Qiang Yu @ 2022-06-09 13:43 UTC (permalink / raw) To: mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel, Qiang Yu 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. 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> #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. + */ + 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); -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 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-20 19:57 ` Jeffrey Hugo 0 siblings, 2 replies; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-09 13:54 UTC (permalink / raw) To: Qiang Yu, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel 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. > + 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); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-09 13:54 ` Jeffrey Hugo @ 2022-06-10 3:21 ` Qiang Yu 2022-06-10 14:00 ` Jeffrey Hugo 2022-06-20 19:57 ` Jeffrey Hugo 1 sibling, 1 reply; 12+ messages in thread From: Qiang Yu @ 2022-06-10 3:21 UTC (permalink / raw) To: Jeffrey Hugo, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel, quic_cang 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); > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-10 3:21 ` Qiang Yu @ 2022-06-10 14:00 ` Jeffrey Hugo 2022-06-13 1:48 ` Qiang Yu 0 siblings, 1 reply; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-10 14:00 UTC (permalink / raw) To: Qiang Yu, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel, quic_cang On 6/9/2022 9:21 PM, Qiang Yu wrote: > 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. On what system is MSI still enabled? I just removed the AIC100 controller on an random x86 system, and lspci is indicating MSIs are disabled - Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ > 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); >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-10 14:00 ` Jeffrey Hugo @ 2022-06-13 1:48 ` Qiang Yu 2022-06-13 13:07 ` Jeffrey Hugo 0 siblings, 1 reply; 12+ messages in thread From: Qiang Yu @ 2022-06-13 1:48 UTC (permalink / raw) To: Jeffrey Hugo, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel, quic_cang On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: > On 6/9/2022 9:21 PM, Qiang Yu wrote: >> 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. > > On what system is MSI still enabled? I just removed the AIC100 > controller on an random x86 system, and lspci is indicating MSIs are > disabled - > > Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ 3.40GHz After removing MHI driver, I also see MSI enable is cleared. But I don't think free_irq clears it. I add log before free_irq and after free_irq as following show: [62777.625111] msi cap before free irq [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 [62777.625301] msi cap after free irq [62777.625313] msi control=0x1bb, address=0x0, data=0x0 [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, block 90 secs. # lspci -vvs 01:00.0 Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ Address: 0000000000000000 Data: 0000 Masking: ffffffff Pending: 00000000 [62868.692186] mhi-pci-generic 0000:01:00.0: mhi_pci_remove 90 sec expire. # lspci -vvs 01:00.0 Capabilities: [50] MSI: Enable- Count=8/32 Maskable+ 64bit+ Address: 0000000000000000 Data: 0000 Masking: 00000000 Pending: 00000000 I also add msleep() at last of remove callback to block the remove operation, then lspci shows MSI is still enabled and after MHI driver is removed, lspci shows MSI is disabled. It proves free_irq does not clear MSI enable, although I am not sure who does it (probably pci framework clears but I don 't find it). I delete pci_free_irq_vectors() when I test. > >> 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); >>> > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-13 1:48 ` Qiang Yu @ 2022-06-13 13:07 ` Jeffrey Hugo 2022-06-15 21:16 ` Manivannan Sadhasivam 0 siblings, 1 reply; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-13 13:07 UTC (permalink / raw) To: Qiang Yu, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel, quic_cang On 6/12/2022 7:48 PM, Qiang Yu wrote: > > On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: >> On 6/9/2022 9:21 PM, Qiang Yu wrote: >>> 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. >> >> On what system is MSI still enabled? I just removed the AIC100 >> controller on an random x86 system, and lspci is indicating MSIs are >> disabled - >> >> Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ > > system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ > 3.40GHz > > After removing MHI driver, I also see MSI enable is cleared. But I > don't think free_irq clears it. I add log before free_irq and after > free_irq as following show: > > [62777.625111] msi cap before free irq > [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 > [62777.625301] msi cap after free irq > [62777.625313] msi control=0x1bb, address=0x0, data=0x0 > [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, > block 90 secs. > # lspci -vvs 01:00.0 > Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ > Address: 0000000000000000 Data: 0000 > Masking: ffffffff Pending: 00000000 At this point, the MSI functionality is still enabled, but every MSI is masked out (Masking), so per the PCIe spec, the endpoint may not trigger a MSI to the host. The device advertises that it supports maskable MSIs (Maskable+), so this is appropiate. If your device can still send a MSI at this point, then it violates the PCIe spec. disable_irq() will not help you with this as it will do the same thing. I still think you are trying to fix an issue in the wrong location (host vs EP), and causing additional issues by doing so. > [62868.692186] mhi-pci-generic 0000:01:00.0: mhi_pci_remove 90 sec expire. > # lspci -vvs 01:00.0 > Capabilities: [50] MSI: Enable- Count=8/32 Maskable+ 64bit+ > Address: 0000000000000000 Data: 0000 > Masking: 00000000 Pending: 00000000 > > I also add msleep() at last of remove callback to block the remove > operation, then lspci shows MSI is still enabled and after MHI driver > is removed, > > lspci shows MSI is disabled. It proves free_irq does not clear MSI > enable, although I am not sure who does it (probably pci framework > clears but I don 't find it). > > I delete pci_free_irq_vectors() when I test. > >> >>> 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); >>>> >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-13 13:07 ` Jeffrey Hugo @ 2022-06-15 21:16 ` Manivannan Sadhasivam 2022-06-16 15:53 ` Jeffrey Hugo 0 siblings, 1 reply; 12+ messages in thread From: Manivannan Sadhasivam @ 2022-06-15 21:16 UTC (permalink / raw) To: Jeffrey Hugo Cc: Qiang Yu, quic_hemantk, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang On Mon, Jun 13, 2022 at 07:07:02AM -0600, Jeffrey Hugo wrote: > On 6/12/2022 7:48 PM, Qiang Yu wrote: > > > > On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: > > > On 6/9/2022 9:21 PM, Qiang Yu wrote: > > > > 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. > > > > > > On what system is MSI still enabled? I just removed the AIC100 > > > controller on an random x86 system, and lspci is indicating MSIs are > > > disabled - > > > > > > Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ > > > > system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ > > 3.40GHz > > > > After removing MHI driver, I also see MSI enable is cleared. But I > > don't think free_irq clears it. I add log before free_irq and after > > free_irq as following show: > > > > [62777.625111] msi cap before free irq > > [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 > > [62777.625301] msi cap after free irq > > [62777.625313] msi control=0x1bb, address=0x0, data=0x0 > > [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, > > block 90 secs. > > # lspci -vvs 01:00.0 > > Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ > > Address: 0000000000000000 Data: 0000 > > Masking: ffffffff Pending: 00000000 > > At this point, the MSI functionality is still enabled, but every MSI is > masked out (Masking), so per the PCIe spec, the endpoint may not trigger a > MSI to the host. The device advertises that it supports maskable MSIs > (Maskable+), so this is appropiate. > > If your device can still send a MSI at this point, then it violates the PCIe > spec. > > disable_irq() will not help you with this as it will do the same thing. > > I still think you are trying to fix an issue in the wrong location (host vs > EP), and causing additional issues by doing so. > Irrespective of caching the MSI data in endpoint, I'd like to get rid of request_irq/free_irq during the mhi_{power_down/power_up} time. As like the MHI endpoint stack, we should just do disable/enable irq. Because, the MHI device may go down several times while running and we do not want to deallocate the IRQs all the time. And if the device gets removed, ultimately the MHI driver will get removed and we are fine while loading it back (even if MSI count changes). I didn't had time to look into the patch in detail but I'm in favour of accepting the proposal. @Jeff: Any specific issue you are seeing with hotplug etc...? Thanks, Mani > > [62868.692186] mhi-pci-generic 0000:01:00.0: mhi_pci_remove 90 sec expire. > > # lspci -vvs 01:00.0 > > Capabilities: [50] MSI: Enable- Count=8/32 Maskable+ 64bit+ > > Address: 0000000000000000 Data: 0000 > > Masking: 00000000 Pending: 00000000 > > > > I also add msleep() at last of remove callback to block the remove > > operation, then lspci shows MSI is still enabled and after MHI driver > > is removed, > > > > lspci shows MSI is disabled. It proves free_irq does not clear MSI > > enable, although I am not sure who does it (probably pci framework > > clears but I don 't find it). > > > > I delete pci_free_irq_vectors() when I test. > > > > > > > > > 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); > > > > > > > > > -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-15 21:16 ` Manivannan Sadhasivam @ 2022-06-16 15:53 ` Jeffrey Hugo 2022-06-16 20:59 ` Manivannan Sadhasivam 0 siblings, 1 reply; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-16 15:53 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Qiang Yu, quic_hemantk, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang On 6/15/2022 3:16 PM, Manivannan Sadhasivam wrote: > On Mon, Jun 13, 2022 at 07:07:02AM -0600, Jeffrey Hugo wrote: >> On 6/12/2022 7:48 PM, Qiang Yu wrote: >>> >>> On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: >>>> On 6/9/2022 9:21 PM, Qiang Yu wrote: >>>>> 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. >>>> >>>> On what system is MSI still enabled? I just removed the AIC100 >>>> controller on an random x86 system, and lspci is indicating MSIs are >>>> disabled - >>>> >>>> Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ >>> >>> system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ >>> 3.40GHz >>> >>> After removing MHI driver, I also see MSI enable is cleared. But I >>> don't think free_irq clears it. I add log before free_irq and after >>> free_irq as following show: >>> >>> [62777.625111] msi cap before free irq >>> [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 >>> [62777.625301] msi cap after free irq >>> [62777.625313] msi control=0x1bb, address=0x0, data=0x0 >>> [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, >>> block 90 secs. >>> # lspci -vvs 01:00.0 >>> Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ >>> Address: 0000000000000000 Data: 0000 >>> Masking: ffffffff Pending: 00000000 >> >> At this point, the MSI functionality is still enabled, but every MSI is >> masked out (Masking), so per the PCIe spec, the endpoint may not trigger a >> MSI to the host. The device advertises that it supports maskable MSIs >> (Maskable+), so this is appropiate. >> >> If your device can still send a MSI at this point, then it violates the PCIe >> spec. >> >> disable_irq() will not help you with this as it will do the same thing. >> >> I still think you are trying to fix an issue in the wrong location (host vs >> EP), and causing additional issues by doing so. >> > > Irrespective of caching the MSI data in endpoint, I'd like to get rid of > request_irq/free_irq during the mhi_{power_down/power_up} time. As like the MHI > endpoint stack, we should just do disable/enable irq. Because, the MHI device > may go down several times while running and we do not want to deallocate the > IRQs all the time. And if the device gets removed, ultimately the MHI driver > will get removed and we are fine while loading it back (even if MSI count > changes). > > I didn't had time to look into the patch in detail but I'm in favour of > accepting the proposal. > > @Jeff: Any specific issue you are seeing with hotplug etc...? Perhaps I'm getting confused by the commit text of this change. The issue described is that we free the irq, and then the EP sends a MSI, and the host doesn't receive it. To me, that is expected. The host doesn't care about the irq anymore because it freed it, therefore it would be expected that the host doesn't receive the irq. So, the described issue is not an issue since it is expected behavior from what I can tell. The proposed fix, is to disable the interrupts, and not free them until the driver is removed. I interpret removing the driver as "rmmod mhi". Based on this, the problem I see is a scenario where we have N devices in a system, and one device is hotplugged. On hotplug, we would want to clean up all resources (free irq), but according to the description, we need to rmmod mhi, which is both not automatic and also affects the other N-1 devices which are presumed to be operational. Now, if we throw all of that out the window, and say that the goal is to register the irqs when the controller is registered, free them when the controller is unregistered, and enable/disable based on power up/down as a optimization, that could be sane. If that is what this change is attempting to do, it is not what the commit text describes. Under the assumption that you want the optimization I just described, I will re-review the code next week when I get back from my travel. Assuming the implementation is good (other than what I've already pointed out), I think the commit text needs to be rewritten. Does that clarify things for you? ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-16 15:53 ` Jeffrey Hugo @ 2022-06-16 20:59 ` Manivannan Sadhasivam 2022-06-20 19:59 ` Jeffrey Hugo 0 siblings, 1 reply; 12+ messages in thread From: Manivannan Sadhasivam @ 2022-06-16 20:59 UTC (permalink / raw) To: Jeffrey Hugo Cc: Qiang Yu, quic_hemantk, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang On Thu, Jun 16, 2022 at 09:53:34AM -0600, Jeffrey Hugo wrote: > On 6/15/2022 3:16 PM, Manivannan Sadhasivam wrote: > > On Mon, Jun 13, 2022 at 07:07:02AM -0600, Jeffrey Hugo wrote: > > > On 6/12/2022 7:48 PM, Qiang Yu wrote: > > > > > > > > On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: > > > > > On 6/9/2022 9:21 PM, Qiang Yu wrote: > > > > > > 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. > > > > > > > > > > On what system is MSI still enabled? I just removed the AIC100 > > > > > controller on an random x86 system, and lspci is indicating MSIs are > > > > > disabled - > > > > > > > > > > Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ > > > > > > > > system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ > > > > 3.40GHz > > > > > > > > After removing MHI driver, I also see MSI enable is cleared. But I > > > > don't think free_irq clears it. I add log before free_irq and after > > > > free_irq as following show: > > > > > > > > [62777.625111] msi cap before free irq > > > > [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 > > > > [62777.625301] msi cap after free irq > > > > [62777.625313] msi control=0x1bb, address=0x0, data=0x0 > > > > [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, > > > > block 90 secs. > > > > # lspci -vvs 01:00.0 > > > > Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ > > > > Address: 0000000000000000 Data: 0000 > > > > Masking: ffffffff Pending: 00000000 > > > > > > At this point, the MSI functionality is still enabled, but every MSI is > > > masked out (Masking), so per the PCIe spec, the endpoint may not trigger a > > > MSI to the host. The device advertises that it supports maskable MSIs > > > (Maskable+), so this is appropiate. > > > > > > If your device can still send a MSI at this point, then it violates the PCIe > > > spec. > > > > > > disable_irq() will not help you with this as it will do the same thing. > > > > > > I still think you are trying to fix an issue in the wrong location (host vs > > > EP), and causing additional issues by doing so. > > > > > > > Irrespective of caching the MSI data in endpoint, I'd like to get rid of > > request_irq/free_irq during the mhi_{power_down/power_up} time. As like the MHI > > endpoint stack, we should just do disable/enable irq. Because, the MHI device > > may go down several times while running and we do not want to deallocate the > > IRQs all the time. And if the device gets removed, ultimately the MHI driver > > will get removed and we are fine while loading it back (even if MSI count > > changes). > > > > I didn't had time to look into the patch in detail but I'm in favour of > > accepting the proposal. > > > > @Jeff: Any specific issue you are seeing with hotplug etc...? > > Perhaps I'm getting confused by the commit text of this change. > > The issue described is that we free the irq, and then the EP sends a MSI, > and the host doesn't receive it. To me, that is expected. The host doesn't > care about the irq anymore because it freed it, therefore it would be > expected that the host doesn't receive the irq. So, the described issue is > not an issue since it is expected behavior from what I can tell. > > The proposed fix, is to disable the interrupts, and not free them until the > driver is removed. I interpret removing the driver as "rmmod mhi". Based > on this, the problem I see is a scenario where we have N devices in a > system, and one device is hotplugged. On hotplug, we would want to clean up > all resources (free irq), but according to the description, we need to rmmod > mhi, which is both not automatic and also affects the other N-1 devices > which are presumed to be operational. No. When the PCI device gets removed during runtime, the remove() callback will get called with relevant "struct pci_dev" and that should take care of all resource cleanup for that particular device (including free_irq). You do not need to manually rmmod the driver as that will be done by the hotplug driver when there are no devices making use of it. And yes, the commit message needs to be changed... > > Now, if we throw all of that out the window, and say that the goal is to > register the irqs when the controller is registered, free them when the > controller is unregistered, and enable/disable based on power up/down as a > optimization, that could be sane. If that is what this change is attempting > to do, it is not what the commit text describes. > > Under the assumption that you want the optimization I just described, I will > re-review the code next week when I get back from my travel. Assuming the > implementation is good (other than what I've already pointed out), I think > the commit text needs to be rewritten. > > Does that clarify things for you? Yep! Thanks, Mani -- மணிவண்ணன் சதாசிவம் ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-16 20:59 ` Manivannan Sadhasivam @ 2022-06-20 19:59 ` Jeffrey Hugo 0 siblings, 0 replies; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-20 19:59 UTC (permalink / raw) To: Manivannan Sadhasivam Cc: Qiang Yu, quic_hemantk, loic.poulain, mhi, linux-arm-msm, linux-kernel, quic_cang On 6/16/2022 2:59 PM, Manivannan Sadhasivam wrote: > On Thu, Jun 16, 2022 at 09:53:34AM -0600, Jeffrey Hugo wrote: >> On 6/15/2022 3:16 PM, Manivannan Sadhasivam wrote: >>> On Mon, Jun 13, 2022 at 07:07:02AM -0600, Jeffrey Hugo wrote: >>>> On 6/12/2022 7:48 PM, Qiang Yu wrote: >>>>> >>>>> On 6/10/2022 10:00 PM, Jeffrey Hugo wrote: >>>>>> On 6/9/2022 9:21 PM, Qiang Yu wrote: >>>>>>> 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. >>>>>> >>>>>> On what system is MSI still enabled? I just removed the AIC100 >>>>>> controller on an random x86 system, and lspci is indicating MSIs are >>>>>> disabled - >>>>>> >>>>>> Capabilities: [50] MSI: Enable- Count=32/32 Maskable+ 64bit+ >>>>> >>>>> system: Ubuntu18.04, 5.4.0-89-generic, Intel(R) Core(TM) i7-6700 CPU @ >>>>> 3.40GHz >>>>> >>>>> After removing MHI driver, I also see MSI enable is cleared. But I >>>>> don't think free_irq clears it. I add log before free_irq and after >>>>> free_irq as following show: >>>>> >>>>> [62777.625111] msi cap before free irq >>>>> [62777.625125] msi control=0x1bb, address=0xfee00318, data=0x0 >>>>> [62777.625301] msi cap after free irq >>>>> [62777.625313] msi control=0x1bb, address=0x0, data=0x0 >>>>> [62777.625496] mhi-pci-generic 0000:01:00.0: mhi_pci_remove end of line, >>>>> block 90 secs. >>>>> # lspci -vvs 01:00.0 >>>>> Capabilities: [50] MSI: Enable+ Count=8/32 Maskable+ 64bit+ >>>>> Address: 0000000000000000 Data: 0000 >>>>> Masking: ffffffff Pending: 00000000 >>>> >>>> At this point, the MSI functionality is still enabled, but every MSI is >>>> masked out (Masking), so per the PCIe spec, the endpoint may not trigger a >>>> MSI to the host. The device advertises that it supports maskable MSIs >>>> (Maskable+), so this is appropiate. >>>> >>>> If your device can still send a MSI at this point, then it violates the PCIe >>>> spec. >>>> >>>> disable_irq() will not help you with this as it will do the same thing. >>>> >>>> I still think you are trying to fix an issue in the wrong location (host vs >>>> EP), and causing additional issues by doing so. >>>> >>> >>> Irrespective of caching the MSI data in endpoint, I'd like to get rid of >>> request_irq/free_irq during the mhi_{power_down/power_up} time. As like the MHI >>> endpoint stack, we should just do disable/enable irq. Because, the MHI device >>> may go down several times while running and we do not want to deallocate the >>> IRQs all the time. And if the device gets removed, ultimately the MHI driver >>> will get removed and we are fine while loading it back (even if MSI count >>> changes). >>> >>> I didn't had time to look into the patch in detail but I'm in favour of >>> accepting the proposal. >>> >>> @Jeff: Any specific issue you are seeing with hotplug etc...? >> >> Perhaps I'm getting confused by the commit text of this change. >> >> The issue described is that we free the irq, and then the EP sends a MSI, >> and the host doesn't receive it. To me, that is expected. The host doesn't >> care about the irq anymore because it freed it, therefore it would be >> expected that the host doesn't receive the irq. So, the described issue is >> not an issue since it is expected behavior from what I can tell. >> >> The proposed fix, is to disable the interrupts, and not free them until the >> driver is removed. I interpret removing the driver as "rmmod mhi". Based >> on this, the problem I see is a scenario where we have N devices in a >> system, and one device is hotplugged. On hotplug, we would want to clean up >> all resources (free irq), but according to the description, we need to rmmod >> mhi, which is both not automatic and also affects the other N-1 devices >> which are presumed to be operational. > > No. When the PCI device gets removed during runtime, the remove() callback will > get called with relevant "struct pci_dev" and that should take care of all > resource cleanup for that particular device (including free_irq). That is what I expected, so I was confused. Seems like we are on the same page now. > You do not need to manually rmmod the driver as that will be done by the > hotplug driver when there are no devices making use of it. And yes, the commit > message needs to be changed. > >> >> Now, if we throw all of that out the window, and say that the goal is to >> register the irqs when the controller is registered, free them when the >> controller is unregistered, and enable/disable based on power up/down as a >> optimization, that could be sane. If that is what this change is attempting >> to do, it is not what the commit text describes. >> >> Under the assumption that you want the optimization I just described, I will >> re-review the code next week when I get back from my travel. Assuming the >> implementation is good (other than what I've already pointed out), I think >> the commit text needs to be rewritten. >> >> Does that clarify things for you? > > Yep! Reviewed, with additional comments. I guess I remove my NACK, but there is a lot to address with v2. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-09 13:54 ` Jeffrey Hugo 2022-06-10 3:21 ` Qiang Yu @ 2022-06-20 19:57 ` Jeffrey Hugo 2022-06-21 9:24 ` Qiang Yu 1 sibling, 1 reply; 12+ messages in thread From: Jeffrey Hugo @ 2022-06-20 19:57 UTC (permalink / raw) To: Qiang Yu, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel 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". >> + >> /* 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? >> } >> 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); > > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] bus: mhi: Disable IRQs instead of freeing them during power down 2022-06-20 19:57 ` Jeffrey Hugo @ 2022-06-21 9:24 ` Qiang Yu 0 siblings, 0 replies; 12+ messages in thread From: Qiang Yu @ 2022-06-21 9:24 UTC (permalink / raw) To: Jeffrey Hugo, mani, quic_hemantk, loic.poulain Cc: mhi, linux-arm-msm, linux-kernel 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); >> >> > ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2022-06-21 9:25 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 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 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).