Re: [PATCH 1/2]MSI(X) save/restore for suspend/resume

[Greg KH <greg@kroah.com>]

On Wed, Jan 11, 2006 at 09:17:19AM +0800, Shaohua Li wrote:
> On Tue, 2006-01-10 at 13:28 -0700, Matthew Wilcox wrote:
> > On Thu, Jan 05, 2006 at 08:58:00AM +0800, Shaohua Li wrote:
> > > +	if ((pos = pci_find_capability(dev, PCI_CAP_ID_MSIX)) <= 0 ||
> > > +		dev->no_msi)
> > 
> > No assignments within conditionals, please.
> Ok.
> 
> > 	pos = pci_find_capability(dev, PCI_CAP_ID_MSIX);
> > 	if (pos <= 0 || dev->no_msi)
> > 
> > >  	u32		saved_config_space[16]; /* config space saved at suspend time */
> > > +	void		*saved_cap_space[PCI_CAP_ID_MAX + 1]; /* ext config space saved at suspend time */
> > >  	struct bin_attribute *rom_attr; /* attribute descriptor for sysfs ROM entry */
> > ...
> > >  #define  PCI_CAP_ID_MSIX	0x11	/* MSI-X */
> > > +#define PCI_CAP_ID_MAX		PCI_CAP_ID_MSIX
> > >  #define PCI_CAP_LIST_NEXT	1	/* Next capability in the list */
> > 
> > Rather than taking all this space in the pci_dev structure (even
> > without CONFIG_PM), how about:
> > 
> > struct pci_cap_saved_state {
> > 	struct pci_cap_saved_state *next;
> > 	char cap_nr;
> > 	char data[0];
> > }
> > 
> > and then just add:
> > 
> > 	struct pci_cap_saved_state *saved_cap_space;
> > 
> > to the struct pci_dev?  One pointer, rather than (currently!) 12.
> > That's an 88 byte saving per PCI device on 64-bit machines!
> It's not that big, right? This will make things a little complex. How
> about just define saved_cap_space[] with CONFIG_PM configured? Anyway,
> if you insist on less space, I'm happy to change it.

A pointer to the structure is much nicer, I'd recommend doing it that
way.

thanks,

greg k-h