Re: [coreboot] [PATCH] v3: various fixes to the device code

[Carl-Daniel Hailfinger]

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

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)