| Message ID | 1239067866-15113-1-git-send-email-yu.zhao@intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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.
On Mon, 6 Apr 2009 19:46:37 -0600 Matthew Wilcox <matthew@wil.cx> 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,
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@wil.cx> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index fe7ac2c..af4db4e 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -679,11 +679,19 @@ pci_power_t pci_choose_state(struct pci_dev *dev, pm_message_t state) EXPORT_SYMBOL(pci_choose_state); -#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 +}; static int pci_save_pcie_state(struct pci_dev *dev) { - int pos, i = 0; + int i, pos; struct pci_cap_saved_state *save_state; u16 *cap; @@ -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]); return 0; } static void pci_restore_pcie_state(struct pci_dev *dev) { - int i = 0, pos; + int i, pos; struct pci_cap_saved_state *save_state; u16 *cap; @@ -721,13 +724,8 @@ static void pci_restore_pcie_state(struct pci_dev *dev) return; cap = (u16 *)&save_state->data[0]; - pci_write_config_word(dev, pos + PCI_EXP_DEVCTL, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_LNKCTL, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_SLTCTL, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_RTCTL, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_DEVCTL2, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_LNKCTL2, cap[i++]); - pci_write_config_word(dev, pos + PCI_EXP_SLTCTL2, cap[i++]); + for (i = 0; i < ARRAY_SIZE(pcie_save_regs); i++) + pci_write_config_word(dev, pos + pcie_save_regs[i], cap[i]); } @@ -1413,7 +1411,7 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev) int error; error = pci_add_cap_save_buffer(dev, PCI_CAP_ID_EXP, - PCI_EXP_SAVE_REGS * sizeof(u16)); + ARRAY_SIZE(pcie_save_regs) * sizeof(u16)); if (error) dev_err(&dev->dev, "unable to preallocate PCI Express save buffer\n");
Signed-off-by: Yu Zhao <yu.zhao@intel.com> --- drivers/pci/pci.c | 34 ++++++++++++++++------------------ 1 files changed, 16 insertions(+), 18 deletions(-)