[PATCH] PCI: designware: Add irq_create_mapping()

[PATCH] PCI: designware: Add irq_create_mapping()

[Jingoo Han]

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);
+
 	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 {
-- 
1.7.10.4

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Kishon Vijay Abraham I]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Jingoo Han]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Kishon Vijay Abraham I]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Jingoo Han]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Kishon Vijay Abraham I]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Jingoo Han]

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

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Pratyush Anand]

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-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> > >>>>> Cc: Kishon Vijay Abraham I <kishon-l0cyMroinI0@public.gmane.org>
> > >>>>> ---
> > >>>>> 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 devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI: designware: Add irq_create_mapping()

[Jingoo Han]

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.