* alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") @ 2017-09-19 2:00 Guenter Roeck 2017-09-19 9:12 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2017-09-19 2:00 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, Bjorn Helgaas, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha Hi, I see the following runtime warnings in mainline when running alpha images in qemu. Floppy drive(s): fd0 is 2.88M ide0: disabled, no IRQ ide0: failed to initialize IDE interface ide0: disabling port cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] cmd64x 0000:00:02.0: can't reserve resources CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports ------------[ cut here ]------------ WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 sysfs: cannot create duplicate filename '/class/ide_port/ide0' ... Trace: [<fffffc00003308a0>] __warn+0x160/0x190 [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 [<fffffc00005ba690>] device_create+0x50/0x70 [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 [<fffffc00005df330>] ide_host_register+0x450/0xa00 [<fffffc00005ba2a0>] device_register+0x20/0x50 [<fffffc00005df330>] ide_host_register+0x450/0xa00 [<fffffc00005df944>] ide_host_add+0x64/0xe0 [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 [<fffffc0000310288>] do_one_initcall+0x68/0x260 [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 ---[ end trace 24a70433c3e4d374 ]--- ide0: disabling port [ multiple times ] A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. Prior to the offending commit, the kernel log looks as follows. ... Uniform Multi-Platform E-IDE driver cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) cmd64x 0000:00:02.0: IDE port disabled cmd64x 0000:00:02.0: 100% native mode on irq 28 PCI: Setting latency timer of device 0000:00:02.0 to 64 ide0: BM-DMA at 0x8040-0x8047 Floppy drive(s): fd0 is 2.88M ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 ide2 at 0x170-0x177,0x376 on irq 15 ide-gd driver 1.18 ide-cd driver 5.00 ... Reverting the commit is not possible due to context changes. Bisect log is attached. Guenter ---------------------- # bad: [ebb2c2437d8008d46796902ff390653822af6cc4] Merge tag 'mmc-v4.14-2' of git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc # good: [5969d1bb3082b41eba8fd2c826559abe38ccb6df] Merge branch 'gperf-removal' git bisect start 'HEAD' '5969d1bb3082' # bad: [ae46654bcff303b33facbbd04a3ad9c21d303f9b] Merge tag 'armsoc-drivers' of git://git.kernel.org/pub/scm/linux/kernel/git/arm/arm-soc git bisect bad ae46654bcff303b33facbbd04a3ad9c21d303f9b # bad: [126e76ffbf78d9e948b641aadb265d16c57f5a3d] Merge branch 'for-4.14/block-postmerge' of git://git.kernel.dk/linux-block git bisect bad 126e76ffbf78d9e948b641aadb265d16c57f5a3d # bad: [0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c] Merge tag 'pci-v4.14-changes' of git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci git bisect bad 0d519f2d1ed1f11e49abc88cfcf6cf13b83ba14c # good: [cf5f9cc8e4e5e8e0ecc35f1c904d98f889be2c0f] Merge branch 'pci/hotplug' into next git bisect good cf5f9cc8e4e5e8e0ecc35f1c904d98f889be2c0f # good: [5f54c8b2d4fad95d1f8ecbe023ebe6038e6d3760] Merge branch 'kvm-ppc-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/paulus/powerpc git bisect good 5f54c8b2d4fad95d1f8ecbe023ebe6038e6d3760 # good: [369130b63178e0e2f863a2da2a5ad0238ded6d9d] selftests: Enhance kselftest_harness.h to print which assert failed git bisect good 369130b63178e0e2f863a2da2a5ad0238ded6d9d # bad: [d872694bac212f76ca13fd20a85e5c1bdb53a945] Merge branch 'pci/pm' into next git bisect bad d872694bac212f76ca13fd20a85e5c1bdb53a945 # bad: [d4fdf844c9c3debc080aea1be8b71d9d0aaa01dc] Merge branch 'pci/irq-fixups' into next git bisect bad d4fdf844c9c3debc080aea1be8b71d9d0aaa01dc # bad: [04c81c7293df875ca6a46e2c9a272c7ec72e5145] MIPS: PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks git bisect bad 04c81c7293df875ca6a46e2c9a272c7ec72e5145 # good: [20d693225ab78f0651b0e116b74196aaf8a950bb] sh/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks git bisect good 20d693225ab78f0651b0e116b74196aaf8a950bb # bad: [19cc4c843f40c6110dd07270414586e7fe4121b2] m68k/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks git bisect bad 19cc4c843f40c6110dd07270414586e7fe4121b2 # bad: [0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b] alpha/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks git bisect bad 0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b # first bad commit: [0e4c2eeb758a91e68b9eaf7a4bee9bd5ed97ff2b] alpha/PCI: Replace pci_fixup_irqs() call with host bridge IRQ mapping hooks ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-19 2:00 alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") Guenter Roeck @ 2017-09-19 9:12 ` Lorenzo Pieralisi 2017-09-19 18:02 ` Guenter Roeck 0 siblings, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2017-09-19 9:12 UTC (permalink / raw) To: Guenter Roeck Cc: linux-kernel, Bjorn Helgaas, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > Hi, > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > Floppy drive(s): fd0 is 2.88M > ide0: disabled, no IRQ > ide0: failed to initialize IDE interface > ide0: disabling port > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > cmd64x 0000:00:02.0: can't reserve resources > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > ------------[ cut here ]------------ > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > ... > > Trace: > [<fffffc00003308a0>] __warn+0x160/0x190 > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > [<fffffc00005ba690>] device_create+0x50/0x70 > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > [<fffffc00005ba2a0>] device_register+0x20/0x50 > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > ---[ end trace 24a70433c3e4d374 ]--- > ide0: disabling port > > [ multiple times ] > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > Prior to the offending commit, the kernel log looks as follows. > > ... > Uniform Multi-Platform E-IDE driver > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > cmd64x 0000:00:02.0: IDE port disabled > cmd64x 0000:00:02.0: 100% native mode on irq 28 > PCI: Setting latency timer of device 0000:00:02.0 to 64 > ide0: BM-DMA at 0x8040-0x8047 > Floppy drive(s): fd0 is 2.88M > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > ide2 at 0x170-0x177,0x376 on irq 15 > ide-gd driver 1.18 > ide-cd driver 5.00 > ... > > Reverting the commit is not possible due to context changes. > > Bisect log is attached. Ok thanks. Can you please check if the diff below fixes the issue ? I think the problem is that now we allocate the IRQ at device_probe instead of device_add time, if the patch below fixes the issue the question is whether we want to move pci_assign_irq() to pci_device_add() for ALL PCI devices or just fix this for IDE subsytem. Lorenzo -- >8 -- diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c index 112d2fe..94ca9cc 100644 --- a/drivers/ide/setup-pci.c +++ b/drivers/ide/setup-pci.c @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, { int pciirq, ret; + pci_assign_irq(dev); + /* * Can we trust the reported IRQ? */ ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-19 9:12 ` Lorenzo Pieralisi @ 2017-09-19 18:02 ` Guenter Roeck 2017-09-20 11:31 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Guenter Roeck @ 2017-09-19 18:02 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: linux-kernel, Bjorn Helgaas, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > Hi, > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > Floppy drive(s): fd0 is 2.88M > > ide0: disabled, no IRQ > > ide0: failed to initialize IDE interface > > ide0: disabling port > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > cmd64x 0000:00:02.0: can't reserve resources > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > ------------[ cut here ]------------ > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > ... > > > > Trace: > > [<fffffc00003308a0>] __warn+0x160/0x190 > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > [<fffffc00005ba690>] device_create+0x50/0x70 > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > ---[ end trace 24a70433c3e4d374 ]--- > > ide0: disabling port > > > > [ multiple times ] > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > Prior to the offending commit, the kernel log looks as follows. > > > > ... > > Uniform Multi-Platform E-IDE driver > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > cmd64x 0000:00:02.0: IDE port disabled > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > ide0: BM-DMA at 0x8040-0x8047 > > Floppy drive(s): fd0 is 2.88M > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > ide2 at 0x170-0x177,0x376 on irq 15 > > ide-gd driver 1.18 > > ide-cd driver 5.00 > > ... > > > > Reverting the commit is not possible due to context changes. > > > > Bisect log is attached. > > Ok thanks. Can you please check if the diff below fixes the issue ? > It does. Feel free to add Tested-by: Guenter Roeck <linux@roeck-us.net> to the actual patch. > I think the problem is that now we allocate the IRQ at device_probe > instead of device_add time, if the patch below fixes the issue the > question is whether we want to move pci_assign_irq() to pci_device_add() > for ALL PCI devices or just fix this for IDE subsytem. > That I can not answer ... Guenter > Lorenzo > > -- >8 -- > diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c > index 112d2fe..94ca9cc 100644 > --- a/drivers/ide/setup-pci.c > +++ b/drivers/ide/setup-pci.c > @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, > { > int pciirq, ret; > > + pci_assign_irq(dev); > + > /* > * Can we trust the reported IRQ? > */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-19 18:02 ` Guenter Roeck @ 2017-09-20 11:31 ` Lorenzo Pieralisi 2017-09-27 10:30 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2017-09-20 11:31 UTC (permalink / raw) To: Guenter Roeck, Bjorn Helgaas Cc: linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha On Tue, Sep 19, 2017 at 11:02:33AM -0700, Guenter Roeck wrote: > On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > > Hi, > > > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > > > > Floppy drive(s): fd0 is 2.88M > > > ide0: disabled, no IRQ > > > ide0: failed to initialize IDE interface > > > ide0: disabling port > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > > cmd64x 0000:00:02.0: can't reserve resources > > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > ------------[ cut here ]------------ > > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > > ... > > > > > > Trace: > > > [<fffffc00003308a0>] __warn+0x160/0x190 > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > > [<fffffc00005ba690>] device_create+0x50/0x70 > > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > ---[ end trace 24a70433c3e4d374 ]--- > > > ide0: disabling port > > > > > > [ multiple times ] > > > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > > > Prior to the offending commit, the kernel log looks as follows. > > > > > > ... > > > Uniform Multi-Platform E-IDE driver > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > cmd64x 0000:00:02.0: IDE port disabled > > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > > ide0: BM-DMA at 0x8040-0x8047 > > > Floppy drive(s): fd0 is 2.88M > > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > > ide2 at 0x170-0x177,0x376 on irq 15 > > > ide-gd driver 1.18 > > > ide-cd driver 5.00 > > > ... > > > > > > Reverting the commit is not possible due to context changes. > > > > > > Bisect log is attached. > > > > Ok thanks. Can you please check if the diff below fixes the issue ? > > > It does. Feel free to add > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > to the actual patch. > > > I think the problem is that now we allocate the IRQ at device_probe > > instead of device_add time, if the patch below fixes the issue the > > question is whether we want to move pci_assign_irq() to pci_device_add() > > for ALL PCI devices or just fix this for IDE subsytem. > > > > That I can not answer ... It is not a clean-cut answer. Moving pci_assign_irq() from pci_device_probe() to pci_device_add() would fix this issue too and just map/swizzle() irqs for ALL PCI devices earlier (even ones that are not even probed); it should have no side effects (famous last words) and would be a cleaner fix. Fix below is fine but I am not a big fan of it, I would like to get Bjorn's opinion before I send a fix out. Thanks, Lorenzo > Guenter > > > Lorenzo > > > > -- >8 -- > > diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c > > index 112d2fe..94ca9cc 100644 > > --- a/drivers/ide/setup-pci.c > > +++ b/drivers/ide/setup-pci.c > > @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, > > { > > int pciirq, ret; > > > > + pci_assign_irq(dev); > > + > > /* > > * Can we trust the reported IRQ? > > */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-20 11:31 ` Lorenzo Pieralisi @ 2017-09-27 10:30 ` Lorenzo Pieralisi 2017-09-27 19:55 ` Bjorn Helgaas 0 siblings, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2017-09-27 10:30 UTC (permalink / raw) To: Guenter Roeck, Bjorn Helgaas Cc: linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha Bjorn, Guenter, On Wed, Sep 20, 2017 at 12:31:04PM +0100, Lorenzo Pieralisi wrote: > On Tue, Sep 19, 2017 at 11:02:33AM -0700, Guenter Roeck wrote: > > On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > > > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > > > Hi, > > > > > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > > > > > > > Floppy drive(s): fd0 is 2.88M > > > > ide0: disabled, no IRQ > > > > ide0: failed to initialize IDE interface > > > > ide0: disabling port > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > > > cmd64x 0000:00:02.0: can't reserve resources > > > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > ------------[ cut here ]------------ > > > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > > > ... > > > > > > > > Trace: > > > > [<fffffc00003308a0>] __warn+0x160/0x190 > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > > > [<fffffc00005ba690>] device_create+0x50/0x70 > > > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > > > ---[ end trace 24a70433c3e4d374 ]--- > > > > ide0: disabling port > > > > > > > > [ multiple times ] > > > > > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > > > > > Prior to the offending commit, the kernel log looks as follows. > > > > > > > > ... > > > > Uniform Multi-Platform E-IDE driver > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > cmd64x 0000:00:02.0: IDE port disabled > > > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > > > ide0: BM-DMA at 0x8040-0x8047 > > > > Floppy drive(s): fd0 is 2.88M > > > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > > > ide2 at 0x170-0x177,0x376 on irq 15 > > > > ide-gd driver 1.18 > > > > ide-cd driver 5.00 > > > > ... > > > > > > > > Reverting the commit is not possible due to context changes. > > > > > > > > Bisect log is attached. > > > > > > Ok thanks. Can you please check if the diff below fixes the issue ? > > > > > It does. Feel free to add > > > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > > > to the actual patch. > > > > > I think the problem is that now we allocate the IRQ at device_probe > > > instead of device_add time, if the patch below fixes the issue the > > > question is whether we want to move pci_assign_irq() to pci_device_add() > > > for ALL PCI devices or just fix this for IDE subsytem. > > > > > > > That I can not answer ... > > It is not a clean-cut answer. Moving pci_assign_irq() from > > pci_device_probe() > > to > > pci_device_add() > > would fix this issue too and just map/swizzle() irqs for ALL PCI devices > earlier (even ones that are not even probed); it should have no side > effects (famous last words) and would be a cleaner fix. > > Fix below is fine but I am not a big fan of it, I would like to > get Bjorn's opinion before I send a fix out. I think that for now the fix below should be ok, I would like to send it out before -rc3, the only question is related to the "Fixes:" tag, I am reluctant to add: Fixes: 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host bridge mapping hooks") since this is not necessarily just an alpha/PCI bug (ie it is related to IDE legacy IRQ routing - which Guenter hit on Alpha), the tag can be misleading in that sense. Please let me know what you think, I will send out the patch then. Thanks, Lorenzo > Thanks, > Lorenzo > > > Guenter > > > > > Lorenzo > > > > > > -- >8 -- > > > diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c > > > index 112d2fe..94ca9cc 100644 > > > --- a/drivers/ide/setup-pci.c > > > +++ b/drivers/ide/setup-pci.c > > > @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, > > > { > > > int pciirq, ret; > > > > > > + pci_assign_irq(dev); > > > + > > > /* > > > * Can we trust the reported IRQ? > > > */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-27 10:30 ` Lorenzo Pieralisi @ 2017-09-27 19:55 ` Bjorn Helgaas 2017-09-28 9:25 ` Lorenzo Pieralisi 0 siblings, 1 reply; 8+ messages in thread From: Bjorn Helgaas @ 2017-09-27 19:55 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Guenter Roeck, Bjorn Helgaas, linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha, linux-pci, David S. Miller, linux-ide [+cc linux-pci, linux-ide, DaveM] On Wed, Sep 27, 2017 at 11:30:34AM +0100, Lorenzo Pieralisi wrote: > Bjorn, Guenter, > > On Wed, Sep 20, 2017 at 12:31:04PM +0100, Lorenzo Pieralisi wrote: > > On Tue, Sep 19, 2017 at 11:02:33AM -0700, Guenter Roeck wrote: > > > On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > > > > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > > > > Hi, > > > > > > > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > > > > > > > > > > Floppy drive(s): fd0 is 2.88M > > > > > ide0: disabled, no IRQ > > > > > ide0: failed to initialize IDE interface > > > > > ide0: disabling port > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > > > > cmd64x 0000:00:02.0: can't reserve resources > > > > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > ------------[ cut here ]------------ > > > > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > > > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > > > > ... > > > > > > > > > > Trace: > > > > > [<fffffc00003308a0>] __warn+0x160/0x190 > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > > > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > > > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > > > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > > > > [<fffffc00005ba690>] device_create+0x50/0x70 > > > > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > > > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > > > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > > > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > > > > > ---[ end trace 24a70433c3e4d374 ]--- > > > > > ide0: disabling port > > > > > > > > > > [ multiple times ] > > > > > > > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > > > > > > > Prior to the offending commit, the kernel log looks as follows. > > > > > > > > > > ... > > > > > Uniform Multi-Platform E-IDE driver > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > cmd64x 0000:00:02.0: IDE port disabled > > > > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > > > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > > > > ide0: BM-DMA at 0x8040-0x8047 > > > > > Floppy drive(s): fd0 is 2.88M > > > > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > > > > ide2 at 0x170-0x177,0x376 on irq 15 > > > > > ide-gd driver 1.18 > > > > > ide-cd driver 5.00 > > > > > ... > > > > > > > > > > Reverting the commit is not possible due to context changes. > > > > > > > > > > Bisect log is attached. > > > > > > > > Ok thanks. Can you please check if the diff below fixes the issue ? > > > > > > > It does. Feel free to add > > > > > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > > > > > to the actual patch. > > > > > > > I think the problem is that now we allocate the IRQ at device_probe > > > > instead of device_add time, if the patch below fixes the issue the > > > > question is whether we want to move pci_assign_irq() to pci_device_add() > > > > for ALL PCI devices or just fix this for IDE subsytem. > > > > > > > > > > That I can not answer ... > > > > It is not a clean-cut answer. Moving pci_assign_irq() from > > > > pci_device_probe() > > > > to > > > > pci_device_add() > > > > would fix this issue too and just map/swizzle() irqs for ALL PCI devices > > earlier (even ones that are not even probed); it should have no side > > effects (famous last words) and would be a cleaner fix. Hmmm, this is ugly. I *think* this is related to ide_pci_init_two(). We already call pci_assign_irq() from pci_device_probe(), where we try to bind one device to a driver. But ide_pci_init_two() deals with *two* devices: the one pci_device_probe() is trying to bind, and another random one found via pci_get_slot(). We have not called pci_assign_irq() for this second device. That breaks the model the PCI core expects, where we call the driver probe method for device X, and that method deals only with device X. We can fix this by doing the pci_assign_irq() either in the PCI core's device add path or in the driver's probe path (ide_pci_init_two(), as in the patch below). Doing it in the PCI core is sort of ugly because there's no obvious reason why we should map/swizzle the IRQ if there's no driver. Doing it in the driver is also sort of ugly because this is something that should be confined to the core (pci_assign_irq() probably should be declared in drivers/pci/pci.h so it's not available outside the core). I think the patch below means we call pci_assign_irq() *twice* on the device we're binding: once from pci_device_probe() and again from do_ide_setup_pci_device(). And I guess we might call it twice for the second device, too: once from do_ide_setup_pci_device() and again if pci_device_probe() tries to bind it. It's probably idempotent, but it does seem a little ugly. I guess I agree that calling pci_assign_irq() from pci_device_add() is the lesser evil. That will do it unnecessarily in cases where a device doesn't have a driver, but it should be safe. It sets pdev->irq but probably doesn't touch the device itself. We do more IRQ setup at pci_enable_device()-time (or probe-time for arm64) via acpi_pci_irq_enable(). This also updates pdev->irq. I'd rather not do that part at device add-time, because it does things like allocate and enable interrupt links, and we shouldn't do that until we know we have a driver for the device. But this is potentially another problem for this "second-device" thing IDE does. We've called pci_assign_irq() but not acpi_pci_irq_enable() for the second device, so we may still get the wrong IRQ for it. > > Fix below is fine but I am not a big fan of it, I would like to > > get Bjorn's opinion before I send a fix out. > > I think that for now the fix below should be ok, I would like to > send it out before -rc3, the only question is related to the > "Fixes:" tag, I am reluctant to add: > > Fixes: 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host bridge mapping hooks") > > since this is not necessarily just an alpha/PCI bug (ie it is related to > IDE legacy IRQ routing - which Guenter hit on Alpha), the tag can be > misleading in that sense. > > Please let me know what you think, I will send out the patch then. > > Thanks, > Lorenzo > > > Thanks, > > Lorenzo > > > > > Guenter > > > > > > > Lorenzo > > > > > > > > -- >8 -- > > > > diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c > > > > index 112d2fe..94ca9cc 100644 > > > > --- a/drivers/ide/setup-pci.c > > > > +++ b/drivers/ide/setup-pci.c > > > > @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, > > > > { > > > > int pciirq, ret; > > > > > > > > + pci_assign_irq(dev); > > > > + > > > > /* > > > > * Can we trust the reported IRQ? > > > > */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-27 19:55 ` Bjorn Helgaas @ 2017-09-28 9:25 ` Lorenzo Pieralisi 2017-09-28 17:53 ` Bjorn Helgaas 0 siblings, 1 reply; 8+ messages in thread From: Lorenzo Pieralisi @ 2017-09-28 9:25 UTC (permalink / raw) To: Bjorn Helgaas Cc: Guenter Roeck, Bjorn Helgaas, linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha, linux-pci, David S. Miller, linux-ide On Wed, Sep 27, 2017 at 02:55:02PM -0500, Bjorn Helgaas wrote: > [+cc linux-pci, linux-ide, DaveM] > > On Wed, Sep 27, 2017 at 11:30:34AM +0100, Lorenzo Pieralisi wrote: > > Bjorn, Guenter, > > > > On Wed, Sep 20, 2017 at 12:31:04PM +0100, Lorenzo Pieralisi wrote: > > > On Tue, Sep 19, 2017 at 11:02:33AM -0700, Guenter Roeck wrote: > > > > On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > > > > > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > > > > > Hi, > > > > > > > > > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > > > > > > > > > > > > > Floppy drive(s): fd0 is 2.88M > > > > > > ide0: disabled, no IRQ > > > > > > ide0: failed to initialize IDE interface > > > > > > ide0: disabling port > > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > > > > > cmd64x 0000:00:02.0: can't reserve resources > > > > > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > > ------------[ cut here ]------------ > > > > > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > > > > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > > > > > ... > > > > > > > > > > > > Trace: > > > > > > [<fffffc00003308a0>] __warn+0x160/0x190 > > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > > > > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > > > > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > > > > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > > > > > [<fffffc00005ba690>] device_create+0x50/0x70 > > > > > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > > > > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > > > > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > > > > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > > > > > > > ---[ end trace 24a70433c3e4d374 ]--- > > > > > > ide0: disabling port > > > > > > > > > > > > [ multiple times ] > > > > > > > > > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > > > > > > > > > Prior to the offending commit, the kernel log looks as follows. > > > > > > > > > > > > ... > > > > > > Uniform Multi-Platform E-IDE driver > > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > > cmd64x 0000:00:02.0: IDE port disabled > > > > > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > > > > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > > > > > ide0: BM-DMA at 0x8040-0x8047 > > > > > > Floppy drive(s): fd0 is 2.88M > > > > > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > > > > > ide2 at 0x170-0x177,0x376 on irq 15 > > > > > > ide-gd driver 1.18 > > > > > > ide-cd driver 5.00 > > > > > > ... > > > > > > > > > > > > Reverting the commit is not possible due to context changes. > > > > > > > > > > > > Bisect log is attached. > > > > > > > > > > Ok thanks. Can you please check if the diff below fixes the issue ? > > > > > > > > > It does. Feel free to add > > > > > > > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > > > > > > > to the actual patch. > > > > > > > > > I think the problem is that now we allocate the IRQ at device_probe > > > > > instead of device_add time, if the patch below fixes the issue the > > > > > question is whether we want to move pci_assign_irq() to pci_device_add() > > > > > for ALL PCI devices or just fix this for IDE subsytem. > > > > > > > > > > > > > That I can not answer ... > > > > > > It is not a clean-cut answer. Moving pci_assign_irq() from > > > > > > pci_device_probe() > > > > > > to > > > > > > pci_device_add() > > > > > > would fix this issue too and just map/swizzle() irqs for ALL PCI devices > > > earlier (even ones that are not even probed); it should have no side > > > effects (famous last words) and would be a cleaner fix. > > Hmmm, this is ugly. I *think* this is related to ide_pci_init_two(). > We already call pci_assign_irq() from pci_device_probe(), where we try > to bind one device to a driver. But ide_pci_init_two() deals with > *two* devices: the one pci_device_probe() is trying to bind, and > another random one found via pci_get_slot(). We have not called > pci_assign_irq() for this second device. > > That breaks the model the PCI core expects, where we call the driver > probe method for device X, and that method deals only with device X. > > We can fix this by doing the pci_assign_irq() either in the PCI core's > device add path or in the driver's probe path (ide_pci_init_two(), as > in the patch below). Doing it in the PCI core is sort of ugly because > there's no obvious reason why we should map/swizzle the IRQ if there's > no driver. > > Doing it in the driver is also sort of ugly because this is something > that should be confined to the core (pci_assign_irq() probably should > be declared in drivers/pci/pci.h so it's not available outside the > core). > > I think the patch below means we call pci_assign_irq() *twice* on the > device we're binding: once from pci_device_probe() and again from > do_ide_setup_pci_device(). And I guess we might call it twice for the > second device, too: once from do_ide_setup_pci_device() and again if > pci_device_probe() tries to bind it. It's probably idempotent, but it > does seem a little ugly. > > I guess I agree that calling pci_assign_irq() from pci_device_add() is > the lesser evil. That will do it unnecessarily in cases where a > device doesn't have a driver, but it should be safe. It sets > pdev->irq but probably doesn't touch the device itself. I will do that, actually this would reinstate the exact boot stage at which pci_fixup_irqs() was called (except that pci_assign_irq() does that on a per host-bridge basis and in core code) because pci_fixup_irqs() was called after enumeration before PCI devices were added (ie before calling pci_bus_add_devices()), it *should* be safe. > We do more IRQ setup at pci_enable_device()-time (or probe-time for > arm64) via acpi_pci_irq_enable(). This also updates pdev->irq. I'd > rather not do that part at device add-time, because it does things > like allocate and enable interrupt links, and we shouldn't do that > until we know we have a driver for the device. > > But this is potentially another problem for this "second-device" thing > IDE does. We've called pci_assign_irq() but not acpi_pci_irq_enable() > for the second device, so we may still get the wrong IRQ for it. I have not touched the ACPI irq assignment path with my patches on purpose. Put it differently, PNP0A03 host bridges do not have any {map/swizzle}_irq() hook (yet) so pci_assign_irq() does nothing on them. ACPI based platforms never used pci_fixup_irqs() so I changed nothing on them (and I doubt I can given how complicated x86 legacy IRQ assignment code is in arch code). I will send the fix shortly - even though I am not sure I can add a sensible Fixes: tag to it, I think: Fixes: 30fdfb929e82 ("PCI: Add a call to pci_assign_irq() in pci_device_probe()") is what makes most sense. Thanks, Lorenzo > > > Fix below is fine but I am not a big fan of it, I would like to > > > get Bjorn's opinion before I send a fix out. > > > > I think that for now the fix below should be ok, I would like to > > send it out before -rc3, the only question is related to the > > "Fixes:" tag, I am reluctant to add: > > > > Fixes: 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host bridge mapping hooks") > > > > since this is not necessarily just an alpha/PCI bug (ie it is related to > > IDE legacy IRQ routing - which Guenter hit on Alpha), the tag can be > > misleading in that sense. > > > > Please let me know what you think, I will send out the patch then. > > > > Thanks, > > Lorenzo > > > > > Thanks, > > > Lorenzo > > > > > > > Guenter > > > > > > > > > Lorenzo > > > > > > > > > > -- >8 -- > > > > > diff --git a/drivers/ide/setup-pci.c b/drivers/ide/setup-pci.c > > > > > index 112d2fe..94ca9cc 100644 > > > > > --- a/drivers/ide/setup-pci.c > > > > > +++ b/drivers/ide/setup-pci.c > > > > > @@ -502,6 +502,8 @@ static int do_ide_setup_pci_device(struct pci_dev *dev, > > > > > { > > > > > int pciirq, ret; > > > > > > > > > > + pci_assign_irq(dev); > > > > > + > > > > > /* > > > > > * Can we trust the reported IRQ? > > > > > */ ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") 2017-09-28 9:25 ` Lorenzo Pieralisi @ 2017-09-28 17:53 ` Bjorn Helgaas 0 siblings, 0 replies; 8+ messages in thread From: Bjorn Helgaas @ 2017-09-28 17:53 UTC (permalink / raw) To: Lorenzo Pieralisi Cc: Guenter Roeck, Bjorn Helgaas, linux-kernel, Richard Henderson, Ivan Kokshaysky, Matt Turner, alpha, linux-pci, David S. Miller, linux-ide On Thu, Sep 28, 2017 at 10:25:33AM +0100, Lorenzo Pieralisi wrote: > On Wed, Sep 27, 2017 at 02:55:02PM -0500, Bjorn Helgaas wrote: > > On Wed, Sep 27, 2017 at 11:30:34AM +0100, Lorenzo Pieralisi wrote: > > > On Wed, Sep 20, 2017 at 12:31:04PM +0100, Lorenzo Pieralisi wrote: > > > > On Tue, Sep 19, 2017 at 11:02:33AM -0700, Guenter Roeck wrote: > > > > > On Tue, Sep 19, 2017 at 10:12:32AM +0100, Lorenzo Pieralisi wrote: > > > > > > On Mon, Sep 18, 2017 at 07:00:55PM -0700, Guenter Roeck wrote: > > > > > > > I see the following runtime warnings in mainline when running alpha images in qemu. > > > > > > > > > > > > > > > > > > > > > Floppy drive(s): fd0 is 2.88M > > > > > > > ide0: disabled, no IRQ > > > > > > > ide0: failed to initialize IDE interface > > > > > > > ide0: disabling port > > > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > > > CMD64x_IDE 0000:00:02.0: BAR 0: can't reserve [io 0x8050-0x8057] > > > > > > > cmd64x 0000:00:02.0: can't reserve resources > > > > > > > CMD64x_IDE: probe of 0000:00:02.0 failed with error -16 > > > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > > > ------------[ cut here ]------------ > > > > > > > WARNING: CPU: 0 PID: 1 at fs/sysfs/dir.c:31 sysfs_warn_dup+0x94/0xd0 > > > > > > > sysfs: cannot create duplicate filename '/class/ide_port/ide0' > > > > > > > ... > > > > > > > > > > > > > > Trace: > > > > > > > [<fffffc00003308a0>] __warn+0x160/0x190 > > > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > > > [<fffffc0000330928>] warn_slowpath_fmt+0x58/0x70 > > > > > > > [<fffffc000048c9f4>] sysfs_warn_dup+0x94/0xd0 > > > > > > > [<fffffc0000486d40>] kernfs_path_from_node+0x30/0x60 > > > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > > > [<fffffc00004874ac>] kernfs_put+0x16c/0x2c0 > > > > > > > [<fffffc000048d010>] sysfs_do_create_link_sd.isra.2+0x100/0x120 > > > > > > > [<fffffc00005b9d64>] device_add+0x2a4/0x7c0 > > > > > > > [<fffffc00005ba5cc>] device_create_groups_vargs+0x14c/0x170 > > > > > > > [<fffffc00005ba518>] device_create_groups_vargs+0x98/0x170 > > > > > > > [<fffffc00005ba690>] device_create+0x50/0x70 > > > > > > > [<fffffc00005df36c>] ide_host_register+0x48c/0xa00 > > > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > > > [<fffffc00005ba2a0>] device_register+0x20/0x50 > > > > > > > [<fffffc00005df330>] ide_host_register+0x450/0xa00 > > > > > > > [<fffffc00005df944>] ide_host_add+0x64/0xe0 > > > > > > > [<fffffc000079b41c>] kobject_uevent_env+0x16c/0x710 > > > > > > > [<fffffc0000310288>] do_one_initcall+0x68/0x260 > > > > > > > [<fffffc00007b13bc>] kernel_init+0x1c/0x1a0 > > > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > > [<fffffc0000311868>] ret_from_kernel_thread+0x18/0x20 > > > > > > > [<fffffc00007b13a0>] kernel_init+0x0/0x1a0 > > > > > > > > > > > > > > ---[ end trace 24a70433c3e4d374 ]--- > > > > > > > ide0: disabling port > > > > > > > > > > > > > > [ multiple times ] > > > > > > > > > > > > > > A complete log is available at http://kerneltests.org/builders/qemu-alpha-master. > > > > > > > > > > > > > > Prior to the offending commit, the kernel log looks as follows. > > > > > > > > > > > > > > ... > > > > > > > Uniform Multi-Platform E-IDE driver > > > > > > > cmd64x 0000:00:02.0: IDE controller (0x1095:0x0646 rev 0x07) > > > > > > > cmd64x 0000:00:02.0: IDE port disabled > > > > > > > cmd64x 0000:00:02.0: 100% native mode on irq 28 > > > > > > > PCI: Setting latency timer of device 0000:00:02.0 to 64 > > > > > > > ide0: BM-DMA at 0x8040-0x8047 > > > > > > > Floppy drive(s): fd0 is 2.88M > > > > > > > ide0 at 0x8050-0x8057,0x8062 on irq 28 (serialized) > > > > > > > ide_generic: please use "probe_mask=0x3f" module parameter for probing all legacy ISA IDE ports > > > > > > > ide1 at 0x1f0-0x1f7,0x3f6 on irq 14 > > > > > > > ide2 at 0x170-0x177,0x376 on irq 15 > > > > > > > ide-gd driver 1.18 > > > > > > > ide-cd driver 5.00 > > > > > > > ... > > > > > > > > > > > > > > Reverting the commit is not possible due to context changes. > > > > > > > > > > > > > > Bisect log is attached. > > > > > > > > > > > > Ok thanks. Can you please check if the diff below fixes the issue ? > > > > > > > > > > > It does. Feel free to add > > > > > > > > > > Tested-by: Guenter Roeck <linux@roeck-us.net> > > > > > > > > > > to the actual patch. > > > > > > > > > > > I think the problem is that now we allocate the IRQ at device_probe > > > > > > instead of device_add time, if the patch below fixes the issue the > > > > > > question is whether we want to move pci_assign_irq() to pci_device_add() > > > > > > for ALL PCI devices or just fix this for IDE subsytem. > > > > > > > > > > > > > > > > That I can not answer ... > > > > > > > > It is not a clean-cut answer. Moving pci_assign_irq() from > > > > > > > > pci_device_probe() > > > > > > > > to > > > > > > > > pci_device_add() > > > > > > > > would fix this issue too and just map/swizzle() irqs for ALL PCI devices > > > > earlier (even ones that are not even probed); it should have no side > > > > effects (famous last words) and would be a cleaner fix. > > > > Hmmm, this is ugly. I *think* this is related to ide_pci_init_two(). > > We already call pci_assign_irq() from pci_device_probe(), where we try > > to bind one device to a driver. But ide_pci_init_two() deals with > > *two* devices: the one pci_device_probe() is trying to bind, and > > another random one found via pci_get_slot(). We have not called > > pci_assign_irq() for this second device. > > > > That breaks the model the PCI core expects, where we call the driver > > probe method for device X, and that method deals only with device X. > > > > We can fix this by doing the pci_assign_irq() either in the PCI core's > > device add path or in the driver's probe path (ide_pci_init_two(), as > > in the patch below). Doing it in the PCI core is sort of ugly because > > there's no obvious reason why we should map/swizzle the IRQ if there's > > no driver. > > > > Doing it in the driver is also sort of ugly because this is something > > that should be confined to the core (pci_assign_irq() probably should > > be declared in drivers/pci/pci.h so it's not available outside the > > core). > > > > I think the patch below means we call pci_assign_irq() *twice* on the > > device we're binding: once from pci_device_probe() and again from > > do_ide_setup_pci_device(). And I guess we might call it twice for the > > second device, too: once from do_ide_setup_pci_device() and again if > > pci_device_probe() tries to bind it. It's probably idempotent, but it > > does seem a little ugly. > > > > I guess I agree that calling pci_assign_irq() from pci_device_add() is > > the lesser evil. That will do it unnecessarily in cases where a > > device doesn't have a driver, but it should be safe. It sets > > pdev->irq but probably doesn't touch the device itself. > > I will do that, actually this would reinstate the exact boot stage at > which pci_fixup_irqs() was called (except that pci_assign_irq() does > that on a per host-bridge basis and in core code) because > pci_fixup_irqs() was called after enumeration before PCI devices were > added (ie before calling pci_bus_add_devices()), it *should* be safe. > > > We do more IRQ setup at pci_enable_device()-time (or probe-time for > > arm64) via acpi_pci_irq_enable(). This also updates pdev->irq. I'd > > rather not do that part at device add-time, because it does things > > like allocate and enable interrupt links, and we shouldn't do that > > until we know we have a driver for the device. > > > > But this is potentially another problem for this "second-device" thing > > IDE does. We've called pci_assign_irq() but not acpi_pci_irq_enable() > > for the second device, so we may still get the wrong IRQ for it. > > I have not touched the ACPI irq assignment path with my patches on > purpose. Put it differently, PNP0A03 host bridges do not have any > {map/swizzle}_irq() hook (yet) so pci_assign_irq() does nothing on > them. ACPI based platforms never used pci_fixup_irqs() so I changed > nothing on them (and I doubt I can given how complicated x86 legacy > IRQ assignment code is in arch code). Right. This isn't a problem with your patches; it's just a problem with the ide_pci_init_two() strategy of looking at dev->irq for a device for which the core hasn't called the probe path. Bjorn ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-09-28 17:53 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-09-19 2:00 alpha runtime warnings due to commit 0e4c2eeb758 ("alpha/PCI: Replace pci_fixup_irqs() call with host ...") Guenter Roeck 2017-09-19 9:12 ` Lorenzo Pieralisi 2017-09-19 18:02 ` Guenter Roeck 2017-09-20 11:31 ` Lorenzo Pieralisi 2017-09-27 10:30 ` Lorenzo Pieralisi 2017-09-27 19:55 ` Bjorn Helgaas 2017-09-28 9:25 ` Lorenzo Pieralisi 2017-09-28 17:53 ` Bjorn Helgaas
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).