On 09/29/2014 11:50 AM, Thomas Renninger wrote: > Hi, > > first: > Corey's email address in MAINTAINERS is: [email protected] > But the last commits where from: [email protected] > > I added both now in this mail. Just a heads up if the > MAINTAINERS email address is or will be invalid...
Either works. > > I try to understand why or whether there can be improvment > in (auto-) loading ipmi drivers. > > Some things I realized which look as they could be improved: > > - ipmi_devintf not loaded > When impi_si is loaded, ipmi_msghandler gets loaded, but > the user interface is missing. > This might be wanted if only watchdog functionality is needed, > but nice would be if ipmi_si or ipmi_msghandler would load > this one automatically (at least via a y/n Kconfig statically > or even dynamically by e.g. making ipmi_devinitf part of > ipmi_msghandler, for the latter, find an example patch at > the end). I've debated this a bit, and I don't have a strong opinion either way. I'm sure many people will be using IPMI as part of ACPI, but without using the device interface. But ipmi_devintf is not huge, and making it part of the ipmi_msghandler module wouldn't be the end of the world. The current design is similar to I2C, which has i2c-core and i2c-dev. You have to load both (and a low-level driver) to get I2C services to userland. I would preference to keep it as it is. Does anyone else have an opinion on this? > When trying on a system which has no IPMI controller: > > - acpi_ipmi succeeds and stays loaded > > - when trying to load ipmi_si, it fails, but ipmi_msghandler > got requested and stays loaded > > > I try to understand why this is and whether it's worth digging > deeper into this and try to improve things. > > Typically I guess one wants to have the HW controlling driver > (e.g. ipmi_si, what others are there?) to autoload, requesting > all other useful drivers like ipmi_msghandler, ipmi_devintf and > if ACPI tables show an IPM adress space handler acpi_ipmi. > > If there is no underlying HW driver all the others: ipmi_msghandler, > ipmi_devintf, acpi_ipmi should not load successfully? > > Thanks for any hints. I am willing to look deeper into this (if > it makes sense and there hasn't be a consense like: Does not work > because...). The behavior is not unlike other subsystems such as I2C, USB, etc. If you try to load an I2C device that's not there, i2c-core gets loaded and doesn't get unloaded. At least I think that's the way it works. I'm not sure how you can accomplish this, either. ipmi_msghandler provides services to all the other IPMI modules, so it has to be loaded for them to work. So you can't not load ipmi_msghandler if ipmi_si doesn't find anything; ipmi_msghandler has to be loaded first. You would need some mechanism to unload ipmi_msghandler (after unloading all modules that used it) if it got loaded but there were no devices found. It's doable, but nothing else I know of does that. IMHO this is better solved as part of the upper level infrastructure. Don't try to load any of these unless you see an IPMI device in ACPI or SMBIOS or PCI. I think it already works that way to some extent. -corey > > Find attached a dirty patch, which still needs cleanup. > This one should address one of above issues. > It is compile tested only (CONFIG_IPMI_HANDLER=m). > > > EXAMPLE PATCH DO NOT ADD!!! > > ipmi: Setup ipmi_devintf automatically if ipmi_msghandler gets loaded > > This removes the ipmi_devintf to be a module, but it will automatically > compiled in if ipmi_msghandler is set. > ipmi_devintf is really small and typically is always wanted if IPMI > functionality is used. If really needed, a boot param could be added > to not export userspace interface to the ipmi_msghandler driver, but > I cannot see why this would be needed... > > Thanks, > > Thomas > > > Signed-off-by: Thomas Renninger <[email protected]> > > Index: kernel_ipmi/drivers/char/ipmi/Kconfig > =================================================================== > --- kernel_ipmi.orig/drivers/char/ipmi/Kconfig > +++ kernel_ipmi/drivers/char/ipmi/Kconfig > @@ -37,12 +37,6 @@ config IPMI_PANIC_STRING > You can fetch these events and use the sequence numbers to piece the > string together. > > -config IPMI_DEVICE_INTERFACE > - tristate 'Device interface for IPMI' > - help > - This provides an IOCTL interface to the IPMI message handler so > - userland processes may use IPMI. It supports poll() and select(). > - > config IPMI_SI > tristate 'IPMI System Interface handler' > help > Index: kernel_ipmi/drivers/char/ipmi/Makefile > =================================================================== > --- kernel_ipmi.orig/drivers/char/ipmi/Makefile > +++ kernel_ipmi/drivers/char/ipmi/Makefile > @@ -5,7 +5,7 @@ > ipmi_si-y := ipmi_si_intf.o ipmi_kcs_sm.o ipmi_smic_sm.o ipmi_bt_sm.o > > obj-$(CONFIG_IPMI_HANDLER) += ipmi_msghandler.o > -obj-$(CONFIG_IPMI_DEVICE_INTERFACE) += ipmi_devintf.o > +obj-$(CONFIG_IPMI_HANDLER) += ipmi_devintf.o > obj-$(CONFIG_IPMI_SI) += ipmi_si.o > obj-$(CONFIG_IPMI_WATCHDOG) += ipmi_watchdog.o > obj-$(CONFIG_IPMI_POWEROFF) += ipmi_poweroff.o > Index: kernel_ipmi/drivers/char/ipmi/ipmi_devintf.c > =================================================================== > --- kernel_ipmi.orig/drivers/char/ipmi/ipmi_devintf.c > +++ kernel_ipmi/drivers/char/ipmi/ipmi_devintf.c > @@ -928,7 +928,7 @@ static struct ipmi_smi_watcher smi_watch > .smi_gone = ipmi_smi_gone, > }; > > -static int __init init_ipmi_devintf(void) > +int __init init_ipmi_devintf(void) > { > int rv; > > @@ -964,9 +964,8 @@ static int __init init_ipmi_devintf(void > > return 0; > } > -module_init(init_ipmi_devintf); > > -static void __exit cleanup_ipmi(void) > +void cleanup_ipmi_dev(void) > { > struct ipmi_reg_list *entry, *entry2; > mutex_lock(®_list_mutex); > @@ -980,7 +979,6 @@ static void __exit cleanup_ipmi(void) > ipmi_smi_watcher_unregister(&smi_watcher); > unregister_chrdev(ipmi_major, DEVICE_NAME); > } > -module_exit(cleanup_ipmi); > > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Corey Minyard <[email protected]>"); > Index: kernel_ipmi/drivers/char/ipmi/ipmi_msghandler.c > =================================================================== > --- kernel_ipmi.orig/drivers/char/ipmi/ipmi_msghandler.c > +++ kernel_ipmi/drivers/char/ipmi/ipmi_msghandler.c > @@ -4545,6 +4545,7 @@ static int ipmi_init_msghandler(void) > proc_ipmi_root = proc_mkdir("ipmi", NULL); > if (!proc_ipmi_root) { > printk(KERN_ERR PFX "Unable to create IPMI proc dir"); > + driver_unregister(&ipmidriver.driver); > return -ENOMEM; > } > > @@ -4560,13 +4561,27 @@ static int ipmi_init_msghandler(void) > return 0; > } > > +void cleanup_ipmi_dev(void); > +static void cleanup_ipmi(void); > +int init_ipmi_devintf(void); > + > + > static int __init ipmi_init_msghandler_mod(void) > { > - ipmi_init_msghandler(); > + int ret; > + > + ret = ipmi_init_msghandler(); > + if (ret) > + return ret; > + ret = init_ipmi_devintf(); > + if (ret) { > + cleanup_ipmi(); > + return ret; > + } > return 0; > } > > -static void __exit cleanup_ipmi(void) > +static void cleanup_ipmi(void) > { > int count; > > @@ -4605,6 +4620,7 @@ static void __exit cleanup_ipmi(void) > if (count != 0) > printk(KERN_WARNING PFX "recv message count %d at exit\n", > count); > + cleanup_ipmi_dev(); > } > module_exit(cleanup_ipmi); > > > > ------------------------------------------------------------------------------ > Slashdot TV. Videos for Nerds. Stuff that Matters. > http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk > _______________________________________________ > Openipmi-developer mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ------------------------------------------------------------------------------ Slashdot TV. Videos for Nerds. Stuff that Matters. http://pubads.g.doubleclick.net/gampad/clk?id=160591471&iu=/4140/ostg.clktrk _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
