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

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

[Corey Minyard]

On 10/14/2014 09:40 AM, trenn@suse.de 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@suse.de>
> CC: minyard@acm.org
> CC: martin.wilck@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 <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);
>  
>

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

[Thomas Renninger]

Hi,

On Tuesday, October 14, 2014 08:22:20 PM Corey Minyard wrote:
> On 10/14/2014 09:40 AM, trenn@suse.de 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.
Ok.

> 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?

> 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.

> 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.


> > 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.

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

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

[Corey Minyard]

On 10/17/2014 02:49 AM, Thomas Renninger wrote:
> Hi,
>
> On Tuesday, October 14, 2014 08:22:20 PM Corey Minyard wrote:
>> On 10/14/2014 09:40 AM, trenn@suse.de 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.
> Ok.
>
>> 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?
>
>> 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.

>> 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.
>
>
>>> 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.
> 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.

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

> Thanks,
>
>      Thomas

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

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 753 bytes --]

On Fri, 2014-10-17 at 18:14 +0200, Corey Minyard wrote:

> 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.

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


ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Thomas Renninger]

On Monday, October 20, 2014 10:28:53 AM Wilck, Martin wrote:
> On Fri, 2014-10-17 at 18:14 +0200, Corey Minyard wrote:
> 
> 
> > 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.

I do not like this approach for 2 reasons:
1) Same as Martin mentioned: Not avail on all distros yet (not double checked,
   but I trust you).
2) It's still wrong or say, far away from being as it should be:
   Instead of autoloading via userspace (this is what I expect happens here),
   ipmi_devintf should directly be compiled into ipmi_msghandler module
   and be available as soon as ipmi_msghandler is available.
   This saves the overhead of an unneeded additional module
   (memory and loading time overhead -> going through userspace, etc.).

> This is nice, but not commonly available in distro kernels so far.
Sorry, it's not that nice...
> AFAICS, out of the distros Fujitsu supports, only RHEL7 supports it.
Bad.
 
> I vote for MODULE_SOFTDEP for upstream, and modalias for distros that
> don't support MODULE_SOFTDEP yet.
I vote for ipmi_msghandler.c renaming into ipmi_handler.c
I looked around a bit. Renaming is not bad and happens often.
Documentation/laptops/hpfall.c
got renamed into:
Documentation/laptops/freefall.c
history is preserved by using:
git log --follow Documentation/laptops/freefall.c
git blame Documentation/laptops/freefall.c
works as well, showing modifications before the renaming.

I'll send a patchset.

Thanks,

   Thomas

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

[Wilck, Martin]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 546 bytes --]

Thomas,

> > I vote for MODULE_SOFTDEP for upstream, and modalias for distros that
> > don't support MODULE_SOFTDEP yet.
> I vote for ipmi_msghandler.c renaming into ipmi_handler.c

sorry, I am not with you here. This is a small problem that should be
handled with small effort. Why apply a 3-part patch set for a problem
that can be solved with 2 modalias lines in user space?

Martin
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

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

[Thomas Renninger]

On Friday, October 24, 2014 12:40:56 PM Wilck, Martin wrote:
> Thomas,
> 
> 
> > > I vote for MODULE_SOFTDEP for upstream, and modalias for distros that
> > > don't support MODULE_SOFTDEP yet.
> > 
> > I vote for ipmi_msghandler.c renaming into ipmi_handler.c
> 
> 
> sorry, I am not with you here. This is a small problem that should be
> handled with small effort. Why apply a 3-part patch set for a problem
> that can be solved with 2 modalias lines in user space?

Because it is wrong.
In fact this is about a 10 line change.
The file move is no code change at all.
With a modalias added in userspace this will show up in different
distributions again and again.
I agree that a modalias makes sense for SLE and this is probably
what we will go for.
But it would be nice to have this fixed in mainline properly.

Thanks,

    Thomas

Re: [patch 2/3] ipmi: Remove ipmi_major module parameter and define global IPMI_MAJOR

[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.

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, trenn@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 <trenn@suse.de>
CC: minyard@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 */
 

Re: [patch 2/3] ipmi: Remove ipmi_major module parameter and define global IPMI_MAJOR

[Thomas Renninger]

Hi,

On Tuesday, October 14, 2014 08:26:26 PM Corey Minyard wrote:
> 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?

> 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

 
> -corey
> 
> On 10/14/2014 09:40 AM, trenn@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 <trenn@suse.de>
> CC: minyard@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 
*/

Re: [patch 2/3] ipmi: Remove ipmi_major module parameter and define global IPMI_MAJOR

[Corey Minyard]

On 10/17/2014 02:55 AM, Thomas Renninger wrote:
> Hi,
>
> On Tuesday, October 14, 2014 08:26:26 PM Corey Minyard wrote:
>> 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.

>
>> 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).

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


>
> Thanks,
>
>      Thomas
>
>  
>> -corey
>>
>> On 10/14/2014 09:40 AM, trenn@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 <trenn@suse.de>
>> CC: minyard@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 
> */
>

Re: [patch 3/3] ipmi: Unregister previously registered driver in error case

[Corey Minyard]

Queued, thanks for spotting this.

-corey

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

Signed-off-by: Thomas Renninger <trenn@suse.de>
CC: minyard@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;
 	}