| Message ID | 000401cec4c6$dc415180$94c3f480$%han@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
Hi Jingoo, On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: > Without irq_create_mapping(), the correct irq number cannot be > provided. In this case, it makes problem such as NULL deference. > Thus, irq_create_mapping() should be added for MSI. > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > Cc: Kishon Vijay Abraham I <kishon@ti.com> > --- > Tested on Exynos5440. > > drivers/pci/host/pcie-designware.c | 10 ++++------ > drivers/pci/host/pcie-designware.h | 1 + > 2 files changed, 5 insertions(+), 6 deletions(-) > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 8963017..e536bb6 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) > } > } > > + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); > + I think irq_create_mapping should be done for all the MSI irq lines instead of only the first line. So you might have to do for MAX_MSI_IRQS lines. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote: > On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: > > Without irq_create_mapping(), the correct irq number cannot be > > provided. In this case, it makes problem such as NULL deference. > > Thus, irq_create_mapping() should be added for MSI. > > > > Signed-off-by: Jingoo Han <jg1.han@samsung.com> > > Cc: Kishon Vijay Abraham I <kishon@ti.com> > > --- > > Tested on Exynos5440. > > > > drivers/pci/host/pcie-designware.c | 10 ++++------ > > drivers/pci/host/pcie-designware.h | 1 + > > 2 files changed, 5 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > > index 8963017..e536bb6 100644 > > --- a/drivers/pci/host/pcie-designware.c > > +++ b/drivers/pci/host/pcie-designware.c > > @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) > > } > > } > > > > + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); > > + > > I think irq_create_mapping should be done for all the MSI irq lines instead of > only the first line. So you might have to do for MAX_MSI_IRQS lines. I tested PCIe LAN card after I replaced 0 with MAX_MSI_IRQS. However, it makes an error message as below: But, PCIe LAN card works properly. WARNING: CPU: 1 PID: 1 at kernel/irq/irqdomain.c:276 irq_domain_associate+0x150/0x1a8() error: hwirq 0x20 is too large for (null) Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 09 October 2013 02:47 PM, Jingoo Han wrote: > On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote: >> On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: >>> Without irq_create_mapping(), the correct irq number cannot be >>> provided. In this case, it makes problem such as NULL deference. >>> Thus, irq_create_mapping() should be added for MSI. >>> >>> Signed-off-by: Jingoo Han <jg1.han@samsung.com> >>> Cc: Kishon Vijay Abraham I <kishon@ti.com> >>> --- >>> Tested on Exynos5440. >>> >>> drivers/pci/host/pcie-designware.c | 10 ++++------ >>> drivers/pci/host/pcie-designware.h | 1 + >>> 2 files changed, 5 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >>> index 8963017..e536bb6 100644 >>> --- a/drivers/pci/host/pcie-designware.c >>> +++ b/drivers/pci/host/pcie-designware.c >>> @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) >>> } >>> } >>> >>> + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); >>> + >> >> I think irq_create_mapping should be done for all the MSI irq lines instead of >> only the first line. So you might have to do for MAX_MSI_IRQS lines. Maybe it should be only till MAX_MSI_IRQS-1? Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 09, 2013 6:48 PM, Kishon Vijay Abraham I wrote: > On Wednesday 09 October 2013 02:47 PM, Jingoo Han wrote: > > On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote: > >> On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: > >>> Without irq_create_mapping(), the correct irq number cannot be > >>> provided. In this case, it makes problem such as NULL deference. > >>> Thus, irq_create_mapping() should be added for MSI. > >>> > >>> Signed-off-by: Jingoo Han <jg1.han@samsung.com> > >>> Cc: Kishon Vijay Abraham I <kishon@ti.com> > >>> --- > >>> Tested on Exynos5440. > >>> > >>> drivers/pci/host/pcie-designware.c | 10 ++++------ > >>> drivers/pci/host/pcie-designware.h | 1 + > >>> 2 files changed, 5 insertions(+), 6 deletions(-) > >>> > >>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > >>> index 8963017..e536bb6 100644 > >>> --- a/drivers/pci/host/pcie-designware.c > >>> +++ b/drivers/pci/host/pcie-designware.c > >>> @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) > >>> } > >>> } > >>> > >>> + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); > >>> + > >> > >> I think irq_create_mapping should be done for all the MSI irq lines instead of > >> only the first line. So you might have to do for MAX_MSI_IRQS lines. > > Maybe it should be only till MAX_MSI_IRQS-1? I am not sure; however, it doesn't look correct. irq_create_mapping() is defined as below. According to the comment, irq_create_mapping() maps 'a' hardware interrupt, not max value of MSI IRQ lines. ./kernel/irq/irqdomain.c /** * irq_create_mapping() - Map a hardware interrupt into linux irq space * @domain: domain owning this hardware interrupt or NULL for default domain * @hwirq: hardware irq number in that domain space [.....] unsigned int irq_create_mapping(struct irq_domain *domain, irq_hw_number_t hwirq) Also, another value of 'irq' can be selected, because 'pos0' is added. pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); irq = (pp->msi_irq_start + pos0); Pratyush Anand, If I am wrong, please let me know. :-) Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday 09 October 2013 03:35 PM, Jingoo Han wrote: > On Wednesday, October 09, 2013 6:48 PM, Kishon Vijay Abraham I wrote: >> On Wednesday 09 October 2013 02:47 PM, Jingoo Han wrote: >>> On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote: >>>> On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: >>>>> Without irq_create_mapping(), the correct irq number cannot be >>>>> provided. In this case, it makes problem such as NULL deference. >>>>> Thus, irq_create_mapping() should be added for MSI. >>>>> >>>>> Signed-off-by: Jingoo Han <jg1.han@samsung.com> >>>>> Cc: Kishon Vijay Abraham I <kishon@ti.com> >>>>> --- >>>>> Tested on Exynos5440. >>>>> >>>>> drivers/pci/host/pcie-designware.c | 10 ++++------ >>>>> drivers/pci/host/pcie-designware.h | 1 + >>>>> 2 files changed, 5 insertions(+), 6 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c >>>>> index 8963017..e536bb6 100644 >>>>> --- a/drivers/pci/host/pcie-designware.c >>>>> +++ b/drivers/pci/host/pcie-designware.c >>>>> @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) >>>>> } >>>>> } >>>>> >>>>> + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); >>>>> + >>>> >>>> I think irq_create_mapping should be done for all the MSI irq lines instead of >>>> only the first line. So you might have to do for MAX_MSI_IRQS lines. >> >> Maybe it should be only till MAX_MSI_IRQS-1? I meant something like this, for (i = 0; i < MAX_MSI_IRQS; i++) irq_create_mapping(pp->irq_domain, i); That didn't give me any issues though. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, October 09, 2013 7:27 PM, Kishon Vijay Abraham I wrote: > On Wednesday 09 October 2013 03:35 PM, Jingoo Han wrote: > > On Wednesday, October 09, 2013 6:48 PM, Kishon Vijay Abraham I wrote: > >> On Wednesday 09 October 2013 02:47 PM, Jingoo Han wrote: > >>> On Wednesday, October 09, 2013 6:06 PM, Kishon Vijay Abraham I wrote: > >>>> On Wednesday 09 October 2013 01:39 PM, Jingoo Han wrote: > >>>>> Without irq_create_mapping(), the correct irq number cannot be > >>>>> provided. In this case, it makes problem such as NULL deference. > >>>>> Thus, irq_create_mapping() should be added for MSI. > >>>>> > >>>>> Signed-off-by: Jingoo Han <jg1.han@samsung.com> > >>>>> Cc: Kishon Vijay Abraham I <kishon@ti.com> > >>>>> --- > >>>>> Tested on Exynos5440. > >>>>> > >>>>> drivers/pci/host/pcie-designware.c | 10 ++++------ > >>>>> drivers/pci/host/pcie-designware.h | 1 + > >>>>> 2 files changed, 5 insertions(+), 6 deletions(-) > >>>>> > >>>>> diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > >>>>> index 8963017..e536bb6 100644 > >>>>> --- a/drivers/pci/host/pcie-designware.c > >>>>> +++ b/drivers/pci/host/pcie-designware.c > >>>>> @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) > >>>>> } > >>>>> } > >>>>> > >>>>> + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); > >>>>> + > >>>> > >>>> I think irq_create_mapping should be done for all the MSI irq lines instead of > >>>> only the first line. So you might have to do for MAX_MSI_IRQS lines. > >> > >> Maybe it should be only till MAX_MSI_IRQS-1? > > I meant something like this, > > for (i = 0; i < MAX_MSI_IRQS; i++) > irq_create_mapping(pp->irq_domain, i); > > That didn't give me any issues though. However, no driver calls irq_create_mapping() like this. For example, Tegra PCI driver gives 'hwirq' as single offset value to irq_create_mapping() without any loop. static int tegra_msi_setup_irq(struct msi_chip *chip, struct pci_dev *pdev, struct msi_desc *desc) { hwirq = tegra_msi_alloc(msi); irq = irq_create_mapping(msi->domain, hwirq); Maybe, the following can be used, it uses 'pos0' as the offset value. pp->msi_irq_start = irq_create_mapping(pp->irq_domain, pos0); irq = pp->msi_irq_start; Best regards, Jingoo Han -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c index 8963017..e536bb6 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -237,6 +237,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) } } + pp->msi_irq_start = irq_create_mapping(pp->irq_domain, 0); + irq = (pp->msi_irq_start + pos0); if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1)) @@ -372,8 +374,6 @@ int __init dw_pcie_host_init(struct pcie_port *pp) struct of_pci_range_parser parser; u32 val; - struct irq_domain *irq_domain; - if (of_pci_range_parser_init(&parser, np)) { dev_err(pp->dev, "missing ranges property\n"); return -EINVAL; @@ -441,15 +441,13 @@ int __init dw_pcie_host_init(struct pcie_port *pp) } if (IS_ENABLED(CONFIG_PCI_MSI)) { - irq_domain = irq_domain_add_linear(pp->dev->of_node, + pp->irq_domain = irq_domain_add_linear(pp->dev->of_node, MAX_MSI_IRQS, &msi_domain_ops, &dw_pcie_msi_chip); - if (!irq_domain) { + if (!pp->irq_domain) { dev_err(pp->dev, "irq domain init failed\n"); return -ENXIO; } - - pp->msi_irq_start = irq_find_mapping(irq_domain, 0); } if (pp->ops->host_init) diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index faccbbf..240fc11 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -50,6 +50,7 @@ struct pcie_port { int msi_irq_start; unsigned long msi_data; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); + struct irq_domain *irq_domain; }; struct pcie_host_ops {
Without irq_create_mapping(), the correct irq number cannot be provided. In this case, it makes problem such as NULL deference. Thus, irq_create_mapping() should be added for MSI. Signed-off-by: Jingoo Han <jg1.han@samsung.com> Cc: Kishon Vijay Abraham I <kishon@ti.com> --- Tested on Exynos5440. drivers/pci/host/pcie-designware.c | 10 ++++------ drivers/pci/host/pcie-designware.h | 1 + 2 files changed, 5 insertions(+), 6 deletions(-)