Tejun,

thanks for your review and answer.

On 04.05.15 12:06:52, Tejun Heo wrote:

quoted

This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.

Please don't mix these two changes in the same patch.

I will split the patch.

quoted

+   /* per-port msix interrupts are not supported */
+   if (n_ports > 1 && nvec >= n_ports)
+           return -ENOSYS;

Hmm... can you please elaborate why the condition isn't nvec > 1?

I slightly changed the check and added a comment that explains that's
going on in the function. This is the new version:

static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
			  struct ahci_host_priv *hpriv)
{
	int rc, nvec;
	struct msix_entry entry = {};

	/* check if msix is supported */
	nvec = pci_msix_vec_count(pdev);
	if (nvec <= 0)
		return 0;

	/*
	 * Per-port msix interrupts are not supported. Assume single
	 * port interrupts for:
	 *
	 *  n_ports == 1, or
	 *  nvec < n_ports.
	 *
	 * We also need to check for n_ports != 0 which is implicitly
	 * covered here since nvec > 0.
	 */
	if (n_ports != 1 && nvec >= n_ports)
		return -ENOSYS;

	/*
	 * There can exist more than one vector (e.g. for error
	 * detection or hdd hotplug). Then the first vector is used,
	 * all others are ignored. Only enable the first entry here
	 * (entry.entry = 0).
	 */
	rc = pci_enable_msix_exact(pdev, &entry, 1);
	if (rc < 0)
		return rc;

	return 1;
}

Note that the check changed to n_ports != 1 to also cover the case
n_ports == 0 which should return -ENOSYS.

Also, shouldn't we be printing a warning message here explaining why
probing is failing?

I didn't want to print a warning in case -ENOSYS for backward
compatability. Only if msi-x code fails there is a message, see
__ahci_init_interrupts(). In any other case the behaviour is as
before, thus no message is printed.

quoted

+
+   /* only enable the first entry (entry.entry = 0) */
+   rc = pci_enable_msix_exact(pdev, &entry, 1);

So, enabling the first msix works if nvec > 1 && nvec < n_ports but
not if nvec >= n_ports?

For n_ports > 1 && nvec >= n_ports we need to assume per-port
interrupts. There are enough vectors for all ports then.

quoted

+	if (rc < 0)
+		return rc;
+
+	return 1;
+}
+
+static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				  struct ahci_host_priv *hpriv)
 {
 	int rc, nvec;

+	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	if (nvec > 0)
+		return nvec;
+
+	if (nvec && nvec != -ENOSYS)
+		dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
+
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
 		goto intx;

@@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	return 0;
 }

+static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
+{
+	struct msi_desc *desc;
+
+	list_for_each_entry(desc, &dev->msi_list, list) {
+		if (desc->msi_attrib.entry_nr == entry)
+			return desc;
+	}
+
+	return NULL;
+}
+
+static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+                           struct ahci_host_priv *hpriv)
+{
+   struct msi_desc *desc;
+
+   __ahci_init_interrupts(pdev, n_ports, hpriv);
+
+   if (!pdev->msix_enabled)
+           return pdev->irq;
+
+   desc = msix_get_desc(pdev, 0);  /* first entry */
+   if (!desc)
+           return -ENODEV;
+
+   return desc->irq;
+}

Can we please do this properly?  We should be able to move port priv
allocation to host allocaotion time and add and use pp->irq instead,
right?

I started working implementing this.

Will send an updated patch set once finished.

Thanks,

-Robert