diff mbox

PCI: clean up the PCIe capability save/restore

Message ID 1239067866-15113-1-git-send-email-yu.zhao@intel.com
State Accepted, archived
Headers show

Commit Message

Yu Zhao April 7, 2009, 1:31 a.m. UTC
Signed-off-by: Yu Zhao <yu.zhao@intel.com>
---
 drivers/pci/pci.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

Comments

Matthew Wilcox April 7, 2009, 1:46 a.m. UTC | #1
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.
Jesse Barnes April 7, 2009, 9:53 p.m. UTC | #2
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,
Yu Zhao April 8, 2009, 1:43 a.m. UTC | #3
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 mbox

Patch

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");