| Message ID | 1441697190-27993-1-git-send-email-jiang.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Jiang Liu wrote on 08/09/15 16:56: > Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and > pcibios_free_irq()") changes the way to allocate PCI legacy IRQ > for PCI devices on x86 platforms. Instead of allocating PCI legacy > IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq() > will be called by pci_device_probe() to allocate PCI legacy IRQs > when binding PCI drivers to PCI devices. > > But some device drivers, such as eata, directly access PCI devices > without implementing corresponding PCI drivers, so pcibios_alloc_irq() > won't be called for those PCI devices and wrong IRQ number may be > used to manage the PCI device. > > So detect such a case in pcibios_enable_device() by checking > pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI > legacy IRQs. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > arch/x86/pci/common.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 09d3afc0a181..60b237783582 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev) > > int pcibios_enable_device(struct pci_dev *dev, int mask) > { > + /* > + * By design, pcibios_alloc_irq() will be called by pci_device_probe() > + * when binding a PCI device to a PCI driver. But some device drivers, > + * such as eata, directly make use of PCI devices without implementing > + * PCI device drivers, so pcibios_alloc_irq() won't be called for those > + * PCI devices. > + */ > + if (!dev->driver) > + pcibios_alloc_irq(dev); > + > return pci_enable_resources(dev, mask); > } > > Thanks, I removed the test patch and applied the revised patch and built and rebooted the kernel and successfully mounted file systems on a disk attached to the DPT 2044W card using the eata driver: [ 0.000000] Linux version 4.2.0+ (root@victoria) (gcc version 5.2.1 20150903 (Debian 5.2.1-16) ) #31 SMP PREEMPT Tue Sep 8 17:36:28 ACST 2015 ... [ 80.691097] EATA0: IRQ 10 mapped to IO-APIC IRQ 17. [ 80.724519] EATA/DMA 2.0x: Copyright (C) 1994-2003 Dario Ballabio. [ 80.752035] EATA config options -> tm:1, lc:y, mq:16, rs:y, et:n, ip:n, ep:n, pp:y. [ 80.777063] EATA0: 2.0C, PCI 0xd890, IRQ 17, BMST, SG 122, MB 64. [ 80.802391] EATA0: wide SCSI support enabled, max_id 16, max_lun 8. [ 80.827959] EATA0: SCSI channel 0 enabled, host target ID 7. [ 80.853413] scsi host3: EATA/DMA 2.0x rev. 8.10.00 [ 82.445662] scsi 3:0:6:0: Direct-Access IBM DCAS-34330W S65A PQ: 0 ANSI: 2 [ 82.471584] scsi 3:0:6:0: cmds/lun 16, sorted, simple tags. [ 84.571451] sd 3:0:6:0: Attached scsi generic sg4 type 0 [ 84.597572] sd 3:0:6:0: [sdd] 8466688 512-byte logical blocks: (4.33 GB/4.03 GiB) [ 84.659874] sd 3:0:6:0: [sdd] Write Protect is off [ 84.688543] sd 3:0:6:0: [sdd] Mode Sense: b3 00 00 08 [ 84.714021] sd 3:0:6:0: [sdd] Write cache: enabled, read cache: enabled, doesn't support DPO or FUA [ 84.817682] sdd: sdd1 sdd2 < sdd5 > [ 84.919267] sd 3:0:6:0: [sdd] Attached SCSI disk Arthur. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 2015/9/8 17:03, Arthur Marsh wrote: > > > Jiang Liu wrote on 08/09/15 16:56: > Thanks, I removed the test patch and applied the revised patch and built > and rebooted the kernel and successfully mounted file systems on a disk > attached to the DPT 2044W card using the eata driver: > > [ 0.000000] Linux version 4.2.0+ (root@victoria) (gcc version 5.2.1 > 20150903 > (Debian 5.2.1-16) ) #31 SMP PREEMPT Tue Sep 8 17:36:28 ACST 2015 > ... > > [ 80.691097] EATA0: IRQ 10 mapped to IO-APIC IRQ 17. > [ 80.724519] EATA/DMA 2.0x: Copyright (C) 1994-2003 Dario Ballabio. > [ 80.752035] EATA config options -> tm:1, lc:y, mq:16, rs:y, et:n, > ip:n, ep:n, pp:y. > [ 80.777063] EATA0: 2.0C, PCI 0xd890, IRQ 17, BMST, SG 122, MB 64. > [ 80.802391] EATA0: wide SCSI support enabled, max_id 16, max_lun 8. > [ 80.827959] EATA0: SCSI channel 0 enabled, host target ID 7. > [ 80.853413] scsi host3: EATA/DMA 2.0x rev. 8.10.00 > [ 82.445662] scsi 3:0:6:0: Direct-Access IBM DCAS-34330W > S65A PQ: 0 ANSI: 2 > [ 82.471584] scsi 3:0:6:0: cmds/lun 16, sorted, simple tags. > [ 84.571451] sd 3:0:6:0: Attached scsi generic sg4 type 0 > [ 84.597572] sd 3:0:6:0: [sdd] 8466688 512-byte logical blocks: (4.33 > GB/4.03 GiB) > [ 84.659874] sd 3:0:6:0: [sdd] Write Protect is off > [ 84.688543] sd 3:0:6:0: [sdd] Mode Sense: b3 00 00 08 > [ 84.714021] sd 3:0:6:0: [sdd] Write cache: enabled, read cache: > enabled, doesn't support DPO or FUA > [ 84.817682] sdd: sdd1 sdd2 < sdd5 > > [ 84.919267] sd 3:0:6:0: [sdd] Attached SCSI disk Hi Arthur, Thanks for testing:) > > Arthur. > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Jiang, I object to subject lines like "Correctly do such and such." Nobody writes code to do things *incorrectly*, so the word "correctly" takes up space without contributing meaning. In this case, it's at least debatable whether this is even the "correct" approach; see below. On Tue, Sep 08, 2015 at 03:26:29PM +0800, Jiang Liu wrote: > Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and > pcibios_free_irq()") changes the way to allocate PCI legacy IRQ > for PCI devices on x86 platforms. Instead of allocating PCI legacy > IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq() > will be called by pci_device_probe() to allocate PCI legacy IRQs > when binding PCI drivers to PCI devices. > > But some device drivers, such as eata, directly access PCI devices > without implementing corresponding PCI drivers, so pcibios_alloc_irq() > won't be called for those PCI devices and wrong IRQ number may be > used to manage the PCI device. I'm not sure this is wise. We normally call pcibios_alloc_irq() from pci_device_probe(), just before we call the driver's .probe() method. The eata driver does not use pci_register_driver(), so there is no .probe() method (also no .remove(), .suspend(), etc.) But eata *does* use pci_enable_device() and other PCI interfaces. So this patch adds code in the x86 pci_enable_device() path for this case. AFAICT, there's no real reason why eata doesn't register a PCI driver; it's just a case of legacy code where nobody has been motivated to update it. I'm not in favor of catering to code like that because then we have random special cases like this that clutter up the core code. I don't think we should necessarily expect the PCI core to support calls to PCI interfaces when it hasn't had a chance to initialize itself via driver registration. > So detect such a case in pcibios_enable_device() by checking > pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI > legacy IRQs. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > arch/x86/pci/common.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 09d3afc0a181..60b237783582 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev) > > int pcibios_enable_device(struct pci_dev *dev, int mask) > { > + /* > + * By design, pcibios_alloc_irq() will be called by pci_device_probe() > + * when binding a PCI device to a PCI driver. But some device drivers, > + * such as eata, directly make use of PCI devices without implementing > + * PCI device drivers, so pcibios_alloc_irq() won't be called for those > + * PCI devices. > + */ > + if (!dev->driver) > + pcibios_alloc_irq(dev); This is a point fix for x86 only, but I think eata can be built for any architecture. Won't other architectures still have the same problem? > return pci_enable_resources(dev, mask); > } > > -- > 1.7.10.4 > -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Jiang Liu wrote on 08/09/15 16:56: > Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and > pcibios_free_irq()") changes the way to allocate PCI legacy IRQ > for PCI devices on x86 platforms. Instead of allocating PCI legacy > IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq() > will be called by pci_device_probe() to allocate PCI legacy IRQs > when binding PCI drivers to PCI devices. > > But some device drivers, such as eata, directly access PCI devices > without implementing corresponding PCI drivers, so pcibios_alloc_irq() > won't be called for those PCI devices and wrong IRQ number may be > used to manage the PCI device. > > So detect such a case in pcibios_enable_device() by checking > pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI > legacy IRQs. > > Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> > --- > arch/x86/pci/common.c | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c > index 09d3afc0a181..60b237783582 100644 > --- a/arch/x86/pci/common.c > +++ b/arch/x86/pci/common.c > @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev) > > int pcibios_enable_device(struct pci_dev *dev, int mask) > { > + /* > + * By design, pcibios_alloc_irq() will be called by pci_device_probe() > + * when binding a PCI device to a PCI driver. But some device drivers, > + * such as eata, directly make use of PCI devices without implementing > + * PCI device drivers, so pcibios_alloc_irq() won't be called for those > + * PCI devices. > + */ > + if (!dev->driver) > + pcibios_alloc_irq(dev); > + > return pci_enable_resources(dev, mask); > } > > Sorry for the late report but this patch messes up things for kexec - rebooting is delayed with the error messages as shown in the fuzzy screen image here: http://www.users.on.net/~arthur.marsh/20150910541.jpg (the error messages are similar to what I was seeing on boot-up before Jiang Liu's patch) and the SCSI card is not recognised by the kernel after a kexec restart, and eata fails to load. Arthur. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c index 09d3afc0a181..60b237783582 100644 --- a/arch/x86/pci/common.c +++ b/arch/x86/pci/common.c @@ -685,6 +685,16 @@ void pcibios_free_irq(struct pci_dev *dev) int pcibios_enable_device(struct pci_dev *dev, int mask) { + /* + * By design, pcibios_alloc_irq() will be called by pci_device_probe() + * when binding a PCI device to a PCI driver. But some device drivers, + * such as eata, directly make use of PCI devices without implementing + * PCI device drivers, so pcibios_alloc_irq() won't be called for those + * PCI devices. + */ + if (!dev->driver) + pcibios_alloc_irq(dev); + return pci_enable_resources(dev, mask); }
Commit 991de2e59090 ("PCI, x86: Implement pcibios_alloc_irq() and pcibios_free_irq()") changes the way to allocate PCI legacy IRQ for PCI devices on x86 platforms. Instead of allocating PCI legacy IRQs when pcibios_enable_device() gets called, now pcibios_alloc_irq() will be called by pci_device_probe() to allocate PCI legacy IRQs when binding PCI drivers to PCI devices. But some device drivers, such as eata, directly access PCI devices without implementing corresponding PCI drivers, so pcibios_alloc_irq() won't be called for those PCI devices and wrong IRQ number may be used to manage the PCI device. So detect such a case in pcibios_enable_device() by checking pci_dev->driver is NULL and call pcibios_alloc_irq() to allocate PCI legacy IRQs. Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com> --- arch/x86/pci/common.c | 10 ++++++++++ 1 file changed, 10 insertions(+)