PCI's final quirks (pci_fixup_final) are currently invoked by
pci_apply_final_quirk() which traverses the platform's list of PCI
devices. The calling mechanism, and to some point the use of the device
list, limits the quirk invocations to a single instance during boot. As
such, hot-plugable devices do not have their associated final quirks
called upon hot-plug events.
This series implements a interim solution to integrate pci_fixup_final
quirks into the various hot-plug event paths[1].
The series basis is
git://git.kernel.org/pub/scm/linux/kernel/git/helgaas/pci.git next
[1] I intended to come up with a single, uniform, solution that would
satisfy both the boot path and the various hot-plug event paths with
respect to final quirks. From an architectural perspective, the proper
placement for the final quirks is somewhere just prior to when drivers can
probe and attach which would be the device_add path: pci_bus_add_devices
or pci_bus_add device.
I originally started with that approach but eventually realized that there
are issues with moving the quirks into the device_add path with respect to
booting. Using the 'initcall_debug' boot command instrumentation, one
can see that moving the final quirks into the device_add path would cause
the quirks to be called substantially earlier during boot. While there
may be additional issues, two that were especially concerning were that
the final quirks would be called *before* both 'pci_subsys_init' and
'pcibios_assign_resources'.
Calling the quirks prior to resource assignment seems fraught with
potential issues so I started looking into the various hot-plug paths and
quickly noticed asymmetry with respect to PCI device setup between the
boot path and the hot-plug paths.
Currently, the boot path scans the PCI devices, adds the devices, assigns
resources, and then call the final quirks whereas the hot-plug paths scan,
assign resources, and then add the devices which is better sequencing with
respect to the assignment of resources and the addition of devices (i.e.
resource assignment occurs *before* a driver can probe and attach).
All of this suggests that we should change PCI device setup in the boot
path to be more like hot-plug: scan, assign resources, (final fixups,)
then add. While I think that is the correct approach, and something that
we should be addressing, it will require a lot of work. So until that
occurs, this series should serve as a stop-gap solution for the interim.
When the boot path's PCI device setup is addressed we should end up with a
single, uniform, device_add based solution for applying final quirks
after:
o removing 'fs_initcall_sync(pci_apply_final_quirks);',
o removing the global variable 'pci_fixup_final_inited' and all
of its usages,
o renaming, and moving, the 'pci_cache_line_size' related code
currently embedded in 'pci_apply_final_quirks()'.
---
Myron Stowe (9):
PCI: integrate 'pci_fixup_final' quirks into hot-plug paths
PCI: move final fixup quirks from __init to __devinit
x86/PCI: move final fixup quirks from __init to __devinit
powerpc/PCI: move final fixup quirks from __init to __devinit
parisc/PCI: move final fixup quirks from __init to __devinit
MIPS/PCI: move final fixup quirks from __init to __devinit
arm/PCI: move final fixup quirks from __init to __devinit
PCI: release temporary reference in __nv_msi_ht_cap_quirk()
PCI: Remove redundant debug output in pci_do_fixups
arch/arm/mach-iop32x/n2100.c | 2 +
arch/mips/txx9/generic/pci.c | 6 ++-
arch/powerpc/platforms/powermac/pci.c | 2 +
arch/x86/pci/fixup.c | 2 +
drivers/parisc/superio.c | 3 +-
drivers/pci/bus.c | 4 ++
drivers/pci/quirks.c | 61 +++++++++++++++++++++++----------
7 files changed, 53 insertions(+), 27 deletions(-)
--
When the boot argument 'initcall_debug' is specified, redundant debug
output occurs for each device as a quirk is applied:
...
pci 0000:00:1a.0: calling quirk_usb_early_handoff+0x0/0x620
calling quirk_usb_early_handoff+0x0/0x620 @ 1 for 0000:00:1a.0
pci fixup quirk_usb_early_handoff+0x0/0x620 returned after 32 usecs for 0000:00: 1a.0
...
This patch removes the redundancy by eliminating the first debug output
occurence in the sequence shown above when 'initcall_debug' is specified.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/pci/quirks.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index a2d9d33..9c93558 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2953,11 +2953,12 @@ static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
f->vendor == (u16) PCI_ANY_ID) &&
(f->device == dev->device ||
f->device == (u16) PCI_ANY_ID)) {
- dev_dbg(&dev->dev, "calling %pF\n", f->hook);
if (initcall_debug)
do_one_fixup_debug(f->hook, dev);
- else
+ else {
+ dev_dbg(&dev->dev, "calling %pF\n", f->hook);
f->hook(dev);
+ }
}
}
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
arch/arm/mach-iop32x/n2100.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
index 5a7ae91..04c4110 100644
--- a/arch/arm/mach-iop32x/n2100.c
+++ b/arch/arm/mach-iop32x/n2100.c
@@ -126,7 +126,7 @@ static struct hw_pci n2100_pci __initdata = {
* the ->broken_parity_status flag for both ports so that the r8169
* driver knows it should ignore error interrupts.
*/
-static void n2100_fixup_r8169(struct pci_dev *dev)
+static void __devinit n2100_fixup_r8169(struct pci_dev *dev)
{
if (dev->bus->number == 0 &&
(dev->devfn == PCI_DEVFN(1, 0) ||
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
arch/mips/txx9/generic/pci.c | 6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/mips/txx9/generic/pci.c b/arch/mips/txx9/generic/pci.c
index 682efb0..de83b30 100644
--- a/arch/mips/txx9/generic/pci.c
+++ b/arch/mips/txx9/generic/pci.c
@@ -304,7 +304,7 @@ static void __init quirk_slc90e66_bridge(struct pci_dev *dev)
smsc_fdc37m81x_config_end();
}
-static void quirk_slc90e66_ide(struct pci_dev *dev)
+static void __devinit quirk_slc90e66_ide(struct pci_dev *dev)
{
unsigned char dat;
int regs[2] = {0x41, 0x43};
@@ -339,7 +339,7 @@ static void quirk_slc90e66_ide(struct pci_dev *dev)
}
#endif /* CONFIG_TOSHIBA_FPCIB0 */
-static void tc35815_fixup(struct pci_dev *dev)
+static void __devinit tc35815_fixup(struct pci_dev *dev)
{
/* This device may have PM registers but not they are not suported. */
if (dev->pm_cap) {
@@ -348,7 +348,7 @@ static void tc35815_fixup(struct pci_dev *dev)
}
}
-static void final_fixup(struct pci_dev *dev)
+static void __devinit final_fixup(struct pci_dev *dev)
{
unsigned char bist;
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/parisc/superio.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/parisc/superio.c b/drivers/parisc/superio.c
index 5003458..2d24cf2 100644
--- a/drivers/parisc/superio.c
+++ b/drivers/parisc/superio.c
@@ -149,8 +149,7 @@ superio_interrupt(int parent_irq, void *devp)
}
/* Initialize Super I/O device */
-static void
-superio_init(struct pci_dev *pcidev)
+static void __devinit superio_init(struct pci_dev *pcidev)
{
struct superio_device *sio = &sio_dev;
struct pci_dev *pdev = sio->lio_pdev;
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
arch/x86/pci/fixup.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/x86/pci/fixup.c b/arch/x86/pci/fixup.c
index af8a224..8cefdbe 100644
--- a/arch/x86/pci/fixup.c
+++ b/arch/x86/pci/fixup.c
@@ -250,7 +250,7 @@ static struct pci_ops quirk_pcie_aspm_ops = {
* the root port in an array for fast indexing. Replace the bus ops
* with the modified one.
*/
-static void pcie_rootport_aspm_quirk(struct pci_dev *pdev)
+static void __devinit pcie_rootport_aspm_quirk(struct pci_dev *pdev)
{
int cap_base, i;
struct pci_bus *pbus;
PCI's final quirks (pci_fixup_final) are currently invoked by
pci_apply_final_quirk() which traverses the platform's list of PCI
devices. The calling mechanism limits the quirk invocations to a single
instance during boot. As such, hot-plugable devices do not have their
associated final quirks called upon hot-plug events.
This series implements a interim solution[1] to integrate pci_fixup_final
quirks into the various hot-plug event paths.
As I intend for the global variable introduced by this patch to be
temporary I purposely chose not to include its 'extern' declaration within
a header file (i.e. include/linux/pci.h).
[1] I intended to come up with a single, uniform, solution that would
satisfy both the boot path and the various hot-plug event paths with
respect to final quirks. From an architectural perspective, the proper
placement for the final quirks is somewhere just prior to, or within, the
device_add path.
I originally started with that approach but eventually realized that there
are issues with moving the quirks into the device_add path with respect to
the boot path. Currently, the boot path scans the PCI devices, adds the
devices, assigns resources, and then call the final quirks whereas the
hot-plug paths scan, assign resources, and then add the devices which is
better sequencing with respect to the assignment of resources and the
addition of devices.
All of this suggests that we should change PCI device setup in the boot
path to be more like hot-plug: scan, assign resources, (final fixups),
then add. While I think that is the correct approach, and something that
we should be addressing, it will require a lot of work. So until that
occurs, this series should serve as a stop-gap solution for the interim by
keeping the current boot path sequencing in place and then adding the
final quirk processing into the device_add path for hot-plug events via a
(temporary) global variable qualifier.
When the boot path's PCI device setup is addressed we should end up with a
single, uniform, device_add based solution for applying final quirks
by:
o removing 'fs_initcall_sync(pci_apply_final_quirks);',
o removing the global variable 'pci_fixup_final_inited' and all
of its usages,
o renaming, and moving, the 'pci_cache_line_size' related code
currently embedded in 'pci_apply_final_quirks()'.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/pci/bus.c | 4 ++++
drivers/pci/quirks.c | 18 ++++++++++++++++++
2 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 4ce5ef2..b511bd4 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -164,6 +164,10 @@ pci_bus_alloc_resource(struct pci_bus *bus, struct resource *res,
int pci_bus_add_device(struct pci_dev *dev)
{
int retval;
+ extern bool pci_fixup_final_inited;
+
+ if (pci_fixup_final_inited)
+ pci_fixup_device(pci_fixup_final, dev);
retval = device_add(&dev->dev);
if (retval)
return retval;
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 815c009..79c3351 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -3030,6 +3030,22 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
}
EXPORT_SYMBOL(pci_fixup_device);
+
+/*
+ * The global variable 'pci_fixup_final_inited' is being used as a interim
+ * solution for calling the final quirks only during hot-plug events (not
+ * during boot processing).
+ *
+ * When the boot path's PCI device setup sequencing is addressed, we can
+ * remove the instance, and usages of, 'pci_fixup_final_inited' along with
+ * removing 'fs_initcall_sync(pci_apply_final_quirks);' and end up with a
+ * single, uniform, solution that satisfies both the boot path and the
+ * various hot-plug event paths.
+ *
+ * ToDo: Remove 'pci_fixup_final_inited'
+ */
+bool pci_fixup_final_inited;
+
static int __init pci_apply_final_quirks(void)
{
struct pci_dev *dev = NULL;
@@ -3060,6 +3076,8 @@ static int __init pci_apply_final_quirks(void)
pci_cache_line_size = pci_dfl_cache_line_size;
}
}
+ pci_fixup_final_inited = 1;
+
if (!pci_cache_line_size) {
printk(KERN_DEBUG "PCI: CLS %u bytes, default %u\n",
cls << 2, pci_dfl_cache_line_size << 2);
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/pci/quirks.c | 31 ++++++++++++++++---------------
1 files changed, 16 insertions(+), 15 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 1a7effa..815c009 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -57,7 +57,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_MELLANOX,PCI_DEVICE_ID_MELLANOX_TAVOR_BRID
/* Deal with broken BIOS'es that neglect to enable passive release,
which can cause problems in combination with the 82441FX/PPro MTRRs */
-static void quirk_passive_release(struct pci_dev *dev)
+static void __devinit quirk_passive_release(struct pci_dev *dev)
{
struct pci_dev *d = NULL;
unsigned char dlc;
@@ -173,7 +173,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82439TX, quir
* Updated based on further information from the site and also on
* information provided by VIA
*/
-static void quirk_vialatency(struct pci_dev *dev)
+static void __devinit quirk_vialatency(struct pci_dev *dev)
{
struct pci_dev *p;
u8 busarb;
@@ -733,7 +733,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_TI, PCI_DEVICE_ID_TI_XIO2000A,
* TODO: When we have device-specific interrupt routers,
* this code will go away from quirks.
*/
-static void quirk_via_ioapic(struct pci_dev *dev)
+static void __devinit quirk_via_ioapic(struct pci_dev *dev)
{
u8 tmp;
@@ -757,7 +757,7 @@ DECLARE_PCI_FIXUP_RESUME_EARLY(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C686, quir
* Set this bit to get rid of cycle wastage.
* Otherwise uncritical.
*/
-static void quirk_via_vt8237_bypass_apic_deassert(struct pci_dev *dev)
+static void __devinit quirk_via_vt8237_bypass_apic_deassert(struct pci_dev *dev)
{
u8 misc_control2;
#define BYPASS_APIC_DEASSERT 8
@@ -939,7 +939,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_VIA, PCI_DEVICE_ID_VIA_82C597_0, quirk_vt
* do this even if the Linux CardBus driver is not loaded, because
* the Linux i82365 driver does not (and should not) handle CardBus.
*/
-static void quirk_cardbus_legacy(struct pci_dev *dev)
+static void __devinit quirk_cardbus_legacy(struct pci_dev *dev)
{
pci_write_config_dword(dev, PCI_CB_LEGACY_MODE_BASE, 0);
}
@@ -955,7 +955,7 @@ DECLARE_PCI_FIXUP_CLASS_RESUME_EARLY(PCI_ANY_ID, PCI_ANY_ID,
* To be fair to AMD, it follows the spec by default, its BIOS people
* who turn it off!
*/
-static void quirk_amd_ordering(struct pci_dev *dev)
+static void __devinit quirk_amd_ordering(struct pci_dev *dev)
{
u32 pcic;
pci_read_config_dword(dev, 0x4C, &pcic);
@@ -1005,7 +1005,7 @@ DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_TOSHIBA, 0x605, quirk_transparent_bridge)
* datasheets found at http://www.national.com/analog for info on what
* these bits do. <[email protected]>
*/
-static void quirk_mediagx_master(struct pci_dev *dev)
+static void __devinit quirk_mediagx_master(struct pci_dev *dev)
{
u8 reg;
pci_read_config_byte(dev, 0x41, ®);
@@ -1023,7 +1023,7 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_CYRIX, PCI_DEVICE_ID_CYRIX_PCI_MASTER, qu
* the BIOS but in the odd case it is not the results are corruption
* hence the presence of a Linux check
*/
-static void quirk_disable_pxb(struct pci_dev *pdev)
+static void __devinit quirk_disable_pxb(struct pci_dev *pdev)
{
u16 config;
@@ -1591,7 +1591,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_PXHV, quirk_pci
* Some Intel PCI Express chipsets have trouble with downstream
* device power management.
*/
-static void quirk_intel_pcie_pm(struct pci_dev * dev)
+static void __devinit quirk_intel_pcie_pm(struct pci_dev *dev)
{
pci_pm_d3_delay = 120;
dev->no_d1d2 = 1;
@@ -1626,7 +1626,8 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x260b, quirk_intel_pcie_pm);
* that a PCI device's interrupt handler is installed on the boot interrupt
* line instead.
*/
-static void quirk_reroute_to_boot_interrupts_intel(struct pci_dev *dev)
+static void __devinit
+quirk_reroute_to_boot_interrupts_intel(struct pci_dev *dev)
{
if (noioapicquirk || noioapicreroute)
return;
@@ -1664,7 +1665,7 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_80332_1, quirk
#define INTEL_6300_IOAPIC_ABAR 0x40
#define INTEL_6300_DISABLE_BOOT_IRQ (1<<14)
-static void quirk_disable_intel_boot_interrupt(struct pci_dev *dev)
+static void __devinit quirk_disable_intel_boot_interrupt(struct pci_dev *dev)
{
u16 pci_config_word;
@@ -1689,7 +1690,7 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_ESB_10, qui
#define BC_HT1000_MAP_IDX 0xC00
#define BC_HT1000_MAP_DATA 0xC01
-static void quirk_disable_broadcom_boot_interrupt(struct pci_dev *dev)
+static void __devinit quirk_disable_broadcom_boot_interrupt(struct pci_dev *dev)
{
u32 pci_config_dword;
u8 irq;
@@ -1727,7 +1728,7 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_SERVERWORKS, PCI_DEVICE_ID_SERVERWORKS_
#define AMD_813X_REV_B1 0x12
#define AMD_813X_REV_B2 0x13
-static void quirk_disable_amd_813x_boot_interrupt(struct pci_dev *dev)
+static void __devinit quirk_disable_amd_813x_boot_interrupt(struct pci_dev *dev)
{
u32 pci_config_dword;
@@ -1751,7 +1752,7 @@ DECLARE_PCI_FIXUP_RESUME(PCI_VENDOR_ID_AMD, PCI_DEVICE_ID_AMD_8132_BRIDGE, quirk
#define AMD_8111_PCI_IRQ_ROUTING 0x56
-static void quirk_disable_amd_8111_boot_interrupt(struct pci_dev *dev)
+static void __devinit quirk_disable_amd_8111_boot_interrupt(struct pci_dev *dev)
{
u16 pci_config_word;
@@ -1989,7 +1990,7 @@ DECLARE_PCI_FIXUP_FINAL(PCI_VENDOR_ID_INTEL, 0x1460, quirk_p64h2_1k_io_fix_iobl
* Force it to be linked by setting the corresponding control bit in the
* config space.
*/
-static void quirk_nvidia_ck804_pcie_aer_ext_cap(struct pci_dev *dev)
+static void __devinit quirk_nvidia_ck804_pcie_aer_ext_cap(struct pci_dev *dev)
{
uint8_t b;
if (pci_read_config_byte(dev, 0xf41, &b) == 0) {
The PCI subsystem's final fixups are executed once during boot, after the
pci-device is found. As long as the system does not support hot-plug,
specifying __init is fine.
With hot-plug, either physically based hot-plug events or pseudo hot-plug
events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
PCI bus during run time and have it rediscovered which will require the
call of the fixups again in order for the device to function properly.
This patch prepares specific quirk(s) for use with hot-plug events.
Signed-off-by: Myron Stowe <[email protected]>
---
arch/powerpc/platforms/powermac/pci.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/arch/powerpc/platforms/powermac/pci.c b/arch/powerpc/platforms/powermac/pci.c
index 43bbe1b..6b09dd7 100644
--- a/arch/powerpc/platforms/powermac/pci.c
+++ b/arch/powerpc/platforms/powermac/pci.c
@@ -1173,7 +1173,7 @@ void __init pmac_pcibios_after_init(void)
}
}
-void pmac_pci_fixup_cardbus(struct pci_dev* dev)
+static void __devinit pmac_pci_fixup_cardbus(struct pci_dev *dev)
{
if (!machine_is(powermac))
return;
__nv_msi_ht_cap_quirk() acquires a temporary reference via
'pci_get_bus_and_slot()' that is never released.
This patch releases the temporary reference.
Signed-off-by: Myron Stowe <[email protected]>
---
drivers/pci/quirks.c | 7 +++++--
1 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 9c93558..1a7effa 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -2541,15 +2541,18 @@ static void __devinit __nv_msi_ht_cap_quirk(struct pci_dev *dev, int all)
else
nv_ht_enable_msi_mapping(dev);
}
- return;
+ goto out;
}
/* HT MSI is not enabled */
if (found == 1)
- return;
+ goto out;
/* Host bridge is not to HT, disable HT MSI mapping on this device */
ht_disable_msi_mapping(dev);
+
+out:
+ pci_dev_put(host_bridge);
}
static void __devinit nv_msi_ht_cap_quirk_all(struct pci_dev *dev)
On Thu, Jun 21, 2012 at 2:24 PM, Myron Stowe <[email protected]> wrote:
> When the boot argument 'initcall_debug' is specified, redundant debug
> output occurs for each device as a quirk is applied:
> ?...
> ?pci 0000:00:1a.0: calling quirk_usb_early_handoff+0x0/0x620
> ?calling ?quirk_usb_early_handoff+0x0/0x620 @ 1 for 0000:00:1a.0
> ?pci fixup quirk_usb_early_handoff+0x0/0x620 returned after 32 usecs for 0000:00: 1a.0
> ?...
>
> This patch removes the redundancy by eliminating the first debug output
> occurence in the sequence shown above when 'initcall_debug' is specified.
Here's what I don't like about this: adding "initcall_debug" *removes*
some output. My expectation is that it would only *add* output.
> Signed-off-by: Myron Stowe <[email protected]>
> ---
>
> ?drivers/pci/quirks.c | ? ?5 +++--
> ?1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index a2d9d33..9c93558 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -2953,11 +2953,12 @@ static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> ? ? ? ? ? ? ? ? ? ? f->vendor == (u16) PCI_ANY_ID) &&
> ? ? ? ? ? ? ? ? ? ?(f->device == dev->device ||
> ? ? ? ? ? ? ? ? ? ? f->device == (u16) PCI_ANY_ID)) {
> - ? ? ? ? ? ? ? ? ? ? ? dev_dbg(&dev->dev, "calling %pF\n", f->hook);
> ? ? ? ? ? ? ? ? ? ? ? ?if (initcall_debug)
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?do_one_fixup_debug(f->hook, dev);
> - ? ? ? ? ? ? ? ? ? ? ? else
> + ? ? ? ? ? ? ? ? ? ? ? else {
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? dev_dbg(&dev->dev, "calling %pF\n", f->hook);
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?f->hook(dev);
This part isn't something you changed, but I also think it's a bit
ugly that we have two possible call sites for the quirk: either inside
do_one_fixup_debug() or directly in pci_do_fixups(). I wonder if this
could be restructured a bit in the style of initcall_debug_start() and
initcall_debug_report(), so we could have this:
ktime_t calltime;
calltime = initcall_debug_start(dev);
f->hook(dev);
initcall_debug_report(dev, calltime);
where initcall_debug_report() would only print something when
initcall_debug is enabled.
> + ? ? ? ? ? ? ? ? ? ? ? }
> ? ? ? ? ? ? ? ?}
> ?}
On Thu, Jun 21, 2012 at 2:24 PM, Myron Stowe <[email protected]> wrote:
> The PCI subsystem's final fixups are executed once during boot, after the
> pci-device is found. ?As long as the system does not support hot-plug,
> specifying __init is fine.
>
> With hot-plug, either physically based hot-plug events or pseudo hot-plug
> events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
> PCI bus during run time and have it rediscovered which will require the
> call of the fixups again in order for the device to function properly.
>
> This patch prepares specific quirk(s) for use with hot-plug events.
>
> Signed-off-by: Myron Stowe <[email protected]>
> ---
>
> ?arch/arm/mach-iop32x/n2100.c | ? ?2 +-
> ?1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
> index 5a7ae91..04c4110 100644
> --- a/arch/arm/mach-iop32x/n2100.c
> +++ b/arch/arm/mach-iop32x/n2100.c
> @@ -126,7 +126,7 @@ static struct hw_pci n2100_pci __initdata = {
> ?* the ->broken_parity_status flag for both ports so that the r8169
> ?* driver knows it should ignore error interrupts.
> ?*/
> -static void n2100_fixup_r8169(struct pci_dev *dev)
> +static void __devinit n2100_fixup_r8169(struct pci_dev *dev)
These actually move functions from normal text to __devinit, not from
__init to __devinit.
That should be safe for most quirks (although enable, suspend, resume,
and resume_early quirks can be called after we free initmem, even if
we have CONFIG_HOTPLUG=n), but given the discussion about whether we
should even bother with __devinit any more, I think I'll hold off on
these for now. If we were to get rid of __devinit, these would all
have to revert to being normal text.
> ?{
> ? ? ? ?if (dev->bus->number == 0 &&
> ? ? ? ? ? ?(dev->devfn == PCI_DEVFN(1, 0) ||
>
On Tue, 2012-06-26 at 16:33 -0600, Bjorn Helgaas wrote:
> On Thu, Jun 21, 2012 at 2:24 PM, Myron Stowe <[email protected]> wrote:
> > The PCI subsystem's final fixups are executed once during boot, after the
> > pci-device is found. As long as the system does not support hot-plug,
> > specifying __init is fine.
> >
> > With hot-plug, either physically based hot-plug events or pseudo hot-plug
> > events such as "echo 1 > /sys/bus/pci/rescan", it is possible to remove a
> > PCI bus during run time and have it rediscovered which will require the
> > call of the fixups again in order for the device to function properly.
> >
> > This patch prepares specific quirk(s) for use with hot-plug events.
> >
> > Signed-off-by: Myron Stowe <[email protected]>
> > ---
> >
> > arch/arm/mach-iop32x/n2100.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/arch/arm/mach-iop32x/n2100.c b/arch/arm/mach-iop32x/n2100.c
> > index 5a7ae91..04c4110 100644
> > --- a/arch/arm/mach-iop32x/n2100.c
> > +++ b/arch/arm/mach-iop32x/n2100.c
> > @@ -126,7 +126,7 @@ static struct hw_pci n2100_pci __initdata = {
> > * the ->broken_parity_status flag for both ports so that the r8169
> > * driver knows it should ignore error interrupts.
> > */
> > -static void n2100_fixup_r8169(struct pci_dev *dev)
> > +static void __devinit n2100_fixup_r8169(struct pci_dev *dev)
>
> These actually move functions from normal text to __devinit, not from
> __init to __devinit.
Yes (as I hang my head sheepishly). What got me started down this path
was seeing the recent series by Sabastian Andrzej
( http://marc.info/?l=linux-pci&m=133875179103880&w=2 ).
>
> That should be safe for most quirks (although enable, suspend, resume,
> and resume_early quirks can be called after we free initmem, even if
> we have CONFIG_HOTPLUG=n), but given the discussion about whether we
> should even bother with __devinit any more, I think I'll hold off on
> these for now. If we were to get rid of __devinit, these would all
> have to revert to being normal text.
Agreed, and I've only recently (after posting this series) became aware
of James Bottomley's thoughts about __devinit so lets skip these changes
within the series. I don't want to start a lot of unnecessary churn if
we end up just dropping __devinit altogether.
It's the last patch in the series that is of the most interest - trying
to solve PCI's "final" quirks with respect to hot-plug events and the
asymmetry I ended up discovering while working that issue. Wish we
would have received some feedback from others about that.
Thanks,
Myron
Thanks,
Myron
>
> > {
> > if (dev->bus->number == 0 &&
> > (dev->devfn == PCI_DEVFN(1, 0) ||
> >
On Tue, 2012-06-26 at 16:26 -0600, Bjorn Helgaas wrote:
> On Thu, Jun 21, 2012 at 2:24 PM, Myron Stowe <[email protected]> wrote:
> > When the boot argument 'initcall_debug' is specified, redundant debug
> > output occurs for each device as a quirk is applied:
> > ...
> > pci 0000:00:1a.0: calling quirk_usb_early_handoff+0x0/0x620
> > calling quirk_usb_early_handoff+0x0/0x620 @ 1 for 0000:00:1a.0
> > pci fixup quirk_usb_early_handoff+0x0/0x620 returned after 32 usecs for 0000:00: 1a.0
> > ...
> >
> > This patch removes the redundancy by eliminating the first debug output
> > occurence in the sequence shown above when 'initcall_debug' is specified.
>
> Here's what I don't like about this: adding "initcall_debug" *removes*
> some output. My expectation is that it would only *add* output.
Well, the point to me was that it only removed output that was
completely redundant when "initcall_debug" was true.
I *really* wanted to change the "initcall_debug" output to follow the
convention already in place via 'dev_dbg()' but did not as I'm vaguely
aware of some scripting that parses "initcall_debug" output and did not
want to break that.
>
> > Signed-off-by: Myron Stowe <[email protected]>
> > ---
> >
> > drivers/pci/quirks.c | 5 +++--
> > 1 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> > index a2d9d33..9c93558 100644
> > --- a/drivers/pci/quirks.c
> > +++ b/drivers/pci/quirks.c
> > @@ -2953,11 +2953,12 @@ static void pci_do_fixups(struct pci_dev *dev, struct pci_fixup *f,
> > f->vendor == (u16) PCI_ANY_ID) &&
> > (f->device == dev->device ||
> > f->device == (u16) PCI_ANY_ID)) {
> > - dev_dbg(&dev->dev, "calling %pF\n", f->hook);
> > if (initcall_debug)
> > do_one_fixup_debug(f->hook, dev);
> > - else
> > + else {
> > + dev_dbg(&dev->dev, "calling %pF\n", f->hook);
> > f->hook(dev);
>
> This part isn't something you changed, but I also think it's a bit
> ugly that we have two possible call sites for the quirk: either inside
> do_one_fixup_debug() or directly in pci_do_fixups(). I wonder if this
> could be restructured a bit in the style of initcall_debug_start() and
> initcall_debug_report(), so we could have this:
>
> ktime_t calltime;
>
> calltime = initcall_debug_start(dev);
> f->hook(dev);
> initcall_debug_report(dev, calltime);
>
> where initcall_debug_report() would only print something when
> initcall_debug is enabled.
>
Nice observation! Such a change would yield a better result. Perhaps,
transitioning to this approach would also eventually help with my
earlier peeve concerning the redundant output.
Thanks,
Myron
> > + }
> > }
> > }