| Message ID | 20240731-pci-qcom-hotplug-v3-6-a1426afdee3b@linaro.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: qcom: Enumerate endpoints based on Link up event in 'global_irq' interrupt | expand |
On Wed, Jul 31, 2024 at 04:20:09PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > Currently, the IRQ device name for both of these IRQs doesn't have Qcom > specific prefix and PCIe domain number. This causes 2 issues: > > 1. Pollutes the global IRQ namespace since 'global' is a common name. > 2. When more than one EP controller instance is present in the SoC, naming > conflict will occur. > > Hence, add 'qcom_pcie_ep_' prefix and PCIe domain number suffix to the IRQ > names to uniquely identify the IRQs and also to fix the above mentioned > issues. > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > --- > drivers/pci/controller/dwc/pcie-qcom-ep.c | 16 ++++++++++++++-- > 1 file changed, 14 insertions(+), 2 deletions(-) > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > index 0bb0a056dd8f..d0a27fa6fdc8 100644 > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > @@ -711,8 +711,15 @@ static irqreturn_t qcom_pcie_ep_perst_irq_thread(int irq, void *data) > static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > struct qcom_pcie_ep *pcie_ep) > { > + struct device *dev = pcie_ep->pci.dev; > + char *name; > int ret; > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_global_irq%d", > + pcie_ep->pci.ep.epc->domain_nr); > + if (!name) > + return -ENOMEM; I assume this is what shows up in /proc/interrupts? I always wonder why it doesn't include dev_name(). A few drivers do that, but apparently it's not universally desirable. It's sort of annoying that, e.g., we get a bunch of "aerdrv" interrupts with no clue which device they relate to. > pcie_ep->global_irq = platform_get_irq_byname(pdev, "global"); > if (pcie_ep->global_irq < 0) > return pcie_ep->global_irq; > @@ -720,18 +727,23 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->global_irq, NULL, > qcom_pcie_ep_global_irq_thread, > IRQF_ONESHOT, > - "global_irq", pcie_ep); > + name, pcie_ep); > if (ret) { > dev_err(&pdev->dev, "Failed to request Global IRQ\n"); > return ret; > } > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_perst_irq%d", > + pcie_ep->pci.ep.epc->domain_nr); > + if (!name) > + return -ENOMEM; > + > pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset); > irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN); > ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL, > qcom_pcie_ep_perst_irq_thread, > IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > - "perst_irq", pcie_ep); > + name, pcie_ep); > if (ret) { > dev_err(&pdev->dev, "Failed to request PERST IRQ\n"); > disable_irq(pcie_ep->global_irq); > > -- > 2.25.1 > >
On Thu, Aug 01, 2024 at 12:23:08PM -0500, Bjorn Helgaas wrote: > On Wed, Jul 31, 2024 at 04:20:09PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > Currently, the IRQ device name for both of these IRQs doesn't have Qcom > > specific prefix and PCIe domain number. This causes 2 issues: > > > > 1. Pollutes the global IRQ namespace since 'global' is a common name. > > 2. When more than one EP controller instance is present in the SoC, naming > > conflict will occur. > > > > Hence, add 'qcom_pcie_ep_' prefix and PCIe domain number suffix to the IRQ > > names to uniquely identify the IRQs and also to fix the above mentioned > > issues. > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > --- > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 16 ++++++++++++++-- > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > index 0bb0a056dd8f..d0a27fa6fdc8 100644 > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > @@ -711,8 +711,15 @@ static irqreturn_t qcom_pcie_ep_perst_irq_thread(int irq, void *data) > > static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > > struct qcom_pcie_ep *pcie_ep) > > { > > + struct device *dev = pcie_ep->pci.dev; > > + char *name; > > int ret; > > > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_global_irq%d", > > + pcie_ep->pci.ep.epc->domain_nr); > > + if (!name) > > + return -ENOMEM; > > I assume this is what shows up in /proc/interrupts? Yes. > I always wonder > why it doesn't include dev_name(). A few drivers do that, but > apparently it's not universally desirable. It's sort of annoying > that, e.g., we get a bunch of "aerdrv" interrupts with no clue which > device they relate to. > dev_name() can be big at times. I wouldn't recommend to include it unless there are no other ways to differentiate between IRQs. Luckily PCIe has the domain number that we can use to differentiate these IRQs. - Mani > > pcie_ep->global_irq = platform_get_irq_byname(pdev, "global"); > > if (pcie_ep->global_irq < 0) > > return pcie_ep->global_irq; > > @@ -720,18 +727,23 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > > ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->global_irq, NULL, > > qcom_pcie_ep_global_irq_thread, > > IRQF_ONESHOT, > > - "global_irq", pcie_ep); > > + name, pcie_ep); > > if (ret) { > > dev_err(&pdev->dev, "Failed to request Global IRQ\n"); > > return ret; > > } > > > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_perst_irq%d", > > + pcie_ep->pci.ep.epc->domain_nr); > > + if (!name) > > + return -ENOMEM; > > + > > pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset); > > irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN); > > ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL, > > qcom_pcie_ep_perst_irq_thread, > > IRQF_TRIGGER_HIGH | IRQF_ONESHOT, > > - "perst_irq", pcie_ep); > > + name, pcie_ep); > > if (ret) { > > dev_err(&pdev->dev, "Failed to request PERST IRQ\n"); > > disable_irq(pcie_ep->global_irq); > > > > -- > > 2.25.1 > > > >
On Fri, Aug 02, 2024 at 01:13:19PM +0530, Manivannan Sadhasivam wrote: > On Thu, Aug 01, 2024 at 12:23:08PM -0500, Bjorn Helgaas wrote: > > On Wed, Jul 31, 2024 at 04:20:09PM +0530, Manivannan Sadhasivam via B4 Relay wrote: > > > From: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > > > > Currently, the IRQ device name for both of these IRQs doesn't have Qcom > > > specific prefix and PCIe domain number. This causes 2 issues: > > > > > > 1. Pollutes the global IRQ namespace since 'global' is a common name. > > > 2. When more than one EP controller instance is present in the SoC, naming > > > conflict will occur. > > > > > > Hence, add 'qcom_pcie_ep_' prefix and PCIe domain number suffix to the IRQ > > > names to uniquely identify the IRQs and also to fix the above mentioned > > > issues. > > > > > > Reviewed-by: Konrad Dybcio <konrad.dybcio@linaro.org> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org> > > > --- > > > drivers/pci/controller/dwc/pcie-qcom-ep.c | 16 ++++++++++++++-- > > > 1 file changed, 14 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > index 0bb0a056dd8f..d0a27fa6fdc8 100644 > > > --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c > > > @@ -711,8 +711,15 @@ static irqreturn_t qcom_pcie_ep_perst_irq_thread(int irq, void *data) > > > static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > > > struct qcom_pcie_ep *pcie_ep) > > > { > > > + struct device *dev = pcie_ep->pci.dev; > > > + char *name; > > > int ret; > > > > > > + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_global_irq%d", > > > + pcie_ep->pci.ep.epc->domain_nr); > > > + if (!name) > > > + return -ENOMEM; > > > > I assume this is what shows up in /proc/interrupts? > > Yes. > > > I always wonder > > why it doesn't include dev_name(). A few drivers do that, but > > apparently it's not universally desirable. It's sort of annoying > > that, e.g., we get a bunch of "aerdrv" interrupts with no clue which > > device they relate to. > > dev_name() can be big at times. I wouldn't recommend to include it > unless there are no other ways to differentiate between IRQs. > Luckily PCIe has the domain number that we can use to differentiate > these IRQs. /proc/interrupts is 159 characters wide even on my puny 8 CPU laptop, so I don't think width is a strong argument, and having to use per-device heuristics (instance number like dmarX, idma64.X, nvmeXqY, domain number, etc) to find the related device is ... well, a hassle. But like I said, obviously devm_request_threaded_irq() *could* have been implemented to include dev_name() internally but wasn't, so I acknowledge there must be good reasons not to, and I'm fine with this patch as-is since it continues the existing practice. > > > pcie_ep->global_irq = platform_get_irq_byname(pdev, "global"); > > > if (pcie_ep->global_irq < 0) > > > return pcie_ep->global_irq; > > > @@ -720,18 +727,23 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, > > > ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->global_irq, NULL, > > > qcom_pcie_ep_global_irq_thread, > > > IRQF_ONESHOT, > > > - "global_irq", pcie_ep); > > > + name, pcie_ep);
diff --git a/drivers/pci/controller/dwc/pcie-qcom-ep.c b/drivers/pci/controller/dwc/pcie-qcom-ep.c index 0bb0a056dd8f..d0a27fa6fdc8 100644 --- a/drivers/pci/controller/dwc/pcie-qcom-ep.c +++ b/drivers/pci/controller/dwc/pcie-qcom-ep.c @@ -711,8 +711,15 @@ static irqreturn_t qcom_pcie_ep_perst_irq_thread(int irq, void *data) static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, struct qcom_pcie_ep *pcie_ep) { + struct device *dev = pcie_ep->pci.dev; + char *name; int ret; + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_global_irq%d", + pcie_ep->pci.ep.epc->domain_nr); + if (!name) + return -ENOMEM; + pcie_ep->global_irq = platform_get_irq_byname(pdev, "global"); if (pcie_ep->global_irq < 0) return pcie_ep->global_irq; @@ -720,18 +727,23 @@ static int qcom_pcie_ep_enable_irq_resources(struct platform_device *pdev, ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->global_irq, NULL, qcom_pcie_ep_global_irq_thread, IRQF_ONESHOT, - "global_irq", pcie_ep); + name, pcie_ep); if (ret) { dev_err(&pdev->dev, "Failed to request Global IRQ\n"); return ret; } + name = devm_kasprintf(dev, GFP_KERNEL, "qcom_pcie_ep_perst_irq%d", + pcie_ep->pci.ep.epc->domain_nr); + if (!name) + return -ENOMEM; + pcie_ep->perst_irq = gpiod_to_irq(pcie_ep->reset); irq_set_status_flags(pcie_ep->perst_irq, IRQ_NOAUTOEN); ret = devm_request_threaded_irq(&pdev->dev, pcie_ep->perst_irq, NULL, qcom_pcie_ep_perst_irq_thread, IRQF_TRIGGER_HIGH | IRQF_ONESHOT, - "perst_irq", pcie_ep); + name, pcie_ep); if (ret) { dev_err(&pdev->dev, "Failed to request PERST IRQ\n"); disable_irq(pcie_ep->global_irq);