diff mbox

[Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers

Message ID 1441697190-27993-1-git-send-email-jiang.liu@linux.intel.com (mailing list archive)
State New, archived
Delegated to: Bjorn Helgaas
Headers show

Commit Message

Jiang Liu Sept. 8, 2015, 7:26 a.m. UTC 
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(+)

Comments

Arthur Marsh Sept. 8, 2015, 9:03 a.m. UTC | #1 
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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jiang Liu Sept. 8, 2015, 9:44 a.m. UTC | #2 
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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjorn Helgaas Sept. 8, 2015, 4:27 p.m. UTC | #3 
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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arthur Marsh Sept. 9, 2015, 7:04 p.m. UTC | #4 
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-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);
 }