On 10/14/2014 09:40 AM, [email protected] 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 <[email protected]>
> CC: [email protected]
> CC: [email protected]
>
> 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(®_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 <[email protected]>");
> -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);
>
>
Hi,
On Tuesday, October 14, 2014 08:22:20 PM Corey Minyard wrote:
> On 10/14/2014 09:40 AM, [email protected] 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
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, [email protected] 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
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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
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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
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