Skip to contentSkip to conversation

PCI, x86: fix default vga ref_count

PATCH In review Yinghai Lu 5 participants 17 messages Sep 15, 2012 – Sep 23, 2012 1 message not in the archive

Subsystems (5)
DRM drivers x86 DRM misc ACPI +1 more subsystems
lore
Share & tools
lore

Yinghai Lu

author
#0 lore raw

PATCH 1/1 PCI, x86: fix default vga ref_count

when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used,

for static path, vga_default setting is through vga_arbiter_add_pci_device. and for x86 pci_fixup_video, will skip that.
because subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device.

for hotplug path, even vga_arbiter_add_pci_device is called via notifier, but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. So x86 pci_fixup_video will take over to call vga_set_default_device().

We need to hold one dev reference there.

otherwise vga_arbiter_del_pci_device that does not check VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to decrease extra. that will have that device get deleted early wrongly.

Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/fixup.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
arch/x86/pci/fixup.c +5 -1
Index: linux-2.6/arch/x86/pci/fixup.c
===================================================================
@@ -349,8 +349,12 @@ static void __devinit pci_fixup_video(st
 	if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
 		pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
 		dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
-		if (!vga_default_device())
+		if (!vga_default_device()) {
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+			pdev = pci_dev_get(pdev);
+#endif
 			vga_set_default_device(pdev);
+		}
 	}
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
author
#1 lore raw

PATCH 1/1 PCI, x86: clear initial value for root info resources

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/acpi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/x86/pci/acpi.c +1 -2
Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
@@ -305,7 +305,6 @@ setup_resource(struct acpi_resource *acp
 	res->flags = flags;
 	res->start = start;
 	res->end = end;
-	res->child = NULL;
 
 	if (!pci_use_crs) {
 		dev_printk(KERN_DEBUG, &info->bridge->dev,

@@ -434,7 +433,7 @@ probe_pci_root_info(struct pci_root_info
 
 	size = sizeof(*info->res) * info->res_num;
 	info->res_num = 0;
-	info->res = kmalloc(size, GFP_KERNEL);
+	info->res = kzalloc(size, GFP_KERNEL);
 	if (!info->res)
 		return;
maintainer
#2 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

Quoted 11 lines · Yinghai Lu

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Don't we need the same fix for ia64 in pci_acpi_scan_root()? Here's what it does:

        if (windows) {
                controller->window =
                        kmalloc_node(sizeof(*controller->window) * windows,
                                     GFP_KERNEL, controller->node);
Quoted 28 lines
Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/acpi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================

arch/x86/pci/acpi.c +1 -2
@@ -305,7 +305,6 @@ setup_resource(struct acpi_resource *acp
        res->flags = flags;
        res->start = start;
        res->end = end;
-       res->child = NULL;
 
        if (!pci_use_crs) {
                dev_printk(KERN_DEBUG, &info->bridge->dev,

@@ -434,7 +433,7 @@ probe_pci_root_info(struct pci_root_info
 
        size = sizeof(*info->res) * info->res_num;
        info->res_num = 0;
-       info->res = kmalloc(size, GFP_KERNEL);
+       info->res = kzalloc(size, GFP_KERNEL);
        if (!info->res)
                return;
author
#3 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · Yinghai Lu

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Quoted 8 lines

Don't we need the same fix for ia64 in pci_acpi_scan_root()? Here's what it does:

        if (windows) {
                controller->window =
                        kmalloc_node(sizeof(*controller->window) * windows,
                                     GFP_KERNEL, controller->node);

yes, but they don't support pci_set_host_bridge_release yet. so they should not meet this problem yet.

Thanks

Yinghai

maintainer
#4 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

Quoted 24 lines · Yinghai Lu
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · Yinghai Lu

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Quoted 8 lines

Don't we need the same fix for ia64 in pci_acpi_scan_root()? Here's what it does:

        if (windows) {
                controller->window =
                        kmalloc_node(sizeof(*controller->window) * windows,
                                     GFP_KERNEL, controller->node);
Quoted 2 lines

yes, but they don't support pci_set_host_bridge_release yet. so they should not meet this problem yet.

Why should we wait to fix that bug until later? I'm not interested in debugging this in the future. If you fix a bug, you should always try to fix other occurrences of the same bug at the same time.

author
#5 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

Quoted 6 lines · Bjorn Helgaas
Quoted 2 lines

yes, but they don't support pci_set_host_bridge_release yet. so they should not meet this problem yet.

Why should we wait to fix that bug until later? I'm not interested in debugging this in the future. If you fix a bug, you should always try to fix other occurrences of the same bug at the same time.

ok, will update the patch.

author
#6 lore raw

PATCH 1/1 PCI, x86: clear initial value for root info resources

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/acpi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
arch/x86/pci/acpi.c +1 -2
Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================
@@ -305,7 +305,6 @@ setup_resource(struct acpi_resource *acp
 	res->flags = flags;
 	res->start = start;
 	res->end = end;
-	res->child = NULL;
 
 	if (!pci_use_crs) {
 		dev_printk(KERN_DEBUG, &info->bridge->dev,

@@ -434,7 +433,7 @@ probe_pci_root_info(struct pci_root_info
 
 	size = sizeof(*info->res) * info->res_num;
 	info->res_num = 0;
-	info->res = kmalloc(size, GFP_KERNEL);
+	info->res = kzalloc(size, GFP_KERNEL);
 	if (!info->res)
 		return;
author
#7 lore raw

PATCH 1/1 PCI, ia64: clear initial value for root info resources

after
PCI, ia64: clear initial value for root info resources

change ia64 code accordingly, because later ia64 will support root bus removal too.

Signed-off-by Yinghai Lu <[email protected]>
Cc Tony Luck <[email protected]>
Cc Fenghua Yu <[email protected]>
Cc [email protected]
---
 arch/ia64/pci/pci.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-2.6/arch/ia64/pci/pci.c =================================--- linux-2.6.orig/arch/ia64/pci/pci.c +++ linux-2.6/arch/ia64/pci/pci.c
@@ -295,7 +295,6 @@ static __devinit acpi_status add_window(
window->resource.flags = flags;
window->resource.start = addr.minimum + offset; window->resource.end = window->resource.start + addr.address_length - 1;
- window->resource.child = NULL;
window->offset = offset;

if (insert_resource(root, &window->resource)) {
@@ -357,7 +356,7 @@ pci_acpi_scan_root(struct acpi_pci_root
&windows);
if (windows) {
controller->window - kmalloc_node(sizeof(*controller->window) * windows,
+ kzalloc_node(sizeof(*controller->window) * windows,
GFP_KERNEL, controller->node);
if (!controller->window)
goto out2;

#8 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

In reply to Yinghai Lu
Quoted 1 line · Yinghai Lu

Found one system one root bus hot remove get panic.

Boy, this is mangled.

Can you include the name of the system? Perhaps you can rephrase this to say:

"On XYZ removing the root bus (through ACPI unplug) causes a panic."

Quoted 1 line

Panic happens when try to release hostbridge resource.

Can you include the stack trace?

Quoted 3 lines

It turns out that resource get reject during put into resource tree because of conflicts.

Come again? Are you saying:

"The reason for this is that the resources was never initialized properly because of .. (what type of conflict?)"?

Quoted 6 lines

Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

It is not just try, it _is_ using that now.

Quoted 35 lines
Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/acpi.c |    3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Index: linux-2.6/arch/x86/pci/acpi.c
===================================================================

arch/x86/pci/acpi.c +1 -3
@@ -305,7 +305,6 @@ setup_resource(struct acpi_resource *acp
 	res->flags = flags;
 	res->start = start;
 	res->end = end;
-	res->child = NULL;
 
 	if (!pci_use_crs) {
 		dev_printk(KERN_DEBUG, &info->bridge->dev,

@@ -434,7 +433,7 @@ probe_pci_root_info(struct pci_root_info
 
 	size = sizeof(*info->res) * info->res_num;
 	info->res_num = 0;
-	info->res = kmalloc(size, GFP_KERNEL);
+	info->res = kzalloc(size, GFP_KERNEL);
 	if (!info->res)
 		return;
 
--

To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

A message that is not in the archive (sent off-list or not yet received): CAE9FiQV9WK4NG5+aGwVrGO3ueFH3TmmmG5zea+JjwgtQyngNRg@mail.gmail.com

#10 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

Quoted 73 lines · Yinghai Lu
Quoted 3 lines · Konrad Rzeszutek Wilk

On Wed, Sep 19, 2012 at 10:49:00AM -0700, Yinghai Lu wrote:

Can you include the stack trace?

[  414.997281] pci_bus 0000:00: freeing pci_bus info
[  415.017221]  pci0000:00: freeing pci_host_bridge info
[  415.017468] general protection fault: 0000 [#1] SMP
[  415.036758] Modules linked in:
[  415.036959] CPU 0
[  415.037051] Pid: 4, comm: kworker/0:0 Not tainted
3.6.0-rc6-yh-03463-gbcab6f1-dirty #303 Oracle Corporation  Sun Fire
X4800 M2 /
[  415.056989] RIP: 0010:[<ffffffff81075218>]  [<ffffffff81075218>]
__release_resource+0x11/0x3e
[  415.076854] RSP: 0018:ffff883f4cea39f0  EFLAGS: 00010206
[  415.077112] RAX: 0d00000000000030 RBX: ffff897ea6cd4070 RCX: 8c6318c6318c6320
[  415.097000] RDX: ffff883f9d60e190 RSI: ffffffff8265f228 RDI: ffff897ea6cd4070
[  415.116627] RBP: ffff883f4cea39f0 R08: 0000000000000005 R09: ffffffff810755de
[  415.116953] R10: ffffffff81e2e7df R11: 0000000000000000 R12: 0000000000000002
[  415.136790] R13: ffff897ea6d18600 R14: ffffffff826f18c0 R15: ffff883f9d60d800
[  415.156541] FS:  0000000000000000(0000) GS:ffff883f9d600000(0000)
knlGS:0000000000000000
[  415.156953] CS:  0010 DS: 0000 ES: 0000 CR0: 000000008005003b
[  415.176622] CR2: 0000000000619718 CR3: 000000000263f000 CR4: 00000000000007f0
[  415.196401] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[  415.196761] DR3: 0000000000000000 DR6: 00000000ffff0ff0 DR7: 0000000000000400
[  415.216702] Process kworker/0:0 (pid: 4, threadinfo
ffff883f4cea2000, task ffff883f4cea8000)
[  415.236370] Stack:
[  415.236475]  ffff883f4cea3a20 ffffffff810755e6 ffff883f4cea3a60
ffff883f00000000
[  415.236866]  ffffffff824884ad ffff88fea6ce2440 ffff883f4cea3a40
ffffffff81c2cfc5
[  415.256682]  ffff88bf4a1ec800 ffff88bf4a1ec800 ffff883f4cea3a50
ffffffff81c2d023
[  415.276452] Call Trace:
[  415.276611]  [<ffffffff810755e6>] release_resource+0x25/0x3e
[  415.296206]  [<ffffffff81c2cfc5>] __release_pci_root_info+0x35/0x7e
[  415.296556]  [<ffffffff81c2d023>] release_pci_root_info+0x15/0x1a
[  415.316249]  [<ffffffff813f568a>] pci_release_bus_bridge_dev+0x37/0x4f
[  415.316581]  [<ffffffff815c04d8>] device_release+0x93/0xcb
[  415.336365]  [<ffffffff813d5888>] kobject_release+0x51/0x67
[  415.336666]  [<ffffffff813d579a>] kobject_put+0x4a/0x4e
[  415.361466]  [<ffffffff815c026a>] put_device+0x17/0x19
[  415.361734]  [<ffffffff813f75b2>] pci_remove_root_bus+0x5a/0x64
[  415.376188]  [<ffffffff81437dd8>] acpi_pci_root_remove+0xeb/0x114
[  415.376509]  [<ffffffff81434c13>] acpi_device_remove+0x86/0xa5
[  415.396230]  [<ffffffff815c33b6>] __device_release_driver+0x86/0xdc
[  415.396551]  [<ffffffff815c3431>] device_release_driver+0x25/0x32
[  415.416198]  [<ffffffff8143658b>] acpi_bus_trim+0x86/0x110
[  415.416473]  [<ffffffff81439ae1>] handle_root_bridge_removal+0x4f/0x118
[  415.436227]  [<ffffffff81439c8b>] _handle_hotplug_event_root+0xe1/0x121
[  415.455957]  [<ffffffff81085515>] process_one_work+0x28a/0x453
[  415.456228]  [<ffffffff81085480>] ? process_one_work+0x1f5/0x453
[  415.475980]  [<ffffffff81439baa>] ? handle_root_bridge_removal+0x118/0x118
[  415.476311]  [<ffffffff81085bd1>] worker_thread+0x12a/0x201
[  415.495995]  [<ffffffff81085aa7>] ? manage_workers+0xed/0xed
[  415.496301]  [<ffffffff8108b93a>] kthread+0xe7/0xef
[  415.515957]  [<ffffffff81e2ef03>] ? _raw_spin_unlock_irq+0x2e/0x33
[  415.516257]  [<ffffffff81e372e4>] kernel_thread_helper+0x4/0x10
[  415.535926]  [<ffffffff81e2f259>] ? retint_restore_args+0xe/0xe
[  415.536242]  [<ffffffff8108b853>] ? __init_kthread_worker+0x5a/0x5a
[  415.555959]  [<ffffffff81e372e0>] ? gs_change+0xb/0xb
[  415.556209] Code: 48 89 31 48 89 46 20 31 c0 eb 0d 48 39 7a 08 48
8d 4a 28 72 da 48 89 d0 5d c3 66 66 66 66 90 48 8b 47 20 55 48 89 e5
48 83 c0 30 <48> 8b 10 48 85 d2 74 1e 48 39 fa 75 13 48 8b 57 28 48 89
10 48
[  415.596509] RIP  [<ffffffff81075218>] __release_resource+0x11/0x3e
[  415.615741]  RSP <ffff883f4cea39f0>
[  415.616020] ---[ end trace 8bcbc1bea9c3e0bc ]---
Quoted 1 line

whole log is attached.

I meant it (and a filtered with just the valid lines in it) in terms of the patch commit description.

#11 lore raw

Re: [PATCH] PCI, x86: clear initial value for root info resources

In reply to Yinghai Lu
Quoted 24 lines · Yinghai Lu
Quoted 13 lines · Bjorn Helgaas
Quoted 11 lines · Yinghai Lu

Found one system one root bus hot remove get panic. Panic happens when try to release hostbridge resource.

It turns out that resource get reject during put into resource tree because of conflicts.
Also that resource parent pointer have random value.

That invalid value cause it pass through check __release_pci_root_info and panic in release_resource.

Try to use kzalloc instead.

Quoted 8 lines

Don't we need the same fix for ia64 in pci_acpi_scan_root()? Here's what it does:

        if (windows) {
                controller->window =
                        kmalloc_node(sizeof(*controller->window) * windows,
                                     GFP_KERNEL, controller->node);
Quoted 2 lines

yes, but they don't support pci_set_host_bridge_release yet. so they should not meet this problem yet.

Hi Yinghai,
We are trying to add pci_set_host_bridge_release() for IA64, so would
appreciate it if you could help to fix IA64 too.

	--Gerry
author
#12 lore raw

PATCH 1/1 PCI: Use correct type when freeing bus resource list

In reply to Yinghai Lu

Should use struct pci_bus_resource instead of struct pci_host_bridge_window

Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/pci/bus.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
drivers/pci/bus.c +5 -1
Index: linux-2.6/drivers/pci/bus.c
===================================================================
@@ -87,11 +87,15 @@ EXPORT_SYMBOL_GPL(pci_bus_resource_n);
 void pci_bus_remove_resources(struct pci_bus *bus)
 {
 	int i;
+	struct pci_bus_resource *bus_res, *tmp;
 
 	for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++)
 		bus->resource[i] = NULL;
 
-	pci_free_resource_list(&bus->resources);
+	list_for_each_entry_safe(bus_res, tmp, &bus->resources, list) {
+		list_del(&bus_res->list);
+		kfree(bus_res);
+	}
 }
 
 /**
maintainer
#13 lore raw

Re: [PATCH] PCI: Use correct type when freeing bus resource list

Quoted 1 line · Yinghai Lu

Should use struct pci_bus_resource instead of struct pci_host_bridge_window

Looks good, and this is all my fault, sorry. Applied to a pci/yinghai-misc branch.

Quoted 27 lines
Signed-off-by Yinghai Lu <[email protected]>
---
 drivers/pci/bus.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/drivers/pci/bus.c
===================================================================

drivers/pci/bus.c +5 -1
@@ -87,11 +87,15 @@ EXPORT_SYMBOL_GPL(pci_bus_resource_n);
 void pci_bus_remove_resources(struct pci_bus *bus)
 {
        int i;
+       struct pci_bus_resource *bus_res, *tmp;
 
        for (i = 0; i < PCI_BRIDGE_RESOURCE_NUM; i++)
                bus->resource[i] = NULL;
 
-       pci_free_resource_list(&bus->resources);
+       list_for_each_entry_safe(bus_res, tmp, &bus->resources, list) {
+               list_del(&bus_res->list);
+               kfree(bus_res);
+       }
 }
 
 /**
maintainer
#14 lore raw
In reply to Yinghai Lu
Quoted 10 lines · Yinghai Lu

when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used,

for static path, vga_default setting is through vga_arbiter_add_pci_device. and for x86 pci_fixup_video, will skip that.
because subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device.

for hotplug path, even vga_arbiter_add_pci_device is called via notifier, but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. So x86 pci_fixup_video will take over to call vga_set_default_device().

Quoted 5 lines

We need to hold one dev reference there.

otherwise vga_arbiter_del_pci_device that does not check VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to decrease extra. that will have that device get deleted early wrongly.

I'm sure you're fixing a real bug, but this patch is completely unintelligible.

I tried to decipher the changelog and I failed. I tried to figure out the PCI reference counting from the vgaarb code, and I failed there too.

Apparently the reference is connected with the vga_default device, since that's what vga_arbiter_del_pci_device() checks, but vga_set_default_device() is blissfully ignorant of references (and it isn't used consistently anyway).

If you can do some rework to make this all make more sense, that would be great.

While you're at it, look at both the x86 and ia64 versions of pci_fixup_video(). They are 90% identical, and it's not clear why they should be different. In fact, it's not clear why there's a fixup for x86 and ia64 but not for any other architecture with PCI.

Quoted 25 lines
Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
---
 arch/x86/pci/fixup.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/arch/x86/pci/fixup.c
===================================================================

arch/x86/pci/fixup.c +5 -1
@@ -349,8 +349,12 @@ static void __devinit pci_fixup_video(st
        if (config & (PCI_COMMAND_IO | PCI_COMMAND_MEMORY)) {
                pdev->resource[PCI_ROM_RESOURCE].flags |= IORESOURCE_ROM_SHADOW;
                dev_printk(KERN_DEBUG, &pdev->dev, "Boot video device\n");
-               if (!vga_default_device())
+               if (!vga_default_device()) {
+#ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
+                       pdev = pci_dev_get(pdev);
+#endif
                        vga_set_default_device(pdev);
+               }
        }
 }
 DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_ANY_ID, PCI_ANY_ID,
author
#15 lore raw
Quoted 11 lines · Bjorn Helgaas
Quoted 10 lines · Yinghai Lu

when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used,

for static path, vga_default setting is through vga_arbiter_add_pci_device. and for x86 pci_fixup_video, will skip that.
because subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device.

for hotplug path, even vga_arbiter_add_pci_device is called via notifier, but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. So x86 pci_fixup_video will take over to call vga_set_default_device().

Quoted 7 lines
Quoted 5 lines

We need to hold one dev reference there.

otherwise vga_arbiter_del_pci_device that does not check VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to decrease extra. that will have that device get deleted early wrongly.

I'm sure you're fixing a real bug, but this patch is completely unintelligible.

yes, only can hit this bug while remove root bus with vga adapater.

or remove bridge with child pci device that is vga adapter.

for root bus removal, the less one ref_count will call that pci device get destroyed too early.

Quoted 9 lines

I tried to decipher the changelog and I failed. I tried to figure out the PCI reference counting from the vgaarb code, and I failed there too.

Apparently the reference is connected with the vga_default device, since that's what vga_arbiter_del_pci_device() checks, but vga_set_default_device() is blissfully ignorant of references (and it isn't used consistently anyway).

yes, could fix vga_set_default_device instead, but it has two versions
- no __ARCH_HAS_VGA_DEFAULT_DEVICE
- EFI version...

also there is one other user for vga switching...

Quoted 7 lines

If you can do some rework to make this all make more sense, that would be great.

While you're at it, look at both the x86 and ia64 versions of pci_fixup_video(). They are 90% identical, and it's not clear why they should be different. In fact, it's not clear why there's a fixup for x86 and ia64 but not for any other architecture with PCI.

ok, will give it a try.

Thanks

Yinghai

author
#16 lore raw

PATCH 1/1 PCI: fix default vga ref_count

In reply to Bjorn Helgaas

when __ARCH_HAS_VGA_DEFAULT_DEVICE is not defined, aka EFIFB is not used,
for static path, vga_default setting is through vga_arbiter_add_pci_device.
and later x86 pci_fixup_video, will skip setting again.
- subsys_initcall(vga_arb_device_init) come first to call vga_arbiter_add_pci_device. It will call pci_get_dev to hold one reference.

for hotplug add path, even vga_arbiter_add_pci_device is called via notifier, but it will check VGA_RSRC_LEGACY_MASK that is not set for hotplug path. So x86 pci_fixup_video will take over to call vga_set_default_device(). It will not hold one refrence.

Later for hotplug remove path, vga_arbiter_del_pci_device that does not check VGA_RSRC_LEGACY_MASK will call put_device and it will cause ref_count to decrease extra. that will have that pci device get deleted early wrongly.

Need to make get/put balance for both cases.

-v2: According to Bjorn, update vga_set_default_device instead...

Signed-off-by Yinghai Lu <[email protected]>
Cc [email protected]
Cc Dave Airlie <[email protected]>
Cc Andrew Morton <[email protected]>
Cc Julia Lawall <[email protected]>
Cc Matthew Garrett <[email protected]>
---
 drivers/gpu/vga/vgaarb.c |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)
drivers/gpu/vga/vgaarb.c +9 -5
Index: linux-2.6/drivers/gpu/vga/vgaarb.c
===================================================================
@@ -141,6 +141,12 @@ EXPORT_SYMBOL_GPL(vga_default_device);
 
 void vga_set_default_device(struct pci_dev *pdev)
 {
+	if (vga_default)
+		pci_dev_put(vga_default);
+
+	if (pdev)
+		pdev = pci_dev_get(pdev);
+
 	vga_default = pdev;
 }
 #endif

@@ -577,7 +583,7 @@ static bool vga_arbiter_add_pci_device(s
 #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
 	if (vga_default == NULL &&
 	    ((vgadev->owns & VGA_RSRC_LEGACY_MASK) == VGA_RSRC_LEGACY_MASK))
-		vga_default = pci_dev_get(pdev);
+		vga_set_default_device(pdev);
 #endif
 
 	vga_arbiter_check_bridge_sharing(vgadev);

@@ -613,10 +619,8 @@ static bool vga_arbiter_del_pci_device(s
 	}
 
 #ifndef __ARCH_HAS_VGA_DEFAULT_DEVICE
-	if (vga_default == pdev) {
-		pci_dev_put(vga_default);
-		vga_default = NULL;
-	}
+	if (vga_default == pdev)
+		vga_set_default_device(NULL);
 #endif
 
 	if (vgadev->decodes & (VGA_RSRC_LEGACY_IO | VGA_RSRC_LEGACY_MEM))
#17 lore raw

Re: [PATCH] PCI: fix default vga ref_count

drivers/gpu/vga/vgaarb.c EXPORT_SYMBOL_GPL(vga_default_device); · line 150

Quoted 9 lines · Yinghai Lu

void vga_set_default_device(struct pci_dev *pdev) {

drivers/gpu/vga/vgaarb.c +6 -0
+	if (vga_default)
+		pci_dev_put(vga_default);
+
+	if (pdev)
+		pdev = pci_dev_get(pdev);
+
 	vga_default = pdev;

It shouldn't happen, but do we want to check for the vga_default == pdev case?

Signature · 1 line
Matthew Garrett | [email protected]