Re: CVS commit: src

[jnemeth%victoria.tc.ca@localhost (John Nemeth)]

On Oct 14,  3:05pm, Paul Goyette wrote:
}
} Module Name:  src
} Committed By: pgoyette
} Date:         Mon May 24 20:29:41 UTC 2010
} 
} Modified Files:
}       src/distrib/sets/lists/modules: mi
}       src/sys/dev/pci: files.pci pci.c pci_subr.c pcivar.h
}       src/sys/modules: Makefile
} Added Files:
}       src/sys/dev/pci: pci_verbose.c
}       src/sys/modules/pciverbose: Makefile
} 
} Log Message:
} Extract the vendor/product tables and related access routines into a
} separate kernel module.  Update pci bus attach routine to load the
} module (if available) when we're about to start scanning the bus, and
} unload the module after the scan is finished.

     There is no for it unload the autoloaded module.

} XXX Cardbus (and possibly other) drivers should also be modified to
} XXX load the module before scanning/attaching devices.

     Yep.  :->

} Modified files:
} 
} Index: src/sys/dev/pci/pci.c
} diff -u src/sys/dev/pci/pci.c:1.127 src/sys/dev/pci/pci.c:1.128
} --- src/sys/dev/pci/pci.c:1.127       Wed Feb 24 22:38:01 2010
} +++ src/sys/dev/pci/pci.c     Mon May 24 20:29:41 2010
} @@ -1,4 +1,4 @@
} -/*   $NetBSD: pci.c,v 1.127 2010/02/24 22:38:01 dyoung Exp $ */
} +/*   $NetBSD: pci.c,v 1.128 2010/05/24 20:29:41 pgoyette Exp $       */
}  
}  /*
}   * Copyright (c) 1995, 1996, 1997, 1998
} @@ -36,12 +36,13 @@
}   */
}  
}  #include <sys/cdefs.h>
} -__KERNEL_RCSID(0, "$NetBSD: pci.c,v 1.127 2010/02/24 22:38:01 dyoung Exp $");
} +__KERNEL_RCSID(0, "$NetBSD: pci.c,v 1.128 2010/05/24 20:29:41 pgoyette Exp 
$");
}  
}  #include "opt_pci.h"
}  
}  #include <sys/param.h>
}  #include <sys/malloc.h>
} +#include <sys/module.h>
}  #include <sys/systm.h>
}  #include <sys/device.h>
}  
} @@ -106,7 +107,12 @@
}       KASSERT(ifattr && !strcmp(ifattr, "pci"));
}       KASSERT(locators);
}  
} +     pci_verbose_ctl(true);  /* Try to load the pciverbose module */
} +

     Does this only cover boot time, or will it also cover bus rescans
(i.e. drvctl -r)?

}       pci_enumerate_bus(sc, locators, NULL, NULL);
} +
} +     pci_verbose_ctl(false); /* Now try to unload it */

     Don't forcibly unload the module.  No reason for this module to be
any different then any other module that is autoloaded.

} +
}       return 0;
}  }
}  
} 
} Index: src/sys/dev/pci/pci_subr.c
} diff -u src/sys/dev/pci/pci_subr.c:1.79 src/sys/dev/pci/pci_subr.c:1.80
} --- src/sys/dev/pci/pci_subr.c:1.79   Thu Mar  4 22:55:20 2010
} +++ src/sys/dev/pci/pci_subr.c        Mon May 24 20:29:41 2010
} [...]
} -const char *
} -pci_findproduct(pcireg_t id_reg)
} +#if defined(_KERNEL)
} +/*
} + * Routine to load/unload the pciverbose kernel module as needed
} + */
} +void pci_verbose_ctl(bool load)
}  {
} -#ifdef PCIVERBOSE
} -     static char buf[256];
} -     pci_vendor_id_t vendor = PCI_VENDOR(id_reg);
} -     pci_product_id_t product = PCI_PRODUCT(id_reg);
} -     size_t n;
} -
} -     for (n = 0; n < __arraycount(pci_products); n++) {
} -             if (pci_products[n] == vendor && pci_products[n+1] == product)
} -                     return pci_untokenstring(&pci_products[n+2], buf,
} -                         sizeof(buf));
} -
} -             /* Skip Tokens */
} -             n += 2;
} -             while (pci_products[n] != 0 && n < __arraycount(pci_products))
} -                     n++;
} +     static int loaded = 0;
} +
} +     if (load) {
} +             if (loaded++ == 0)
} +                     module_load("pciverbose", MODCTL_LOAD_FORCE, NULL,
} +                                 MODULE_CLASS_MISC);

     As discussed previously, module_load() is for the use of
modctl(2); it should not be used when automatically loading modules.
By doing this, you're ignoring the value of the kern.module.autoload
sysctl variable.  In other words, you're overriding the administrator
and that is a very bad thing.  This must be changed to a call to
module_autoload().

} +             return;
}       }
} -#endif
} -     return (NULL);
} +     if (--loaded == 0)
} +             module_unload("pciverbose");

     There is no need to forcibly unload the module.  Let the MODULAR
subsystem and/or administrator handle this.

}  }
} +#endif
}  
} Index: src/sys/dev/pci/pci_verbose.c
} diff -u /dev/null src/sys/dev/pci/pci_verbose.c:1.1
} --- /dev/null Mon May 24 20:29:41 2010
} +++ src/sys/dev/pci/pci_verbose.c     Mon May 24 20:29:40 2010
} @@ -0,0 +1,155 @@
} +/*   $NetBSD: pci_verbose.c,v 1.1 2010/05/24 20:29:40 pgoyette Exp $ */
} +
} +/*
} + * Copyright (c) 1997 Zubin D. Dittia.  All rights reserved.
} + * Copyright (c) 1995, 1996, 1998, 2000
} + *   Christopher G. Demetriou.  All rights reserved.
} + * Copyright (c) 1994 Charles M. Hannum.  All rights reserved.
} + *
} + * Redistribution and use in source and binary forms, with or without
} + * modification, are permitted provided that the following conditions
} + * are met:
} + * 1. Redistributions of source code must retain the above copyright
} + *    notice, this list of conditions and the following disclaimer.
} + * 2. Redistributions in binary form must reproduce the above copyright
} + *    notice, this list of conditions and the following disclaimer in the
} + *    documentation and/or other materials provided with the distribution.
} + * 3. All advertising materials mentioning features or use of this software
} + *    must display the following acknowledgement:
} + *   This product includes software developed by Charles M. Hannum.
} + * 4. The name of the author may not be used to endorse or promote products
} + *    derived from this software without specific prior written permission.
} + *
} + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR ``AS IS'' AND ANY EXPRESS OR
} + * IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
} + * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED.
} + * IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY DIRECT, INDIRECT,
} + * INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
} + * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
} + * DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
} + * THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
} + * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF
} + * THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
} + */
} +
} +/*
} + * PCI autoconfiguration support functions.
} + *
} + * Note: This file is also built into a userland library (libpci).
} + * Pay attention to this when you make modifications.
} + */
} +
} +#include <sys/cdefs.h>
} +__KERNEL_RCSID(0, "$NetBSD: pci_verbose.c,v 1.1 2010/05/24 20:29:40 pgoyette 
Exp $");
} +
} +#ifdef _KERNEL_OPT
} +#include "opt_pci.h"
} +#endif
} +
} +#include <sys/param.h>
} +
} +#ifdef _KERNEL
} +#include <sys/module.h>
} +#else
} +#include <pci.h>
} +#endif
} +
} +#include <dev/pci/pcireg.h>
} +#ifdef _KERNEL
} +#include <dev/pci/pcivar.h>
} +#endif
} +
} +#include <dev/pci/pcidevs.h>
} +
} +/*
} + * Descriptions of of known vendors and devices ("products").
} + */
} +
} +#include <dev/pci/pcidevs_data.h>
} +
} +#ifndef _KERNEL
} +#include <string.h>
} +#endif
} +
} +#ifdef _KERNEL
} +static int pciverbose_modcmd(modcmd_t, void *);
} +
} +MODULE(MODULE_CLASS_MISC, pciverbose, NULL);
} +
} +static int
} +pciverbose_modcmd(modcmd_t cmd, void *arg)
} +{
} +     aprint_normal("%s: cmd %d\n", __func__, cmd);   /* XXX */
} +     switch (cmd) {
} +     case MODULE_CMD_INIT:
} +             pci_findvendor_vec = pci_findvendor;
} +             pci_findproduct_vec = pci_findproduct;
} +             pci_unmatched = "unmatched ";
} +             return 0;
} +     case MODULE_CMD_FINI:
} +             pci_findvendor_vec = pci_null;
} +             pci_findproduct_vec = pci_null;
} +             pci_unmatched = "";
} +             return 0;

     I would much prefer to see the module save the previous value of
*_vec and restore it when unloading.  Most likely pci_null is the
correct value, but I don't think modules should be making assumptions
like this.

} +     default:
} +             return ENOTTY;
} +     }
} +}
} +#endif /* KERNEL */
} +
} 
}-- End of excerpt from Paul Goyette