Re: [PATCH] PCI: designware: Add irq_create_mapping()
From: Pratyush Anand
Date: Wed Oct 09 2013 - 07:14:14 EST
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@xxxxxxxxxxx>
> > >>>>> Cc: Kishon Vijay Abraham I <kishon@xxxxxx>
> > >>>>> ---
> > >>>>> 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);
};
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.
Regards
Pratyush
> Best regards,
> Jingoo Han
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/