On Friday 15 January 2010 04:36:50 pm Yinghai Lu wrote:
> On 01/15/2010 03:32 PM, Myron Stowe wrote:
> > Alex Williamson pointed out that the IOH_VTBAR read, as currently
> > implemented, could silently fail and, if this occurred, 'dword' would
> > be -1. Under these conditions the check of whether or not the VTBAR
> > was enabled - if (dword & 0x1 && ... - would be incorrectly interpreted
> > as true. Thus began the slippery slope of adding checks to register
> > reads.
> >
> > Signed-off-by: Myron Stowe <myron.stowe@xxxxxx>
> > ---
> >
> > arch/x86/pci/intel_bus.c | 31 ++++++++++++++++---------------
> > 1 files changed, 16 insertions(+), 15 deletions(-)
> >
> > diff --git a/arch/x86/pci/intel_bus.c b/arch/x86/pci/intel_bus.c
> > index 8be4e1d..07d773d 100644
> > --- a/arch/x86/pci/intel_bus.c
> > +++ b/arch/x86/pci/intel_bus.c
> > @@ -46,40 +46,37 @@ static void __devinit pci_root_bus_res(struct pci_dev *dev)
> > {
> > u16 word;
> > u32 dword;
> > - struct pci_root_info *info;
> > + struct pci_root_info tmp, *info = &tmp;
> > u16 io_base, io_end;
> > u32 mmiol_base, mmiol_end, vtbar;
> > u64 mmioh_base, mmioh_end;
> > int bus_base, bus_end;
> >
> > - /* some sys doesn't get mmconf enabled */
> > - if (dev->cfg_size < 0x120)
> > - return;
> > -
> > if (pci_root_num >= PCI_ROOT_NR) {
> > printk(KERN_DEBUG "intel_bus.c: PCI_ROOT_NR is too small\n");
> > return;
> > }
> >
> > - info = &pci_root_info[pci_root_num];
> > - pci_root_num++;
> > -
> > - pci_read_config_word(dev, IOH_LCFGBUS, &word);
> > + if (pci_read_config_word(dev, IOH_LCFGBUS, &word))
> > + return;
> > bus_base = (word & 0xff);
> > bus_end = (word & 0xff00) >> 8;
> > sprintf(info->name, "PCI Bus #%02x", bus_base);
> > info->bus_min = bus_base;
> > info->bus_max = bus_end;
> >
> > - pci_read_config_word(dev, IOH_LIO, &word);
> > + if (pci_read_config_word(dev, IOH_LIO, &word))
> > + return;
> > io_base = (word & 0xf0) << (12 - 4);
> > io_end = (word & 0xf000) | 0xfff;
> > update_res(info, io_base, io_end, IORESOURCE_IO, 0);
> >
> > - pci_read_config_dword(dev, IOH_LMMIOL, &dword);
> > + if (pci_read_config_dword(dev, IOH_LMMIOL, &dword))
> > + return;
> > mmiol_base = (dword & 0xff00) << (24 - 8);
> > mmiol_end = (dword & 0xff000000) | 0xffffff;
> > - pci_read_config_dword(dev, IOH_VTBAR, &dword);
> > + if (pci_read_config_dword(dev, IOH_VTBAR, &dword))
> > + return;
> > vtbar = dword & 0xfffffffe;
> > if (dword & 0x1 &&
> > (mmiol_base < vtbar + IOH_VTSIZE - 1 && vtbar < mmiol_end)) {
> > @@ -96,15 +93,19 @@ static void __devinit pci_root_bus_res(struct pci_dev *dev)
> > }
> > update_res(info, mmiol_base, mmiol_end, IORESOURCE_MEM, 0);
> >
> > - pci_read_config_dword(dev, IOH_LMMIOH, &dword);
> > + if (pci_read_config_dword(dev, IOH_LMMIOH, &dword))
> > + return;
> > mmioh_base = ((u64)(dword & 0xfc00)) << (26 - 10);
> > mmioh_end = ((u64)(dword & 0xfc000000) | 0x3ffffff);
> > - pci_read_config_dword(dev, IOH_LMMIOH_BASEU, &dword);
> > + if (pci_read_config_dword(dev, IOH_LMMIOH_BASEU, &dword))
> > + return;
> > mmioh_base |= ((u64)(dword & 0x7ffff)) << 32;
> > - pci_read_config_dword(dev, IOH_LMMIOH_LIMITU, &dword);
> > + if (pci_read_config_dword(dev, IOH_LMMIOH_LIMITU, &dword))
> > + return;
> > mmioh_end |= ((u64)(dword & 0x7ffff)) << 32;
> > update_res(info, mmioh_base, mmioh_end, IORESOURCE_MEM, 0);
> >
> > + pci_root_info[pci_root_num++] = *info;
> > print_ioh_resources(info);
> > }
> >
>
> can you just read last one like IOH_VTBAR to see if need to get out?
I think you're suggesting that it would be sufficient to only check
whether the IOH_VTBAR read succeeded. I think that's a bad idea
because if somebody else adds a read of a register past IOH_VTBAR,
it'd be easy to forget to update the check.
> too much return here...
Well, it always simplifies things if you can ignore failures. But
I think those returns are a pretty small price to pay, and the pattern
requires less maintenance effort than trying to remember to update
something like "if (dev->cfg_size < 0x120)" every time we add a new
CSR read.
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
