Re: [PATCH 2/2] pci: Repair pci_save/restore_state so we can restore one save many times.

Kok, Auke
2007-03-12T22:46:51Z

Eric W. Biederman wrote:
> Because we do not reserve space for the pci-x and pci-e state in struct
> pci dev we need to dynamically allocate it. However because we need
> to support restore being called multiple times after a single save
> it is never safe to free the buffers we have allocated to hold the
> state.
>
> So this patch modifies the save routines to first check to see
> if we have already allocated a state buffer before allocating
> a new one. Then the restore routines are modified to not free
> the state after restoring it. Simple and it fixes some subtle
> error path handling bugs, that are hard to test for.
>
> Signed-off-by: Eric W. Biederman <ebiederm@xmission.com>

I tested this patch and the other 2 in this series:

[PATCH 0/2] Repair pci_restore_state when used with device resets
[PATCH 1/2] msi: Safer state caching.

against e1000 with suspend/resume functionality. Apart from a minor symmetry
violation in e1000 for which I will send a patch later, these patches appear to
work fine on my ich8 with 5 msi capable e1000 ports.

Feel free to add my Signed-off-by: Auke Kok <auke-jan.h.kok@intel.com>

Cheers,

Auke

> ---
> drivers/pci/pci.c | 12 ++++++------
> include/linux/pci.h | 5 -----
> 2 files changed, 6 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 6fb78df..b292c9a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -551,7 +551,9 @@ static int pci_save_pcie_state(struct pci_dev *dev)
> if (pos <= 0)
> return 0;
>
> - save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 4, GFP_KERNEL);
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> + if (!save_state)
> + save_state = kzalloc(sizeof(*save_state) + sizeof(u16) * 4, GFP_KERNEL);
> if (!save_state) {
> dev_err(&dev->dev, "Out of memory in pci_save_pcie_state\n");
> return -ENOMEM;
> @@ -582,8 +584,6 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
> 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_remove_saved_cap(save_state);
> - kfree(save_state);
> }
>
>
> @@ -597,7 +597,9 @@ static int pci_save_pcix_state(struct pci_dev *dev)
> if (pos <= 0)
> return 0;
>
> - save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
> + save_state = pci_find_saved_cap(dev, PCI_CAP_ID_EXP);
> + if (!save_state)
> + save_state = kzalloc(sizeof(*save_state) + sizeof(u16), GFP_KERNEL);
> if (!save_state) {
> dev_err(&dev->dev, "Out of memory in pci_save_pcie_state\n");
> return -ENOMEM;
> @@ -622,8 +624,6 @@ static void pci_restore_pcix_state(struct pci_dev *dev)
> cap = (u16 *)&save_state->data[0];
>
> pci_write_config_word(dev, pos + PCI_X_CMD, cap[i++]);
> - pci_remove_saved_cap(save_state);
> - kfree(save_state);
> }
>
>
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 78417e4..481ea06 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -209,11 +209,6 @@ static inline void pci_add_saved_cap(struct pci_dev *pci_dev,
> hlist_add_head(&new_cap->next, &pci_dev->saved_cap_space);
> }
>
> -static inline void pci_remove_saved_cap(struct pci_cap_saved_state *cap)
> -{
> - hlist_del(&cap->next);
> -}
> -
> /*
> * For PCI devices, the region numbers are assigned this way:
> *