Re: [patch 1/3] ipmi: Setup ipmi_devintf automatically if ipmi_msghandler gets loaded

From: Corey Minyard
Date: Tue Oct 14 2014 - 21:22:32 EST

On 10/14/2014 09:40 AM, trenn@xxxxxxx wrote:
> This removes the ipmi_devintf to be a module, but it will automatically
> compiled in if ipmi_msghandler is set.
>
> ipmi_msghandler module is renamed to ipmi_handler because of the name
> clash with the ipmi_msghandler.o object file (see Makefile for details).
> Alternatively ipmi_msghandler.c could be renamed to ipmi_handler.c, but
> not polluting git history should be more of an advantage than module renaming.
>
> cleanup_ipmi_dev() and init_ipmi_devintf() implemented in ipmi_devintf.c
> are are simply declared ipmi_msghandler.c without creating a separate header
> file which looks more reasonable to me. Hope that is ok.

One minor style issue: the function definitions should really be in a .h
file.

Renaming the module is also probably a bad idea. If this was to go in,
it would certainly need to keep the ipmi_msghandler.ko name to avoid
complete breakage all over the place.

In that vein, I am worried that this would just result in a lot of work
for all the
distros that have set up this already. I'm trying to see the pros and
cons of
this, and I can't see that the pros outweigh the cons. Do you have people
that have issues with this?

Does anyone else care about this? Unless I hear from some people that
the way it is causes issues, I don't think I can let this in.

>
> In fact there already was a kind of autoloading mechanism (gets deleted
> with this patch). I interpret this line that ipmi_devintf should get
> autoloaded if ipmi_si gets loaded?:
> -MODULE_ALIAS("platform:ipmi_si");
> But:
> - It's wrong: There are other low lever drivers which can be used by
> ipmi_devintf
> - It does not work (anymore?)
> - There is no need to keep ipmi_devintf as a module (resource and load time
> overhead)

This change is certainly a good idea. Whatever it was trying to do is
wrong.

-corey

>
> Signed-off-by: Thomas Renninger <trenn@xxxxxxx>
> CC: minyard@xxxxxxx
> CC: martin.wilck@xxxxxxxxxxxxxx
>
> Index: kernel_ipmi/drivers/char/ipmi/Kconfig
> ===================================================================
> --- kernel_ipmi.orig/drivers/char/ipmi/Kconfig
> +++ kernel_ipmi/drivers/char/ipmi/Kconfig
> @@ -8,6 +8,8 @@ menuconfig IPMI_HANDLER
> help
> This enables the central IPMI message handler, required for IPMI
> to work.
> + It also provides userspace interface /dev/ipmiX, so that userspace
> + tools can query the BMC.
>
> IPMI is a standard for managing sensors (temperature,
> voltage, etc.) in a system.
> @@ -37,12 +39,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
> @@ -4,8 +4,10 @@
>
> 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_handler.o
> +
> +ipmi_handler-objs := ipmi_msghandler.o 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(&reg_list_mutex);
> @@ -980,9 +979,3 @@ 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 <minyard@xxxxxxxxxx>");
> -MODULE_DESCRIPTION("Linux device interface for the IPMI message handler.");
> -MODULE_ALIAS("platform:ipmi_si");
> 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
> @@ -4560,13 +4560,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 +4619,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);
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/