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

["Kok, Auke" <auke-jan.h.kok@intel.com>]

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:
>   *