Re: pkg/35416: agp driver hangs on ALi/ULi M1689 chipset

[Nicolas Joly <njoly%pasteur.fr@localhost>]

The following reply was made to PR kern/35416; it has been noted by GNATS.

From: Nicolas Joly <njoly%pasteur.fr@localhost>
To: gnats-bugs%NetBSD.org@localhost
Cc: 
Subject: Re: pkg/35416: agp driver hangs on ALi/ULi M1689 chipset
Date: Mon, 7 Apr 2008 10:03:55 +0200

 --vtzGhvizbBRQ85DL
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 On Sun, Apr 06, 2008 at 04:50:02PM +0000, Martin Husemann wrote:
 > The following reply was made to PR kern/35416; it has been noted by GNATS.
 > 
 > From: Martin Husemann <martin%duskware.de@localhost>
 > To: Nicolas Joly <njoly%pasteur.fr@localhost>
 > Cc: gnats-bugs%NetBSD.org@localhost, Gary Duzan <gary%duzan.org@localhost>
 > Subject: Re: pkg/35416: agp driver hangs on ALi/ULi M1689 chipset
 > Date: Sun, 6 Apr 2008 18:49:12 +0200
 > 
 >  On Sun, Apr 06, 2008 at 11:50:04AM -0400, Gary Duzan wrote:
 >  > agp0 at pchb1: 1 Miscellaneous Control unit(s) found.
 >  > agp0: aperture at 0xdc000000, size 0x4000000
 >  
 >  Looks good, but I'd prefer if we make the non-matching on the ALI part
 >  explicit (i.e. don't rely on just the order of entries in the array).
 
 I see your point, but ...
 
 Ordering is already needed there, at least for the vendor IDs. And
 asking for specific detection to be tested before generic ones in the
 loop doesn't seems too much here.
 
 In all cases, i agree that agp_ali_attach() should be fixed to avoid
 this problem. Looking at the code, it seems that only aperture size
 bigger that 4MB are currently supported (even if a few datasheets i
 have show that 0MB/1MB/2MB are correct). Checking for an aperture size
 bigger than 1MB (and a power of two value) should be enough.
 
 -- 
 Nicolas Joly
 
 Biological Software and Databanks.
 Institut Pasteur, Paris.
 
 --vtzGhvizbBRQ85DL
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="netbsd-agpali.diff"
 
 Index: sys/dev/pci/agp.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/pci/agp.c,v
 retrieving revision 1.55
 diff -u -p -r1.55 agp.c
 --- sys/dev/pci/agp.c  29 Feb 2008 06:13:39 -0000      1.55
 +++ sys/dev/pci/agp.c  6 Apr 2008 22:54:05 -0000
 @@ -119,6 +119,11 @@ const struct agp_product {
        int             (*ap_match)(const struct pci_attach_args *);
        int             (*ap_attach)(struct device *, struct device *, void *);
  } agp_products[] = {
 +#if NAGP_AMD64 > 0
 +      { PCI_VENDOR_ALI,       PCI_PRODUCT_ALI_M1689,
 +        agp_amd64_match,      agp_amd64_attach },
 +#endif
 +
  #if NAGP_ALI > 0
        { PCI_VENDOR_ALI,       -1,
          NULL,                 agp_ali_attach },
 Index: sys/dev/pci/agp_ali.c
 ===================================================================
 RCS file: /cvsroot/src/sys/dev/pci/agp_ali.c,v
 retrieving revision 1.13
 diff -u -p -r1.13 agp_ali.c
 --- sys/dev/pci/agp_ali.c      4 Jan 2008 21:18:00 -0000       1.13
 +++ sys/dev/pci/agp_ali.c      6 Apr 2008 22:54:05 -0000
 @@ -168,14 +168,14 @@ static const u_int32_t agp_ali_table[] =
        0,                      /* 0 - invalid */
        1,                      /* 1 - invalid */
        2,                      /* 2 - invalid */
 -      4*M,                    /* 3 - invalid */
 -      8*M,                    /* 4 - invalid */
 -      0,                      /* 5 - invalid */
 -      16*M,                   /* 6 - invalid */
 -      32*M,                   /* 7 - invalid */
 -      64*M,                   /* 8 - invalid */
 -      128*M,                  /* 9 - invalid */
 -      256*M,                  /* 10 - invalid */
 +      4*M,                    /* 3 */
 +      8*M,                    /* 4 */
 +      0,                      /* 5 - Reserved */
 +      16*M,                   /* 6 */
 +      32*M,                   /* 7 */
 +      64*M,                   /* 8 */
 +      128*M,                  /* 9 */
 +      256*M,                  /* 10 */
  };
  #define agp_ali_table_size (sizeof(agp_ali_table) / sizeof(agp_ali_table[0]))
  
 @@ -200,6 +200,9 @@ agp_ali_set_aperture(struct agp_softc *s
        int i;
        pcireg_t reg;
  
 +      if (aperture & (aperture - 1) || aperture < 1*M)
 +              return EINVAL;
 +
        for (i = 0; i < agp_ali_table_size; i++)
                if (agp_ali_table[i] == aperture)
                        break;
 
 --vtzGhvizbBRQ85DL--