| Message ID | 20131119013343.GA17294@google.com |
|---|---|
| State | Not Applicable |
| Headers | show |
> The pcie_portdrv .probe() method calls pci_enable_device() once, in > pcie_port_device_register(), but the .remove() method calls > pci_disable_device() twice, in pcie_port_device_remove() and in > pcie_portdrv_remove(). > > That causes a "disabling already-disabled device" warning when removing a > PCIe port device. This happens all the time when removing Thunderbolt > devices, but is also easy to reproduce with, e.g., > "echo 0000:00:1c.3 > /sys/bus/pci/drivers/pcieport/unbind" > > This patch removes the disable from pcie_portdrv_remove(). > > [bhelgaas: changelog, tag for stable] > Reported-by: David Bulkow <David.Bulkow@stratus.com> > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > CC: stable@vger.kernel.org # v2.6.32+ Hi Bjorn, This issue in X86 seems to be introduced after commit 928bea9 "PCI: Delay enabling bridges until they're needed" So this patch needs to back port to 2.6.32+ ? > --- > drivers/pci/pcie/portdrv_pci.c | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c > index cd1e57e51aa7..0d8fdc48e642 100644 > --- a/drivers/pci/pcie/portdrv_pci.c > +++ b/drivers/pci/pcie/portdrv_pci.c > @@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev, > static void pcie_portdrv_remove(struct pci_dev *dev) > { > pcie_port_device_remove(dev); > - pci_disable_device(dev); > } > > static int error_detected_iter(struct device *device, void *data) > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ > > . >
On Mon, Nov 18, 2013 at 06:33:43PM -0700, Bjorn Helgaas wrote: > I fixed the changelog (the extra disable was actually added by d899871936, > not by dc5351784e) and put the patch below in my for-linus branch. I'll > ask Linus to pull it later this week. > > Sorry for the delay, and thanks for the reminder. Thanks! -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Nov 19, 2013 at 09:54:58AM +0800, Yijing Wang wrote: > > The pcie_portdrv .probe() method calls pci_enable_device() once, in > > pcie_port_device_register(), but the .remove() method calls > > pci_disable_device() twice, in pcie_port_device_remove() and in > > pcie_portdrv_remove(). > > > > That causes a "disabling already-disabled device" warning when removing a > > PCIe port device. This happens all the time when removing Thunderbolt > > devices, but is also easy to reproduce with, e.g., > > "echo 0000:00:1c.3 > /sys/bus/pci/drivers/pcieport/unbind" > > > > This patch removes the disable from pcie_portdrv_remove(). > > > > [bhelgaas: changelog, tag for stable] > > Reported-by: David Bulkow <David.Bulkow@stratus.com> > > Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> > > CC: stable@vger.kernel.org # v2.6.32+ > > Hi Bjorn, > This issue in X86 seems to be introduced after commit 928bea9 "PCI: Delay enabling bridges until they're needed" > So this patch needs to back port to 2.6.32+ ? 928bea9 might have made it more visible, but the underlying problem is that we enable the device once in the probe path, and disable it twice in the remove path. That problem exists in 2.6.32.61: pcie_portdrv_probe # .probe() method pcie_port_device_register pci_enable_device <-- enable pcie_portdrv_remove # .remove() method pcie_port_device_remove pci_disable_device <-- disable #1 pci_disable_device <-- disable #2 Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>> [bhelgaas: changelog, tag for stable] >>> Reported-by: David Bulkow <David.Bulkow@stratus.com> >>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> >>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>> CC: stable@vger.kernel.org # v2.6.32+ >> >> Hi Bjorn, >> This issue in X86 seems to be introduced after commit 928bea9 "PCI: Delay enabling bridges until they're needed" >> So this patch needs to back port to 2.6.32+ ? > > 928bea9 might have made it more visible, but the underlying problem is that > we enable the device once in the probe path, and disable it twice in the > remove path. That problem exists in 2.6.32.61: > > pcie_portdrv_probe # .probe() method > pcie_port_device_register > pci_enable_device <-- enable > > pcie_portdrv_remove # .remove() method > pcie_port_device_remove > pci_disable_device <-- disable #1 > pci_disable_device <-- disable #2 During assign unassigned resources, we also enable the port device, fs_initcall(pcibios_assign_resources); pci_assign_unassigned_resources; pci_enable_bridges() pci_enable_device() So I think before the commit 928bea9 , the pci bridge device enable and disable is symmetrical. After the commit 928bea9, we only enable bridge once, but still remove twice. Thanks! Yijing.
On Tue, Nov 19, 2013 at 6:14 PM, Yijing Wang <wangyijing@huawei.com> wrote: >>>> [bhelgaas: changelog, tag for stable] >>>> Reported-by: David Bulkow <David.Bulkow@stratus.com> >>>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> >>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>>> CC: stable@vger.kernel.org # v2.6.32+ >>> >>> Hi Bjorn, >>> This issue in X86 seems to be introduced after commit 928bea9 "PCI: Delay enabling bridges until they're needed" >>> So this patch needs to back port to 2.6.32+ ? >> >> 928bea9 might have made it more visible, but the underlying problem is that >> we enable the device once in the probe path, and disable it twice in the >> remove path. That problem exists in 2.6.32.61: >> >> pcie_portdrv_probe # .probe() method >> pcie_port_device_register >> pci_enable_device <-- enable >> >> pcie_portdrv_remove # .remove() method >> pcie_port_device_remove >> pci_disable_device <-- disable #1 >> pci_disable_device <-- disable #2 > > During assign unassigned resources, we also enable the port device, > > fs_initcall(pcibios_assign_resources); > pci_assign_unassigned_resources; > pci_enable_bridges() > pci_enable_device() > > > So I think before the commit 928bea9 , the pci bridge device enable and disable is symmetrical. > After the commit 928bea9, we only enable bridge once, but still remove twice. The port driver should be symmetrical, regardless of what happens outside it. We have to be able to bind/unbind/bind/unbind indefinitely. Do you think the patch is a problem for current upstream, or are you just saying it doesn't need to be backported as far as 2.6.32? I frankly don't care that much if those old kernels pick it up or not. All I'm saying is that the problem this fixes is present that far back. I'm not really interested in doing a lot more digging about ancient kernels, unless you think it's going to break something if applied to them. Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2013/11/20 9:20, Bjorn Helgaas wrote: > On Tue, Nov 19, 2013 at 6:14 PM, Yijing Wang <wangyijing@huawei.com> wrote: >>>>> [bhelgaas: changelog, tag for stable] >>>>> Reported-by: David Bulkow <David.Bulkow@stratus.com> >>>>> Reported-by: Mika Westerberg <mika.westerberg@linux.intel.com> >>>>> Signed-off-by: Yinghai Lu <yinghai@kernel.org> >>>>> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com> >>>>> CC: stable@vger.kernel.org # v2.6.32+ >>>> >>>> Hi Bjorn, >>>> This issue in X86 seems to be introduced after commit 928bea9 "PCI: Delay enabling bridges until they're needed" >>>> So this patch needs to back port to 2.6.32+ ? >>> >>> 928bea9 might have made it more visible, but the underlying problem is that >>> we enable the device once in the probe path, and disable it twice in the >>> remove path. That problem exists in 2.6.32.61: >>> >>> pcie_portdrv_probe # .probe() method >>> pcie_port_device_register >>> pci_enable_device <-- enable >>> >>> pcie_portdrv_remove # .remove() method >>> pcie_port_device_remove >>> pci_disable_device <-- disable #1 >>> pci_disable_device <-- disable #2 >> >> During assign unassigned resources, we also enable the port device, >> >> fs_initcall(pcibios_assign_resources); >> pci_assign_unassigned_resources; >> pci_enable_bridges() >> pci_enable_device() >> >> >> So I think before the commit 928bea9 , the pci bridge device enable and disable is symmetrical. >> After the commit 928bea9, we only enable bridge once, but still remove twice. > > The port driver should be symmetrical, regardless of what happens > outside it. We have to be able to bind/unbind/bind/unbind > indefinitely. > > Do you think the patch is a problem for current upstream, or are you > just saying it doesn't need to be backported as far as 2.6.32? I > frankly don't care that much if those old kernels pick it up or not. > All I'm saying is that the problem this fixes is present that far > back. > > I'm not really interested in doing a lot more digging about ancient > kernels, unless you think it's going to break something if applied to > them. As you said, this is not a big problem, it's ok for current upstream, and for 3.4 3.5 3.10 stable tree, it seems just a enable and disable symmetry problem. I don't find anything unsafe about this even though when we unbind pcie port driver, the pcie port maybe still enable. I have no objection to backport it to ancient kernels. :) Thanks! Yijing. > > Bjorn > >
diff --git a/drivers/pci/pcie/portdrv_pci.c b/drivers/pci/pcie/portdrv_pci.c index cd1e57e51aa7..0d8fdc48e642 100644 --- a/drivers/pci/pcie/portdrv_pci.c +++ b/drivers/pci/pcie/portdrv_pci.c @@ -223,7 +223,6 @@ static int pcie_portdrv_probe(struct pci_dev *dev, static void pcie_portdrv_remove(struct pci_dev *dev) { pcie_port_device_remove(dev); - pci_disable_device(dev); } static int error_detected_iter(struct device *device, void *data)