[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

Improve/fix PCI device doxygen comments and printks. As a bonus, the
boot messages are more understandable now.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
===================================================================
--- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 759)
+++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
@@ -929,12 +929,15 @@
 /** 
  * Scan a PCI bus.
  *
- * Determine the existence of a given PCI device.
+ * Determine the existence of a given PCI device. Allocate a new struct device
+ * if dev==NULL was passed in and the device exists.
  *
+ * @param dev Pointer to the device structure if it already exists or NULL
  * @param bus Pointer to the bus structure.
  * @param devfn A device/function number.
  * @return The device structure for the device (if found)
- *         or the NULL if no device is found.
+ *         or NULL if no device is found and dev==NULL was passed in
+ *         or the device structure with enabled=0 if no device is found.
  */
 struct device *pci_probe_dev(struct device *dev, struct bus *bus,
    		     unsigned int devfn)
@@ -990,8 +993,8 @@
    	if ((id == 0xffffffff) || (id == 0x00000000) ||
    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
    		if (dev->enabled) {
-				printk(BIOS_INFO,
-				       "Disabling static device: %s\n",
+				printk(BIOS_INFO, "PCI: Static device not "
+				       "found, setting enabled=0: %s\n",
    			       dev_path(dev));
    			dev->enabled = 0;
    		}

[ron minnich]

On Wed, Aug 13, 2008 at 7:10 PM, Carl-Daniel Hailfinger
 wrote:
> Improve/fix PCI device doxygen comments and printks. As a bonus, the
> boot messages are more understandable now.
>
> Signed-off-by: Carl-Daniel Hailfinger
> Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
>
> --- corebootv3-pci_device_better_prints_comments/device/pci_device.c    (Revision 759)
> +++ corebootv3-pci_device_better_prints_comments/device/pci_device.c    (Arbeitskopie)
> @@ -929,12 +929,15 @@
>  /**
> Scan a PCI bus.
> Determine the existence of a given PCI device.
> Determine the existence of a given PCI device. Allocate a new struct device
> if dev==NULL was passed in and the device exists.
> @param dev Pointer to the device structure if it already exists or NULL

"if it already exists" is not qutie it.

"If it is already in the device tree, i.e. was specified in the dts.
It may not exist however. "
> @param bus Pointer to the bus structure.
> @param devfn A device/function number.
> @return The device structure for the device (if found)
>     or the NULL if no device is found.
>     or NULL if no device is found and dev==NULL was passed in
>     or the device structure with enabled=0 if no device is found.
>
> */
>  struct device *pci_probe_dev(struct device *dev, struct bus *bus,
>                             unsigned int devfn)
> @@ -990,8 +993,8 @@
>                if ((id == 0xffffffff) || (id == 0x00000000) ||
>                    (id == 0x0000ffff) || (id == 0xffff0000)) {
>                        if (dev->enabled) {
>                           printk(BIOS_INFO,
>                                  "Disabling static device: %s\n",
>                           printk(BIOS_INFO, "PCI: Static device not "
>                                  "found, setting enabled=0: %s\n",
>                                 dev_path(dev));
>                          dev->enabled = 0;
>                  }

[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

Improve/fix PCI device doxygen comments and printks. As a bonus, the
boot messages are more understandable now.

Thanks to Ron for supplying better explanations. I've included most of
them verbatim.

Signed-off-by: Carl-Daniel Hailfinger

Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
===================================================================
--- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 780)
+++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
@@ -950,12 +950,30 @@
 /** 
  * Scan a PCI bus.
  *
- * Determine the existence of a given PCI device.
+ * Determine the existence of a given PCI device. Allocate a new struct device
+ * if dev==NULL was passed in and the device exists in hardware.
  *
+ * @param dev Pointer to the device structure if it already is in the device
+ *         tree, i.e. was specified in the dts. It may not exist on hardware,
+ *         however. Looking for hardware not yet in the device tree has this
+ *         parameter as NULL.
  * @param bus Pointer to the bus structure.
  * @param devfn A device/function number.
- * @return The device structure for the device (if found)
- *         or the NULL if no device is found.
+ * @return The device structure for the device if it exists in hardware
+ *         or the passed in device structure with enabled=0 if the device
+ *         does not exist in hardware and only in the tree
+ *         or NULL if no device is found and dev==NULL was passed in.
+ *
+ * There are three cases:
+ * - known knowns. In this case the device is in the tree, i.e. not NULL,
+ * and we know it's there: it's soldered down or part of the on-chip
+ * hardware. In this case dev is not NULL.
+ * - known unknowns. This is a device that might be there, but we don't
+ * know. So we have to probe it. It's in the dts, which is why
+ * it is a known unknown.
+ * - unknown unknowns. A PCI card in a PCI slot. We can't cover all
+ * possible cards. dev is NULL. We are checking to see if something is
+ * there; if so, we will allocate a dev and put it in the three.
  */
 struct device *pci_probe_dev(struct device *dev, struct bus *bus,
    		     unsigned int devfn)
@@ -1011,8 +1029,8 @@
    	if ((id == 0xffffffff) || (id == 0x00000000) ||
    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
    		if (dev->enabled) {
-				printk(BIOS_INFO,
-				       "Disabling static device: %s\n",
+				printk(BIOS_INFO, "PCI: Static device not "
+				       "found, setting enabled=0: %s\n",
    			       dev_path(dev));
    			dev->enabled = 0;
    		}

[ron minnich]

Acked-by: Ronald G. Minnich

[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

On 18.08.2008 07:36, ron minnich wrote:
> Acked-by: Ronald G. Minnich

Thanks. The uncontroversial part was checked in as r801.

Regards,
Carl-Daniel

[PATCH] v3: improve PCI device doxygen comments and printks

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
>
> --- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 780)
> +++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
> @@ -950,12 +950,30 @@
>  /**
> Scan a PCI bus.
> Determine the existence of a given PCI device.
> Determine the existence of a given PCI device. Allocate a new struct device
> if dev==NULL was passed in and the device exists in hardware.
> @param dev Pointer to the device structure if it already is in the device
>     tree, i.e. was specified in the dts. It may not exist on hardware,
>     however. Looking for hardware not yet in the device tree has this
>     parameter as NULL.
> @param bus Pointer to the bus structure.
> @param devfn A device/function number.
> @return The device structure for the device (if found)
>     or the NULL if no device is found.
> @return The device structure for the device if it exists in hardware
>     or the passed in device structure with enabled=0 if the device
>     does not exist in hardware and only in the tree
>     or NULL if no device is found and dev==NULL was passed in.
> There are three cases:

.. of what?
> known knowns. In this case the device is in the tree, i.e. not NULL,
> and we know it's there: it's soldered down or part of the on-chip
> hardware. In this case dev is not NULL.
> known unknowns. This is a device that might be there, but we don't
> know. So we have to probe it. It's in the dts, which is why
> it is a known unknown.

How s the function called for the known knowns? Those are neither PCI
cards nor stuff in the DTS according to your description? So where does
it come from?
> unknown unknowns. A PCI card in a PCI slot. We can't cover all
> possible cards. dev is NULL. We are checking to see if something is
> there; if so, we will allocate a dev and put it in the three.
> @@ -1011,8 +1029,8 @@
>    	if ((id == 0xffffffff) || (id == 0x00000000) ||
>    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
>    		if (dev->enabled) {
> 		printk(BIOS_INFO,
> 		       "Disabling static device: %s\n",
> 		printk(BIOS_INFO, "PCI: Static device not "
> 		       "found, setting enabled=0: %s\n",
> 	       dev_path(dev));

I dislike this change of output. It's not a BIOS_DEBUG message.
> 		dev->enabled = 0;
> 	}

[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

On 18.08.2008 09:27, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
> >
> > --- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 780)
> > +++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
> > @@ -950,12 +950,30 @@
> >  /**
> > Scan a PCI bus.
> > Determine the existence of a given PCI device.
> > Determine the existence of a given PCI device. Allocate a new struct device
> > if dev==NULL was passed in and the device exists in hardware.
> > @param dev Pointer to the device structure if it already is in the device
> >     tree, i.e. was specified in the dts. It may not exist on hardware,
> >     however. Looking for hardware not yet in the device tree has this
> >     parameter as NULL.
> > @param bus Pointer to the bus structure.
> > @param devfn A device/function number.
> > @return The device structure for the device (if found)
> >     or the NULL if no device is found.
> > @return The device structure for the device if it exists in hardware
> >     or the passed in device structure with enabled=0 if the device
> >     does not exist in hardware and only in the tree
> >     or NULL if no device is found and dev==NULL was passed in.
> > There are three cases:
>
> .. of what?

What about "There are three cases for which this function is called:"
> > known knowns. In this case the device is in the tree, i.e. not NULL,
> > and we know it's there: it's soldered down or part of the on-chip
> > hardware. In this case dev is not NULL.
> > known unknowns. This is a device that might be there, but we don't
> > know. So we have to probe it. It's in the dts, which is why
> > it is a known unknown.
>
> How s the function called for the known knowns? Those are neither PCI

Did you mean "called for the known UNknowns"?
> cards nor stuff in the DTS according to your description? So where does
> it come from?

The first two cases are for devices which are in the dts and therefore
in the tree.
> > unknown unknowns. A PCI card in a PCI slot. We can't cover all
> > possible cards. dev is NULL. We are checking to see if something is
> > there; if so, we will allocate a dev and put it in the three.
> > @@ -1011,8 +1029,8 @@
> >    	if ((id == 0xffffffff) || (id == 0x00000000) ||
> >    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
> >    		if (dev->enabled) {
> > 		printk(BIOS_INFO,
> > 		       "Disabling static device: %s\n",
> > 		printk(BIOS_INFO, "PCI: Static device not "
> > 		       "found, setting enabled=0: %s\n",
> > 	       dev_path(dev));
>
> I dislike this change of output. It's not a BIOS_DEBUG message.

Do you dislike the severity level (same as before) or the new wording
(the old wording was wrong)?
> > 		dev->enabled = 0;
> > 	}

Regards,
Carl-Daniel

[PATCH] v3: improve PCI device doxygen comments and printks

[Stefan Reinauer]

Carl-Daniel Hailfinger wrote:
> > > known knowns. In this case the device is in the tree, i.e. not NULL,
> > > and we know it's there: it's soldered down or part of the on-chip
> > > hardware. In this case dev is not NULL.
> > > known unknowns. This is a device that might be there, but we don't
> > > know. So we have to probe it. It's in the dts, which is why
> > > it is a known unknown.
> >
> > How s the function called for the known knowns? Those are neither PCI
>
> Did you mean "called for the known UNknowns"?

No.

1. The known unknowns ("listed, or expected pci devices") are called
from the dts, as you write.
2. The unknown unknowns ("scanned pci devices") are not in the dts but
scanned.

So what's the third case? A device is both listed in the dts and found
while scanning? Are those "expected and found"?
> > > @@ -1011,8 +1029,8 @@
> > >    	if ((id == 0xffffffff) || (id == 0x00000000) ||
> > >    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
> > >    		if (dev->enabled) {
> > > 		printk(BIOS_INFO,
> > > 		       "Disabling static device: %s\n",
> > > 		printk(BIOS_INFO, "PCI: Static device not "
> > > 		       "found, setting enabled=0: %s\n",
> > > 	       dev_path(dev));
> >
> > I dislike this change of output. It's not a BIOS_DEBUG message.
>
> Do you dislike the severity level (same as before) or the new wording
> (the old wording was wrong)?

Why did you consider it wrong?

Either the message should stay or the level should be raised to debug.
Not sure. "Setting enabled=0" implies you have to understand the code in
order to understand the message. This is bad for BIOS_INFO type messages
(Imho, for everything more "severe" than DEBUG)

[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

On 18.08.2008 13:26, Stefan Reinauer wrote:
> Carl-Daniel Hailfinger wrote:
> > > > known knowns. In this case the device is in the tree, i.e. not NULL,
> > > > and we know it's there: it's soldered down or part of the on-chip
> > > > hardware. In this case dev is not NULL.

The point above is the zeroth/third case.
> > > > known unknowns. This is a device that might be there, but we don't
> > > > know. So we have to probe it. It's in the dts, which is why
> > > > it is a known unknown.
> > >
> > > How s the function called for the known knowns? Those are neither PCI
> >
> > Did you mean "called for the known UNknowns"?
>
> No.
> The known unknowns ("listed, or expected pci devices") are called
>
> from the dts, as you write.

1. is "listed, but not necessarily present in hardware"
> The unknown unknowns ("scanned pci devices") are not in the dts but
>
> scanned.
>
> So what's the third case? A device is both listed in the dts and found
> while scanning? Are those "expected and found"?

The third (or zeroth) case are the known knowns ("listed and present in
hardware").
> > > > @@ -1011,8 +1029,8 @@
> > > >    	if ((id == 0xffffffff) || (id == 0x00000000) ||
> > > >    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
> > > >    		if (dev->enabled) {
> > > > 		printk(BIOS_INFO,
> > > > 		       "Disabling static device: %s\n",
> > > > 		printk(BIOS_INFO, "PCI: Static device not "
> > > > 		       "found, setting enabled=0: %s\n",
> > > > 	       dev_path(dev));
> > >
> > > I dislike this change of output. It's not a BIOS_DEBUG message.
> >
> > Do you dislike the severity level (same as before) or the new wording
> > (the old wording was wrong)?
>
> Why did you consider it wrong?

The old message said "disabling static device", but the device is not
touched at all.
> Either the message should stay or the level should be raised to debug.
> Not sure. "Setting enabled=0" implies you have to understand the code in
> order to understand the message. This is bad for BIOS_INFO type messages
> (Imho, for everything more "severe" than DEBUG)

I have yet to find a better wording which says that we don't touch the
physical device and set the matching struct device to disabled instead.

Regards,
Carl-Daniel

[PATCH] v3: improve PCI device doxygen comments and printks

[Carl-Daniel Hailfinger]

Stefan?

I merged the part which you did not object to. If you answer the
questions below, we should be able to resolve this completely.

Regards,
Carl-Daniel

On 18.08.2008 13:39, Carl-Daniel Hailfinger wrote:
> On 18.08.2008 13:26, Stefan Reinauer wrote:
> > Carl-Daniel Hailfinger wrote:
> > > > > known knowns. In this case the device is in the tree, i.e. not NULL,
> > > > > and we know it's there: it's soldered down or part of the on-chip
> > > > > hardware. In this case dev is not NULL.
>
> The point above is the zeroth/third case.
> > > > > known unknowns. This is a device that might be there, but we don't
> > > > > know. So we have to probe it. It's in the dts, which is why
> > > > > it is a known unknown.
> > > >
> > > > How s the function called for the known knowns? Those are neither PCI
> > >
> > > Did you mean "called for the known UNknowns"?
> >
> > No.
> > The known unknowns ("listed, or expected pci devices") are called
> >
> > from the dts, as you write.
> is "listed, but not necessarily present in hardware"
> > The unknown unknowns ("scanned pci devices") are not in the dts but
> >
> > scanned.
> >
> > So what's the third case? A device is both listed in the dts and found
> > while scanning? Are those "expected and found"?
>
> The third (or zeroth) case are the known knowns ("listed and present in
> hardware").
> > > > > @@ -1011,8 +1029,8 @@
> > > > >    	if ((id == 0xffffffff) || (id == 0x00000000) ||
> > > > >    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
> > > > >    		if (dev->enabled) {
> > > > > 		printk(BIOS_INFO,
> > > > > 		       "Disabling static device: %s\n",
> > > > > 		printk(BIOS_INFO, "PCI: Static device not "
> > > > > 		       "found, setting enabled=0: %s\n",
> > > > > 	       dev_path(dev));
> > > >
> > > > I dislike this change of output. It's not a BIOS_DEBUG message.
> > >
> > > Do you dislike the severity level (same as before) or the new wording
> > > (the old wording was wrong)?
> >
> > Why did you consider it wrong?
>
> The old message said "disabling static device", but the device is not
> touched at all.
> > Either the message should stay or the level should be raised to debug.
> > Not sure. "Setting enabled=0" implies you have to understand the code in
> > order to understand the message. This is bad for BIOS_INFO type messages
> > (Imho, for everything more "severe" than DEBUG)
>
> I have yet to find a better wording which says that we don't touch the
> physical device and set the matching struct device to disabled instead.

Improve/fix PCI device doxygen comments and printks. As a bonus, the
boot messages are more understandable now.

Thanks to Ron for supplying better explanations. I've included most of
them verbatim.

Signed-off-by: Carl-Daniel Hailfinger 
Acked-by: Ronald G. Minnich

Index: corebootv3-pci_device_better_prints_comments/device/pci_device.c
===================================================================
--- corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Revision 801)
+++ corebootv3-pci_device_better_prints_comments/device/pci_device.c	(Arbeitskopie)
@@ -963,6 +963,17 @@
  *         or the passed in device structure with enabled=0 if the device
  *         does not exist in hardware and only in the tree
  *         or NULL if no device is found and dev==NULL was passed in.
+ *
+ * There are three cases:
+ * - known knowns. In this case the device is in the tree, i.e. not NULL,
+ * and we know it's there: it's soldered down or part of the on-chip
+ * hardware. In this case dev is not NULL.
+ * - known unknowns. This is a device that might be there, but we don't
+ * know. So we have to probe it. It's in the dts, which is why
+ * it is a known unknown.
+ * - unknown unknowns. A PCI card in a PCI slot. We can't cover all
+ * possible cards. dev is NULL. We are checking to see if something is
+ * there; if so, we will allocate a dev and put it in the three.
  */
 struct device *pci_probe_dev(struct device *dev, struct bus *bus,
    		     unsigned int devfn)
@@ -1018,8 +1029,8 @@
    	if ((id == 0xffffffff) || (id == 0x00000000) ||
    	    (id == 0x0000ffff) || (id == 0xffff0000)) {
    		if (dev->enabled) {
-				printk(BIOS_INFO,
-				       "Disabling static device: %s\n",
+				printk(BIOS_INFO, "PCI: Static device not "
+				       "found, setting enabled=0: %s\n",
    			       dev_path(dev));
    			dev->enabled = 0;
    		}