On Wed, Apr 08, 2009 at 05:53:49AM +0800, Jesse Barnes wrote:
> On Mon, 6 Apr 2009 19:46:37 -0600
> Matthew Wilcox <matthew@xxxxxx> wrote:
>
> > On Tue, Apr 07, 2009 at 09:31:06AM +0800, Yu Zhao wrote:
> > > -#define PCI_EXP_SAVE_REGS 7
> > > +static const int pcie_save_regs[] = {
> > > + PCI_EXP_DEVCTL,
> > > + PCI_EXP_LNKCTL,
> > > + PCI_EXP_SLTCTL,
> > > + PCI_EXP_RTCTL,
> > > + PCI_EXP_DEVCTL2,
> > > + PCI_EXP_LNKCTL2,
> > > + PCI_EXP_SLTCTL2
> > > +};
> >
> > We could save a bit of space and store these as unsigned char, right?
> >
> > > @@ -698,20 +706,15 @@ static int pci_save_pcie_state(struct pci_dev
> > > *dev) }
> > > cap = (u16 *)&save_state->data[0];
> > >
> > > - pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_LNKCTL, &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_SLTCTL, &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_RTCTL, &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_DEVCTL2,
> > > &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_LNKCTL2,
> > > &cap[i++]);
> > > - pci_read_config_word(dev, pos + PCI_EXP_SLTCTL2,
> > > &cap[i++]);
> > > + for (i = 0; i < ARRAY_SIZE(pcie_save_regs); i++)
> > > + pci_read_config_word(dev, pos + pcie_save_regs[i],
> > > &cap[i]);
> >
> > I never looked at this code before ... but this is terribly wrong.
> > None of the capabilities after DEVCTL are guaranteed to be there. We
> > could be writing to some other register entirely.
> >
> > Unfortunately, I think we need something like:
> >
> > pci_read_config_word(dev, pos + PCI_EXP_DEVCTL, &cap[i++]);
> > if (has_link)
> > pci_read_config_word(dev, pos + PCI_EXP_LNKCTL,
> > &cap[i++]); if (has_slot)
> > pci_read_config_word(dev, pos + PCI_EXP_SLTCTL,
> > &cap[i++]); if (root_port || root event collector)
> > pci_read_config_word(dev, pos + PCI_EXP_RTCTL,
> > &cap[i++]); if (cap_version < 2)
> > return;
> > ... and so on ...
> >
> > and that means that your rather good patch looks a lot harder to
> > implement. We could maybe implement an array with pairs of (feature,
> > address), but that's probably more complex than it's worth.
>
> I should have caught that when it went in; Yu can you code up a
> complete fix along the lines of what Matthew suggests? We really can't
> touch potentially non-existent registers, and we definitely don't want
> to overwrite regs blindly...
Yes, I'll fix this. I was referring to the PCIe 2.0 spec, which requires
all registers that are not implemented by the device to be hardwired to 0.
I should check the PCIe 1.1 spec too...
--
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
