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

[Jiang Liu]

On 2015/9/9 0:27, Bjorn Helgaas wrote:
> 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@xxxxxxxxxxxxxxx>
>> ---
>>  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?
Hi Bjorn,
	We have used another draft version to fix this issue by changing
eata driver as below. But that needs to export pcibios_alloc_irq. And
I'm not sure whether there are other drivers having the same behavior.
If we think it's a legacy behavior and only a few drivers may have
such a behavior, I prefer changing drivers to fix the issue too.
Thanks!
Gerry
---
 drivers/pci/pci-driver.c |    1 +
 drivers/scsi/eata.c      |    2 ++
 2 files changed, 3 insertions(+)

diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index 52a880ca1768..17d2a0b1de18 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -392,6 +392,7 @@ int __weak pcibios_alloc_irq(struct pci_dev *dev)
 {
 	return 0;
 }
+EXPORT_SYMBOL_GPL(pcibios_alloc_irq);

 void __weak pcibios_free_irq(struct pci_dev *dev)
 {
diff --git a/drivers/scsi/eata.c b/drivers/scsi/eata.c
index 227dd2c2ec2f..7e6eaf867987 100644
--- a/drivers/scsi/eata.c
+++ b/drivers/scsi/eata.c
@@ -1061,6 +1061,7 @@ static void enable_pci_ports(void)
 		       driver_name, dev->bus->number, dev->devfn);
 #endif

+		pcibios_alloc_irq(dev);
 		if (pci_enable_device(dev))
 			printk
 			    ("%s: warning, pci_enable_device failed, bus %d devfn 0x%x.\n",
@@ -1520,6 +1521,7 @@ static void add_pci_ports(void)
 		if (!(dev = pci_get_class(PCI_CLASS_STORAGE_SCSI << 8, dev)))
 			break;

+		pcibios_alloc_irq(dev);
 		if (pci_enable_device(dev)) {
 #if defined(DEBUG_PCI_DETECT)
 			printk


> 
>>  	return pci_enable_resources(dev, mask);
>>  }
>>  
>> -- 
>> 1.7.10.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/