[PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[Matthew Minter]

Some boards which contain physically hot-pluggable PCIe slots such as
8639 connectors can hot-add devices simply by connecting them while up
and writing to /sys/bus/pci/rescan.

This probes the device and loads a driver. However when a device is
added in this way it is not given an interrupt. Irqs seem to be passed
by the PCI BIOS, or emulated PCI BIOS in the case of non x86 arches
and so these hot-added cards naturally do not get one.

This means that when a device is added in this way, unless the device
and driver use only MSI/MSI-X it will try to request irq vector 0
which does not end well. To prevent this I have added a check for an
empty irq vector and use pdev_fixup_irq to set one at run time if one
has not been provided. This is done inside an unlikely block in order
to make negligible the performance impact on the normal PCI device
adding code path, this should not be a performance critical area
anyway as devices are not often added.

I have looked into instead modifying the rescan code however this
would be a less effective solution as there is no convenient way there
to run code between device probing and the driver loading and so would
need a significant redesign.

If anyone has any suggestions or feedback on this patch, or a better
way to solve this issue should there be one, I would be very grateful.

In addition I have only had the opportunity to test this code on non
x86 platforms so if anyone has a x86 or AMD64 PC with physical hot-add
capability I would be very appreciative if you could try running this.

Best regards,
Matthew

drivers/pci/bus.c | 11 +++++++++++
drivers/pci/setup-irq.c | 2 +-
include/linux/pci.h | 2 ++
3 files changed, 14 insertions(+), 1 deletion(-)

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

Re: [PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[Bjorn Helgaas]

On Thu, Jul 10, 2014 at 04:47:55PM +0100, Matthew Minter wrote:
> Some boards which contain physically hot-pluggable PCIe slots such as
> 8639 connectors can hot-add devices simply by connecting them while up
> and writing to /sys/bus/pci/rescan.

This is a tangent, but I'm curious about this part of the hotplug
process.  Since this is PCIe, I assume the switch leading to the 8639
connector supports hotplug and the pciehp driver would be involved.
Why doesn't it notice the device addition and handle it automatically?
You shouldn't have to do anything with /sys/bus/pci/rescan.

> This probes the device and loads a driver. However when a device is
> added in this way it is not given an interrupt. Irqs seem to be passed
> by the PCI BIOS, or emulated PCI BIOS in the case of non x86 arches
> and so these hot-added cards naturally do not get one.
> 
> This means that when a device is added in this way, unless the device
> and driver use only MSI/MSI-X it will try to request irq vector 0
> which does not end well. To prevent this I have added a check for an
> empty irq vector and use pdev_fixup_irq to set one at run time if one
> has not been provided. This is done inside an unlikely block in order
> to make negligible the performance impact on the normal PCI device
> adding code path, this should not be a performance critical area
> anyway as devices are not often added.

pci_fixup_irqs() has been broken from the beginning because it is only
done for devices present at boot-time, and nothing happens for
hot-added devices.

I think you're on the right path by looking at the generic
pci_bus_add_device() path that is used both at boot-time and hot
add-time.  I would like to see something that works the same way at
both times and gets rid of pci_fixup_irqs() altogether.

I'm not sure this needs to be done as early as pci_bus_add_device();
it could probably be done somewhere in the pci_enable_device() path,
since drivers can't use interrupts before that anyway.

Bjorn

> I have looked into instead modifying the rescan code however this
> would be a less effective solution as there is no convenient way there
> to run code between device probing and the driver loading and so would
> need a significant redesign.
> 
> If anyone has any suggestions or feedback on this patch, or a better
> way to solve this issue should there be one, I would be very grateful.
> 
> In addition I have only had the opportunity to test this code on non
> x86 platforms so if anyone has a x86 or AMD64 PC with physical hot-add
> capability I would be very appreciative if you could try running this.
> 
> Best regards,
> Matthew
> 
> drivers/pci/bus.c | 11 +++++++++++
> drivers/pci/setup-irq.c | 2 +-
> include/linux/pci.h | 2 ++
> 3 files changed, 14 insertions(+), 1 deletion(-)
> 
> -- 
> 
> 
> ------------------------------
> For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com
> 
> ------------------------------

Re: [PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[Matthew Minter]

On 16 July 2014 23:00, Bjorn Helgaas <bhelgaas@google.com> wrote:
> This is a tangent, but I'm curious about this part of the hotplug
> process.  Since this is PCIe, I assume the switch leading to the 8639
> connector supports hotplug and the pciehp driver would be involved.
> Why doesn't it notice the device addition and handle it automatically?
> You shouldn't have to do anything with /sys/bus/pci/rescan.

That is a good question and possibly exposes some of the background here.

8639 connectors do not require hot-plug support from PCIe layer at
all. This is because the hot-plug does not rely on the power to the
port being shut down first. They have a power connector based on a
modified SATA power connector including the staged power pins and such
can be surprised hot-(un)plugged without consequence (of course the
file-systems should be unmounted and ideally the driver stopped
first).

Having said that, this has the additional implication that this form
of hot-plug works on architectures which seem to have no PCIe hot-plug
support such as ARM. On the board we have here (based on an ARM server
chip-set) the PCIe switch we are using supports resource
pre-allocation for hot-adding but does not support power control or
plug/unplug interrupts but this is fine again as the connectors are
all physically hot-plug safe.

I was concerned myself about using hot-plug without a hot-plug driver
but the guys on the arm kernel mailing list confirmed that this was
normal and should be fine. Thus we have taken a system for hot adding
where we just connect the device to a pre-allocated resource port and
the run rescan. To hot-swap a device we unmount it's file-systems,
stop it's driver where possible, remove it using
/sys/bus/pci/devices/xxxx/remove then follow the hot-add procedure
above to install it's replacement. (note in our case the devices are
storage devices but this could apply equally to any peripheral).

Hopefully this explains the situation a little. Please note however,
this applies to a wider issue, any system where there is no BIOS/BIOS
like object/ACPI to map irqs to unused slots (or if the firmware is
buggy and will not do so) currently has no way to allocate an irq to
those slots later should a device become connected to them. It does
not seem correct that this code should be reserved only to boot time
and would seem beneficial to have routines to do this later.

> pci_fixup_irqs() has been broken from the beginning because it is only
> done for devices present at boot-time, and nothing happens for
> hot-added devices.
>
> I think you're on the right path by looking at the generic
> pci_bus_add_device() path that is used both at boot-time and hot
> add-time.  I would like to see something that works the same way at
> both times and gets rid of pci_fixup_irqs() altogether.
>
> I'm not sure this needs to be done as early as pci_bus_add_device();
> it could probably be done somewhere in the pci_enable_device() path,
> since drivers can't use interrupts before that anyway.
>
> Bjorn

It should be entirely possible to factor out pci_fixup_irqs
completely, it seems most of the calls to it are in the virtual PCI
BIOSes of platforms which have no BIOS, I agree it would be far neater
to avoid the platform independent code as far as possible and unify
PCI irqs into a single place.

If it is OK with you I would like to rework the patch-set so that that
instead of the boot time PCI code assigning the irqs it can instead
register an irq swizzling and an irq mapping function (which would
probably be stored either in the pci_host_bridge struct) or a default
could be used. Thich can then be called during the drvice-add code
path to both fix hot-plug irqs and unify the infrastructure a little
so it relies less on platform code. This registration could be done in
pcibios_root_bridge_prepare (as this can be overridden by each arch).

I might be way off here but if that is the sort of thing you were
looking for I can certainly begin working on a new patch-set however I
also have a  question which is possibly on a bit of a tangent, the
pci_assign_unassigned_resources function seems to be called in every
PCI supporting arch yet the function is still called from the arch
specific code, is there reason for this?

Many thanks,
Matthew

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

Re: [PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[Bjorn Helgaas]

On Thu, Jul 17, 2014 at 4:54 AM, Matthew Minter
<matthew_minter@xyratex.com> wrote:
> On 16 July 2014 23:00, Bjorn Helgaas <bhelgaas@google.com> wrote:
>> This is a tangent, but I'm curious about this part of the hotplug
>> process.  Since this is PCIe, I assume the switch leading to the 8639
>> connector supports hotplug and the pciehp driver would be involved.
>> Why doesn't it notice the device addition and handle it automatically?
>> You shouldn't have to do anything with /sys/bus/pci/rescan.
>
> That is a good question and possibly exposes some of the background here.
>
> 8639 connectors do not require hot-plug support from PCIe layer at
> all. This is because the hot-plug does not rely on the power to the
> port being shut down first. They have a power connector based on a
> modified SATA power connector including the staged power pins and such
> can be surprised hot-(un)plugged without consequence (of course the
> file-systems should be unmounted and ideally the driver stopped
> first).

This is basically the way ExpressCard works.  We use pciehp for
ExpressCard, and it likely would work for you, too.

> Having said that, this has the additional implication that this form
> of hot-plug works on architectures which seem to have no PCIe hot-plug
> support such as ARM. On the board we have here (based on an ARM server
> chip-set) the PCIe switch we are using supports resource
> pre-allocation for hot-adding but does not support power control or
> plug/unplug interrupts but this is fine again as the connectors are
> all physically hot-plug safe.

The hotplug drivers are generally architecture-independent.  I haven't
exercised pciehp on ARM, but it should work.

I assume this:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202371.html

is your system, and the Downstream Ports on bus 03 lead to the 8639
connectors.  These ports claim to support hotplug, e.g.,

        SltCap:    AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug+ Surprise-
            Slot #1, PowerLimit 25.000W; Interlock+ NoCompl-

so pciehp should claim these slots and should notice device addition
and removal.

> I was concerned myself about using hot-plug without a hot-plug driver
> but the guys on the arm kernel mailing list confirmed that this was
> normal and should be fine. Thus we have taken a system for hot adding
> where we just connect the device to a pre-allocated resource port and
> the run rescan. To hot-swap a device we unmount it's file-systems,
> stop it's driver where possible, remove it using
> /sys/bus/pci/devices/xxxx/remove then follow the hot-add procedure
> above to install it's replacement. (note in our case the devices are
> storage devices but this could apply equally to any peripheral).

Sure, you *can* use /sys/bus/pci/rescan, but you shouldn't have to.
That basically exists as a workaround for various inadequacies in
Linux PCI.

> Hopefully this explains the situation a little. Please note however,
> this applies to a wider issue, any system where there is no BIOS/BIOS
> like object/ACPI to map irqs to unused slots (or if the firmware is
> buggy and will not do so) currently has no way to allocate an irq to
> those slots later should a device become connected to them. It does
> not seem correct that this code should be reserved only to boot time
> and would seem beneficial to have routines to do this later.
>
>> pci_fixup_irqs() has been broken from the beginning because it is only
>> done for devices present at boot-time, and nothing happens for
>> hot-added devices.
>>
>> I think you're on the right path by looking at the generic
>> pci_bus_add_device() path that is used both at boot-time and hot
>> add-time.  I would like to see something that works the same way at
>> both times and gets rid of pci_fixup_irqs() altogether.
>>
>> I'm not sure this needs to be done as early as pci_bus_add_device();
>> it could probably be done somewhere in the pci_enable_device() path,
>> since drivers can't use interrupts before that anyway.
>>
>> Bjorn
>
> It should be entirely possible to factor out pci_fixup_irqs
> completely, it seems most of the calls to it are in the virtual PCI
> BIOSes of platforms which have no BIOS, I agree it would be far neater
> to avoid the platform independent code as far as possible and unify
> PCI irqs into a single place.
>
> If it is OK with you I would like to rework the patch-set so that that
> instead of the boot time PCI code assigning the irqs it can instead
> register an irq swizzling and an irq mapping function (which would
> probably be stored either in the pci_host_bridge struct) or a default
> could be used. Thich can then be called during the drvice-add code
> path to both fix hot-plug irqs and unify the infrastructure a little
> so it relies less on platform code. This registration could be done in
> pcibios_root_bridge_prepare (as this can be overridden by each arch).

That seems reasonable.

> I might be way off here but if that is the sort of thing you were
> looking for I can certainly begin working on a new patch-set however I
> also have a  question which is possibly on a bit of a tangent, the
> pci_assign_unassigned_resources function seems to be called in every
> PCI supporting arch yet the function is still called from the arch
> specific code, is there reason for this?

No.  It's only in the arch code because hotplug has been added on
afterwards, and it's not very well integrated yet.  It should be done
in the core.  But it's not trivial to fix, because some arches have
ordering dependencies.  For example, on x86, we enumerate PCI devices,
then ACPI devices, then we assign unassigned PCI resources.  We can't
assign those PCI resources without knowledge of what the ACPI devices
use, of course.  The sensible order would be to enumerate ACPI devices
first, then PCI (since the PCI host bridges themselves are ACPI
devices), but again, there's lots of historical baggage that makes it
hard to clean this up.

I'm going to be on vacation for a few weeks, but I would love to hear
about any issues you find with using pciehp on ARM.

Bjorn

Re: [PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[Matthew Minter]

> This is basically the way ExpressCard works.  We use pciehp for
> ExpressCard, and it likely would work for you, too.
>
>> Having said that, this has the additional implication that this form
>> of hot-plug works on architectures which seem to have no PCIe hot-plug
>> support such as ARM. On the board we have here (based on an ARM server
>> chip-set) the PCIe switch we are using supports resource
>> pre-allocation for hot-adding but does not support power control or
>> plug/unplug interrupts but this is fine again as the connectors are
>> all physically hot-plug safe.
>
> The hotplug drivers are generally architecture-independent.  I haven't
> exercised pciehp on ARM, but it should work.
>
> I assume this:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-October/202371.html
>
> is your system, and the Downstream Ports on bus 03 lead to the 8639
> connectors.  These ports claim to support hotplug, e.g.,
>
>         SltCap:    AttnBtn+ PwrCtrl+ MRL+ AttnInd+ PwrInd+ HotPlug+ Surprise-
>             Slot #1, PowerLimit 25.000W; Interlock+ NoCompl-
>
> so pciehp should claim these slots and should notice device addition
> and removal.
>
> Sure, you *can* use /sys/bus/pci/rescan, but you shouldn't have to.
> That basically exists as a workaround for various inadequacies in
> Linux PCI.

It appears that the HOTPLUG_PCI option has not been added to the
arch/arm/Kconfig file, thus making it hidden in the configuration tools.

This can simply be fixed as so:

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 245058b..cab167f 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -1369,6 +1369,7 @@ config PCI_HOST_ITE8152
        select DMABOUNCE

 source "drivers/pci/Kconfig"
+source "drivers/pci/hotplug/Kconfig"
 source "drivers/pci/pcie/Kconfig"

 source "drivers/pcmcia/Kconfig"


However I would therefore suppose that it has probably not been tested
before.

After some testing, I can say, the PCIe hotplug driver does seem to work
on arm, at least in the context of my own board. The only caveat seems
to be that my PCIe bridge does not correctly report the GPIOs for hotplug
forcing me to hard-code them into the device tree to make plug detection
work properly.

If the above patch was applied it should be possible to enable this on
any arm based board.

Thanks kindly for the suggestion to try that!

>> Hopefully this explains the situation a little. Please note however,
>> this applies to a wider issue, any system where there is no BIOS/BIOS
>> like object/ACPI to map irqs to unused slots (or if the firmware is
>> buggy and will not do so) currently has no way to allocate an irq to
>> those slots later should a device become connected to them. It does
>> not seem correct that this code should be reserved only to boot time
>> and would seem beneficial to have routines to do this later.
>>
>>> pci_fixup_irqs() has been broken from the beginning because it is only
>>> done for devices present at boot-time, and nothing happens for
>>> hot-added devices.
>>>
>>> I think you're on the right path by looking at the generic
>>> pci_bus_add_device() path that is used both at boot-time and hot
>>> add-time.  I would like to see something that works the same way at
>>> both times and gets rid of pci_fixup_irqs() altogether.
>>>
>>> I'm not sure this needs to be done as early as pci_bus_add_device();
>>> it could probably be done somewhere in the pci_enable_device() path,
>>> since drivers can't use interrupts before that anyway.
>>>
>>> Bjorn
>>
>> It should be entirely possible to factor out pci_fixup_irqs
>> completely, it seems most of the calls to it are in the virtual PCI
>> BIOSes of platforms which have no BIOS, I agree it would be far neater
>> to avoid the platform independent code as far as possible and unify
>> PCI irqs into a single place.
>>
>> If it is OK with you I would like to rework the patch-set so that that
>> instead of the boot time PCI code assigning the irqs it can instead
>> register an irq swizzling and an irq mapping function (which would
>> probably be stored either in the pci_host_bridge struct) or a default
>> could be used. Thich can then be called during the drvice-add code
>> path to both fix hot-plug irqs and unify the infrastructure a little
>> so it relies less on platform code. This registration could be done in
>> pcibios_root_bridge_prepare (as this can be overridden by each arch).
>
> That seems reasonable.

I have just emailed out the second version of this patch set, it should be a lot
more stable, eliminates pci_fixup_irqs completely and simplifies some of
the PCI init and fixup code.The only flaw is that it has required me to edit
architecture specific code for arches which I don't have and as such I have
not been able to test some of the small changes.

Thanks for the suggestions regarding  delaying the assignment until the device
is being enabled, I had not noticed that IRQs were unavailable before then.

In the mean time I hope that your holiday goes well.

Many thanks,
Matthew

-- 


------------------------------
For additional information including the registered office and the treatment of Xyratex confidential information please visit www.xyratex.com

------------------------------

[PATCH RFC] Added code to ensure hot-added PCI devices are given an IRQ on rescan

[matthew_minter]

From: matthew_minter <matthew_minter@xyratex.com>

Signed-off-by: matthew_minter <matthew_minter@xyratex.com>
---
 drivers/pci/bus.c       | 11 +++++++++++
 drivers/pci/setup-irq.c |  2 +-
 include/linux/pci.h     |  2 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 73aef51..eb4f93c 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/pci.h>
+#include <linux/of_pci.h>
 #include <linux/errno.h>
 #include <linux/ioport.h>
 #include <linux/proc_fs.h>
@@ -245,6 +246,16 @@ void pci_bus_add_device(struct pci_dev *dev)
 	 * are not assigned yet for some devices.
 	 */
 	pci_fixup_device(pci_fixup_final, dev);
+	/*
+	 * Devices which are hot-added after boot have not
+	 * been assigned an irq by the bios.
+	 */
+	if (unlikely(!dev->irq)) {
+		dev_dbg(&dev->dev,
+			"PCI device missing IRQ, attempting to assign one\n");
+		pdev_fixup_irq(dev, pci_common_swizzle,
+			of_irq_parse_and_map_pci);
+	}
 	pci_create_sysfs_dev_files(dev);
 	pci_proc_attach_device(dev);
 
diff --git a/drivers/pci/setup-irq.c b/drivers/pci/setup-irq.c
index 4e2d595..38c96c8 100644
--- a/drivers/pci/setup-irq.c
+++ b/drivers/pci/setup-irq.c
@@ -22,7 +22,7 @@ void __weak pcibios_update_irq(struct pci_dev *dev, int irq)
 	pci_write_config_byte(dev, PCI_INTERRUPT_LINE, irq);
 }
 
-static void pdev_fixup_irq(struct pci_dev *dev,
+void pdev_fixup_irq(struct pci_dev *dev,
 			   u8 (*swizzle)(struct pci_dev *, u8 *),
 			   int (*map_irq)(const struct pci_dev *, u8, u8))
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 466bcd1..4c1b1b3 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1056,6 +1056,8 @@ void pci_assign_unassigned_bus_resources(struct pci_bus *bus);
 void pci_assign_unassigned_root_bus_resources(struct pci_bus *bus);
 void pdev_enable_device(struct pci_dev *);
 int pci_enable_resources(struct pci_dev *, int mask);
+void pdev_fixup_irq(struct pci_dev *, u8 (*)(struct pci_dev *, u8 *),
+		    int (*)(const struct pci_dev *, u8, u8));
 void pci_fixup_irqs(u8 (*)(struct pci_dev *, u8 *),
 		    int (*)(const struct pci_dev *, u8, u8));
 #define HAVE_PCI_REQ_REGIONS	2
-- 
2.0.0