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@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-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/