svn commit: r329458 - head/sbin/devmatch

[Warner Losh]

On Sat, Feb 17, 2018 at 7:34 AM, Hans Petter Selasky <hselasky at freebsd.org>
wrote:

> Author: hselasky
> Date: Sat Feb 17 14:34:47 2018
> New Revision: 329458
> URL: https://svnweb.freebsd.org/changeset/base/329458
>
> Log:
>   Fix USB driver matching in devmatch(8).
>
>   Multiple drivers can match on the same USB device and the order of
> loading
>   decides which driver gets the device. Use the supplied mask value as an
>   indication of priority, so that vendor specific device drivers are loaded
>   before more generic ones.
>

This change isn't right. I mean, it might cause the special case of USB to
get the driver you want loaded, but using the MASK this way is wrong. It's
fixing the wrong problem. devmatch can't know who the winners will be and
has to recommend all the drivers to load and those drivers have to cope
with that.

The right fix there, I think, is to load them all at once, in one kldload
operation and not loop in /etc/rc.d/devmatch.

I'd request you make no further changes to devmatch without a review.

Warner



>
>   Sponsored by: Mellanox Technologies
>
> Modified:
>   head/sbin/devmatch/devmatch.c
>
> Modified: head/sbin/devmatch/devmatch.c
> ============================================================
> ==================
> --- head/sbin/devmatch/devmatch.c       Sat Feb 17 14:30:39 2018
> (r329457)
> +++ head/sbin/devmatch/devmatch.c       Sat Feb 17 14:34:47 2018
> (r329458)
> @@ -54,6 +54,14 @@ static struct option longopts[] = {
>         { NULL,                 0,                      NULL,   0 }
>  };
>
> +#define        DEVMATCH_MAX_HITS 256
> +
> +static struct match_data {
> +       char *descr;
> +       int priority;
> +} match_data[DEVMATCH_MAX_HITS];
> +
> +static int hit_index;
>  static int all_flag;
>  static int dump_flag;
>  static char *linker_hints;
> @@ -236,6 +244,35 @@ pnpval_as_str(const char *val, const char *pnpinfo)
>         return retval;
>  }
>
> +static int
> +match_data_compare(const void *_pa, const void *_pb)
> +{
> +       const struct match_data *pa = _pa;
> +       const struct match_data *pb = _pb;
> +
> +       /* biggest value first */
> +       if (pa->priority > pb->priority)
> +               return (-1);
> +       else if (pa->priority < pb->priority)
> +               return (1);
> +
> +       /* then sort by string */
> +       return (strcmp(pa->descr, pb->descr));
> +}
> +
> +static int
> +bitrev16(int input)
> +{
> +       int retval = 0;
> +       int x;
> +
> +       for (x = 0; x != 16; x++) {
> +               if ((input >> x) & 1)
> +                       retval |= (0x8000 >> x);
> +       }
> +       return (retval);
> +}
> +
>  static void
>  search_hints(const char *bus, const char *dev, const char *pnpinfo)
>  {
> @@ -367,9 +404,20 @@ search_hints(const char *bus, const char *dev, const c
>                                         printf("\n");
>                                 else if (!notme) {
>                                         if (!unbound_flag) {
> +                                               char *descr = NULL;
> +
>                                                 if (all_flag)
> -                                                       printf("%s: ",
> *dev ? dev : "unattached" );
> -                                               printf("%s\n", lastmod);
> +                                                       asprintf(&descr,
> "%s: %s", *dev ? dev : "unattached", lastmod);
> +                                               else
> +                                                       asprintf(&descr,
> "%s", lastmod);
> +
> +                                               if (descr != NULL &&
> hit_index < DEVMATCH_MAX_HITS) {
> +
>  match_data[hit_index].descr = descr;
> +
>  match_data[hit_index].priority = bitrev16(mask);
> +                                                       hit_index++;
> +                                               } else {
> +                                                       free(descr);
> +                                               }
>                                         }
>                                         found++;
>                                 }
> @@ -382,6 +430,19 @@ search_hints(const char *bus, const char *dev, const c
>                 }
>                 walker = (void *)(len - sizeof(int) + (intptr_t)walker);
>         }
> +       if (hit_index != 0) {
> +               /* sort hits by priority */
> +               mergesort(match_data, hit_index, sizeof(match_data[0]),
> &match_data_compare);
> +
> +               /* printout */
> +               for (i = 0; i != hit_index; i++) {
> +                       puts(match_data[i].descr);
> +                       free(match_data[i].descr);
> +               }
> +
> +               /* reset hit_index */
> +               hit_index = 0;
> +       }
>         if (unbound_flag && found == 0 && *pnpinfo) {
>                 if (verbose_flag)
>                         printf("------------------------- ");
>
>