linux-kernel - Re: [PATCH v2] AHCI: Add generic MSI-X interrupt support to SATA PCI driver

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: <20150511171810.GB29499@rric.localhost>
Date:	Mon, 11 May 2015 19:18:10 +0200
From:	Robert Richter <robert.richter@...iumnetworks.com>
To:	Tejun Heo <tj@...nel.org>
CC:	Robert Richter <rric@...nel.org>,
	Catalin Marinas <catalin.marinas@....com>,
	Will Deacon <will.deacon@....com>,
	Sunil Goutham <sgoutham@...ium.com>,
	Jiang Liu <jiang.liu@...ux.intel.com>,
	<linux-arm-kernel@...ts.infradead.org>,
	<linux-kernel@...r.kernel.org>, <linux-ide@...r.kernel.org>,
	Alexander Gordeev <agordeev@...hat.com>
Subject: Re: [PATCH v2] AHCI: Add generic MSI-X interrupt support to SATA PCI
 driver

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