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@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);
> };
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
