| Message ID | 20131009111333.GC14126@pratyush-vbox (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
On Wednesday, October 09, 2013 8:14 PM, Pratyush Anand wrote: > On Wed, Oct 09, 2013 at 06:49:15PM +0800, Jingoo Han wrote: > > 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; > > > > Documentation/IRQ-domain.txt says that "The irq_create_mapping() > function must be called *atleast once* before any call to > irq_find_mapping(), lest the descriptor will not be allocated." > > So for sure current code need to be modified. > > Now, you can create the mapping statically during initialization and > then use it dynamically whenever needed. In that case what Kishon > suggested is right, something like following should work. > > for (i = 0; i < MAX_MSI_IRQS; i++) > irq_create_mapping(pp->irq_domain, i); > pp->msi_irq_start = irq_find_mapping(pp->irq_domain, 0); > > But, I am not sure that created mapping is continuous. I mean, > difference between irq returned by > irq_find_mapping(pp->irq_domain, 0) > & > irq_find_mapping(pp->irq_domain, 9) > is always 9. If that is not the case then, static assignment of > msi_irq_start will not work. May be you need something as follows: > > diff --git a/drivers/pci/host/pcie-designware.c b/drivers/pci/host/pcie-designware.c > index 8963017..e6749e8 100644 > --- a/drivers/pci/host/pcie-designware.c > +++ b/drivers/pci/host/pcie-designware.c > @@ -157,7 +157,7 @@ static struct irq_chip dw_msi_irq_chip = { > void dw_handle_msi_irq(struct pcie_port *pp) > { > unsigned long val; > - int i, pos; > + int i, pos, irq; > > for (i = 0; i < MAX_MSI_CTRLS; i++) { > dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, > @@ -165,8 +165,9 @@ void dw_handle_msi_irq(struct pcie_port *pp) > if (val) { > pos = 0; > while ((pos = find_next_bit(&val, 32, pos)) != 32) { > - generic_handle_irq(pp->msi_irq_start > - + (i * 32) + pos); > + irq = irq_find_mapping(pp->irq_domain, > + i * 32 + pos); > + generic_handle_irq(irq); > pos++; > } > } > @@ -237,9 +238,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) > } > } > > - irq = (pp->msi_irq_start + pos0); > - > - if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1)) > + irq = irq_find_mapping(pp->irq_domain, pos0); > + if (!irq) > goto no_valid_irq; > > i = 0; > @@ -270,6 +270,7 @@ static void clear_irq(unsigned int irq) > struct irq_desc *desc; > struct msi_desc *msi; > struct pcie_port *pp; > + struct irq_data *data = irq_get_irq_data(irq); > > /* get the port structure */ > desc = irq_to_desc(irq); > @@ -280,7 +281,7 @@ static void clear_irq(unsigned int irq) > return; > } > > - pos = irq - pp->msi_irq_start; > + pos = data->hwirq; > > irq_free_desc(irq); > > @@ -371,6 +372,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > struct of_pci_range range; > struct of_pci_range_parser parser; > u32 val; > + int i; > > struct irq_domain *irq_domain; > > @@ -449,7 +451,8 @@ int __init dw_pcie_host_init(struct pcie_port *pp) > return -ENXIO; > } > > - pp->msi_irq_start = irq_find_mapping(irq_domain, 0); > + for (i = 0; i < MAX_MSI_IRQS; i++) > + irq_create_mapping(irq_domain, i); > } > > if (pp->ops->host_init) > diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h > index faccbbf..2ad56e4 100644 > --- a/drivers/pci/host/pcie-designware.h > +++ b/drivers/pci/host/pcie-designware.h > @@ -47,7 +47,7 @@ struct pcie_port { > u32 lanes; > struct pcie_host_ops *ops; > int msi_irq; > - int msi_irq_start; > + struct irq_domain *irq_domain; > unsigned long msi_data; > DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); > }; Hi Pratyush Anand, Ah, I see. Thank you for your kind and detailed comment. Also I tested your patch on Exynos5440 with two Intel e1000e LAN cards. It works properly with some very trivial modification. I will send v2 patch as a committer. Of course, you will be the author of v2 patch. Thank you. Kishon, I would appreciate the opportunity to discuss with you. :-) Best regards, Jingoo Han > > Other could be what you are suggesting or Tegra is using. Do no create > static mapping. Whenever you need a mapping call irq_create_mapping rather > irq_find_mapping. That should also work, as multiple calling of irq_create_mapping > will not do anything more than that of irq_find_mapping. -- 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..e6749e8 100644 --- a/drivers/pci/host/pcie-designware.c +++ b/drivers/pci/host/pcie-designware.c @@ -157,7 +157,7 @@ static struct irq_chip dw_msi_irq_chip = { void dw_handle_msi_irq(struct pcie_port *pp) { unsigned long val; - int i, pos; + int i, pos, irq; for (i = 0; i < MAX_MSI_CTRLS; i++) { dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_STATUS + i * 12, 4, @@ -165,8 +165,9 @@ void dw_handle_msi_irq(struct pcie_port *pp) if (val) { pos = 0; while ((pos = find_next_bit(&val, 32, pos)) != 32) { - generic_handle_irq(pp->msi_irq_start - + (i * 32) + pos); + irq = irq_find_mapping(pp->irq_domain, + i * 32 + pos); + generic_handle_irq(irq); pos++; } } @@ -237,9 +238,8 @@ static int assign_irq(int no_irqs, struct msi_desc *desc, int *pos) } } - irq = (pp->msi_irq_start + pos0); - - if ((irq + no_irqs) > (pp->msi_irq_start + MAX_MSI_IRQS-1)) + irq = irq_find_mapping(pp->irq_domain, pos0); + if (!irq) goto no_valid_irq; i = 0; @@ -270,6 +270,7 @@ static void clear_irq(unsigned int irq) struct irq_desc *desc; struct msi_desc *msi; struct pcie_port *pp; + struct irq_data *data = irq_get_irq_data(irq); /* get the port structure */ desc = irq_to_desc(irq); @@ -280,7 +281,7 @@ static void clear_irq(unsigned int irq) return; } - pos = irq - pp->msi_irq_start; + pos = data->hwirq; irq_free_desc(irq); @@ -371,6 +372,7 @@ int __init dw_pcie_host_init(struct pcie_port *pp) struct of_pci_range range; struct of_pci_range_parser parser; u32 val; + int i; struct irq_domain *irq_domain; @@ -449,7 +451,8 @@ int __init dw_pcie_host_init(struct pcie_port *pp) return -ENXIO; } - pp->msi_irq_start = irq_find_mapping(irq_domain, 0); + for (i = 0; i < MAX_MSI_IRQS; i++) + irq_create_mapping(irq_domain, i); } if (pp->ops->host_init) diff --git a/drivers/pci/host/pcie-designware.h b/drivers/pci/host/pcie-designware.h index faccbbf..2ad56e4 100644 --- a/drivers/pci/host/pcie-designware.h +++ b/drivers/pci/host/pcie-designware.h @@ -47,7 +47,7 @@ struct pcie_port { u32 lanes; struct pcie_host_ops *ops; int msi_irq; - int msi_irq_start; + struct irq_domain *irq_domain; unsigned long msi_data; DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS); };