Tejun,
thanks for your review and answer.
On 04.05.15 12:06:52, Tejun Heo wrote:
> > 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.
> > + /* 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.
> > +
> > + /* 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.
> > + 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
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
