lkml.org 
[lkml]   [2015]   [Sep]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRe: [Bugfix] PCI, x86: Correctly allocate IRQs for PCI devices managed by non-PCI drivers
    From
    Date


    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@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?
    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
    >>


    \
     
     \ /
      Last update: 2015-09-08 19:01    [W:4.151 / U:0.324 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site