From: Eran Liberty <liberty@extricom.com>
To: Matthew Wilcox <matthew@wil.cx>
Cc: eran liberty <eran.liberty@gmail.com>,
linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org,
Jesse Barnes <jbarnes@virtuousgeek.org>
Subject: Re: [PATCH 2.6.26] PCI: refuse to re-add a device to a bus upon pci_scan_child_bus()
Date: Wed, 23 Jul 2008 21:31:25 +0300 [thread overview]
Message-ID: <488778FD.1050206@extricom.com> (raw)
In-Reply-To: <20080722181132.GE7337@parisc-linux.org>
Matthew Wilcox wrote:
> Look at the consequences of calling fixup_resource() twice on the same
> resource:
>
> res->start = (res->start + offset) & mask;
> res->end = (res->end + offset) & mask;
>
> You'll add 'offset' to the other resources twice.
>
Tring to find heads and tails in the code, I dug in. This is what I understand from the overall flow... and my points of interests:
1. pci_scan_child_bus() starts with iterating over all the devfn and scanning for a device with that devfn
drivers/pci/probe.c
1056 unsigned int __devinit pci_scan_child_bus(struct pci_bus *bus)
1057 {
1063 /* Go find them, Rover! */
1064 for (devfn = 0; devfn < 0x100; devfn += 8)
1065 pci_scan_slot(bus, devfn);
2. pci_scan_slot() will continue to scan all the functions that devfn might have to over
drivers/pci/probe.c
1019 int pci_scan_slot(struct pci_bus *bus, int devfn)
1020 {
1026 for (func = 0; func < 8; func++, devfn++) {
1029 dev = pci_scan_single_device(bus, devfn);
3. pci_scan_single_device() will scan / test if this dev is valid.
drivers/pci/probe.c
996 struct pci_dev *__ref pci_scan_single_device(struct pci_bus *bus, int devfn)
997 {
998 struct pci_dev *dev;
999
1000 dev = pci_scan_device(bus, devfn);
and add it. REGARDLESS if it is already on the pci bus list or not.
1001 if (!dev)
1002 return NULL;
1003
1004 pci_device_add(dev, bus);
1005
1006 return dev;
1007 }
This is the first thing that needs to be fixed IMHO.
4. pci_scan_child_bus() will go on to fixup the bus whether it needs fixing or not.
drivers/pci/probe.c
1072 pcibios_fixup_bus(bus);
This might be the second thing that needs amending.
5. pcibios_fixup_bus(bus) calls _pcibios_fixup_bus which will call fixup_resource() once per a bus resource (in contrast of per device on that bus) and fixes the resources of that bus.
arch/powerpc/kernel/pci-common.c
784 static void __devinit __pcibios_fixup_bus(struct pci_bus *bus)
785 {
787 struct pci_dev *dev = bus->self;
798 for (i = 0; i < PCI_BUS_NUM_RESOURCES; ++i) {
833 fixup_resource(res, dev);
834 }
835 }
850 }
As you have correctly observed without other changes this would cause trouble on a bus that has resources.
Since i am not pulling the plug on the whole bus just on selected devices I can defensively skip this part
6. Now I should be able to call pci_bus_assign_resources(bus) which will go over all the newly added device and assign resource to them.
7. Last step I should be able to pci_bus_add_devices(bus) the, now, resource fixed devices.
Steps 6 and 7 are the one I miss most over which my device wont work after being re-born.
Soooo ...
If I really wanted to make it, working my way around the current code, I would have done something like this.
bus = null;
while ((bus = pci_find_next_bus(bus)) != NULL) {
for (devfn = 0; devfn < 0x100; devfn += 8) {
for (func = 0; func < 8; func++) {
struct pci_dev *dev = <http://liberty/lxr/ident?v=e500-linux-2.6.26-rc4;i=pci_get_slot>pci_get_slot(struct pci_bus *bus, unsigned int devfn);
if (dev)
continue;
dev = pci_scan_single_device(bus, devfn);
if (!dev)
continue;
pci_device_add(dev, bus);
}
}
pci_bus_assign_resources(bus);
pci_bus_add_devices(bus);
}
WOW.... first time for me to code in Mozilla Thunderbird.
Had I was this smart to begin with I would have done exactly that and go home to sleep like a decent person.
But since we are here, I feel there should be a either correction in the existing code to allow the:
pci_scan_child_bus(bus) -> pci_bus_assign_resources(bus) -> pci_bus_add_devices(bus) sequence.
Or a new helper function to the PCI that does more or less what I have just described.
If you want I can code it, patch it, and rip the glory :)
That was a long one :)
Liberty
next prev parent reply other threads:[~2008-07-23 18:35 UTC|newest]
Thread overview: 81+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-06-18 14:18 2.6.24-rc4: pci_remove_bus_device() => pci_scan_child_bus() => pci_bus_add_devices bug? Eran Liberty
2008-07-20 10:31 ` [PATCH 2.6.24-rc4] PCI: refuse to re-add a device to a bus upon pci_scan_child_bus() Eran Liberty
2008-07-20 16:48 ` Eran Liberty
2008-07-21 19:18 ` [PATCH 2.6.26] " Eran Liberty
2008-07-21 19:49 ` Matthew Wilcox
2008-07-22 8:21 ` eran liberty
2008-07-22 11:49 ` Matthew Wilcox
2008-07-22 13:08 ` Eran Liberty
2008-07-22 13:14 ` Eran Liberty
2008-07-22 14:13 ` Matthew Wilcox
2008-07-22 15:25 ` Eran Liberty
2008-07-22 16:52 ` Matthew Wilcox
2008-07-22 17:41 ` Eran Liberty
2008-07-22 18:11 ` Matthew Wilcox
2008-07-23 18:31 ` Eran Liberty [this message]
2008-07-27 11:01 ` Eran Liberty
2008-07-27 15:08 ` Matthew Wilcox
2008-08-18 8:08 ` ftrace introduces instability into kernel 2.6.27(-rc2,-rc3) Eran Liberty
2008-08-18 15:07 ` Steven Rostedt
2008-08-18 15:47 ` Mathieu Desnoyers
2008-08-18 16:12 ` Steven Rostedt
2008-08-18 17:04 ` Mathieu Desnoyers
2008-08-18 17:21 ` Scott Wood
2008-08-18 18:27 ` Steven Rostedt
2008-08-18 18:29 ` Scott Wood
2008-08-19 1:53 ` Benjamin Herrenschmidt
2008-08-19 2:28 ` Steven Rostedt
2008-08-19 2:39 ` Benjamin Herrenschmidt
2008-08-19 2:41 ` Steven Rostedt
2008-08-19 2:47 ` Mathieu Desnoyers
2008-08-19 3:32 ` Steven Rostedt
2008-08-19 3:36 ` Mathieu Desnoyers
2008-08-19 4:00 ` Steven Rostedt
2008-08-19 16:47 ` Steven Rostedt
2008-08-19 17:34 ` Mathieu Desnoyers
2008-08-19 21:08 ` Steven Rostedt
2008-08-20 9:40 ` Nick Piggin
2008-08-19 21:47 ` Benjamin Herrenschmidt
2008-08-19 23:58 ` Jeremy Fitzhardinge
2008-08-20 1:17 ` Benjamin Herrenschmidt
2008-08-19 2:56 ` Benjamin Herrenschmidt
2008-08-19 3:12 ` Steven Rostedt
2008-08-19 4:17 ` Benjamin Herrenschmidt
2008-08-20 7:18 ` Benjamin Herrenschmidt
2008-08-20 13:14 ` Steven Rostedt
2008-08-20 13:19 ` Steven Rostedt
2008-08-20 13:36 ` Eran Liberty
2008-08-20 13:43 ` Steven Rostedt
2008-08-20 14:02 ` Eran Liberty
2008-08-20 14:55 ` Jon Smirl
2008-08-20 15:23 ` Steven Rostedt
2008-08-20 18:23 ` Eran Liberty
2008-08-20 18:33 ` Steven Rostedt
2008-08-20 15:27 ` Steven Rostedt
2008-08-20 21:37 ` Benjamin Herrenschmidt
2008-08-20 14:16 ` Josh Boyer
2008-08-20 14:22 ` Steven Rostedt
2008-08-20 14:50 ` Josh Boyer
2008-09-15 16:30 ` [PATCH 2.6.26] SERIAL DRIVER: Handle Multiple consecutive sysrq from the serial Eran Liberty
2008-09-17 23:46 ` Andrew Morton
2008-09-18 6:58 ` Eran Liberty
2008-08-20 21:36 ` ftrace introduces instability into kernel 2.6.27(-rc2,-rc3) Benjamin Herrenschmidt
2008-08-20 21:44 ` Steven Rostedt
2008-08-18 18:47 ` Steven Rostedt
2008-08-18 18:56 ` Scott Wood
2008-08-18 19:28 ` Steven Rostedt
2008-08-18 18:25 ` Eran Liberty
2008-08-18 18:41 ` Mathieu Desnoyers
2008-08-19 1:54 ` Benjamin Herrenschmidt
2008-08-19 9:56 ` Eran Liberty
2008-08-19 13:02 ` Mathieu Desnoyers
2008-08-19 21:46 ` Benjamin Herrenschmidt
2008-08-18 18:50 ` Steven Rostedt
2008-08-19 12:09 ` Eran Liberty
2008-08-19 13:05 ` Mathieu Desnoyers
2008-08-19 14:21 ` Eran Liberty
2008-08-19 14:42 ` Mathieu Desnoyers
2008-08-19 20:15 ` Steven Rostedt
2008-08-20 11:18 ` Eran Liberty
2008-08-20 13:12 ` Steven Rostedt
2008-08-19 1:51 ` Benjamin Herrenschmidt
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=488778FD.1050206@extricom.com \
--to=liberty@extricom.com \
--cc=eran.liberty@gmail.com \
--cc=jbarnes@virtuousgeek.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pci@vger.kernel.org \
--cc=matthew@wil.cx \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).