Re: [patch 3/7] Check root chipset no_msi flag instead of all parent busses flags

[Grant Grundler]

On Sun, Jul 02, 2006 at 08:40:02PM -0400, Brice Goglin wrote:
> MSI only requires support in the root chipset, which may not even have
> a subordinate bus.

Brice,
The above comment and the main part of this patch don't look right to me.
Maybe it's the glass of wine I had two hours ago and it's around midnight
now...

If the "root chipset" is not a PCI device and the architecture doesn't
fake it, the root chipset (aka "PCI Host bus controller") is not visible
to PCI subsystem. Some of the arch code (e.g. drivers/parisc/dino.c)
uses "bus->self == NULL" to differentiate between PCI-PCI secondary busses
and PCI bus below a "root chipset". ISTR alpha and sparc doing the same.

Can you confirm I'm remembering/understanding this bit correctly?

> pci_msi_supported() now checks the no_msi flag in the root_chipset pci_dev
> struct instead of the PCI_BUS_FLAGS_NO_MSI flag of all its parent busses.
> The MSI quirk now sets the no_msi flag accordingly.

I don't see how this could work for alpha/parisc/sparc (IIRC) or any other
architecture that doesn't create "fake" PCI Root busses.
I think your previous patch was correct in this regard.

> Index: linux-git/drivers/pci/msi.c
> ===================================================================
> --- linux-git.orig/drivers/pci/msi.c	2006-07-02 20:28:28.000000000 -0400
> +++ linux-git/drivers/pci/msi.c	2006-07-02 20:28:58.000000000 -0400
> @@ -905,19 +905,24 @@
>   * @dev: pointer to the pci_dev data structure of MSI device function
>   *
>   * MSI must be globally enabled and supported by the device and
> - * its parent busses.
> + * its root chipset.
>   **/
>  static
>  int pci_msi_supported(struct pci_dev * dev)
>  {
> -	struct pci_bus *bus;
> +	struct pci_dev *pdev;
>  
>  	if (!pci_msi_enable || !dev || dev->no_msi)
>  		return -1;
>  
> -	for (bus = dev->bus; bus; bus = bus->parent)
> -		if (bus->bus_flags & PCI_BUS_FLAGS_NO_MSI)
> -			return -1;
> +	/* find root complex for our device */
> +	pdev = dev;
> +	while (pdev->bus && pdev->bus->self)
> +		pdev = pdev->bus->self;

In particular, I'm thinking this loop isn't correct though I'm not
in a particular, erm, "certain" frame of mind right now.
Under a parisc root device, pdev->bus->self would be NULL.

Maybe that's why parisc uses pci_scan_bus_parented()
instead of pci_scan_bus(). (I've forgot the background
behind that change).

thanks,
grant
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/