Re: [RFC][AGPGART]intel-agp: save whole config space in suspend/resume

[Jesse Barnes <jesse.barnes@intel.com>]

On Monday, June 11, 2007 8:59:14 Dave Jones wrote:
> On Tue, Jun 12, 2007 at 11:34:25AM +0800, Wang Zhenyu wrote:
>  > I understand. Before James reported his problem on i915, I have
>  > thought the basic restore on that chip should already be enough,
>  > but he proved I was wrong and I'm not sure if this also happens on
>  > other i915 board with different BIOS.
>  >
>  > And with my patch it has already removed the restore cases for
>  > 440BX like type, coz it's gmch_chip_id == 0 and
>  > intel_private.pcidev is NULL, so it won't save extra space on
>  > those chips.
>
> The 440BX was one example, for all we know there are similar ordering
> issues with other chipsets.  We hit this problem with the code that
> restores the first 64 bytes first of all. Then we found out we had
> to restore them in reverse order to be safe.  We were able to do
> this generically, because those bytes are standardised across
> devices.
>
> The upper config space isn't standardised, so we have to obey the
> per-device rules as to what order we read/write things.
> Writing back an "enable" bit somewhere before we've written back
> addresses in later registers for example may result in really
> bizarre things happening.  These are the kind of bugs that aren't
> obvious, and turn out to be "that weird reboot that happens
> every 3rd tuesday" six months after we've merged the changes
> and everyones forgotten all about the potential problems.
>
> The AGP code has had more than its fair share of really nasty
> bugs like this to track down, so I'm strongly opposed to introducing
> hacks that may trip us up later.
>
> Whilst I'm not a huge fan of the 815 patch in -mm as it stands,
> I think it's a better direction to go in to have per-chipset
> save/restore routines.

I agree.  I think saving the registers explicitly rather than as a block 
is the best long term direction.  That way we can carefully evaluate 
which ones actually need to be saved/restored, and it what order.  
Doing it as a block hides that and makes it seem chipset independent 
when it really might not be...

Jesse