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...
Thanks,
--
Jesse Barnes, Intel Open Source Technology Center
--
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
