pci_device.c cleanup

[Myles Watson]

This patch clarifies/adds comments and changes names in device/pci_device.c

It also changes %p debug statements in various places.  I think they get in
the way of diffs when you have log files to compare.  I don't want to see
the
allocation differences most of the time.  I turned most of them into NULL
checks.  If they were supposed to be "Where are we in device allocation?"
checks, we could make them into that too.

It's a work-in-progress. Comments welcome.

I think most of the changes are self explanatory, but this one might not be:

If you are reading all the BARs from a device, and you come to a 64-bit BAR.
No matter why you skip it, you should skip it as a 64-bit BAR, and not try
to
read the upper half as the next 32-bit BAR.

Because of that, set the 64-bit flag IORESOURCE_PCI64 early, and don't clear
it on return.

Signed-off-by: Myles Watson

Thanks,
Myles

[ron minnich]

I can not test this week.

Acked-by: Ronald G. Minnich

[Myles Watson]

On Tue, Nov 4, 2008 at 2:17 PM, ron minnich  wrote:
> I can not test this week.
>
> Acked-by: Ronald G. Minnich

Rev 980.

Thanks,
Myles

[Carl-Daniel Hailfinger]

On 04.11.2008 21:04, Myles Watson wrote:
> This patch clarifies/adds comments and changes names in device/pci_device.c
>
> It also changes %p debug statements in various places.  I think they get in
> the way of diffs when you have log files to compare.  I don't want to see
> the
> allocation differences most of the time.  I turned most of them into NULL
> checks.  If they were supposed to be "Where are we in device allocation?"
> checks, we could make them into that too.
>
> It's a work-in-progress. Comments welcome.
>
> I think most of the changes are self explanatory, but this one might not be:
>
> If you are reading all the BARs from a device, and you come to a 64-bit BAR.
> No matter why you skip it, you should skip it as a 64-bit BAR, and not try
> to
> read the upper half as the next 32-bit BAR.
>
> Because of that, set the 64-bit flag IORESOURCE_PCI64 early, and don't clear
> it on return.
>
> Signed-off-by: Myles Watson
>
> @@ -899,18 +924,18 @@
> Given a linked list of PCI device structures and a devfn number, find the
> device structure correspond to the devfn, if present. This function also
> removes the device structure from the linked list.
> @param list The device structure list.
> @param devfn A device/function number.
> @return Pointer to the device structure found or NULL of we have not
>   allocated a device for this devfn yet.
> */
>
> -static struct device *pci_scan_get_dev(struct device **list, unsigned int devfn)
> +static struct device *pci_get_dev(struct device **list, unsigned int devfn)
>  {
>    struct device *dev;
>    dev = 0;
> printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list,
>       *list);
> printk(BIOS_SPEW, "%s: list is %sNULL, *list is %sNULL\n", __func__,
> list?"NOT ":"", *list?"NOT ":"");
> for (; *list; list = &(*list)->sibling) {
> printk(BIOS_SPEW, "%s: check dev %s \n", __func__,
>       (*list)->dtsname);

Was that change really intentional? Sure, it makes diffing easier, but
some diagnostics (especially considering the still buggy device
enumeration) are now more difficult.
> @@ -1043,15 +1068,22 @@
>    dev->irq_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
>    dev->min_gnt = pci_read_config8(dev, PCI_MIN_GNT);
>    dev->max_lat = pci_read_config8(dev, PCI_MAX_LAT);
> -#warning Per-device subsystem ID should only be read from the device if none has been specified for the device in the dts.
>
> dev->subsystem_vendor = pci_read_config16(dev, PCI_SUBSYSTEM_VENDOR_ID);
>
> dev->subsystem_device = pci_read_config16(dev, PCI_SUBSYSTEM_ID);
>
> /* Store the interesting information in the device structure. */
> /*Per-device subsystem ID should only be read from the device if none
> * has been specified for the device in the dts.
> */
> if (!dev->subsystem_vendor && !dev->subsystem_device) {
> dev->subsystem_vendor = 
> 		pci_read_config16(dev, PCI_SUBSYSTEM_VENDOR_ID);
> dev->subsystem_device = 
> 		pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> }
> dev->id.type = DEVICE_ID_PCI;
>  dev->id.pci.vendor = id & 0xffff;
>  dev->id.pci.device = (id >> 16) & 0xffff;
>  dev->hdr_type = hdr_type;
> /* Class code, the upper 3 bytes of PCI_CLASS_REVISION. */
>  dev->class = class >> 8;

Hm. Although you fixed the code as indicated in the #warning, I fear
there's still something missing and that's the reason the warning was
there. Where do we set the subsystem ID of the real device (not struct
device)?
> @@ -1105,6 +1135,8 @@
>
> printk(BIOS_DEBUG, "%s start bus %p, bus->dev %p\n", __func__, bus,
>           bus->dev);
>
> +#warning This check needs checking.
>    if (bus->dev->path.type != DEVICE_PATH_PCI_BUS)
>    	printk(BIOS_ERR, "ERROR: pci_scan_bus called with incorrect "
>    	       "bus->dev->path.type, path is %s\n", dev_path(bus->dev));

If you manage to fix the device code in a way which doesn't trigger this
anymore, your static devices will suddenly be found. Since this check is
only triggered if your code and/or device tree is really buggy, we could
upgrade it to a die().

Regards,
Carl-Daniel

[Myles Watson]

> -----Original Message-----
> From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> Sent: Tuesday, November 04, 2008 5:09 PM
> To: Myles Watson
> Cc: Coreboot
> Subject: Re: [coreboot] pci_device.c cleanup
>
> On 04.11.2008 21:04, Myles Watson wrote:
> > This patch clarifies/adds comments and changes names in
>
> device/pci_device.c
> > It also changes %p debug statements in various places.  I think they get
>
> in
> > the way of diffs when you have log files to compare.  I don't want to
>
> see
> > the
> > allocation differences most of the time.  I turned most of them into
>
> NULL
> > checks.  If they were supposed to be "Where are we in device
>
> allocation?"
> > checks, we could make them into that too.
> >
> > It's a work-in-progress. Comments welcome.
> >
> > I think most of the changes are self explanatory, but this one might not
>
> be:
> > If you are reading all the BARs from a device, and you come to a 64-bit
>
> BAR.
> > No matter why you skip it, you should skip it as a 64-bit BAR, and not
>
> try
> > to
> > read the upper half as the next 32-bit BAR.
> >
> > Because of that, set the 64-bit flag IORESOURCE_PCI64 early, and don't
>
> clear
> > it on return.
> >
> > Signed-off-by: Myles Watson
> >
> > @@ -899,18 +924,18 @@
> > Given a linked list of PCI device structures and a devfn number,
>
> find the
> > device structure correspond to the devfn, if present. This function
>
> also
> > removes the device structure from the linked list.
> > @param list The device structure list.
> > @param devfn A device/function number.
> > @return Pointer to the device structure found or NULL of we have not
> >   allocated a device for this devfn yet.
> > */
> >
> > -static struct device *pci_scan_get_dev(struct device **list, unsigned
>
> int devfn)
> > +static struct device *pci_get_dev(struct device **list, unsigned int
>
> devfn)
> > {
> >    struct device *dev;
> >    dev = 0;
> > printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list,
> >       *list);
> > printk(BIOS_SPEW, "%s: list is %sNULL, *list is %sNULL\n", __func__,
> > list?"NOT ":"", *list?"NOT ":"");
> > for (; *list; list = &(*list)->sibling) {
> > printk(BIOS_SPEW, "%s: check dev %s \n", __func__,
> >       (*list)->dtsname);
>
> Was that change really intentional? Sure, it makes diffing easier, but
> some diagnostics (especially considering the still buggy device
> enumeration) are now more difficult.

Since this is looking through the list of children, and the function prints
out the dtsname of each child, I didn't find the actual pointer to be
useful.  The dtsname is much more useful.
> > @@ -1043,15 +1068,22 @@
> >    dev->irq_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
> >    dev->min_gnt = pci_read_config8(dev, PCI_MIN_GNT);
> >    dev->max_lat = pci_read_config8(dev, PCI_MAX_LAT);
> > -#warning Per-device subsystem ID should only be read from the device if
>
> none has been specified for the device in the dts.
> > dev->subsystem_vendor = pci_read_config16(dev,
>
> PCI_SUBSYSTEM_VENDOR_ID);
> > dev->subsystem_device = pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> >
> > /* Store the interesting information in the device structure. */
> > /*Per-device subsystem ID should only be read from the device if
>
> none
> > * has been specified for the device in the dts.
> > */
> > if (!dev->subsystem_vendor && !dev->subsystem_device) {
> > dev->subsystem_vendor =
> > 		pci_read_config16(dev,

PCI_SUBSYSTEM_VENDOR_ID);
> > dev->subsystem_device =
> > 		pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> > }
> > dev->id.type = DEVICE_ID_PCI;
> >  dev->id.pci.vendor = id & 0xffff;
> >  dev->id.pci.device = (id >> 16) & 0xffff;
> >  dev->hdr_type = hdr_type;
> > /* Class code, the upper 3 bytes of PCI_CLASS_REVISION. */
> >  dev->class = class >> 8;
>
> Hm. Although you fixed the code as indicated in the #warning, I fear
> there's still something missing and that's the reason the warning was
> there. Where do we set the subsystem ID of the real device (not struct
> device)?

There's another place in the code where there's a warning that addresses
this same issue.  I didn't think we needed it in both places.
> > @@ -1105,6 +1135,8 @@
> >
> > printk(BIOS_DEBUG, "%s start bus %p, bus->dev %p\n", __func__, bus,
> >           bus->dev);
> >
> > +#warning This check needs checking.
> >    if (bus->dev->path.type != DEVICE_PATH_PCI_BUS)
> >    	printk(BIOS_ERR, "ERROR: pci_scan_bus called with incorrect

"
> >        "bus->dev->path.type, path is %s\n", dev_path(bus-
> >
> > dev));
>
> If you manage to fix the device code in a way which doesn't trigger this
> anymore, your static devices will suddenly be found. Since this check is
> only triggered if your code and/or device tree is really buggy, we could
> upgrade it to a die().

I realize it's a problem.  Since I removed a couple of the other warnings, I
thought I'd put one in this trouble spot.

Thanks for the review,

Myles

[Carl-Daniel Hailfinger]

On 05.11.2008 03:58, Myles Watson wrote:
> > -----Original Message-----
> > From: Carl-Daniel Hailfinger [mailto:[e-mail]]
> > Sent: Tuesday, November 04, 2008 5:09 PM
> > To: Myles Watson
> > Cc: Coreboot
> > Subject: Re: [coreboot] pci_device.c cleanup
> >
> > On 04.11.2008 21:04, Myles Watson wrote:
> > > This patch clarifies/adds comments and changes names in
> >
> > device/pci_device.c
> > > It also changes %p debug statements in various places.  I think they get
> >
> > in
> > > the way of diffs when you have log files to compare.  I don't want to
> >
> > see
> > > the
> > > allocation differences most of the time.  I turned most of them into
> >
> > NULL
> > > checks.  If they were supposed to be "Where are we in device
> >
> > allocation?"
> > > checks, we could make them into that too.
> > >
> > > It's a work-in-progress. Comments welcome.
> > >
> > > I think most of the changes are self explanatory, but this one might not
> >
> > be:
> > > If you are reading all the BARs from a device, and you come to a 64-bit
> >
> > BAR.
> > > No matter why you skip it, you should skip it as a 64-bit BAR, and not
> >
> > try
> > > to
> > > read the upper half as the next 32-bit BAR.
> > >
> > > Because of that, set the 64-bit flag IORESOURCE_PCI64 early, and don't
> >
> > clear
> > > it on return.
> > >
> > > Signed-off-by: Myles Watson
> > >
> > > @@ -899,18 +924,18 @@
> > > Given a linked list of PCI device structures and a devfn number,
> >
> > find the
> > > device structure correspond to the devfn, if present. This function
> >
> > also
> > > removes the device structure from the linked list.
> > > @param list The device structure list.
> > > @param devfn A device/function number.
> > > @return Pointer to the device structure found or NULL of we have not
> > >   allocated a device for this devfn yet.
> > > */
> > >
> > > -static struct device *pci_scan_get_dev(struct device **list, unsigned
> >
> > int devfn)
> > > +static struct device *pci_get_dev(struct device **list, unsigned int
> >
> > devfn)
> > > {
> > >    struct device *dev;
> > >    dev = 0;
> > > printk(BIOS_SPEW, "%s: list is %p, *list is %p\n", __func__, list,
> > >       *list);
> > > printk(BIOS_SPEW, "%s: list is %sNULL, *list is %sNULL\n", __func__,
> > > list?"NOT ":"", *list?"NOT ":"");
> > > for (; *list; list = &(*list)->sibling) {
> > > printk(BIOS_SPEW, "%s: check dev %s \n", __func__,
> > >       (*list)->dtsname);
> >
> > Was that change really intentional? Sure, it makes diffing easier, but
> > some diagnostics (especially considering the still buggy device
> > enumeration) are now more difficult.
>
> Since this is looking through the list of children, and the function prints
> out the dtsname of each child, I didn't find the actual pointer to be
> useful.  The dtsname is much more useful.

Point taken.
> > > @@ -1043,15 +1068,22 @@
> > >    dev->irq_pin = pci_read_config8(dev, PCI_INTERRUPT_PIN);
> > >    dev->min_gnt = pci_read_config8(dev, PCI_MIN_GNT);
> > >    dev->max_lat = pci_read_config8(dev, PCI_MAX_LAT);
> > > -#warning Per-device subsystem ID should only be read from the device if
> >
> > none has been specified for the device in the dts.
> > > dev->subsystem_vendor = pci_read_config16(dev,
> >
> > PCI_SUBSYSTEM_VENDOR_ID);
> > > dev->subsystem_device = pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> > >
> > > /* Store the interesting information in the device structure. */
> > > /*Per-device subsystem ID should only be read from the device if
> >
> > none
> > > * has been specified for the device in the dts.
> > > */
> > > if (!dev->subsystem_vendor && !dev->subsystem_device) {
> > > dev->subsystem_vendor =
> > > 		pci_read_config16(dev,
>
> PCI_SUBSYSTEM_VENDOR_ID);
> > > dev->subsystem_device =
> > > 		pci_read_config16(dev, PCI_SUBSYSTEM_ID);
> > > }
> > > dev->id.type = DEVICE_ID_PCI;
> > >  dev->id.pci.vendor = id & 0xffff;
> > >  dev->id.pci.device = (id >> 16) & 0xffff;
> > >  dev->hdr_type = hdr_type;
> > > /* Class code, the upper 3 bytes of PCI_CLASS_REVISION. */
> > >  dev->class = class >> 8;
> >
> > Hm. Although you fixed the code as indicated in the #warning, I fear
> > there's still something missing and that's the reason the warning was
> > there. Where do we set the subsystem ID of the real device (not struct
> > device)?
>
> There's another place in the code where there's a warning that addresses
> this same issue.  I didn't think we needed it in both places.

Agreed. I'll try to send a patch which fixes this FIXME.
> > > @@ -1105,6 +1135,8 @@
> > >
> > > printk(BIOS_DEBUG, "%s start bus %p, bus->dev %p\n", __func__, bus,
> > >           bus->dev);
> > >
> > > +#warning This check needs checking.
> > >    if (bus->dev->path.type != DEVICE_PATH_PCI_BUS)
> > >    	printk(BIOS_ERR, "ERROR: pci_scan_bus called with incorrect
>
> "
> > >        "bus->dev->path.type, path is %s\n", dev_path(bus-
> > >
> > > dev));
> >
> > If you manage to fix the device code in a way which doesn't trigger this
> > anymore, your static devices will suddenly be found. Since this check is
> > only triggered if your code and/or device tree is really buggy, we could
> > upgrade it to a die().
>
> I realize it's a problem.  Since I removed a couple of the other warnings, I
> thought I'd put one in this trouble spot.

Good idea.
> Thanks for the review,

Thanks for going over this code. It's easy to get lost in there and your
patches definitely improve the code.

Regards,
Carl-Daniel