From: Kenji Kaneshige <kaneshige.kenji@jp•fujitsu.com>
To: Breno Leitao <leitao@linux•vnet.ibm.com>
Cc: Ron Mercer <ron.mercer@qlogic•com>,
Linux PCI <linux-pci@vger•kernel.org>,
Jay Vosburgh <fubar@us•ibm.com>,
linuxppc-dev@lists•ozlabs.org
Subject: Re: [RFC PATCH] PCI-E broken on PPC (regression)
Date: Tue, 26 Jan 2010 13:18:37 +0900 [thread overview]
Message-ID: <4B5E6D1D.5080503@jp.fujitsu.com> (raw)
In-Reply-To: <4B5D9FC5.5070600@linux.vnet.ibm.com>
Breno Leitao wrote:
> Hello,
>
> I found that qlge is broken on PPC, and it got broken after commit
> 06a1cbafb253c4c60d6a54a994887f5fbceabcc0. It happens because dev->pcie
> is not set on PPC, because the function set_pcie_port_type(), who sets
> dev->pcie, is not being called on PPC PCI code.
Hi Breno,
I'm sorry for the regression. I didn't realize that PPC uses open-coded
PCI scan.
I guess the problem is caused by dev->pcie_cap, right? But I don't
understand what is happening clearly. Could you send me the detailed
information about the problem?
>
> So, I have two ideas to fix it, the first one is to call
> set_pcie_port_type() on pci_device_add() instead of pci_setup_device().
> Since that PPC device flow calls pci_device_add(), it fixes the problem
> without duplicating the caller for this function.
>
The set_pcie_port_type() needs to be called before pci_fixup_device() in
pci_setup_device() because fixup code might refer dev->pcie_cap. So I
don't think the first one is a good idea.
> OTOH, it's also possible to add set_pcie_port_type() on pci.h and call
> it inside the PPC PCI files, specifically on of_create_pci_dev().
>
> I tested both ideas and they work perfect, so I'd like to figure out
> which one is the most correct one. Both patches are attached here.
I think the second one is better. But to prevent similar problems, I
think it would be better to remove open-coded PCI scan in PPC in a
long term, though I don't know the background about that.
Thanks,
Kenji Kaneshige
>
> Thanks,
> Breno
>
> ----- First idea -----
>
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 98ffb2d..328c3ab 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -746,7 +746,6 @@ int pci_setup_device(struct pci_dev *dev)
> dev->hdr_type = hdr_type & 0x7f;
> dev->multifunction = !!(hdr_type & 0x80);
> dev->error_state = pci_channel_io_normal;
> - set_pcie_port_type(dev);
> set_pci_aer_firmware_first(dev);
>
> list_for_each_entry(slot, &dev->bus->slots, list)
> @@ -1052,6 +1051,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
> /* Initialize various capabilities */
> pci_init_capabilities(dev);
>
> + set_pcie_port_type(dev);
> /*
> * Add the device to our list of discovered devices
> * and the bus list for fixup functions, etc.
>
> ----- Second idea -----
>
> diff --git a/arch/powerpc/kernel/pci_of_scan.c b/arch/powerpc/kernel/pci_of_scan.c
> index 7311fdf..f8820e8 100644
> --- a/arch/powerpc/kernel/pci_of_scan.c
> +++ b/arch/powerpc/kernel/pci_of_scan.c
> @@ -160,6 +160,7 @@ struct pci_dev *of_create_pci_dev(struct device_node *node,
> dev->error_state = pci_channel_io_normal;
> dev->dma_mask = 0xffffffff;
>
> + set_pcie_port_type(dev);
> if (!strcmp(type, "pci") || !strcmp(type, "pciex")) {
> /* a PCI-PCI bridge */
> dev->hdr_type = PCI_HEADER_TYPE_BRIDGE;
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 98ffb2d..f787eea 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -681,7 +681,7 @@ static void pci_read_irq(struct pci_dev *dev)
> dev->irq = irq;
> }
>
> -static void set_pcie_port_type(struct pci_dev *pdev)
> +void set_pcie_port_type(struct pci_dev *pdev)
> {
> int pos;
> u16 reg16;
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 174e539..765095b 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1269,6 +1269,7 @@ int pcibios_add_platform_entries(struct pci_dev *dev);
> void pcibios_disable_device(struct pci_dev *dev);
> int pcibios_set_pcie_reset_state(struct pci_dev *dev,
> enum pcie_reset_state state);
> +extern void set_pcie_port_type(struct pci_dev *pdev);
>
> #ifdef CONFIG_PCI_MMCONFIG
> extern void __init pci_mmcfg_early_init(void);
>
>
prev parent reply other threads:[~2010-01-26 4:35 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-01-25 13:42 [RFC PATCH] PCI-E broken on PPC (regression) Breno Leitao
2010-01-25 20:38 ` Jesse Barnes
2010-01-26 1:50 ` Jesse Barnes
2010-01-26 2:59 ` Benjamin Herrenschmidt
2010-01-26 4:36 ` Kenji Kaneshige
2010-01-27 2:10 ` Benjamin Herrenschmidt
2010-01-27 16:26 ` Jesse Barnes
2010-01-27 22:00 ` Benjamin Herrenschmidt
2010-01-28 0:01 ` David Miller
2010-01-28 0:03 ` Jesse Barnes
2010-01-29 3:45 ` Matthew Wilcox
2010-01-29 3:54 ` Benjamin Herrenschmidt
2010-02-18 0:22 ` David Miller
2010-02-18 0:29 ` Jesse Barnes
2010-01-26 4:18 ` Kenji Kaneshige [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4B5E6D1D.5080503@jp.fujitsu.com \
--to=kaneshige.kenji@jp$(echo .)fujitsu.com \
--cc=fubar@us$(echo .)ibm.com \
--cc=leitao@linux$(echo .)vnet.ibm.com \
--cc=linux-pci@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=ron.mercer@qlogic$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox