[PATCH] Automatically load video drivers based on PCI ID's

Aaron Plattner aplattner at nvidia.com
Thu Feb 21 18:17:39 PST 2008


-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Hi David,

Sorry to take so long to look at this again.  This looks better, I just
have a few remaining suggestions.


I'd check that strlen(direntry->d_name) > 3, or else bad stuff could happen
when you do strcmp(&direntry->d_name[len-3], ".so").


strncpy followed by strlen is not safe if the source string (in this case
direntry->d_name) is too long because it won't write a terminating '\0'.


You've got an extraneous "drec = NULL" right before "drec =
(DriverPtr)...".


Instead of

        retval = (char*)xalloc(strlen(drec->driverName));
        strcpy(retval, drec->driverName);

you could simply do

        retval = xstrdup(direc->driverName);


For constructing the driver path, XNFprintf looks pretty convenient, if it
works:

        driverpath = XNFprintf("%s/drivers/", xf86ModulePath);

Otherwise, you should probably use xnfalloc or better yet, fail gracefully
if xalloc fails.


I don't think

        char *matches[matchmax]

works on all compilers, though I could be wrong.


- -- Aaron

On Tue, Feb 05, 2008 at 09:19:29PM -0500, David Nusinow wrote:
> Drivers must be pci-reworked and claim a set of ID's in their driver rec.
> 
> This patch also removes previous attempts at driver autodetection,
> including my misguided text file based version and the previous heuristic
> based only on the vendor ID.
> ---
>  configure.ac                       |    1 -
>  hw/xfree86/common/xf86AutoConfig.c |  271 +++++++++++++-----------------------
>  hw/xfree86/common/xf86Config.h     |    1 -
>  include/xorg-config.h.in           |    3 -
>  4 files changed, 99 insertions(+), 177 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 566ddcb..7ee97c8 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -1043,7 +1043,6 @@ if test "x$XDMAUTH" = xyes; then
>  fi
>  
>  AC_DEFINE_DIR(COMPILEDDEFAULTFONTPATH, FONTPATH, [Default font path])
> -AC_DEFINE_DIR(PCI_TXT_IDS_PATH, PCI_TXT_IDS_DIR, [Default PCI text file ID path])
>  AC_DEFINE_DIR(SERVER_MISC_CONFIG_PATH, SERVERCONFIG, [Server miscellaneous config path])
>  AC_DEFINE_DIR(BASE_FONT_PATH, FONTDIR, [Default base font path])
>  AC_DEFINE_DIR(DRI_DRIVER_PATH, DRI_DRIVER_PATH, [Default DRI driver path])
> diff --git a/hw/xfree86/common/xf86AutoConfig.c b/hw/xfree86/common/xf86AutoConfig.c
> index c6e1972..1c74904 100644
> --- a/hw/xfree86/common/xf86AutoConfig.c
> +++ b/hw/xfree86/common/xf86AutoConfig.c
> @@ -33,6 +33,7 @@
>  #include <xorg-config.h>
>  #endif
>  
> +#include <dlfcn.h>
>  #include "xf86.h"
>  #include "xf86Parser.h"
>  #include "xf86tokens.h"
> @@ -136,61 +137,6 @@ AppendToConfig(const char *s)
>      AppendToList(s, &builtinConfig, &builtinLines);
>  }
>  
> -static const char *
> -videoPtrToDriverName(struct pci_device *dev)
> -{
> -    /*
> -     * things not handled yet:
> -     * amd/cyrix/nsc
> -     * xgi
> -     */
> -
> -    switch (dev->vendor_id)
> -    {
> -	case 0x1142:		    return "apm";
> -	case 0xedd8:		    return "ark";
> -	case 0x1a03:		    return "ast";
> -	case 0x1002:		    return "ati";
> -	case 0x102c:		    return "chips";
> -	case 0x1013:		    return "cirrus";
> -	case 0x8086:
> -	    if ((dev->device_id == 0x00d1) || (dev->device_id == 0x7800))
> -		return "i740";
> -	    else return "intel";
> -	case 0x102b:		    return "mga";
> -	case 0x10c8:		    return "neomagic";
> -	case 0x105d:		    return "i128";
> -	case 0x10de: case 0x12d2:   return "nv";
> -	case 0x1163:		    return "rendition";
> -	case 0x5333:
> -	    switch (dev->device_id)
> -	    {
> -		case 0x88d0: case 0x88d1: case 0x88f0: case 0x8811:
> -		case 0x8812: case 0x8814: case 0x8901:
> -		    return "s3";
> -		case 0x5631: case 0x883d: case 0x8a01: case 0x8a10:
> -		case 0x8c01: case 0x8c03: case 0x8904: case 0x8a13:
> -		    return "s3virge";
> -		default:
> -		    return "savage";
> -	    }
> -	case 0x1039:		    return "sis";
> -	case 0x126f:		    return "siliconmotion";
> -	case 0x121a:
> -	    if (dev->device_id < 0x0003)
> -	        return "voodoo";
> -	    else
> -	        return "tdfx";
> -	case 0x3d3d:		    return "glint";
> -	case 0x1023:		    return "trident";
> -	case 0x100c:		    return "tseng";
> -	case 0x1106:		    return "via";
> -	case 0x15ad:		    return "vmware";
> -	default: break;
> -    }
> -    return NULL;
> -}
> -
>  Bool
>  xf86AutoConfig(void)
>  {
> @@ -250,22 +196,6 @@ xf86AutoConfig(void)
>      return (ret == CONFIG_OK);
>  }
>  
> -int 
> -xchomp(char *line)
> -{
> -    size_t len = 0;
> -
> -    if (!line) {
> -        return 1;
> -    }
> -
> -    len = strlen(line);
> -    if (line[len - 1] == '\n' && len > 0) {
> -        line[len - 1] = '\0';
> -    }
> -    return 0;
> -}
> -
>  GDevPtr
>  autoConfigDevice(GDevPtr preconf_device)
>  {
> @@ -307,109 +237,108 @@ autoConfigDevice(GDevPtr preconf_device)
>      return ptr;
>  }
>  
> -#ifdef __linux__
> -/* This function is used to provide a workaround for binary drivers that
> - * don't export their PCI ID's properly. If distros don't end up using this
> - * feature it can and should be removed because the symbol-based resolution
> - * scheme should be the primary one */
> +static char*
> +driverClaimsID(struct dirent *direntry, struct pci_device *match, char *driverpath)
> +{
> +    char *retval = NULL;
> +    char *modulepath = NULL;
> +    char *drivername = NULL;
> +    int len;
> +    int buflen = 256;
> +    int  tmp, i;
> +    void *handle;
> +    const struct pci_id_match *driver_pci;
> +    DriverPtr drec;
> +
> +    if (!direntry)
> +        goto end;
> +    len = strlen(direntry->d_name);
> +    if ((direntry->d_name[0] == '.') ||
> +        (strcmp(&(direntry->d_name[len-3]), ".so") != 0 ) ) {
> +            goto end;
> +    }
> +
> +    drivername = (char*)xalloc(buflen);
> +    modulepath = (char*)xalloc(strlen(driverpath) + buflen);
> +
> +    strcpy(modulepath, driverpath);
> +    strncat(modulepath, direntry->d_name, buflen);
> +    handle = dlopen(modulepath, RTLD_LAZY);
> +    if (!handle) {
> +        xf86Msg(X_ERROR, "Could not dlopen the module %s\n", direntry->d_name);
> +        goto end;
> +    }
> +
> +    /* Figure out driver name to get the right symbol */
> +    strncpy(drivername, direntry->d_name, buflen);
> +    tmp = strlen(drivername);
> +    for (i = 0 ; i < tmp ; i++) {
> +        if (isalnum(drivername[i])) 
> +            drivername[i] = toupper(drivername[i]);
> +        else
> +            drivername[i] = '\0';
> +    }
> +    /* Get the DriverRec and look for a PCI ID match */
> +    drec = NULL;
> +    drec = (DriverPtr)dlsym(handle, drivername);
> +    if (!drec) {
> +        goto end;
> +    }
> +
> +    driver_pci = drec->supported_devices;
> +    while (driver_pci->vendor_id != 0) {
> +        if (PCI_ID_COMPARE(driver_pci->vendor_id, match->vendor_id) &&
> +            PCI_ID_COMPARE(driver_pci->device_id, match->device_id) &&
> +            PCI_ID_COMPARE(driver_pci->subvendor_id, match->subvendor_id) &&
> +            PCI_ID_COMPARE(driver_pci->subdevice_id, match->subdevice_id)) {
> +               xf86Msg(X_DEFAULT, "Driver %s supports the primary video device.\n", drec->driverName);
> +               retval = (char*)xalloc(strlen(drec->driverName));
> +               strcpy(retval, drec->driverName);
> +               break;
> +        }
> +        driver_pci++;
> +    }
> +    dlclose(handle);
> +    
> +    end:
> +    xfree(modulepath);
> +    xfree(drivername);
> +    return retval;
> +}
> +
>  static void
> -matchDriverFromFiles (char** matches, uint16_t match_vendor, uint16_t match_chip)
> +matchDriverFromSymbols (char** matches, int matchmax, struct pci_device *match)
>  {
> -    DIR *idsdir;
> -    FILE *fp;
> +    char *driverpath, *matcheddriver;
> +    DIR *dir;
>      struct dirent *direntry;
> -    char *line = NULL;
> -    size_t len;
> -    ssize_t read;
> -    char path_name[256], vendor_str[5], chip_str[5];
> -    uint16_t vendor, chip;
> -    int i, j;
> -
> -    idsdir = opendir(PCI_TXT_IDS_PATH);
> -    if (idsdir) {
> -         xf86Msg(X_INFO, "Scanning %s directory for additional PCI ID's supported by the drivers\n", PCI_TXT_IDS_PATH);
> -        direntry = readdir(idsdir);
> -        /* Read the directory */
> +    int i;
> +
> +    xf86Msg(X_INFO, "Searching for drivers that support the primary pci video\n");
> +    driverpath = (char*)xalloc(strlen(xf86ModulePath) + strlen("/drivers/") + 1);
> +    strcpy(driverpath, xf86ModulePath);
> +    strcat(driverpath, "/drivers/");
> +    dir = opendir(driverpath);
> +    if (dir) {
> +        direntry = readdir(dir);
>          while (direntry) {
> -            if (direntry->d_name[0] == '.') {
> -                direntry = readdir(idsdir);
> -                continue;
> -            }
> -            len = strlen(direntry->d_name);
> -            /* A tiny bit of sanity checking. We should probably do better */
> -            if (strncmp(&(direntry->d_name[len-4]), ".ids", 4) == 0) {
> -                /* We need the full path name to open the file */
> -                strncpy(path_name, PCI_TXT_IDS_PATH, 256);
> -                strncat(path_name, "/", 1);
> -                strncat(path_name, direntry->d_name, (256 - strlen(path_name) - 1));
> -                fp = fopen(path_name, "r");
> -                if (fp == NULL) {
> -                    xf86Msg(X_ERROR, "Could not open %s for reading. Exiting.\n", path_name);
> -                    goto end;
> -                }
> -                /* Read the file */
> -                #ifdef __GLIBC__
> -                while ((read = getline(&line, &len, fp)) != -1) {
> -                #else
> -                while ((line = fgetln(fp, &len)) != (char *)NULL) {
> -                #endif /* __GLIBC __ */
> -                    xchomp(line);
> -                    if (isdigit(line[0])) {
> -                        strncpy(vendor_str, line, 4);
> -                        vendor_str[4] = '\0';
> -                        vendor = (int)strtol(vendor_str, NULL, 16);
> -                        if ((strlen(&line[4])) == 0) {
> -                                chip_str[0] = '\0';
> -                                chip = -1;
> -                        } else {
> -                                /* Handle trailing whitespace */
> -                                if (isspace(line[4])) {
> -                                    chip_str[0] = '\0';
> -                                    chip = -1;
> -                                } else {
> -                                /* Ok, it's a real ID */
> -                                    strncpy(chip_str, &line[4], 4);
> -                                    chip_str[4] = '\0';
> -                                    chip = (int)strtol(chip_str, NULL, 16);
> -                                }
> -                        }
> -                        if (vendor == match_vendor && chip == match_chip ) {
> -                            i = 0;
> -                            while (matches[i]) {
> -                                i++;
> -                            }
> -                            matches[i] = (char*)xalloc(sizeof(char) * strlen(direntry->d_name) -  3);
> -                            if (!matches[i]) {
> -                                xf86Msg(X_ERROR, "Could not allocate space for the module name. Exiting.\n");
> -                                goto end;
> -                            }
> -                            /* hack off the .ids suffix. This should guard
> -                             * against other problems, but it will end up
> -                             * taking off anything after the first '.' */
> -                            for (j = 0; j < (strlen(direntry->d_name) - 3) ; j++) {
> -                                if (direntry->d_name[j] == '.') {
> -                                    matches[i][j] = '\0';
> -                                    break;
> -                                } else {
> -                                    matches[i][j] = direntry->d_name[j];
> -                                }
> -                            }
> -                            xf86Msg(X_INFO, "Matched %s from file name %s\n", matches[i], direntry->d_name);
> -                        }
> -                    } else {
> -                        /* TODO Handle driver overrides here */
> +            matcheddriver = driverClaimsID(direntry, match, driverpath);
> +            if (matcheddriver) {
> +                for (i = 0 ; i < matchmax ; i++) {
> +                    if (matches[i] == NULL) {
> +                        matches[i] = matcheddriver;
> +                        break;
>                      }
>                  }
> -                fclose(fp);
>              }
> -            direntry = readdir(idsdir);
> -        }
> +            direntry = readdir(dir);
> +        } 
> +    } else {
> +        xf86Msg(X_ERROR, "Could not open driver dir %s for reading.\n", driverpath);
>      }
> -    end:
> -    xfree(line);
> -    closedir(idsdir);
> +    closedir(dir);
> +    xfree(driverpath);
>  }
> -#endif /* __linux__ */
>  
>  char*
>  chooseVideoDriver(void)
> @@ -418,9 +347,10 @@ chooseVideoDriver(void)
>      struct pci_device_iterator *iter;
>      char *chosen_driver = NULL;
>      int i;
> -    char *matches[20]; /* If we have more than 20 drivers we're in trouble */
> +    int matchmax = 20; /* If we have more than 20 drivers we're in trouble */
> +    char *matches[matchmax]; 
>      
> -    for (i=0 ; i<20 ; i++)
> +    for (i=0 ; i < matchmax ; i++)
>          matches[i] = NULL;
>  
>      /* Find the primary device, and get some information about it. */
> @@ -437,17 +367,14 @@ chooseVideoDriver(void)
>  	ErrorF("Primary device is not PCI\n");
>      }
>  
> -#ifdef __linux__
> -    matchDriverFromFiles(matches, info->vendor_id, info->device_id);
> -#endif /* __linux__ */
> +    matchDriverFromSymbols(matches, matchmax, info);
>  
>      /* TODO Handle multiple drivers claiming to support the same PCI ID */
>      if (matches[0]) {
>          chosen_driver = matches[0];
>      } else {
> -	if (info != NULL)
> -	    chosen_driver = videoPtrToDriverName(info);
>  	if (chosen_driver == NULL) {
> +            xf86Msg(X_DEFAULT, "Could not find a driver that explicitly supports the primary video card. Falling back to the default driver.\n");
>  #if defined  __i386__ || defined __amd64__ || defined __hurd__
>  	    chosen_driver = "vesa";
>  #elif defined __alpha__
> diff --git a/hw/xfree86/common/xf86Config.h b/hw/xfree86/common/xf86Config.h
> index a174e46..3411423 100644
> --- a/hw/xfree86/common/xf86Config.h
> +++ b/hw/xfree86/common/xf86Config.h
> @@ -69,6 +69,5 @@ ConfigStatus xf86HandleConfigFile(Bool);
>  Bool xf86AutoConfig(void);
>  GDevPtr autoConfigDevice(GDevPtr preconf_device);
>  char* chooseVideoDriver(void);
> -int xchomp(char *line);
>  
>  #endif /* _xf86_config_h */
> diff --git a/include/xorg-config.h.in b/include/xorg-config.h.in
> index b91ea92..97d53a2 100644
> --- a/include/xorg-config.h.in
> +++ b/include/xorg-config.h.in
> @@ -112,7 +112,4 @@
>  /* Have execinfo.h */
>  #undef HAVE_EXECINFO_H
>  
> -/* Path to text files containing PCI IDs */
> -#undef PCI_TXT_IDS_PATH
> -
>  #endif /* _XORG_CONFIG_H_ */
> -- 
> 1.5.4
> 
> _______________________________________________
> xorg mailing list
> xorg at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/xorg
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.7 (GNU/Linux)

iQEVAwUBR74ww3YgpP6LHaLQAQK8Xwf/akV70KUgPacI/7q34qTsmUwqCrNPoIJi
td6KR6o5fMENfiVd9Pmfuq3CWUIFBfB4x4+m+yS4MMDO/DBqc5Pq1fyCCchNsIbO
WqRSf5ZS+733mmcpVjsqeqcfAK32VS6/arhcTMEdsqzQsByIbJm1+3+WLzytdMt6
fJhsBztZSIloBkAWqddbF69HOCUSruM7QLL3LvLYBEJlMBFEiou4PT5fVGVQIOpX
3PTJLGO5Zq8j2pJkIkUmCCItn5NKwuGp+9R4KGCmxXYP1hptE/89k+5hKIN2PPHB
6gJbTnMlRj3XRiceg/sAF9RKcBdYvGegYGjPGZRzpJjPwESjYjAYVg==
=7kFD
-----END PGP SIGNATURE-----



More information about the xorg mailing list