Discussion:
[patch 0/3] Build ipmi_devintf into ipmi_msghandler
t***@suse.de
2014-10-14 14:40:20 UTC
Permalink
Review and comments welcome.

Would be great to see this queued up in a kernel maintainers branch to show
up in Linus' kernel soon.

Thanks a lot,

Thomas
--
Thomas Renninger,SUSE LINUX GmbH; Maxfeldstrasse 5; D-90409 Nuernberg; Zi. 2.3-10,+49-911-740 53-675,,serv=loki,mail=imap,type=real <***@arch.suse.de>
t***@suse.de
2014-10-14 14:40:21 UTC
Permalink
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.

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)

Signed-off-by: Thomas Renninger <***@suse.de>
CC: ***@acm.org
CC: ***@ts.fujitsu.com

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 <***@mvista.com>");
-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);
Corey Minyard
2014-10-15 01:22:20 UTC
Permalink
Post by t***@suse.de
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.
Post by t***@suse.de
In fact there already was a kind of autoloading mechanism (gets deleted
with this patch). I interpret this line that ipmi_devintf should get
-MODULE_ALIAS("platform:ipmi_si");
- 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
Post by t***@suse.de
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_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);
Thomas Renninger
2014-10-17 07:49:24 UTC
Permalink
Hi,
Post by Corey Minyard
Post by t***@suse.de
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.
Ok.
Post by Corey Minyard
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.
? What should break?
It should never be needed to load ipmi_msghandler manually.
Even if autoloading does not work, loading the low level
serving driver (e.g. ipmi_si) is enough. This one will request
ipmi_(msg)handler.ko automatically.
I excpect this is the same for all other ipmi HW serving drivers, right?
Post by Corey Minyard
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.
We also loaded the ipmi driver manually via the OpenIPMI init service
which simply tried to load ipmi drivers.
This results in boot load time, resource and code overhead.
As ipmi autoload seem to work fine for recent HW these days, the userspace
interface should load as well, otherwise the whole autoloading is more
or less useless.
Post by Corey Minyard
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?
Yes. In fact Fujitsu opened a bug for this. Unfortunately I cannot set it
public as it is against SLES12. If I find a way, I'll reference it in
the changelog:
https://bugzilla.novell.com/show_bug.cgi?id=893181
Subject:
ipmi_devintf is not available when IPMI device is detected

I already added Martin to CC: of the patch changelog, but quilt may
not have recogized it and not added him to CC?

Beside Fujitsu, I'd like to have this fixed as well.
We adjust BMC settings via after install script on our servers via
ipmitool. This does not work because of this bug.
One has to:
modprobe ipmi_devintf
This is overhead and makes the whole autoloading mechanism useless.
And even worse, ipmi_devintf seem to load on all machines whether they
have an ipmi controller or not.
Post by Corey Minyard
Post by t***@suse.de
In fact there already was a kind of autoloading mechanism (gets deleted
with this patch). I interpret this line that ipmi_devintf should get
-MODULE_ALIAS("platform:ipmi_si");
- 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.
Thanks. I am convinced that the right way to go for is to integrate the
ipmi_devintf into ipmi_msghandler.

I see 3 options how to do this:
- rename ipmi_msghandler.ko to ipmi_handler.ko
As this only is an interface provider for other modules getting
loaded/requested automatically and never needs to be loaded manually
(pls correct me if I am wrong), I would like to go for this option.

- rename ipmi_msghandler.c to ipmi_handler.c
-> git history lost. But should still be seen via:
git log --follow?

- Someone finds another way how to modify the Makefile to achieve above.

Thanks,

Thomas
Corey Minyard
2014-10-17 16:14:15 UTC
Permalink
Post by Thomas Renninger
Hi,
Post by Corey Minyard
Post by t***@suse.de
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.
Ok.
Post by Corey Minyard
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.
? What should break?
It should never be needed to load ipmi_msghandler manually.
Even if autoloading does not work, loading the low level
serving driver (e.g. ipmi_si) is enough. This one will request
ipmi_(msg)handler.ko automatically.
I excpect this is the same for all other ipmi HW serving drivers, right?
Post by Corey Minyard
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.
We also loaded the ipmi driver manually via the OpenIPMI init service
which simply tried to load ipmi drivers.
This results in boot load time, resource and code overhead.
As ipmi autoload seem to work fine for recent HW these days, the userspace
interface should load as well, otherwise the whole autoloading is more
or less useless.
This is the kind of stuff I'm worried about. If there are script that load
modules, they may be broken. Module dependencies in modprobe.d
may be broken. Things like that.

By changing a name you are, in effect, changing a user interface and
that should be done with extreme care.
Post by Thomas Renninger
Post by Corey Minyard
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?
Yes. In fact Fujitsu opened a bug for this. Unfortunately I cannot set it
public as it is against SLES12. If I find a way, I'll reference it in
https://bugzilla.novell.com/show_bug.cgi?id=893181
ipmi_devintf is not available when IPMI device is detected
I already added Martin to CC: of the patch changelog, but quilt may
not have recogized it and not added him to CC?
Beside Fujitsu, I'd like to have this fixed as well.
We adjust BMC settings via after install script on our servers via
ipmitool. This does not work because of this bug.
modprobe ipmi_devintf
This is overhead and makes the whole autoloading mechanism useless.
And even worse, ipmi_devintf seem to load on all machines whether they
have an ipmi controller or not.
Post by Corey Minyard
Post by t***@suse.de
In fact there already was a kind of autoloading mechanism (gets deleted
with this patch). I interpret this line that ipmi_devintf should get
-MODULE_ALIAS("platform:ipmi_si");
- 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.
Thanks. I am convinced that the right way to go for is to integrate the
ipmi_devintf into ipmi_msghandler.
- rename ipmi_msghandler.ko to ipmi_handler.ko
As this only is an interface provider for other modules getting
loaded/requested automatically and never needs to be loaded manually
(pls correct me if I am wrong), I would like to go for this option.
- rename ipmi_msghandler.c to ipmi_handler.c
git log --follow?
- Someone finds another way how to modify the Makefile to achieve above.
How about this. I did a little research, and there's something called soft
module dependencies. Apparently you can add:

MODULE_SOFTDEP("post: ipmi_devintf")

to ipmi_msghandler.c and modprobe would load ipmi_devintf after loading
ipmi_msghandler if it was available.


-corey
Post by Thomas Renninger
Thanks,
Thomas
Wilck, Martin
2014-10-20 08:28:53 UTC
Permalink
Post by Corey Minyard
How about this. I did a little research, and there's something called soft
MODULE_SOFTDEP("post: ipmi_devintf")
to ipmi_msghandler.c and modprobe would load ipmi_devintf after loading
ipmi_msghandler if it was available.
This is nice, but not commonly available in distro kernels so far.
AFAICS, out of the distros Fujitsu supports, only RHEL7 supports it.

I vote for MODULE_SOFTDEP for upstream, and modalias for distros that
don't support MODULE_SOFTDEP yet.

Martin

t***@suse.de
2014-10-14 14:40:22 UTC
Permalink
There should be no need to specify the major number of /dev/ipmiX via module
parameter. Others do not need this as well.
This is the way msr.c (/dev/cpu/X/msr) is doing it as well.

Side effect of this patch will be that the userspace interface cannot
be disabled at kernel level anymore. But this can also be enforced by not
letting userspace create the device file /dev/ipmiX.

Signed-off-by: Thomas Renninger <***@suse.de>
CC: ***@acm.org

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
@@ -39,6 +39,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/ipmi.h>
+#include <linux/major.h>
#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/device.h>
@@ -867,14 +868,6 @@ static const struct file_operations ipmi

#define DEVICE_NAME "ipmidev"

-static int ipmi_major;
-module_param(ipmi_major, int, 0);
-MODULE_PARM_DESC(ipmi_major, "Sets the major number of the IPMI device. By"
- " default, or if you set it to zero, it will choose the next"
- " available device. Setting it to -1 will disable the"
- " interface. Other values will set the major device number"
- " to that value.");
-
/* Keep track of the devices that are registered. */
struct ipmi_reg_list {
dev_t dev;
@@ -887,7 +880,7 @@ static struct class *ipmi_class;

static void ipmi_new_smi(int if_num, struct device *device)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;

entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -906,7 +899,7 @@ static void ipmi_new_smi(int if_num, str

static void ipmi_smi_gone(int if_num)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;

mutex_lock(&reg_list_mutex);
@@ -932,9 +925,6 @@ int __init init_ipmi_devintf(void)
{
int rv;

- if (ipmi_major < 0)
- return -EINVAL;
-
printk(KERN_INFO "ipmi device interface\n");

ipmi_class = class_create(THIS_MODULE, "ipmi");
@@ -943,20 +933,16 @@ int __init init_ipmi_devintf(void)
return PTR_ERR(ipmi_class);
}

- rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops);
+ rv = register_chrdev(IPMI_MAJOR, DEVICE_NAME, &ipmi_fops);
if (rv < 0) {
class_destroy(ipmi_class);
- printk(KERN_ERR "ipmi: can't get major %d\n", ipmi_major);
+ printk(KERN_ERR "ipmi: can't get major %d\n", IPMI_MAJOR);
return rv;
}

- if (ipmi_major == 0) {
- ipmi_major = rv;
- }
-
rv = ipmi_smi_watcher_register(&smi_watcher);
if (rv) {
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
class_destroy(ipmi_class);
printk(KERN_WARNING "ipmi: can't register smi watcher\n");
return rv;
@@ -977,5 +963,5 @@ void cleanup_ipmi_dev(void)
mutex_unlock(&reg_list_mutex);
class_destroy(ipmi_class);
ipmi_smi_watcher_unregister(&smi_watcher);
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
}
Index: kernel_ipmi/include/uapi/linux/major.h
===================================================================
--- kernel_ipmi.orig/include/uapi/linux/major.h
+++ kernel_ipmi/include/uapi/linux/major.h
@@ -173,6 +173,8 @@

#define VIOTAPE_MAJOR 230

+#define IPMI_MAJOR 248
+
#define BLOCK_EXT_MAJOR 259
#define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device */
Corey Minyard
2014-10-15 01:26:26 UTC
Permalink
Sorry to top post on this, but you attached the file, so it's hard to reply
inline.

There's no way this can go in. There's not enough major device numbers
for all the devices that exist, we have mechanisms to handle dynamically
assigning numbers, and the IPMI driver just isn't important enough to
warrant it's own major device number.

The particular mechanism to pass in the major number was just a temporary
measure. It is no longer really necessary and could be removed.

-corey

On 10/14/2014 09:40 AM, ***@suse.de wrote:

There should be no need to specify the major number of /dev/ipmiX via module
parameter. Others do not need this as well.
This is the way msr.c (/dev/cpu/X/msr) is doing it as well.

Side effect of this patch will be that the userspace interface cannot
be disabled at kernel level anymore. But this can also be enforced by not
letting userspace create the device file /dev/ipmiX.

Signed-off-by: Thomas Renninger <***@suse.de>
CC: ***@acm.org

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
@@ -39,6 +39,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/ipmi.h>
+#include <linux/major.h>
#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/device.h>
@@ -867,14 +868,6 @@ static const struct file_operations ipmi

#define DEVICE_NAME "ipmidev"

-static int ipmi_major;
-module_param(ipmi_major, int, 0);
-MODULE_PARM_DESC(ipmi_major, "Sets the major number of the IPMI device. By"
- " default, or if you set it to zero, it will choose the next"
- " available device. Setting it to -1 will disable the"
- " interface. Other values will set the major device number"
- " to that value.");
-
/* Keep track of the devices that are registered. */
struct ipmi_reg_list {
dev_t dev;
@@ -887,7 +880,7 @@ static struct class *ipmi_class;

static void ipmi_new_smi(int if_num, struct device *device)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;

entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -906,7 +899,7 @@ static void ipmi_new_smi(int if_num, str

static void ipmi_smi_gone(int if_num)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;

mutex_lock(&reg_list_mutex);
@@ -932,9 +925,6 @@ int __init init_ipmi_devintf(void)
{
int rv;

- if (ipmi_major < 0)
- return -EINVAL;
-
printk(KERN_INFO "ipmi device interface\n");

ipmi_class = class_create(THIS_MODULE, "ipmi");
@@ -943,20 +933,16 @@ int __init init_ipmi_devintf(void)
return PTR_ERR(ipmi_class);
}

- rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops);
+ rv = register_chrdev(IPMI_MAJOR, DEVICE_NAME, &ipmi_fops);
if (rv < 0) {
class_destroy(ipmi_class);
- printk(KERN_ERR "ipmi: can't get major %d\n", ipmi_major);
+ printk(KERN_ERR "ipmi: can't get major %d\n", IPMI_MAJOR);
return rv;
}

- if (ipmi_major == 0) {
- ipmi_major = rv;
- }
-
rv = ipmi_smi_watcher_register(&smi_watcher);
if (rv) {
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
class_destroy(ipmi_class);
printk(KERN_WARNING "ipmi: can't register smi watcher\n");
return rv;
@@ -977,5 +963,5 @@ void cleanup_ipmi_dev(void)
mutex_unlock(&reg_list_mutex);
class_destroy(ipmi_class);
ipmi_smi_watcher_unregister(&smi_watcher);
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
}
Index: kernel_ipmi/include/uapi/linux/major.h
===================================================================
--- kernel_ipmi.orig/include/uapi/linux/major.h
+++ kernel_ipmi/include/uapi/linux/major.h
@@ -173,6 +173,8 @@

#define VIOTAPE_MAJOR 230

+#define IPMI_MAJOR 248
+
#define BLOCK_EXT_MAJOR 259
#define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device */
Thomas Renninger
2014-10-17 07:55:19 UTC
Permalink
Hi,
Post by Corey Minyard
Sorry to top post on this, but you attached the file, so it's hard to reply
inline.
There's no way this can go in. There's not enough major device numbers
for all the devices that exist, we have mechanisms to handle dynamically
assigning numbers, and the IPMI driver just isn't important enough to
warrant it's own major device number.
Hm, I always get 248, on 2 different machines. Just luck?
If 248 is still free (and according to major.h it is), why not?
Post by Corey Minyard
The particular mechanism to pass in the major number was just a temporary
measure. It is no longer really necessary and could be removed.
Ok, if I read above right, you mean:
The module param to specify the major number can vanish.
But I must not try to invent a new MAJOR_IPMI definition in major.h.
Instead the whole thing can be dynamic (use MKDEV, remember the return
value and use it later to freeing again).

Thanks,

Thomas
Post by Corey Minyard
-corey
There should be no need to specify the major number of /dev/ipmiX via module
parameter. Others do not need this as well.
This is the way msr.c (/dev/cpu/X/msr) is doing it as well.
Side effect of this patch will be that the userspace interface cannot
be disabled at kernel level anymore. But this can also be enforced by not
letting userspace create the device file /dev/ipmiX.
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
@@ -39,6 +39,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/ipmi.h>
+#include <linux/major.h>
#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/device.h>
@@ -867,14 +868,6 @@ static const struct file_operations ipmi
#define DEVICE_NAME "ipmidev"
-static int ipmi_major;
-module_param(ipmi_major, int, 0);
-MODULE_PARM_DESC(ipmi_major, "Sets the major number of the IPMI device.
By" - " default, or if you set it to zero, it will choose the next"
- " available device. Setting it to -1 will disable the"
- " interface. Other values will set the major device number"
- " to that value.");
-
/* Keep track of the devices that are registered. */
struct ipmi_reg_list {
dev_t dev;
@@ -887,7 +880,7 @@ static struct class *ipmi_class;
static void ipmi_new_smi(int if_num, struct device *device)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -906,7 +899,7 @@ static void ipmi_new_smi(int if_num, str
static void ipmi_smi_gone(int if_num)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;
mutex_lock(&reg_list_mutex);
@@ -932,9 +925,6 @@ int __init init_ipmi_devintf(void)
{
int rv;
- if (ipmi_major < 0)
- return -EINVAL;
-
printk(KERN_INFO "ipmi device interface\n");
ipmi_class = class_create(THIS_MODULE, "ipmi");
@@ -943,20 +933,16 @@ int __init init_ipmi_devintf(void)
return PTR_ERR(ipmi_class);
}
- rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops);
+ rv = register_chrdev(IPMI_MAJOR, DEVICE_NAME, &ipmi_fops);
if (rv < 0) {
class_destroy(ipmi_class);
- printk(KERN_ERR "ipmi: can't get major %d\n", ipmi_major);
+ printk(KERN_ERR "ipmi: can't get major %d\n", IPMI_MAJOR);
return rv;
}
- if (ipmi_major == 0) {
- ipmi_major = rv;
- }
-
rv = ipmi_smi_watcher_register(&smi_watcher);
if (rv) {
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
class_destroy(ipmi_class);
printk(KERN_WARNING "ipmi: can't register smi watcher\n");
return rv;
@@ -977,5 +963,5 @@ void cleanup_ipmi_dev(void)
mutex_unlock(&reg_list_mutex);
class_destroy(ipmi_class);
ipmi_smi_watcher_unregister(&smi_watcher);
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
}
Index: kernel_ipmi/include/uapi/linux/major.h
===================================================================
--- kernel_ipmi.orig/include/uapi/linux/major.h
+++ kernel_ipmi/include/uapi/linux/major.h
@@ -173,6 +173,8 @@
#define VIOTAPE_MAJOR 230
+#define IPMI_MAJOR 248
+
#define BLOCK_EXT_MAJOR 259
#define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device
*/
Corey Minyard
2014-10-17 16:21:20 UTC
Permalink
Post by Thomas Renninger
Hi,
Post by Corey Minyard
Sorry to top post on this, but you attached the file, so it's hard to reply
inline.
There's no way this can go in. There's not enough major device numbers
for all the devices that exist, we have mechanisms to handle dynamically
assigning numbers, and the IPMI driver just isn't important enough to
warrant it's own major device number.
Hm, I always get 248, on 2 different machines. Just luck?
If 248 is still free (and according to major.h it is), why not?
That's only because you are loading the same modules and have similar
hardware, I'm sure. Just luck.

But primarily, that's not the way it's done. My understanding is that
nobody
gets major numbers any more. There's just not enough room.
Post by Thomas Renninger
Post by Corey Minyard
The particular mechanism to pass in the major number was just a temporary
measure. It is no longer really necessary and could be removed.
The module param to specify the major number can vanish.
But I must not try to invent a new MAJOR_IPMI definition in major.h.
Instead the whole thing can be dynamic (use MKDEV, remember the return
value and use it later to freeing again).
Well, IPMI is registered into the sysfs framework and gets a dynamically
allocated major number, so udev takes care of creating the device node.
So there should be no need to do anything on a properly configured system.
No mkdev, no remembering.

-corey
Post by Thomas Renninger
Thanks,
Thomas
Post by Corey Minyard
-corey
There should be no need to specify the major number of /dev/ipmiX via module
parameter. Others do not need this as well.
This is the way msr.c (/dev/cpu/X/msr) is doing it as well.
Side effect of this patch will be that the userspace interface cannot
be disabled at kernel level anymore. But this can also be enforced by not
letting userspace create the device file /dev/ipmiX.
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
@@ -39,6 +39,7 @@
#include <linux/spinlock.h>
#include <linux/slab.h>
#include <linux/ipmi.h>
+#include <linux/major.h>
#include <linux/mutex.h>
#include <linux/init.h>
#include <linux/device.h>
@@ -867,14 +868,6 @@ static const struct file_operations ipmi
#define DEVICE_NAME "ipmidev"
-static int ipmi_major;
-module_param(ipmi_major, int, 0);
-MODULE_PARM_DESC(ipmi_major, "Sets the major number of the IPMI device.
By" - " default, or if you set it to zero, it will choose the next"
- " available device. Setting it to -1 will disable the"
- " interface. Other values will set the major device number"
- " to that value.");
-
/* Keep track of the devices that are registered. */
struct ipmi_reg_list {
dev_t dev;
@@ -887,7 +880,7 @@ static struct class *ipmi_class;
static void ipmi_new_smi(int if_num, struct device *device)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;
entry = kmalloc(sizeof(*entry), GFP_KERNEL);
@@ -906,7 +899,7 @@ static void ipmi_new_smi(int if_num, str
static void ipmi_smi_gone(int if_num)
{
- dev_t dev = MKDEV(ipmi_major, if_num);
+ dev_t dev = MKDEV(IPMI_MAJOR, if_num);
struct ipmi_reg_list *entry;
mutex_lock(&reg_list_mutex);
@@ -932,9 +925,6 @@ int __init init_ipmi_devintf(void)
{
int rv;
- if (ipmi_major < 0)
- return -EINVAL;
-
printk(KERN_INFO "ipmi device interface\n");
ipmi_class = class_create(THIS_MODULE, "ipmi");
@@ -943,20 +933,16 @@ int __init init_ipmi_devintf(void)
return PTR_ERR(ipmi_class);
}
- rv = register_chrdev(ipmi_major, DEVICE_NAME, &ipmi_fops);
+ rv = register_chrdev(IPMI_MAJOR, DEVICE_NAME, &ipmi_fops);
if (rv < 0) {
class_destroy(ipmi_class);
- printk(KERN_ERR "ipmi: can't get major %d\n", ipmi_major);
+ printk(KERN_ERR "ipmi: can't get major %d\n", IPMI_MAJOR);
return rv;
}
- if (ipmi_major == 0) {
- ipmi_major = rv;
- }
-
rv = ipmi_smi_watcher_register(&smi_watcher);
if (rv) {
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
class_destroy(ipmi_class);
printk(KERN_WARNING "ipmi: can't register smi watcher\n");
return rv;
@@ -977,5 +963,5 @@ void cleanup_ipmi_dev(void)
mutex_unlock(&reg_list_mutex);
class_destroy(ipmi_class);
ipmi_smi_watcher_unregister(&smi_watcher);
- unregister_chrdev(ipmi_major, DEVICE_NAME);
+ unregister_chrdev(IPMI_MAJOR, DEVICE_NAME);
}
Index: kernel_ipmi/include/uapi/linux/major.h
===================================================================
--- kernel_ipmi.orig/include/uapi/linux/major.h
+++ kernel_ipmi/include/uapi/linux/major.h
@@ -173,6 +173,8 @@
#define VIOTAPE_MAJOR 230
+#define IPMI_MAJOR 248
+
#define BLOCK_EXT_MAJOR 259
#define SCSI_OSD_MAJOR 260 /* open-osd's OSD scsi device
*/
t***@suse.de
2014-10-14 14:40:23 UTC
Permalink
Signed-off-by: Thomas Renninger <***@suse.de>
CC: ***@acm.org

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;
}
Corey Minyard
2014-10-15 01:27:34 UTC
Permalink
Queued, thanks for spotting this.

-corey

On 10/14/2014 09:40 AM, ***@suse.de wrote:

Signed-off-by: Thomas Renninger <***@suse.de>
CC: ***@acm.org

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;
}
Loading...