From: Benjamin Herrenschmidt <benh@kernel•crashing.org>
To: Chen Yuanquan-B41889 <B41889@freescale•com>
Cc: Bjorn Helgaas <bhelgaas@google•com>,
linux-pci@vger•kernel.org, linuxppc-dev@lists•ozlabs.org,
r61911@freescale•com
Subject: Re: [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device
Date: Wed, 05 Dec 2012 19:26:29 +1100 [thread overview]
Message-ID: <1354695989.2351.13.camel@pasglop> (raw)
In-Reply-To: <50BF03C1.2070304@freescale.com>
On Wed, 2012-12-05 at 16:20 +0800, Chen Yuanquan-B41889 wrote:
> On 12/05/2012 03:17 PM, Benjamin Herrenschmidt wrote:
> > On Wed, 2012-12-05 at 10:31 +0800, Yuanquan Chen wrote:
> >> On powerpc arch, some fixup work of PCI/PCI-e device is just done during the
> >> first scan at booting time. For the PCI/PCI-e device rescanned after linux OS
> >> booting up, the fixup work won't be done, which leads to dma_set_mask error or
> >> irq related issue in rescanned PCI/PCI-e device's driver. So, it does the same
> >> fixup work for the rescanned device to avoid this issue.
> > Hrm, the patch is a bit gross. First the code shouldn't be copy/pasted
> > that way but factored out.
Please, at least format your email properly so I can try to undertand
without needing aspirin.
> There's a judgement "if (!bus->is_added)" before calling of
> pcibios_fixup_bus in pci_scan_child_bus, so for the rescanned device,
> the fixup won't execute, which leads to fatal error in driver of rescanned
> device on freescale powerpc, no this issues on x86 arch.
First, none of that invalidates my statement that you shouldn't
duplicate a whole block of code like this. Even if your approach is
correct (which is debated separately), at the very least you should
factor the code out into a common function between the two copies.
> Remove the judgement, let it to do the pcibios_fixup_bus
> directly, the error won't occur for the rescanned device. But it's
> general code, not proper to change here, so copy the pcibios_fixup_bus
> work to pcibios_enable_device.
>
> > I'm surprised also that is_added is false when pcibios_enable_device()
> > gets called ... that looks strange to me. At what point is that enable
> > happening in the hotplug sequence ?
> All devices are rescanned and then call the pci_enable_devices and
> pci_bus_add_devices.
Where ? How ? What is the sequence happening ? In any case, I think if
we need a proper fixup done per-device like that after scan we ought to
create a new hook at the generic level rather than that sort of hack.
> The patch code will be called by pci_enable_devices. The "dev->is_added"
> is set in pci_bus_add_device
> which is called by pci_bus_add_devices. So "dev->is_added" is false when
> checking it in pcibios_enable_device
> for the rescanned device.
Who calls pci_enable_device() in the rescan case ? Why isn't it left to
the driver ? I don't think we can rely on that behaviour not to change.
> > How do you trigger the rescan anyway ?
> Use the interface under /sys :
> echo 1 > /sys/bus/pci/devices/xxx/remove
>
> then echo 1 to the pci device which is the bus of the removed device
> echo 1 > /sys/bus/pci/devices/xxxx/rescan
> the removed device will be scanned and it's driver module will be loaded
> automatically.
Yeah this code path are known to be fishy. I think the problem is at the
generic abstraction level and that's where it needs to be fixed.
Cheers,
Ben.
> Regards,
> yuanquan
> > I think the problem needs to be solve at a higher level, I'm adding
> > linux-pci & Bjorn to the CC list.
> >
> > Cheers,
> > Ben.
> >
> >> Signed-off-by: Yuanquan Chen <B41889@freescale•com>
> >> ---
> >> arch/powerpc/kernel/pci-common.c | 20 ++++++++++++++++++++
> >> 1 file changed, 20 insertions(+)
> >>
> >> diff --git a/arch/powerpc/kernel/pci-common.c b/arch/powerpc/kernel/pci-common.c
> >> index 7f94f76..f0fb070 100644
> >> --- a/arch/powerpc/kernel/pci-common.c
> >> +++ b/arch/powerpc/kernel/pci-common.c
> >> @@ -1496,6 +1496,26 @@ int pcibios_enable_device(struct pci_dev *dev, int mask)
> >> if (ppc_md.pcibios_enable_device_hook(dev))
> >> return -EINVAL;
> >>
> >> + if (!dev->is_added) {
> >> + /*
> >> + * Fixup NUMA node as it may not be setup yet by the generic
> >> + * code and is needed by the DMA init
> >> + */
> >> + set_dev_node(&dev->dev, pcibus_to_node(dev->bus));
> >> +
> >> + /* Hook up default DMA ops */
> >> + set_dma_ops(&dev->dev, pci_dma_ops);
> >> + set_dma_offset(&dev->dev, PCI_DRAM_OFFSET);
> >> +
> >> + /* Additional platform DMA/iommu setup */
> >> + if (ppc_md.pci_dma_dev_setup)
> >> + ppc_md.pci_dma_dev_setup(dev);
> >> +
> >> + /* Read default IRQs and fixup if necessary */
> >> + pci_read_irq_line(dev);
> >> + if (ppc_md.pci_irq_fixup)
> >> + ppc_md.pci_irq_fixup(dev);
> >> + }
> >> return pci_enable_resources(dev, mask);
> >> }
> >>
> >
> >
> >
> >
>
next prev parent reply other threads:[~2012-12-05 8:26 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2012-12-05 2:31 [linuxppc-release][PATCH] powerpc/pci-hotplug: fix init issue of rescanned pci device Yuanquan Chen
2012-12-05 7:17 ` Benjamin Herrenschmidt
2012-12-05 8:20 ` Chen Yuanquan-B41889
2012-12-05 8:26 ` Benjamin Herrenschmidt [this message]
2012-12-05 9:29 ` Chen Yuanquan-B41889
2012-12-05 21:30 ` Bjorn Helgaas
2012-12-06 11:23 ` Chen Yuanquan-B41889
2012-12-07 21:15 ` Bjorn Helgaas
2013-04-01 10:29 ` Chen Yuanquan-B41889
2013-04-01 16:29 ` Bjorn Helgaas
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=1354695989.2351.13.camel@pasglop \
--to=benh@kernel$(echo .)crashing.org \
--cc=B41889@freescale$(echo .)com \
--cc=bhelgaas@google$(echo .)com \
--cc=linux-pci@vger$(echo .)kernel.org \
--cc=linuxppc-dev@lists$(echo .)ozlabs.org \
--cc=r61911@freescale$(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