linux-kernel - Re: [PATCH V2] PCI: exynos: add support for MSI

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-id: <001801cea3c7$06bca9d0$1435fd70$%han@samsung.com>
Date:	Wed, 28 Aug 2013 17:17:24 +0900
From:	Jingoo Han <jg1.han@...sung.com>
To:	'Pratyush Anand' <pratyush.anand@...com>
Cc:	'Bjorn Helgaas' <bhelgaas@...gle.com>, linux-pci@...r.kernel.org,
	linux-samsung-soc@...r.kernel.org,
	'Kukjin Kim' <kgene.kim@...sung.com>,
	'Mohit KUMAR DCG' <Mohit.KUMAR@...com>,
	'Siva Reddy Kallam' <siva.kallam@...sung.com>,
	'SRIKANTH TUMKUR SHIVANAND' <ts.srikanth@...sung.com>,
	'Arnd Bergmann' <arnd@...db.de>,
	'Sean Cross' <xobs@...agi.com>,
	'Kishon Vijay Abraham I' <kishon@...com>,
	'Thierry Reding' <thierry.reding@...il.com>,
	'Thomas Petazzoni' <thomas.petazzoni@...e-electrons.com>,
	linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
	'Jingoo Han' <jg1.han@...sung.com>
Subject: Re: [PATCH V2] PCI: exynos: add support for MSI

On Friday, August 23, 2013 5:36 PM, Pratyush Anand wrote:
> On Fri, Aug 23, 2013 at 02:04:20PM +0800, Jingoo Han wrote:

[.....]

> > +#define MAX_MSI_IRQS                   32
> 
> DW MSI controller can support upto 256. However, 32 seems a practical
> choice, as there might not be any system which may use more
> than 32. But a comment like as follows can be put here:
> 
> /*
> * Maximum number of MSI IRQs can be 256 per controller. But keep
> * it 32 as of now. Probably we will never need more than 32. If needed,
> * then increment it in multiple of 32.
> */

OK.
I will add this comment.

> 
> > +#define MAX_MSI_CTRLS                  8
> 
> Why to waste cpu cycles when MAX_MSI_IRQS is 32 only.
> #define MAX_MSI_CTRLS   (MAX_MSI_IRQS / 32)

OK.
I will fix it as you guide.

> 
> > +
> > +static unsigned int msi_data;
> > +static DECLARE_BITMAP(msi_irq_in_use, MAX_MSI_IRQS);
> 
> What if one has more than one RC.
> There are SOCs which support 3 RCs.
> So something like this:
> 
> #define MAX_PCIE_PORT_SUPPORTED 3
> static DECLARE_BITMAP(msi_irq_in_use[MAX_PCIE_PORT_SUPPORTED],
>                   NUM_MSI_IRQS);
> static unsigned int *msi_data[MAX_PCIE_PORT_SUPPORTED];

No, there is no need to do it.
Without this, we can use 3 RCs by adding a node of 3rd RC to dt file.

> 
> Allocate msi_data using __get_free_pages(GFP_KERNEL, 0)) as Thierry
> suggested.

OK.
I will use '__get_free_pages(GFP_KERNEL, 0))' as Thierry guide.

> >  int dw_pcie_link_up(struct pcie_port *pp)
> >  {
> >         if (pp->ops->link_up)
> > @@ -225,6 +432,13 @@ int __init dw_pcie_host_init(struct pcie_port *pp)
> >                 return -EINVAL;
> >         }
> >
> > +       if (IS_ENABLED(CONFIG_PCI_MSI)) {
> > +               if (of_property_read_u32(np, "msi-base", &pp->msi_irq_start)) {
> > +                       dev_err(pp->dev, "Failed to parse the number of lanes\n");
> > +                       return -EINVAL;
> > +               }
> > +       }
> > +
> 
> What if an implementor want to use irq_domain method for msi_irq_start
> allocation? Is it fine to return error if msi-base is not passed from
> dt?

Sure, I will consider using irq_domain method.

>
> Also, with the limited knowledge of dt I do not understand one thing, how
> would dt understand that you have used 32 msi irqs (MAX_MSI_IRQS)?

Sorry, I cannot understand exactly. :-(
I will look into it.

I really appreciate your comments. :-)

Best regards,
Jingoo Han

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/