The device code had several unclear messages, leading to confusion when analyzing the logs. Improve readability.
We also are seeing a NULL pointer dereference in reality on some boards. Since device model experts are busy with other stuff, add a #warning here and "upgrade" it to #error in a few weeks.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-stuff/device/device.c =================================================================== --- LinuxBIOSv3-stuff/device/device.c (Revision 658) +++ LinuxBIOSv3-stuff/device/device.c (Arbeitskopie) @@ -290,6 +290,7 @@ for (curdev = bus->children; curdev; curdev = curdev->sibling) { unsigned int links; int i; +#warning Missing NULL check, will explode at runtime printk(BIOS_SPEW, "%s: %s(%s) dtsname %s have_resources %d enabled %d\n", __func__, bus->dev->dtsname, dev_path(bus->dev), Index: LinuxBIOSv3-stuff/device/hypertransport.c =================================================================== --- LinuxBIOSv3-stuff/device/hypertransport.c (Revision 658) +++ LinuxBIOSv3-stuff/device/hypertransport.c (Arbeitskopie) @@ -580,10 +580,11 @@ */ if (old_devices) { struct device *left; + printk(BIOS_ERR, "HT: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) { - printk(BIOS_DEBUG, "%s\n", dev_path(left)); + printk(BIOS_ERR, "%s\n", dev_path(left)); } - printk(BIOS_ERR, "HT: Left over static devices.\n"); + printk(BIOS_ERR, "HT: End of leftover list.\n"); /* Put back the left over static device, and let * pci_scan_bus() disable it. */ Index: LinuxBIOSv3-stuff/device/pci_device.c =================================================================== --- LinuxBIOSv3-stuff/device/pci_device.c (Revision 658) +++ LinuxBIOSv3-stuff/device/pci_device.c (Arbeitskopie) @@ -883,7 +883,7 @@ dev = 0; printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list, *list); - for (/* */; *list; list = &(*list)->sibling) { + for (; *list; list = &(*list)->sibling) { printk(BIOS_SPEW, "%s: check dev %s \n", __func__, (*list)->dtsname); if ((*list)->path.type != DEVICE_PATH_PCI) { @@ -1122,10 +1122,11 @@ */ if (old_devices) { struct device *left; + printk(BIOS_ERR, "PCI: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) { - printk(BIOS_SPEW, "%s\n", left->dtsname); + printk(BIOS_ERR, "%s\n", left->dtsname); } - banner(BIOS_SPEW, "PCI: Left over static devices.\n"); + printk(BIOS_ERR, "PCI: End of leftover list.\n"); }
/* For all children that implement scan_bus() (i.e. bridges)
On Wed, Apr 16, 2008 at 9:21 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Index: LinuxBIOSv3-stuff/device/hypertransport.c
I *think* AGESA will take over this function, this file may be able to be dropped.
=================================================================== --- LinuxBIOSv3-stuff/device/hypertransport.c (Revision 658) +++ LinuxBIOSv3-stuff/device/hypertransport.c (Arbeitskopie) @@ -580,10 +580,11 @@ */ if (old_devices) { struct device *left;
printk(BIOS_ERR, "HT: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) {
printk(BIOS_DEBUG, "%s\n", dev_path(left));
printk(BIOS_ERR, "%s\n", dev_path(left)); }
printk(BIOS_ERR, "HT: Left over static devices.\n");
printk(BIOS_ERR, "HT: End of leftover list.\n"); /* Put back the left over static device, and let * pci_scan_bus() disable it. */Index: LinuxBIOSv3-stuff/device/pci_device.c
--- LinuxBIOSv3-stuff/device/pci_device.c (Revision 658) +++ LinuxBIOSv3-stuff/device/pci_device.c (Arbeitskopie) @@ -883,7 +883,7 @@ dev = 0; printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list, *list);
for (/* */; *list; list = &(*list)->sibling) {
for (; *list; list = &(*list)->sibling) { printk(BIOS_SPEW, "%s: check dev %s \n", __func__, (*list)->dtsname); if ((*list)->path.type != DEVICE_PATH_PCI) {@@ -1122,10 +1122,11 @@ */ if (old_devices) { struct device *left;
printk(BIOS_ERR, "PCI: Left over static devices:\n");
no, leave it at spew. It's not an error. These are leftover *PCI* devices, and that doesn't matter if they are, e.g., a superio.
for (left = old_devices; left; left = left->sibling) {
printk(BIOS_SPEW, "%s\n", left->dtsname);
printk(BIOS_ERR, "%s\n", left->dtsname);=
same here. Needs to be spew, not ERR.
ron
ron minnich wrote:
On Wed, Apr 16, 2008 at 9:21 AM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Index: LinuxBIOSv3-stuff/device/hypertransport.c
I *think* AGESA will take over this function, this file may be able to be dropped.
Yes, for very recent and future CPUs. K8 will still need this (unless we back port).
On 16.04.2008 21:14, ron minnich wrote:
On Wed, Apr 16, 2008 at 9:21 AM, Carl-Daniel Hailfinger wrote
Index: LinuxBIOSv3-stuff/device/hypertransport.c
I *think* AGESA will take over this function, this file may be able to be dropped.
Marc answered that one.
=================================================================== --- LinuxBIOSv3-stuff/device/hypertransport.c (Revision 658) +++ LinuxBIOSv3-stuff/device/hypertransport.c (Arbeitskopie) @@ -580,10 +580,11 @@ */ if (old_devices) { struct device *left;
printk(BIOS_ERR, "HT: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) {
printk(BIOS_DEBUG, "%s\n", dev_path(left));
printk(BIOS_ERR, "%s\n", dev_path(left)); }
printk(BIOS_ERR, "HT: Left over static devices.\n");
printk(BIOS_ERR, "HT: End of leftover list.\n"); /* Put back the left over static device, and let * pci_scan_bus() disable it. */Index: LinuxBIOSv3-stuff/device/pci_device.c
--- LinuxBIOSv3-stuff/device/pci_device.c (Revision 658) +++ LinuxBIOSv3-stuff/device/pci_device.c (Arbeitskopie) @@ -883,7 +883,7 @@ dev = 0; printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list, *list);
for (/* */; *list; list = &(*list)->sibling) {
for (; *list; list = &(*list)->sibling) { printk(BIOS_SPEW, "%s: check dev %s \n", __func__, (*list)->dtsname); if ((*list)->path.type != DEVICE_PATH_PCI) {@@ -1122,10 +1122,11 @@ */ if (old_devices) { struct device *left;
printk(BIOS_ERR, "PCI: Left over static devices:\n");no, leave it at spew. It's not an error. These are leftover *PCI* devices, and that doesn't matter if they are, e.g., a superio.
Sorry, I don't understand. Since when do superios appear on the PCI bus?
for (left = old_devices; left; left = left->sibling) {
printk(BIOS_SPEW, "%s\n", left->dtsname);
printk(BIOS_ERR, "%s\n", left->dtsname);=same here. Needs to be spew, not ERR.
If the condition is significant at all, we should at least use INFO. If the condition is meaningless, we might as well remove the code completely.
The device code had several unclear messages, leading to confusion when analyzing the logs. Improve readability.
We also are seeing a NULL pointer dereference in reality on some boards. Since device model experts are busy with other stuff, add a #warning here and "upgrade" it to #error in a few weeks.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-stuff/device/device.c =================================================================== --- LinuxBIOSv3-stuff/device/device.c (Revision 659) +++ LinuxBIOSv3-stuff/device/device.c (Arbeitskopie) @@ -290,6 +290,7 @@ for (curdev = bus->children; curdev; curdev = curdev->sibling) { unsigned int links; int i; +#warning Missing NULL check, will explode at runtime printk(BIOS_SPEW, "%s: %s(%s) dtsname %s have_resources %d enabled %d\n", __func__, bus->dev->dtsname, dev_path(bus->dev), Index: LinuxBIOSv3-stuff/device/hypertransport.c =================================================================== --- LinuxBIOSv3-stuff/device/hypertransport.c (Revision 659) +++ LinuxBIOSv3-stuff/device/hypertransport.c (Arbeitskopie) @@ -580,10 +580,11 @@ */ if (old_devices) { struct device *left; + printk(BIOS_INFO, "HT: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) { - printk(BIOS_DEBUG, "%s\n", dev_path(left)); + printk(BIOS_INFO, "%s\n", dev_path(left)); } - printk(BIOS_ERR, "HT: Left over static devices.\n"); + printk(BIOS_INFO, "HT: End of leftover list.\n"); /* Put back the left over static device, and let * pci_scan_bus() disable it. */ Index: LinuxBIOSv3-stuff/device/pci_device.c =================================================================== --- LinuxBIOSv3-stuff/device/pci_device.c (Revision 659) +++ LinuxBIOSv3-stuff/device/pci_device.c (Arbeitskopie) @@ -883,7 +883,7 @@ dev = 0; printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list, *list); - for (/* */; *list; list = &(*list)->sibling) { + for (; *list; list = &(*list)->sibling) { printk(BIOS_SPEW, "%s: check dev %s \n", __func__, (*list)->dtsname); if ((*list)->path.type != DEVICE_PATH_PCI) { @@ -1122,10 +1122,11 @@ */ if (old_devices) { struct device *left; + printk(BIOS_INFO, "PCI: Left over static devices:\n"); for (left = old_devices; left; left = left->sibling) { - printk(BIOS_SPEW, "%s\n", left->dtsname); + printk(BIOS_INFO, "%s\n", left->dtsname); } - banner(BIOS_SPEW, "PCI: Left over static devices.\n"); + printk(BIOS_INFO, "PCI: End of leftover list.\n"); }
/* For all children that implement scan_bus() (i.e. bridges)
On Thu, Apr 17, 2008 at 4:18 PM, Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net wrote:
Sorry, I don't understand. Since when do superios appear on the PCI bus?
They don't. But they *do* appear on the static device tree now, along with cpus etc.
The old v2 code was really pci-centric. Now that all static devices are on the device tree, and some are not pci devices, this should be a warning, not an error.
If the condition is significant at all, we should at least use INFO. If the condition is meaningless, we might as well remove the code completely.
I'm comfortable removing it. It's legacy.
ron
-
Carl-Daniel Hailfinger -
Marc Jones -
ron minnich