Re: Bug in PCI core

[Adam Belay <abelay@MIT.EDU>]

On Fri, 2006-10-13 at 11:01 +1000, Benjamin Herrenschmidt wrote:
> On Wed, 2006-10-11 at 16:41 -0400, Alan Stern wrote:
> > When a PCI device is suspended, its driver calls pci_save_state() so that
> > the config space can be restored when the device is resumed.  Then the
> > driver calls pci_set_power_state().
> > 
> > However pci_set_power_state() calls pci_block_user_cfg_access(), and that 
> > routine calls pci_save_state() again.  This overwrites the saved state 
> > with data in which memory, I/O, and bus master accesses are disabled.  As 
> > a result, when the device is resumed it doesn't work.
> > 
> > Obviously pci_block_user_cfg_access() needs to be fixed.  I don't know the 
> > right way to fix it; hopefully somebody else does.
> 
> Well, blocking user cfg access snapshots the config space to be able to
> respond to user space while the device is offline. Maybe it should be
> done from a separate config space image buffer ? ugh....
> 
> Ben.

Personally, I don't think exposing a cached version of the PCI config
space when direct device access is prohibited is the right approach
here.  We really shouldn't be lying about the internal state of PCI
devices (the cached version could be quite inaccurate).  After all, if
the device is in D3cold, then the spec claims it's perfectly valid for
it to not respond to PCI configuration access.

I can only assume this hack was done to satisfy some terribly broken
userspace app.  It's not surprising that even reading PCI config can
easily crash systems.  However, it's the responsibility of those apps
with permission to access the PCI sysfs interface, not the kernel, to be
aware of these constraints.

The PCI configuration space cache was originally introduced to support
power management.  However, it's mostly incorrect, as it unnecessarily
stores the values of read only registers (and even BIST which is
potentially dangerous).  A while back I posted a series of patches that
address this issue, and the net result was that the config cache stays
around wasting memory because of the pci_block_user_cfg_access()
dependency despite being useless to PCI PM.

I'd like to propose that we have the pci config sysfs interface return
-EIO  when it's blocked (e.g. active BIST or D3cold).  This accurately
reflects the state of the device to userspace, reduces complexity, and
could potentially save some memory per PCI device instance.

Thanks,
Adam