On Wed, 2009-11-11 at 16:31 -0800, Yinghai Lu wrote:
> Bjorn Helgaas wrote:
> > This updates printks so they have more context and look more like the
> > usual PCI stuff.
> >
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@xxxxxx>
> > ---
> > arch/x86/pci/amd_bus.c | 43 ++++++++++++++++++++++++++-----------------
> > 1 files changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/arch/x86/pci/amd_bus.c b/arch/x86/pci/amd_bus.c
> > index 995f360..9462455 100644
> > --- a/arch/x86/pci/amd_bus.c
> > +++ b/arch/x86/pci/amd_bus.c
> > @@ -49,14 +49,13 @@ void x86_pci_root_bus_res_quirks(struct pci_bus *b)
> > if (i == pci_root_num)
> > return;
> >
> > - printk(KERN_DEBUG "PCI: peer root bus %02x res updated from pci conf\n",
> > - b->number);
> > -
> > info = &pci_root_info[i];
> > for (j = 0; j < info->res_num; j++) {
> > struct resource *res;
> > struct resource *root;
> >
> > + dev_printk(KERN_DEBUG, &b->dev,
> > + "resource %d %pR (original)\n", j, b->resource[j]);
>
> last time when i looked at %pR, it seems it can not handle all 0 (include flags) resource
>
> here, old one could have two entries: io root, and mmio_root.
>
> new value could have io port, mmio, mmio pref. aka res_num == 3,
>
> so may have %pR to handle all 0 res, or skip the print when !b->resouce[i].flags ?
These are pci_bus resources, which are just pointers to the real
resource somewhere else, and %pR can deal with null pointers fine.
Here's a sample:
pci_bus 0000:00: resource 2 (null) (original)
pci_bus 0000:00: resource 2 [mem 0xfe000000-0xffffffff] (updated from
hardware)
pci_bus 0000:00: resource 3 (null) (original)
pci_bus 0000:00: resource 3 [mem 0x4030000000-0xfcffffffff] (updated
from hardware)
> > res = &info->res[j];
> > b->resource[j] = res;
> > if (res->flags & IORESOURCE_IO)
> > @@ -64,6 +63,8 @@ void x86_pci_root_bus_res_quirks(struct pci_bus *b)
> > else
> > root = &iomem_resource;
> > insert_resource(root, res);
> > + dev_printk(KERN_DEBUG, &b->dev,
> > + "resource %d %pR (updated from hardware)\n", j, res);
> > }
> > }
> >
> > @@ -234,6 +235,7 @@ static int __init early_fill_mp_bus_info(void)
> > struct res_range range[RANGE_NUM];
> > u64 val;
> > u32 address;
> > + u16 vendor, device;
> >
> > if (!early_pci_allowed())
> > return -1;
> > @@ -241,8 +243,6 @@ static int __init early_fill_mp_bus_info(void)
> > found_all_numa_early = 0;
> > for (i = 0; i < ARRAY_SIZE(pci_probes); i++) {
> > u32 id;
> > - u16 device;
> > - u16 vendor;
> >
> > bus = pci_probes[i].bus;
> > slot = pci_probes[i].slot;
> > @@ -260,6 +260,9 @@ static int __init early_fill_mp_bus_info(void)
> > if (!found_all_numa_early)
> > return 0;
> >
> > + printk(KERN_INFO "pci 0000:%02x:%02x.0: AMD Family 10h Northbridge "
> > + "[%04x:%04x]\n", bus, slot, vendor, device);
> > +
>
> on amd system, it happens you can read all layout from one of host bridge pci conf.
> that doesn't mean to that hostbridge only.
> so should link to bus/slot here
If you'd prefer different text than mine here, can you propose
something?
>
> > pci_root_num = 0;
> > for (i = 0; i < 4; i++) {
> > int min_bus;
> > @@ -318,7 +321,8 @@ static int __init early_fill_mp_bus_info(void)
> > continue; /* not found */
> >
> > info = &pci_root_info[j];
> > - printk(KERN_DEBUG "node %d link %d: io port [%llx, %llx]\n",
> > + printk(KERN_DEBUG "pci 0000:%02x:%02x.1: node %d link %d: "
> > + "[io %#llx-%#llx]\n", bus, slot,
> > node, link, (u64)start, (u64)end);
> not here
I think maybe we should just remove this printk altogether.
> > /* kernel only handle 16 bit only */
> > @@ -353,7 +357,8 @@ static int __init early_fill_mp_bus_info(void)
> > address = MSR_K8_TOP_MEM1;
> > rdmsrl(address, val);
> > end = (val & 0xffffff800000ULL);
> > - printk(KERN_INFO "TOM: %016lx aka %ldM\n", end, end>>20);
> > + printk(KERN_INFO "pci 0000:%02x:%02x:0: TOM: %#016lx (%ldM)\n",
> > + bus, slot, end, end>>20);
> not from TOM
This one is not associated with a PCI device at all, so I'd be glad to
change this. But "TOM: %016lx aka %ldM" all by itself is completely
meaningless to the average person, so I'd like to add something to it.
Maybe a hint about what we use the TOM value for? I'll read that code
in more detail tomorrow; it looks like you might be just removing the
RAM range, the MMCONFIG range, etc, from the host bridge apertures?
> > if (end < (1ULL<<32))
> > update_range(range, 0, end - 1);
> >
> > @@ -361,7 +366,9 @@ static int __init early_fill_mp_bus_info(void)
> > get_pci_mmcfg_amd_fam10h_range();
> > /* need to take out mmconf range */
> > if (fam10h_mmconf_end) {
> > - printk(KERN_DEBUG "Fam 10h mmconf [%llx, %llx]\n", fam10h_mmconf_start, fam10h_mmconf_end);
> > + printk(KERN_DEBUG "pci 0000:%02x:%02x.0: MMCONFIG region "
> > + "[mem %#llx-%#llx]\n", bus, slot,
> > + fam10h_mmconf_start, fam10h_mmconf_end);
> not for mmconf.
This one isn't *read* from a PCI device either, but it's certainly
*connected* to PCI. Maybe something like this?
MMCONFIG region [mem %#llx-%#llx] for domains %04x-%04x
> > update_range(range, fam10h_mmconf_start, fam10h_mmconf_end);
> > }
> >
> > @@ -391,7 +398,8 @@ static int __init early_fill_mp_bus_info(void)
> >
> > info = &pci_root_info[j];
> >
> > - printk(KERN_DEBUG "node %d link %d: mmio [%llx, %llx]",
> > + printk(KERN_DEBUG "pci 0000:%02x:%02x.1: node %d link %d: "
> > + "[mem %#llx-%#llx]", bus, slot,
> > node, link, (u64)start, (u64)end);
> not here
Maybe just remove this one too (along with the KERN_CONT lines below)?
Or, if these are really telling us something useful, can you help me
understand what it is and how it connects to PCI resource management.
> > /*
> > * some sick allocation would have range overlap with fam10h
> > @@ -418,13 +426,13 @@ static int __init early_fill_mp_bus_info(void)
> > endx = fam10h_mmconf_start - 1;
> > update_res(info, start, endx, IORESOURCE_MEM, 0);
> > update_range(range, start, endx);
> > - printk(KERN_CONT " ==> [%llx, %llx]", (u64)start, endx);
> > + printk(KERN_CONT " ==> [%#llx-%#llx]", (u64)start, endx);
> > start = fam10h_mmconf_end + 1;
> > changed = 1;
> > }
> > if (changed) {
> > if (start <= end) {
> > - printk(KERN_CONT " %s [%llx, %llx]", endx?"and":"==>", (u64)start, (u64)end);
> > + printk(KERN_CONT " %s [%#llx-%#llx]", endx?"and":"==>", (u64)start, (u64)end);
> > } else {
> > printk(KERN_CONT "%s\n", endx?"":" ==> none");
> > continue;
> > @@ -447,7 +455,8 @@ static int __init early_fill_mp_bus_info(void)
> > address = MSR_K8_TOP_MEM2;
> > rdmsrl(address, val);
> > end = (val & 0xffffff800000ULL);
> > - printk(KERN_INFO "TOM2: %016lx aka %ldM\n", end, end>>20);
> > + printk(KERN_INFO "pci 0000:%02x:%02x.0: TOM2: %#016lx (%ldM)\n",
> > + bus, slot, end, end>>20);
> not here
I'll fix this the same way as "TOM", whatever that turns out to be.
> > update_range(range, 1ULL<<32, end - 1);
> > }
> >
> > @@ -479,14 +488,14 @@ static int __init early_fill_mp_bus_info(void)
> > info = &pci_root_info[i];
> > res_num = info->res_num;
> > busnum = info->bus_min;
> > - printk(KERN_DEBUG "bus: [%02x, %02x] on node %x link %x\n",
> > + printk(KERN_DEBUG "pci 0000:%02x:%02x.1: host bridge to "
> > + "[bus %02x-%02x] on node %x link %x\n", bus, slot,
> > info->bus_min, info->bus_max, info->node, info->link);
> not here
I think you're telling me that this is correct except that the PCI
device (0000:bus:slot.1) is incorrect. How can we print the correct
device ID here?
> > for (j = 0; j < res_num; j++) {
> > res = &info->res[j];
> > - printk(KERN_DEBUG "bus: %02x index %x %s: [%llx, %llx]\n",
> > - busnum, j,
> > - (res->flags & IORESOURCE_IO)?"io port":"mmio",
> > - res->start, res->end);
> > + printk(KERN_DEBUG "pci 0000:%02x:%02x.1: "
> > + "host bridge window %pR to [bus %02x-%02x]\n",
> > + bus, slot, res, info->bus_min, info->bus_max);
> not here
I assume this is the same objection as the previous one. If not, please
elaborate.
Thanks for your comments. I certainly don't want to print something
misleading; I just want to make it intelligible as well as correct.
Bjorn
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
