Message-Id: <200901151742.09227.rjw@sisk.pl>
Date:	Thu, 15 Jan 2009 17:42:08 +0100
From:	"Rafael J. Wysocki" <rjw@...k.pl>
To:	Kenji Kaneshige <kaneshige.kenji@...fujitsu.com>
Cc:	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>,
	Jesse Barnes <jbarnes@...tuousgeek.org>,
	Linux PCI <linux-pci@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 5/8] PCI PCIe portdrv: Fix allocation of interrupts

On Thursday 15 January 2009, Kenji Kaneshige wrote:
> Hidetoshi Seto wrote:
> > Rafael J. Wysocki wrote:
> >> On Wednesday 14 January 2009, Rafael J. Wysocki wrote:
> >>> On Wednesday 14 January 2009, Kenji Kaneshige wrote:
> >> [...]
> >>>> I'm sorry but I don't understand what the problem is.
> >>>> Do you mean pci_disable_msix() doesn't work on some platforms?
> >>> No, I don't.  It was just confusion on my side, sorry.
> >>>
> >>> Please have a look at the new version of the patch I sent yesterday
> >>> (http://marc.info/?l=linux-pci&m=123185510828181&w=4).
> >> BTW, in your patch the first dummy pci_enable_msix() allocates just one
> >> vector, which means that the contents of both
> >> msix_entries[idx_hppme].entry and msix_entries[idx_aer].entry will be the same,
> >> if my reading of the spec (PCI 3.0 in this case) is correct.
> > 
> > According to PCI 3.0 implementation note "Handling MSI-X Vector Shortage,"
> > it seems your reading is not correct.
> > 
> > Assume that the port have 4 entries([0-3]) in MSI-X table, and that entry[2]
> > for hotplug/PME and entry[3] for AER, and that kernel only allocates 2 vector.
> > Spec says that the port could be designed for software to configure entries
> > assigning vectors{A,B} to multiple entries as ABAB, AABB, ABBB etc. 
> > 
> > So if there is just one vector, it could be AAAA.

Our pci_enable_msix() doesn't do that.  It will always do A---.

> BTW, I don't think pci_enable_msix() allows this kind of configuration.
> With the dummy pci_enable_msix() in my patch, it would be A---, I think.

And that exactly is why I'm not sure it's correct.

Namely, if only the first entry is configured, the device is only able to use
one vector, represented by this entry, for any purpose.  Now, for instance, for
PCIE_CAPABILITIES_REG, there are two possibilities:
(1) the value in the register always points to the _valid_ entry in the MSI-X
    table and that would be the first one,
(2) the value in the register may point to an _invalid_ entry (1 - 3).

You seem to assume that (2) is the case, but I'm not sure (that should follow
from the PCI Express spec, but it clearly doesn't, at least I couldn't find
any pointer in the spec).  IMO it wouldn't make sense, because the port
wouldn't have been able to generate interrupts for this service if only one
vector had been configured.

Still, even though (2) is the case, but both PCIE_CAPABILITIES_REG and
PCI_ERR_ROOT_STATUS just happen to point to the same entry, which very well may
be possible, the second pci_enable_msix() in your patch will fail.

In any case, I think we should
(a) get the number of the port's MSI-X table entries _first_, without enabling
    MSI-X,
(b) allocate as many MSI-X vectors as indicated by this number, even though
    some of them may not be used,
(c) use PCIE_CAPABILITIES_REG and PCI_ERR_ROOT_STATUS to check
    which vector has been allocated to which service.

Thanks,
Rafael
--
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/

Powered by blists - more mailing lists