2007-12-28 13:11:50

by Bahadir Balban

[permalink] [raw]
Subject: 2.6.24-rc5 sysfs pci bridge duplicate symlink

Hi,

On ARM with PCI, I get this error since -rc2 (didn't try rc1):

sysfs: duplicate filename 'bridge' can not be created
WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
[<c00287fc>] (dump_stack+0x0/0x14) from [<c00cbc64>] (sysfs_add_one+0x50/0xf4)
[<c00cbc14>] (sysfs_add_one+0x0/0xf4) from [<c00ccd30>]
(sysfs_create_link+0xec/0x184)
r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
[<c00ccc44>] (sysfs_create_link+0x0/0x184) from [<c0160b44>]
(pci_bus_add_devices+0xf0/0x144)
r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
[<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
(pci_bus_add_devices+0xdc/0x144)
r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
[<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
(pci_bus_add_devices+0xdc/0x144)
r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
[<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c000a934>]
(pci_common_init+0x148/0x184)
r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0


Any idea why?

Thanks,
Bahadir


2007-12-28 23:04:55

by Andrew Morton

[permalink] [raw]
Subject: Re: 2.6.24-rc5 sysfs pci bridge duplicate symlink

On Fri, 28 Dec 2007 13:11:37 +0000 "Bahadir Balban" <[email protected]> wrote:

> Hi,
>
> On ARM with PCI, I get this error since -rc2 (didn't try rc1):
>
> sysfs: duplicate filename 'bridge' can not be created
> WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> [<c00287fc>] (dump_stack+0x0/0x14) from [<c00cbc64>] (sysfs_add_one+0x50/0xf4)
> [<c00cbc14>] (sysfs_add_one+0x0/0xf4) from [<c00ccd30>]
> (sysfs_create_link+0xec/0x184)
> r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
> [<c00ccc44>] (sysfs_create_link+0x0/0x184) from [<c0160b44>]
> (pci_bus_add_devices+0xf0/0x144)
> r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
> [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> (pci_bus_add_devices+0xdc/0x144)
> r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
> [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> (pci_bus_add_devices+0xdc/0x144)
> r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
> [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c000a934>]
> (pci_common_init+0x148/0x184)
> r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0
>
>
> Any idea why?
>

(suitable cc added)

2007-12-28 23:51:25

by Russell King - ARM Linux

[permalink] [raw]
Subject: Re: 2.6.24-rc5 sysfs pci bridge duplicate symlink

On Fri, Dec 28, 2007 at 03:03:22PM -0800, Andrew Morton wrote:
> On Fri, 28 Dec 2007 13:11:37 +0000 "Bahadir Balban" <[email protected]> wrote:
>
> > Hi,
> >
> > On ARM with PCI, I get this error since -rc2 (didn't try rc1):
> >
> > sysfs: duplicate filename 'bridge' can not be created
> > WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> > [<c00287fc>] (dump_stack+0x0/0x14) from [<c00cbc64>] (sysfs_add_one+0x50/0xf4)
> > [<c00cbc14>] (sysfs_add_one+0x0/0xf4) from [<c00ccd30>]
> > (sysfs_create_link+0xec/0x184)
> > r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
> > [<c00ccc44>] (sysfs_create_link+0x0/0x184) from [<c0160b44>]
> > (pci_bus_add_devices+0xf0/0x144)
> > r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
> > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > (pci_bus_add_devices+0xdc/0x144)
> > r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
> > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > (pci_bus_add_devices+0xdc/0x144)
> > r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
> > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c000a934>]
> > (pci_common_init+0x148/0x184)
> > r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0
> >
> >
> > Any idea why?
> >
>
> (suitable cc added)

Looks to me as if the code added to pci_bus_add_devices() is off it's
rocker.

In brief, pci_bus_add_devices() is supposed to be callable given *any*
state of the bus, and it will add any new devices found in the bus tree
from the bus that it's called for downwards. (That's how I designed
that bit of PCI code - so it can cope with hotpluggable PCI bridges.)

It still does this. However, some bright spark decided to add sysfs
links to it - and break it. Now, whenever we descend into a subordinate
bus (and return to a higher level) we will try to create a sysfs symlink
even if the bus is one that we already registered.

So... that sysfs_create_link() call in there should not be unconditional.
It should _only_ ever do that call if list_empty(&dev->subordinate->node)
was true. IOW, we only create symlinks for subordinate bus that we don't
already know about.

It looks like it was introduced by Greg's "PCI: fix __must_check warnings"
but why a patch advertised _solely_ as a fix is adding additional sysfs
attributes I've no idea.

Something like the following should fix it. Bahadir - please test.

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 9e5ea07..132a91f 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -131,18 +131,21 @@ void pci_bus_add_devices(struct pci_bus *bus)
* it and then scan for unattached PCI devices.
*/
if (dev->subordinate) {
- if (list_empty(&dev->subordinate->node)) {
+ int new_bus = list_empty(&dev->subordinate->node);
+ if (new_bus) {
down_write(&pci_bus_sem);
list_add_tail(&dev->subordinate->node,
&dev->bus->children);
up_write(&pci_bus_sem);
}
pci_bus_add_devices(dev->subordinate);
- retval = sysfs_create_link(&dev->subordinate->class_dev.kobj,
- &dev->dev.kobj, "bridge");
- if (retval)
- dev_err(&dev->dev, "Error creating sysfs "
- "bridge symlink, continuing...\n");
+ if (new_bus) {
+ retval = sysfs_create_link(&dev->subordinate->class_dev.kobj,
+ &dev->dev.kobj, "bridge");
+ if (retval)
+ dev_err(&dev->dev, "Error creating sysfs "
+ "bridge symlink, continuing...\n");
+ }
}
}
}

2008-01-10 03:51:31

by Greg KH

[permalink] [raw]
Subject: Re: 2.6.24-rc5 sysfs pci bridge duplicate symlink

On Fri, Dec 28, 2007 at 11:50:49PM +0000, Russell King - ARM Linux wrote:
> On Fri, Dec 28, 2007 at 03:03:22PM -0800, Andrew Morton wrote:
> > On Fri, 28 Dec 2007 13:11:37 +0000 "Bahadir Balban" <[email protected]> wrote:
> >
> > > Hi,
> > >
> > > On ARM with PCI, I get this error since -rc2 (didn't try rc1):
> > >
> > > sysfs: duplicate filename 'bridge' can not be created
> > > WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> > > [<c00287fc>] (dump_stack+0x0/0x14) from [<c00cbc64>] (sysfs_add_one+0x50/0xf4)
> > > [<c00cbc14>] (sysfs_add_one+0x0/0xf4) from [<c00ccd30>]
> > > (sysfs_create_link+0xec/0x184)
> > > r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
> > > [<c00ccc44>] (sysfs_create_link+0x0/0x184) from [<c0160b44>]
> > > (pci_bus_add_devices+0xf0/0x144)
> > > r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
> > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > > (pci_bus_add_devices+0xdc/0x144)
> > > r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
> > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > > (pci_bus_add_devices+0xdc/0x144)
> > > r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
> > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c000a934>]
> > > (pci_common_init+0x148/0x184)
> > > r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0
> > >
> > >
> > > Any idea why?
> > >
> >
> > (suitable cc added)
>
> Looks to me as if the code added to pci_bus_add_devices() is off it's
> rocker.
>
> In brief, pci_bus_add_devices() is supposed to be callable given *any*
> state of the bus, and it will add any new devices found in the bus tree
> from the bus that it's called for downwards. (That's how I designed
> that bit of PCI code - so it can cope with hotpluggable PCI bridges.)
>
> It still does this. However, some bright spark decided to add sysfs
> links to it - and break it. Now, whenever we descend into a subordinate
> bus (and return to a higher level) we will try to create a sysfs symlink
> even if the bus is one that we already registered.
>
> So... that sysfs_create_link() call in there should not be unconditional.
> It should _only_ ever do that call if list_empty(&dev->subordinate->node)
> was true. IOW, we only create symlinks for subordinate bus that we don't
> already know about.
>
> It looks like it was introduced by Greg's "PCI: fix __must_check warnings"
> but why a patch advertised _solely_ as a fix is adding additional sysfs
> attributes I've no idea.

I'm sorry, but I don't see where that patch added new sysfs attributes
to anything. It just reports any errors that might have occured and
hopefully properly handle them. Did I mess up somewhere with this?

> Something like the following should fix it. Bahadir - please test.

Any test results?

thanks,

greg k-h

2008-01-16 19:02:43

by Bahadir Balban

[permalink] [raw]
Subject: Re: 2.6.24-rc5 sysfs pci bridge duplicate symlink

On 1/10/08, Greg KH <[email protected]> wrote:
> On Fri, Dec 28, 2007 at 11:50:49PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Dec 28, 2007 at 03:03:22PM -0800, Andrew Morton wrote:
> > > On Fri, 28 Dec 2007 13:11:37 +0000 "Bahadir Balban" <[email protected]> wrote:
> > >
> > > > Hi,
> > > >
> > > > On ARM with PCI, I get this error since -rc2 (didn't try rc1):
> > > >
> > > > sysfs: duplicate filename 'bridge' can not be created
> > > > WARNING: at fs/sysfs/dir.c:424 sysfs_add_one()
> > > > [<c00287fc>] (dump_stack+0x0/0x14) from [<c00cbc64>] (sysfs_add_one+0x50/0xf4)
> > > > [<c00cbc14>] (sysfs_add_one+0x0/0xf4) from [<c00ccd30>]
> > > > (sysfs_create_link+0xec/0x184)
> > > > r6:cfca8b48 r5:cfca96c8 r4:cfc2fec0
> > > > [<c00ccc44>] (sysfs_create_link+0x0/0x184) from [<c0160b44>]
> > > > (pci_bus_add_devices+0xf0/0x144)
> > > > r7:cfc5a2d4 r6:cfc5a2d4 r5:cfc5a2c0 r4:cfcad800
> > > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > > > (pci_bus_add_devices+0xdc/0x144)
> > > > r7:cfc5a3d4 r6:cfc5a3d4 r5:cfc5a3c0 r4:cfcadc00
> > > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c0160b30>]
> > > > (pci_bus_add_devices+0xdc/0x144)
> > > > r7:cfc5a4d4 r6:cfc5a4d4 r5:cfc5a4c0 r4:cfc5d400
> > > > [<c0160a54>] (pci_bus_add_devices+0x0/0x144) from [<c000a934>]
> > > > (pci_common_init+0x148/0x184)
> > > > r7:c035fd58 r6:cfc3e760 r5:c001e030 r4:cfc5a4c0
> > > >
> > > >
> > > > Any idea why?
> > > >
> > >
> > > (suitable cc added)
> >
> > Looks to me as if the code added to pci_bus_add_devices() is off it's
> > rocker.
> >
> > In brief, pci_bus_add_devices() is supposed to be callable given *any*
> > state of the bus, and it will add any new devices found in the bus tree
> > from the bus that it's called for downwards. (That's how I designed
> > that bit of PCI code - so it can cope with hotpluggable PCI bridges.)
> >
> > It still does this. However, some bright spark decided to add sysfs
> > links to it - and break it. Now, whenever we descend into a subordinate
> > bus (and return to a higher level) we will try to create a sysfs symlink
> > even if the bus is one that we already registered.
> >
> > So... that sysfs_create_link() call in there should not be unconditional.
> > It should _only_ ever do that call if list_empty(&dev->subordinate->node)
> > was true. IOW, we only create symlinks for subordinate bus that we don't
> > already know about.
> >
> > It looks like it was introduced by Greg's "PCI: fix __must_check warnings"
> > but why a patch advertised _solely_ as a fix is adding additional sysfs
> > attributes I've no idea.
>
> I'm sorry, but I don't see where that patch added new sysfs attributes
> to anything. It just reports any errors that might have occured and
> hopefully properly handle them. Did I mess up somewhere with this?
>
> > Something like the following should fix it. Bahadir - please test.
>
> Any test results?
>
> thanks,
>
> greg k-h
>

Hi,

I have tested and it works OK with this patch on -rc8. Without the
patch it breaks as in my original email.

While at it, could you please apply the below patch as well? It
enables PCI transactions through bridges on ARM.

Thanks,
Bahadir

From: Bahadir Balban <[email protected]>


---

arch/arm/kernel/bios32.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index e574754..7d3ebd4 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -607,6 +607,11 @@ void __init pci_common_init(struct hw_pci *hw)
* Assign resources.
*/
pci_bus_assign_resources(bus);
+
+ /*
+ * Enable bridges
+ */
+ pci_enable_bridges(bus);
}

/*