Re: Panic during agp_intel attach

[David Young <dyoung%pobox.com@localhost>]

On Fri, Feb 15, 2008 at 12:05:18PM +0000, Iain Hibbert wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> On Mon, 21 Jan 2008, Andreas Gustafsson wrote:
> 
> > I tried booting -current (as of 20080118-1955 EET) on a Compaq DeskPro EN,
> > but it paniced during boot.  Here's a partial backtrace:
> >
> >   #3  0xc05686dd in panic (fmt=0xc0c5ed04 "kernel %sassertion \"%s\" 
> > failed: file \"%s\", line %d") at ../../../../kern/subr_prf.c:204
> >   #4  0xc098486f in __kernassert (t=0xc0beaa93 "diagnostic ", f=0xc0beaa6c 
> > "../../../../arch/x86/pci/pci_machdep.c", l=432, e=0xc0beaa5b "(reg & 0x3) 
> > == 0") at ../../../../../../lib/libkern/__assert.c:50
> >   #5  0xc07c2c88 in pci_conf_write (pc=0x0, tag={mode1 = 2147483648, mode2 
> > = {port = 0, enable = 0 '\0', forward = 128 '\200'}}, reg=145, data=112) at 
> > ../../../../arch/x86/pci/pci_machdep.c:432
> >   #6  0xc07c0932 in agp_intel_init (sc=0xc177e400) at 
> > ../../../../dev/pci/agp_intel.c:266
> >   #7  0xc07c05ea in agp_intel_attach (parent=0xc1785e00, self=0xc177e400, 
> > aux=0xc0e4978c) at ../../../../dev/pci/agp_intel.c:193
> >   #8  0xc07bbd31 in agpattach (parent=0xc1785e00, self=0xc177e400, 
> > aux=0xc0e49788) at ../../../../dev/pci/agp.c:331
> >
> > Apparently this is triggered by the following change:
> >
> >   revision 1.31
> >   date: 2008/01/14 18:44:17;  author: dyoung;  state: Exp;  lines: +4 -2
> >   KASSERT() that reads/writes from/to PCI configuration space are
> >   aligned on 32-bit boundaries.
> >
> > combined with the following code in agp_intel.c:
> >
> >                 pci_conf_write(sc->as_pc, sc->as_tag,
> >                         AGP_INTEL_ERRSTS, 0x70);
> >
> > and the following #define in agpreg.h:
> >
> >    #define AGP_INTEL_ERRSTS        0x91    /* Not i8XX */
> >
> > as 0x91 is not aligned to a 32-bit boundary.  What's the correct fix?
> 
> Hi Andreas,
>             Did you find the correct solution to this? I just updated my
> sources and find the same trouble.. (also in PR kern/37824)
> 
>   It seems that Martin Husemann gave the most correct answer
> 
> On Mon, 21 Jan 2008 Martin Huseman wrote:
> > Check what the code does, use 0x90 and shift all values/constants
> > accordingly, if anythings shifts out (have yet to see that), use 0x94
> > and repeat.
> 
> I guess the below patch should be correct, certainly it gets through the
> KASSERT and I don't detect any misfeature by this..  can anybody with PCI
> knowledge confirm if thats ok to commit?
> 
> iain
> 
> - --- /usr/src/sys/dev/pci/agp_intel.c        2008-02-13 19:35:33.000000000 
> +0000
> +++ agp_intel.c       2008-02-15 11:37:46.000000000 +0000
> @@ -264,7 +264,7 @@
> 
>       default:
>               pci_conf_write(sc->as_pc, sc->as_tag,
> - -                   AGP_INTEL_ERRSTS, 0x70);
> +                     AGP_INTEL_ERRSTS, 0x7000);
>       }
> 
>       return (0);
> - --- /usr/src/sys/dev/pci/agpreg.h   2008-02-13 19:35:33.000000000 +0000
> +++ agpreg.h  2008-02-15 11:53:36.000000000 +0000
> @@ -59,7 +59,7 @@
>  #define      AGP_INTEL_I8XX_ERRSTS   0xc8
> 
>  /* Common register */
> - -#define    AGP_INTEL_ERRSTS        0x91    /* Not i8XX */
> +#define      AGP_INTEL_ERRSTS        0x90    /* Not i8XX */
>  #define AGP_INTEL_AGPCMD     0xa8
>  # define AGPCMD_SBA          (1U << 9)
>  # define AGPCMD_AGPEN                (1U << 8)

The correct way is to read/modify/write the register.

IMO, the device header files should contain only dword-aligned
config-space addresses and bit fields specified with __BITS(start, end).

Dave

-- 
David Young             OJC Technologies
dyoung%ojctech.com@localhost      Urbana, IL * (217) 278-3933 ext 24