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

From: Jesse Barnes
Date: Tue Jun 12 2007 - 13:25:53 EST


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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/