On Tue, Nov 04, 2014 at 08:01:55PM +0800, Jiang Liu wrote:

In your topic:

  PCI/MSI: enhance PCI MSI core to support hierarchy irqdomain

There's no need to repeat "PCI MSI".  Please run "git log --oneline
drivers/pci/msi.c" and make your similar (capitalize the first word).

Space, not tab.

+static inline irq_hw_number_t
+msi_get_hwirq(struct pci_dev *pdev, struct msi_desc *msidesc)

The convention in this file is "struct pci_dev *dev".  And "struct msi_desc
*desc" (or maybe "*entry").  Try to converge things, not diverge them.

+{
+	return (irq_hw_number_t)msidesc->msi_attrib.entry_nr |
+		PCI_DEVID(pdev->bus->number, pdev->devfn) << 11 |
+		(pci_domain_nr(pdev->bus) & 0xFFFFFFFF) << 27;

Where does this bit layout come from?  Is this defined in the spec
somewhere?  A reference would help.

+}
+
+static int msi_domain_alloc(struct irq_domain *domain, unsigned int virq,
+			    unsigned int nr_irqs, void *arg)
+{
+	int i, ret;
+	irq_hw_number_t hwirq = arch_msi_irq_domain_get_hwirq(arg);
+
+	if (irq_find_mapping(domain, hwirq) > 0)
+		return -EEXIST;
+
+	ret = irq_domain_alloc_irqs_parent(domain, virq, nr_irqs, arg);
+	if (ret >= 0)

	if (ret < 0)
		return ret;

and un-indent the mainline code below.  Then it's obvious that this is the
normal case, not the error case.

+		for (i = 0; i < nr_irqs; i++) {
+			irq_domain_set_hwirq_and_chip(domain, virq + i,
+					hwirq + i, &msi_chip, (void *)(long)i);
+			__irq_set_handler(virq + i, handle_edge_irq, 0, "edge");
+		}
+
+	return ret;
+}
+
+static void msi_domain_free(struct irq_domain *domain, unsigned int virq,
+			    unsigned int nr_irqs)
+{
+	int i;
+
+	for (i = 0; i < nr_irqs; i++) {
+		struct msi_desc *msidesc = irq_get_msi_desc(virq);
+
+		if (msidesc)
+			msidesc->irq = 0;
+	}
+	irq_domain_free_irqs_top(domain, virq, nr_irqs);
+}
+
+static int msi_domain_activate(struct irq_domain *domain,
+			       struct irq_data *irq_data)
+{
+	int ret = 0;
+	struct msi_msg msg;
+
+	/*
+	 * irq_data->chip_data is MSI/MSIx offset.

"MSI-X", as you wrote on the next line.

+	 * MSI-X message is written per-IRQ, the offset is always 0.
+	 * MSI message denotes a contiguous group of IRQs, written for 0th IRQ.
+	 */
+	if (!irq_data->chip_data) {

	if (irq_data->chip_data)
		return 0;

and un-indent the mainline code below, and drop the "ret = 0" init above.

+		ret = irq_chip_compose_msi_msg(irq_data, &msg);
+		if (ret == 0)

	if (ret)
		return ret;

+			write_msi_msg(irq_data->irq, &msg);
+	}
+
+	return ret;

	return 0;

+}
+
+static int msi_domain_deactivate(struct irq_domain *domain,
+				 struct irq_data *irq_data)
+{
+	struct msi_msg msg;
+
+	if (irq_data->chip_data) {
+		memset(&msg, 0, sizeof(msg));
+		write_msi_msg(irq_data->irq, &msg);
+	}
+
+	return 0;
+}
+
+static struct irq_domain_ops msi_domain_ops = {
+	.alloc = msi_domain_alloc,
+	.free = msi_domain_free,
+	.activate = msi_domain_activate,
+	.deactivate = msi_domain_deactivate,
+};
+
+struct irq_domain *msi_create_irq_domain(struct irq_domain *parent)
+{
+	struct irq_domain *domain;
+
+	domain = irq_domain_add_tree(NULL, &msi_domain_ops, NULL);
+	if (domain)

	if (!domain)
		return NULL;

and un-indent this:

+		domain->parent = parent;
+
+	return domain;
+}
+
+int msi_irq_domain_alloc_irqs(struct irq_domain *domain, int type,
+			      struct pci_dev *dev, void *arg)
+{
+	int i, virq;
+	struct msi_desc *msidesc;
+	int node = dev_to_node(&dev->dev);
+
+	list_for_each_entry(msidesc, &dev->msi_list, list) {
+		arch_msi_irq_domain_set_hwirq(arg, msi_get_hwirq(dev, msidesc));
+		virq = irq_domain_alloc_irqs(domain, msidesc->nvec_used,
+					     node, arg);
+		if (virq < 0) {
+			/* Special handling for pci_enable_msi_range(). */
+			return (type == PCI_CAP_ID_MSI &&
+				msidesc->nvec_used > 1) ?  1 : -ENOSPC;	

I think "if" would be easier to read than this ternary expression.

Use a space here, not a tab.

+extern struct irq_chip msi_chip;

I don't think "msi_chip" is a good name.  "Chip" only hints that it's a
semiconductor integrated circuit; it doesn't say anything about what it
does.  I've suggested "msi_controller" elsewhere.

Why does this need to be exported?  And why should there be only one in a
system?

+extern struct irq_domain *msi_create_irq_domain(struct irq_domain *parent);
+extern int msi_irq_domain_alloc_irqs(struct irq_domain *domain, int type,
+				     struct pci_dev *dev, void *arg);
+
+extern irq_hw_number_t arch_msi_irq_domain_get_hwirq(void *arg);
+extern void arch_msi_irq_domain_set_hwirq(void *arg, irq_hw_number_t hwirq);

Look at the rest of the file and notice that the existing code does not use
"extern" on function declarations.

+#endif	/* CONFIG_PCI_MSI_IRQ_DOMAIN */

Use a space here (not a tab), like the #endif just below.

 #endif /* LINUX_MSI_H */
-- 
1.7.10.4