Hi Linus, Yinghai !
Please consider reverting:
928bea964827d7824b548c1f8e06eccbbc4d0d7d
PCI: Delay enabling bridges until they're needed
(I'd suggest to revert now and maybe merge a better patch later)
This breaks PCI on the PowerPC "powernv" platform (which is booted via
kexec) and probably x86 as well under similar circumstances. It will
basically break PCIe if the bus master bit of the bridge isn't set at
boot (by the firmware for example, or because kexec'ing cleared it).
The reason is that the PCIe port driver will call pci_enable_device() on
the bridge (on everything under the sun actually), which will marked the
device enabled (but will not do a set_master).
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.
Now, this could probably be fixed by simply doing pci_set_master() in
the PCIe port driver, but I find the whole logic very fragile (anything
that "enables" the bridge without setting master or for some reason
clears master will forever fail to re-enable it).
Maybe a better option is to unconditionally do pci_set_mater() in
pci_enable_bridge(), ie, move the call to before the enabled test.
However I am not too happy with that either. My experience with bridges
is that if bus master isn't set, they will also fail to report AER
errors and other similar upstream transactions. We might want to get
these reported properly even if no downstream device got successfully
enabled.
So I think the premises of the patches are flawed, at least on PCI
express, and we should stick to always enabling bridges (at least the
bus master bit on them).
Cheers,
Ben.
On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
<[email protected]> wrote:
> Hi Linus, Yinghai !
>
> Please consider reverting:
>
> 928bea964827d7824b548c1f8e06eccbbc4d0d7d
> PCI: Delay enabling bridges until they're needed
>
> (I'd suggest to revert now and maybe merge a better patch later)
>
> This breaks PCI on the PowerPC "powernv" platform (which is booted via
> kexec) and probably x86 as well under similar circumstances. It will
> basically break PCIe if the bus master bit of the bridge isn't set at
> boot (by the firmware for example, or because kexec'ing cleared it).
>
> The reason is that the PCIe port driver will call pci_enable_device() on
> the bridge (on everything under the sun actually), which will marked the
> device enabled (but will not do a set_master).
>
> 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.
>
> Now, this could probably be fixed by simply doing pci_set_master() in
> the PCIe port driver, but I find the whole logic very fragile (anything
> that "enables" the bridge without setting master or for some reason
> clears master will forever fail to re-enable it).
>
> Maybe a better option is to unconditionally do pci_set_mater() in
> pci_enable_bridge(), ie, move the call to before the enabled test.
>
> However I am not too happy with that either. My experience with bridges
> is that if bus master isn't set, they will also fail to report AER
> errors and other similar upstream transactions. We might want to get
> these reported properly even if no downstream device got successfully
> enabled.
>
> So I think the premises of the patches are flawed, at least on PCI
> express, and we should stick to always enabling bridges (at least the
> bus master bit on them).
there is pci_set_master everywhere in pci drivers.
So i would like to use the first way that you suggest : call pci_set_master
PCIe port driver.
Thanks
Yinghai
On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <[email protected]> wrote:
>
> So i would like to use the first way that you suggest : call pci_set_master
> PCIe port driver.
So I have to say, that if we can fix this with just adding a single
new pci_set_master() call, we should do that before we decide to
revert.
If other, bigger issues then come up, we can decide to revert. But if
there's a one-liner fix, let's just do that first, ok?
Mind sending a patch?
Linus
On Fri, Sep 27, 2013 at 9:01 AM, Yinghai Lu <[email protected]> wrote:
> On Fri, Sep 27, 2013 at 1:28 AM, Benjamin Herrenschmidt
> <[email protected]> wrote:
>> Hi Linus, Yinghai !
>>
>> Please consider reverting:
>>
>> 928bea964827d7824b548c1f8e06eccbbc4d0d7d
>> PCI: Delay enabling bridges until they're needed
>>
>> (I'd suggest to revert now and maybe merge a better patch later)
>>
>> This breaks PCI on the PowerPC "powernv" platform (which is booted via
>> kexec) and probably x86 as well under similar circumstances. It will
>> basically break PCIe if the bus master bit of the bridge isn't set at
>> boot (by the firmware for example, or because kexec'ing cleared it).
>>
>> The reason is that the PCIe port driver will call pci_enable_device() on
>> the bridge (on everything under the sun actually), which will marked the
>> device enabled (but will not do a set_master).
>>
>> 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.
Ben,
looks like PCIe port driver does call pci_enable_device AND pci_set_master()
| int pcie_port_device_register(struct pci_dev *dev)
| {
| int status, capabilities, i, nr_service;
| int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
| /* Enable PCI Express port device */
| status = pci_enable_device(dev);
| if (status)
| return status;
|
| /* Get and check PCI Express port services */
| capabilities = get_port_device_capability(dev);
| if (!capabilities)
| return 0;
|
| pci_set_master(dev);
so how come that pci_set_master is not called for powerpc?
Can you send out lspci -vvxxx with current linus-tree and v3.11?
Yinghai
On Fri, 2013-09-27 at 10:10 -0700, Linus Torvalds wrote:
> > So i would like to use the first way that you suggest : call pci_set_master
> > PCIe port driver.
>
> So I have to say, that if we can fix this with just adding a single
> new pci_set_master() call, we should do that before we decide to
> revert.
>
> If other, bigger issues then come up, we can decide to revert. But if
> there's a one-liner fix, let's just do that first, ok?
>
> Mind sending a patch?
Wouldn't it be better to simply have pci_enable_device() always set bus
master on a bridge? I don't see any case where it makes sense to have
an enabled bridge without the master bit set on it...
Cheers,
Ben.
On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> Wouldn't it be better to simply have pci_enable_device() always set bus
> master on a bridge? I don't see any case where it makes sense to have
> an enabled bridge without the master bit set on it...
Do you mean attached?
On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <[email protected]> wrote:
>
> > Wouldn't it be better to simply have pci_enable_device() always set bus
> > master on a bridge? I don't see any case where it makes sense to have
> > an enabled bridge without the master bit set on it...
>
> Do you mean attached?
That's an option. I was thinking making pci_enable_device() itself
enable bus master on a bridge but yes, you approach should work.
I'm digging a bit more to figure out what went wrong in the
pcie port driver since that's interesting in its own right and I'll then
test your patch which I think is a more robust approach.
Cheers,
Ben.
On Fri, 2013-09-27 at 10:44 -0700, Yinghai Lu wrote:
> | /* Get and check PCI Express port services */
> | capabilities = get_port_device_capability(dev);
> | if (!capabilities)
> | return 0;
> |
> | pci_set_master(dev);
>
> so how come that pci_set_master is not called for powerpc?
>
> Can you send out lspci -vvxxx with current linus-tree and v3.11?
Ah good point. It should have ... except that there are a number of ways
for get_port_device_capability() to fail and that should *not* leave the
bridge without the bus master capability set.
However I don't think that's what's happening here. I'll have to dig
more, will get back to you.
Cheers,
Ben.
On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
> <[email protected]> wrote:
>
> > Wouldn't it be better to simply have pci_enable_device() always set bus
> > master on a bridge? I don't see any case where it makes sense to have
> > an enabled bridge without the master bit set on it...
>
> Do you mean attached?
So this patch works and fixes the problem. I think it makes the whole
thing more robust and should be applied.
I still don't know why the bridge doesn't get enabled properly without
it yes, tracking it down (the machine in question takes a LONG time to
reboot :-)
Cheers,
Ben.
On Fri, Sep 27, 2013 at 3:38 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Fri, 2013-09-27 at 14:54 -0700, Yinghai Lu wrote:
>> On Fri, Sep 27, 2013 at 2:46 PM, Benjamin Herrenschmidt
>> <[email protected]> wrote:
>>
>> > Wouldn't it be better to simply have pci_enable_device() always set bus
>> > master on a bridge? I don't see any case where it makes sense to have
>> > an enabled bridge without the master bit set on it...
>>
>> Do you mean attached?
>
> So this patch works and fixes the problem. I think it makes the whole
> thing more robust and should be applied.
good.
>
> I still don't know why the bridge doesn't get enabled properly without
> it yes, tracking it down (the machine in question takes a LONG time to
> reboot :-)
ok, please if you are ok attached one instead. It will print some warning about
driver skipping pci_set_master, so we can catch more problem with drivers.
Thanks
Yinghai
On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:
> ok, please if you are ok attached one instead. It will print some warning about
> driver skipping pci_set_master, so we can catch more problem with drivers.
Except that the message is pretty cryptic :-) Especially since the
driver causing the message to be printed is not the one that did
the mistake in the first place, it's the next one coming up that
trips the warning.
In any case, the root cause is indeed the PCIe port driver:
We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
and pcie_ports_auto is true, so we end up with capabilities set to 0.
Thus the port driver bails out before calling pci_set_master(). The fix
is to call pci_set_master() unconditionally. However that lead me to
find to a few interesting oddities in that port driver code:
- If capabilities is 0, it returns after enabling the device and
doesn't disable it. But if it fails for any other reason later on (such
as failing to enable interrupts), it *will* disable the device. This is
inconsistent. In fact, if it had disabled the device as a result of the
0 capabilities, the problem would also not have happened (the subsequent
enable_bridge done by the e1000e driver would have done the right
thing). I've tested "fixing" that instead of moving the set_master call
and it fixes my problem as well.
- In get_port_device_capability(), all capabilities are gated by a
combination of the bit in cap_mask and a corresponding HW check of
the presence of the relevant PCIe capability or similar... except
for the VC one, which is solely read from the HW capability. That means
that the platform pcie_port_platform_notify() has no ability to prevent
the VC capability (so if I have a broken bridge that advertises it but
my platform wants to block it, it can't).
- I am quite nervous with the PCIe port driver disabling bridges. I
understand the intent but what if that bridge has some system device
behind it ? Something you don't even know about (used by ACPI, behind an
ISA bridge for example ?).
I think disabling bridges is a VERY risky proposition at all times
(including during PM) and I don't think the port driver should do it.
Maybe a more robust approach would be to detect one way or another that
the bridge was already enabled and only disable it if it wasn't or
something along those lines (ie ,restore it in the state it was)...
This is not my problem right now of course (in my case the bridge was
disabled to begin with) but I have a long experience with system stuff
hiding behind bridges and the code as it is written makes me a bit
nervous.
Cheers,
Ben.
[+ Rafael]
On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:
>
>> ok, please if you are ok attached one instead. It will print some warning about
>> driver skipping pci_set_master, so we can catch more problem with drivers.
>
> Except that the message is pretty cryptic :-) Especially since the
> driver causing the message to be printed is not the one that did
> the mistake in the first place, it's the next one coming up that
> trips the warning.
>
> In any case, the root cause is indeed the PCIe port driver:
>
> We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
> and pcie_ports_auto is true, so we end up with capabilities set to 0.
in
| commit fe31e69740eddc7316071ed5165fed6703c8cd12
| Author: Rafael J. Wysocki <[email protected]>
| Date: Sun Dec 19 15:57:16 2010 +0100
|
| PCI/PCIe: Clear Root PME Status bits early during system resume
|
| I noticed that PCI Express PMEs don't work on my Toshiba Portege R500
| after the system has been woken up from a sleep state by a PME
| (through Wake-on-LAN). After some investigation it turned out that
| the BIOS didn't clear the Root PME Status bit in the root port that
| received the wakeup PME and since the Requester ID was also set in
| the port's Root Status register, any subsequent PMEs didn't trigger
| interrupts.
|
| This problem can be avoided by clearing the Root PME Status bits in
| all PCI Express root ports during early resume. For this purpose,
| add an early resume routine to the PCIe port driver and make this
| driver be always registered, even if pci_ports_disable is set (in
| which case the driver's only function is to provide the early
| resume callback).
|
|
|@@ -349,15 +349,18 @@ int pcie_port_device_register(struct pci_dev *dev)
| int status, capabilities, i, nr_service;
| int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
|
|- /* Get and check PCI Express port services */
|- capabilities = get_port_device_capability(dev);
|- if (!capabilities)
|- return -ENODEV;
|-
| /* Enable PCI Express port device */
| status = pci_enable_device(dev);
| if (status)
| return status;
|+
|+ /* Get and check PCI Express port services */
|+ capabilities = get_port_device_capability(dev);
|+ if (!capabilities) {
|+ pcie_no_aspm();
|+ return 0;
|+ }
|+
| pci_set_master(dev);
| /*
| * Initialize service irqs. Don't use service devices that
>
> Thus the port driver bails out before calling pci_set_master(). The fix
> is to call pci_set_master() unconditionally. However that lead me to
> find to a few interesting oddities in that port driver code:
can we revert that partially change ? aka we should check get_port....
at first...
like attached.
Thanks
Yinghai
On Fri, 2013-09-27 at 16:44 -0700, Yinghai Lu wrote:
> > Thus the port driver bails out before calling pci_set_master(). The fix
> > is to call pci_set_master() unconditionally. However that lead me to
> > find to a few interesting oddities in that port driver code:
>
> can we revert that partially change ? aka we should check get_port....
> at first...
>
> like attached.
In the meantime, can you properly submit the other one with the warning
to Linus ? It will make things more robust overall...
Also, please read my other comments. I think we are treading on very
fragile ground with that whole business of potentially disabling bridges
in the pcieport driver ...
Cheers,
Ben.
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 <[email protected]>
Signed-off-by: Yinghai Lu <[email protected]>
---
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");
+ pci_set_master(dev);
+ }
return;
+ }
+
retval = pci_enable_device(dev);
if (retval)
dev_err(&dev->dev, "Error enabling bridge (%d), continuing\n",
On Fri, Sep 27, 2013 at 8:05 PM, Benjamin Herrenschmidt
<[email protected]> wrote:
> On Fri, 2013-09-27 at 16:44 -0700, Yinghai Lu wrote:
>
> In the meantime, can you properly submit the other one with the warning
> to Linus ? It will make things more robust overall...
https://patchwork.kernel.org/patch/2959121/
On Friday, September 27, 2013 04:44:20 PM Yinghai Lu wrote:
> [+ Rafael]
>
> On Fri, Sep 27, 2013 at 4:19 PM, Benjamin Herrenschmidt
> <[email protected]> wrote:
> > On Fri, 2013-09-27 at 15:56 -0700, Yinghai Lu wrote:
> >
> >> ok, please if you are ok attached one instead. It will print some warning about
> >> driver skipping pci_set_master, so we can catch more problem with drivers.
> >
> > Except that the message is pretty cryptic :-) Especially since the
> > driver causing the message to be printed is not the one that did
> > the mistake in the first place, it's the next one coming up that
> > trips the warning.
> >
> > In any case, the root cause is indeed the PCIe port driver:
> >
> > We don't have ACPI, so pcie_port_platform_notify() isn't implemented,
> > and pcie_ports_auto is true, so we end up with capabilities set to 0.
>
> in
> | commit fe31e69740eddc7316071ed5165fed6703c8cd12
> | Author: Rafael J. Wysocki <[email protected]>
> | Date: Sun Dec 19 15:57:16 2010 +0100
> |
> | PCI/PCIe: Clear Root PME Status bits early during system resume
> |
> | I noticed that PCI Express PMEs don't work on my Toshiba Portege R500
> | after the system has been woken up from a sleep state by a PME
> | (through Wake-on-LAN). After some investigation it turned out that
> | the BIOS didn't clear the Root PME Status bit in the root port that
> | received the wakeup PME and since the Requester ID was also set in
> | the port's Root Status register, any subsequent PMEs didn't trigger
> | interrupts.
> |
> | This problem can be avoided by clearing the Root PME Status bits in
> | all PCI Express root ports during early resume. For this purpose,
> | add an early resume routine to the PCIe port driver and make this
> | driver be always registered, even if pci_ports_disable is set (in
> | which case the driver's only function is to provide the early
> | resume callback).
> |
> |
> |@@ -349,15 +349,18 @@ int pcie_port_device_register(struct pci_dev *dev)
> | int status, capabilities, i, nr_service;
> | int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
> |
> |- /* Get and check PCI Express port services */
> |- capabilities = get_port_device_capability(dev);
> |- if (!capabilities)
> |- return -ENODEV;
> |-
> | /* Enable PCI Express port device */
> | status = pci_enable_device(dev);
> | if (status)
> | return status;
> |+
> |+ /* Get and check PCI Express port services */
> |+ capabilities = get_port_device_capability(dev);
> |+ if (!capabilities) {
> |+ pcie_no_aspm();
> |+ return 0;
> |+ }
> |+
> | pci_set_master(dev);
> | /*
> | * Initialize service irqs. Don't use service devices that
>
> >
> > Thus the port driver bails out before calling pci_set_master(). The fix
> > is to call pci_set_master() unconditionally. However that lead me to
> > find to a few interesting oddities in that port driver code:
>
> can we revert that partially change ? aka we should check get_port....
> at first...
>
> like attached.
It looks like we can do something like this (just pasting your patch):
diff --git a/drivers/pci/pcie/portdrv_core.c b/drivers/pci/pcie/portdrv_core.c
index 31063ac..1ee6f16 100644
--- a/drivers/pci/pcie/portdrv_core.c
+++ b/drivers/pci/pcie/portdrv_core.c
@@ -362,16 +362,16 @@ int pcie_port_device_register(struct pci_dev *dev)
int status, capabilities, i, nr_service;
int irqs[PCIE_PORT_DEVICE_MAXSERVICES];
- /* Enable PCI Express port device */
- status = pci_enable_device(dev);
- if (status)
- return status;
-
/* Get and check PCI Express port services */
capabilities = get_port_device_capability(dev);
if (!capabilities)
return 0;
+ /* Enable PCI Express port device */
+ status = pci_enable_device(dev);
+ if (status)
+ return status;
+
pci_set_master(dev);
/*
* Initialize service irqs. Don't use service devices that
but I don't have that box with me to test whether or not it still works
correctly after this change. I'll be back home on the next Saturday if
all goes well.
Thanks,
Rafael
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
On Sun, Sep 29, 2013 at 3:41 PM, Theodore Ts'o <[email protected]> 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
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 <[email protected]> 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
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 <[email protected]>
> Signed-off-by: Yinghai Lu <[email protected]>
>
> ---
> 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",
On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <[email protected]> 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
On Thu, Oct 3, 2013 at 5:35 PM, Yinghai Lu <[email protected]> wrote:
> On Thu, Oct 3, 2013 at 3:06 PM, Bjorn Helgaas <[email protected]> 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?