Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

[Bjorn Helgaas]

On Wed, Apr 17, 2013 at 5:06 AM, Artem Savkov <artem.savkov@gmail.com> wrote:
> Introduced in cc8f7f9e4e79a0940af6b4b6fdfbcf18a03aa9f4
> ("PCI: Fix __must_check annotation on pci_create_sysfs_dev_files()")
>
> pci_bus_add_device() should not fail to add device if
> pci_create_sysfs_dev_files() call fails, as sysfs files are not critical.
>
> pci_sysfs_init() is a "late_initcall" and is called after acpi_init, so
> pci_bus_add_device() fails to create devices during boot resulting in a
> bug during acpi_init. It is safe not to create sysfs_dev_files in
> pci_bus_add_device() as they are later created in pci_sysfs_ini().
>
> [    0.316603] ------------[ cut here ]------------
> [    0.317000] kernel BUG at drivers/pci/bus.c:210!
> [    0.317000] invalid opcode: 0000 [#1] SMP
> [    0.317000] Modules linked in:
> [    0.317000] Pid: 1, comm: swapper/0 Not tainted 3.9.0-rc2+ #357 Bochs Bochs
> [    0.317000] EIP: 0060:[<c12d2d7e>] EFLAGS: 00010246 CPU: 0
> [    0.317000] EIP is at pci_bus_add_devices+0x7e/0x80
> [    0.317000] EAX: fffffff3 EBX: df207000 ECX: 00000000 EDX: 00000001
> [    0.317000] ESI: df228000 EDI: df228014 EBP: df0a1dec ESP: df0a1de0
> [    0.317000]  DS: 007b ES: 007b FS: 00d8 GS: 0000 SS: 0068
> [    0.317000] CR0: 8005003b CR2: ffe16000 CR3: 01c32000 CR4: 00000690
> [    0.317000] DR0: 00000000 DR1: 00000000 DR2: 00000000 DR3: 00000000
> [    0.317000] DR6: ffff0ff0 DR7: 00000400
> [    0.317000] Process swapper/0 (pid: 1, ti=df0a0000 task=df098000 task.ti=df0a0000)
> [    0.317000] Stack:
> [    0.317000]  df1702c0 c19f2294 00000000 df0a1e2c c13102d9 c18fec3e c18febce 00000008
> [    0.317000]  00000000 df1702d4 df056044 00000008 00000000 00000000 c130d023 00000006
> [    0.317000]  c19f22fc c173ec9c df17c590 df0a1e5c c130d0f4 df056000 df05603c df02f9c0
> [    0.317000] Call Trace:
> [    0.317000]  [<c13102d9>] acpi_pci_root_add+0x3af/0x3ea
> [    0.317000]  [<c130d023>] ? acpi_bus_type_and_status+0x3d/0x88
> [    0.317000]  [<c130d0f4>] acpi_bus_device_attach+0x86/0xe4
> [    0.317000]  [<c1334d05>] acpi_ns_walk_namespace+0xf5/0x240
> [    0.317000]  [<c1335567>] acpi_walk_namespace+0xdd/0x11c
> [    0.317000]  [<c130d06e>] ? acpi_bus_type_and_status+0x88/0x88
> [    0.317000]  [<c130e021>] acpi_bus_scan+0x95/0xa8
> [    0.317000]  [<c130d06e>] ? acpi_bus_type_and_status+0x88/0x88
> [    0.317000]  [<c1a5dae6>] acpi_scan_init+0x59/0x152
> [    0.317000]  [<c1a5d90d>] acpi_init+0x243/0x28b
> [    0.317000]  [<c1a5d6ca>] ? acpi_sleep_proc_init+0x2e/0x2e
> [    0.317000]  [<c10011f2>] do_one_initcall+0x102/0x150
> [    0.317000]  [<c1a36b40>] kernel_init_freeable+0x135/0x1c7
> [    0.317000]  [<c1a364c8>] ? do_early_param+0x78/0x78
> [    0.317000]  [<c16daae0>] kernel_init+0x10/0x140
> [    0.317000]  [<c16fa4b7>] ret_from_kernel_thread+0x1b/0x28
> [    0.317000]  [<c16daad0>] ? rest_init+0xd0/0xd0
> [    0.317000] Code: 0c 85 f6 74 1b 89 f0 e8 a1 ff ff ff 0f b6 86 18 03 00 00 a8 01 75 09 83 c8 01 88 86 18 03 00 00 8b 1b 39 fb 75 cf 5b 5e 5f 5d c3 <0f> 0b 55 89 e5 53 3e 8d 74 26 00 83 fa 03 7e 32 8b 48 38 8d 58
> [    0.317000] EIP: [<c12d2d7e>] pci_bus_add_devices+0x7e/0x80 SS:ESP 0068:df0a1de0
> [    0.318013] ---[ end trace 9e4164edb7144ba6 ]---
> [    0.319015] swapper/0 (1) used greatest stack depth: 5720 bytes left
> [    0.320011] Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
> [    0.320011]
>
> Signed-off-by: Artem Savkov <artem.savkov@gmail.com>

Sorry for the breakage.  Thanks for the report, analysis, and fix.
I'll fix this up

Bjorn

> ---
>  drivers/pci/bus.c |    4 +---
>  drivers/pci/pci.h |    2 +-
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 445a99d..748f8f3 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -174,9 +174,7 @@ int pci_bus_add_device(struct pci_dev *dev)
>          * Can not put in pci_device_add yet because resources
>          * are not assigned yet for some devices.
>          */
> -       retval = pci_create_sysfs_dev_files(dev);
> -       if (retval)
> -               return retval;
> +       pci_create_sysfs_dev_files(dev);
>
>         dev->match_driver = true;
>         retval = device_attach(&dev->dev);
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index 0f1b306..68678ed 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -8,7 +8,7 @@
>
>  /* Functions internal to the PCI core code */
>
> -int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev);
> +int pci_create_sysfs_dev_files(struct pci_dev *pdev);
>  void pci_remove_sysfs_dev_files(struct pci_dev *pdev);
>  #if !defined(CONFIG_DMI) && !defined(CONFIG_ACPI)
>  static inline void pci_create_firmware_label_files(struct pci_dev *pdev)
> --
> 1.7.0.4
>

Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

[Bjorn Helgaas]

On Wed, Apr 17, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> On Wed, Apr 17, 2013 at 5:06 AM, Artem Savkov <artem.savkov@gmail.com> wrote:
>> Introduced in cc8f7f9e4e79a0940af6b4b6fdfbcf18a03aa9f4
>> ("PCI: Fix __must_check annotation on pci_create_sysfs_dev_files()")
>>
>> pci_bus_add_device() should not fail to add device if
>> pci_create_sysfs_dev_files() call fails, as sysfs files are not critical.
>>
>> pci_sysfs_init() is a "late_initcall" and is called after acpi_init, so
>> pci_bus_add_device() fails to create devices during boot resulting in a
>> bug during acpi_init. It is safe not to create sysfs_dev_files in
>> pci_bus_add_device() as they are later created in pci_sysfs_ini().
>> ...

> Sorry for the breakage.  Thanks for the report, analysis, and fix.
> I'll fix this up

It's not clear to me what the best fix is, so I just dropped my original patch.

It's clearly wrong to have __must_check on the
pci_create_sysfs_dev_files() definition, because it doesn't do
anything there.

We could simply drop the __must_check altogether, as you suggest, but
I'd rather structure things to avoid the failure in the first place
rather than ignoring an error.

It would be ideal if we could get rid of the pci_sysfs_init()
late_initcall and just make the pci_create_sysfs_dev_files() from
pci_bus_add_device() work all the time, even at boot-time.  I didn't
look hard enough to figure out why this needs to be a late_initcall.

In any case, we can figure this out later, after we merge the current
stuff for v3.10.

Bjorn

Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

[Yinghai Lu]

On Wed, Apr 17, 2013 at 9:42 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> It would be ideal if we could get rid of the pci_sysfs_init()
> late_initcall and just make the pci_create_sysfs_dev_files() from
> pci_bus_add_device() work all the time, even at boot-time.  I didn't
> look hard enough to figure out why this needs to be a late_initcall.

have to call that after pci assign unassigned resource.

Re: [PATCH] PCI: pci_bus_add_device() should succeed even if creating sysfs files fails

[Artem Savkov]

On Wed, Apr 17, 2013 at 10:42:12AM -0600, Bjorn Helgaas wrote:
> On Wed, Apr 17, 2013 at 9:54 AM, Bjorn Helgaas <bhelgaas@google.com> wrote:
> > On Wed, Apr 17, 2013 at 5:06 AM, Artem Savkov <artem.savkov@gmail.com> wrote:
> >> Introduced in cc8f7f9e4e79a0940af6b4b6fdfbcf18a03aa9f4
> >> ("PCI: Fix __must_check annotation on pci_create_sysfs_dev_files()")
> >>
> >> pci_bus_add_device() should not fail to add device if
> >> pci_create_sysfs_dev_files() call fails, as sysfs files are not critical.
> >>
> >> pci_sysfs_init() is a "late_initcall" and is called after acpi_init, so
> >> pci_bus_add_device() fails to create devices during boot resulting in a
> >> bug during acpi_init. It is safe not to create sysfs_dev_files in
> >> pci_bus_add_device() as they are later created in pci_sysfs_ini().
> >> ...
> 
> > Sorry for the breakage.  Thanks for the report, analysis, and fix.
> > I'll fix this up
> 
> It's not clear to me what the best fix is, so I just dropped my original patch.
> 
> It's clearly wrong to have __must_check on the
> pci_create_sysfs_dev_files() definition, because it doesn't do
> anything there.
> 
> We could simply drop the __must_check altogether, as you suggest, but
> I'd rather structure things to avoid the failure in the first place
> rather than ignoring an error.
> 
> It would be ideal if we could get rid of the pci_sysfs_init()
> late_initcall and just make the pci_create_sysfs_dev_files() from
> pci_bus_add_device() work all the time, even at boot-time.  I didn't
> look hard enough to figure out why this needs to be a late_initcall.
> In any case, we can figure this out later, after we merge the current
> stuff for v3.10.

It looks like late_initcall is either due to "it belongs there" or maybe
pci_create_sysfs_dev_files() was initially only called from
pci_sysfs_init() and pci subsystem should have been initialized before
this for devices to be there to add to sysfs. I've tried changing
late_initcall to subsys_initcall and didn't see any immediate problems.

Anyway even with things properly aligned it still sounds like a bad idea
to fail pci_bus_add_device() call just because creating sysfs files
fails. Some kind of warning should be enough.

-- 
Regards,
Artem