| Message ID | 1380399187-4962-1-git-send-email-yinghai@kernel.org |
|---|---|
| State | Not Applicable |
| Headers | show |
On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: > Fixed by add checking in pci_enable_bridge, and call pci_set_master > if driver skip that. > That will make the code more robot and wade off problem for missing > pci_set_master in drivers. Petty spelling nit; feel free to ignore unless you need to respin the patch anyway.... s/robot/robost/ Cheers, - Ted -- 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 Sun, Sep 29, 2013 at 3:41 PM, Theodore Ts'o <tytso@mit.edu> wrote: > On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: >> Fixed by add checking in pci_enable_bridge, and call pci_set_master >> if driver skip that. >> That will make the code more robot and wade off problem for missing >> pci_set_master in drivers. > > Petty spelling nit; feel free to ignore unless you need to respin the > patch anyway.... > > s/robot/robost/ That's an oddly spelled nitpick "correction". If you really want to fix it, it's "robust" and "ward off problems". But it's too late now, and the wrong spelling and word choice is in the git tree and released in the -rc3 I just pushed out. Linus -- 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 Sun, Sep 29, 2013 at 03:46:33PM -0700, Linus Torvalds wrote: > On Sun, Sep 29, 2013 at 3:41 PM, Theodore Ts'o <tytso@mit.edu> wrote: > > On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: > >> Fixed by add checking in pci_enable_bridge, and call pci_set_master > >> if driver skip that. > >> That will make the code more robot and wade off problem for missing > >> pci_set_master in drivers. > > > > Petty spelling nit; feel free to ignore unless you need to respin the > > patch anyway.... > > > > s/robot/robost/ > > That's an oddly spelled nitpick "correction". Sorry, typed too quickly. :-( - Ted -- 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 Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: > BenH found: > | 928bea964827d7824b548c1f8e06eccbbc4d0d7d > | PCI: Delay enabling bridges until they're needed > > break PCI on powerpc. The reason is that the PCIe port driver will > call pci_enable_device() on the bridge, so device enabled (but skip > pci_set_master because pcie_port_auto and no acpi on powerpc ). > > Because of that, pci_enable_bridge() later on (called as a result of the > child device driver doing pci_enable_device) will see the bridge as > already enabled and will not call pci_set_master() on it. > > Fixed by add checking in pci_enable_bridge, and call pci_set_master > if driver skip that. > That will make the code more robot and wade off problem for missing > pci_set_master in drivers. > > Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> > Signed-off-by: Yinghai Lu <yinghai@kernel.org> > > --- > drivers/pci/pci.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > Index: linux-2.6/drivers/pci/pci.c > =================================================================== > --- linux-2.6.orig/drivers/pci/pci.c > +++ linux-2.6/drivers/pci/pci.c > @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci > > pci_enable_bridge(dev->bus->self); > > - if (pci_is_enabled(dev)) > + if (pci_is_enabled(dev)) { > + if (!dev->is_busmaster) { > + dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n"); I know this is already in Linus' tree, but if we're going to enable bus mastering here, what's the point of the warning? If somebody fixes the driver by adding a pci_set_master() call there, does that improve something? Bjorn > + pci_set_master(dev); > + } > return; > + } > + > retval = pci_enable_device(dev); > if (retval) > dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n", -- 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 Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: >> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci >> >> pci_enable_bridge(dev->bus->self); >> >> - if (pci_is_enabled(dev)) >> + if (pci_is_enabled(dev)) { >> + if (!dev->is_busmaster) { >> + dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n"); > > I know this is already in Linus' tree, but if we're going to enable > bus mastering here, what's the point of the warning? If somebody > fixes the driver by adding a pci_set_master() call there, does that > improve something? Help us to catch other offender and fix them. Yinghai -- 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 Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <yinghai@kernel.org> wrote: > On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: >> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: >>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci >>> >>> pci_enable_bridge(dev->bus->self); >>> >>> - if (pci_is_enabled(dev)) >>> + if (pci_is_enabled(dev)) { >>> + if (!dev->is_busmaster) { >>> + dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n"); >> >> I know this is already in Linus' tree, but if we're going to enable >> bus mastering here, what's the point of the warning? If somebody >> fixes the driver by adding a pci_set_master() call there, does that >> improve something? > > Help us to catch other offender and fix them. What is improved by doing it in the driver instead of here? -- 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 Fri, 2013-10-04 at 09:55 -0600, Bjorn Helgaas wrote: > On Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <yinghai@kernel.org> wrote: > > On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <bhelgaas@google.com> wrote: > >> On Sat, Sep 28, 2013 at 01:13:07PM -0700, Yinghai Lu wrote: > >>> @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci > >>> > >>> pci_enable_bridge(dev->bus->self); > >>> > >>> - if (pci_is_enabled(dev)) > >>> + if (pci_is_enabled(dev)) { > >>> + if (!dev->is_busmaster) { > >>> + dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n"); > >> > >> I know this is already in Linus' tree, but if we're going to enable > >> bus mastering here, what's the point of the warning? If somebody > >> fixes the driver by adding a pci_set_master() call there, does that > >> improve something? > > > > Help us to catch other offender and fix them. > > What is improved by doing it in the driver instead of here? After booting v3.12 for the first time on a laptop I noticed two new warnings: <4>[ 4.427959] pcieport 0000:00:1c.4: driver skip pci_set_master, fix it! <4>[ 4.448630] pcieport 0000:00:1c.1: driver skip pci_set_master, fix it! These warnings aren't entirely clear, but luckily they are easily greppable. It turns out they can be traced back to this patch. So some further grepping, looking at the code, etc. suggests these warnings could be silenced by calling pci_set_master() before calling pci_enable_device(). Ie, reverse the current order of those calls in drivers/pci/pcie/portdrv_core.c:pcie_port_device_register(). Is that correct? But what should then be done if pci_enable_device() fails? And Bjorn's question - what's the point of this warning if pci_set_master() will be called anyway - also came up when I looked at that code segment for the first time. But I'm not familiar with the PCI code. Paul Bolle -- 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
Index: linux-2.6/drivers/pci/pci.c =================================================================== --- linux-2.6.orig/drivers/pci/pci.c +++ linux-2.6/drivers/pci/pci.c @@ -1156,8 +1156,14 @@ static void pci_enable_bridge(struct pci pci_enable_bridge(dev->bus->self); - if (pci_is_enabled(dev)) + if (pci_is_enabled(dev)) { + if (!dev->is_busmaster) { + dev_warn(&dev->dev, "driver skip pci_set_master, fix it!\n"); + pci_set_master(dev); + } return; + } + retval = pci_enable_device(dev); if (retval) dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
BenH found: | 928bea964827d7824b548c1f8e06eccbbc4d0d7d | PCI: Delay enabling bridges until they're needed break PCI on powerpc. The reason is that the PCIe port driver will call pci_enable_device() on the bridge, so device enabled (but skip pci_set_master because pcie_port_auto and no acpi on powerpc ). Because of that, pci_enable_bridge() later on (called as a result of the child device driver doing pci_enable_device) will see the bridge as already enabled and will not call pci_set_master() on it. Fixed by add checking in pci_enable_bridge, and call pci_set_master if driver skip that. That will make the code more robot and wade off problem for missing pci_set_master in drivers. Reported-by: Benjamin Herrenschmidt <benh@kernel.crashing.org> Signed-off-by: Yinghai Lu <yinghai@kernel.org> --- drivers/pci/pci.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) -- 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