openipmi-developer Mailing List for Open IPMI (Page 162)
Brought to you by:
cminyard
You can subscribe to this list here.
2002 |
Jan
|
Feb
|
Mar
|
Apr
|
May
|
Jun
|
Jul
|
Aug
|
Sep
|
Oct
|
Nov
(8) |
Dec
|
---|---|---|---|---|---|---|---|---|---|---|---|---|
2003 |
Jan
(4) |
Feb
(14) |
Mar
(40) |
Apr
(41) |
May
(17) |
Jun
(50) |
Jul
(16) |
Aug
(37) |
Sep
(57) |
Oct
(44) |
Nov
(48) |
Dec
(35) |
2004 |
Jan
(12) |
Feb
(3) |
Mar
(8) |
Apr
(8) |
May
(22) |
Jun
(23) |
Jul
(14) |
Aug
(51) |
Sep
(21) |
Oct
(38) |
Nov
(8) |
Dec
(17) |
2005 |
Jan
(27) |
Feb
(28) |
Mar
(50) |
Apr
(32) |
May
(55) |
Jun
(38) |
Jul
(26) |
Aug
(40) |
Sep
(67) |
Oct
(86) |
Nov
(25) |
Dec
(29) |
2006 |
Jan
(53) |
Feb
(19) |
Mar
(36) |
Apr
(25) |
May
(27) |
Jun
(56) |
Jul
(28) |
Aug
(15) |
Sep
(37) |
Oct
(63) |
Nov
(63) |
Dec
(105) |
2007 |
Jan
(54) |
Feb
(29) |
Mar
(23) |
Apr
(42) |
May
(6) |
Jun
(70) |
Jul
(51) |
Aug
(58) |
Sep
(27) |
Oct
(43) |
Nov
(52) |
Dec
(24) |
2008 |
Jan
(39) |
Feb
(76) |
Mar
(23) |
Apr
(18) |
May
(5) |
Jun
(7) |
Jul
(12) |
Aug
(7) |
Sep
(2) |
Oct
(6) |
Nov
(22) |
Dec
(31) |
2009 |
Jan
(4) |
Feb
(2) |
Mar
(32) |
Apr
(5) |
May
(22) |
Jun
(5) |
Jul
(9) |
Aug
(6) |
Sep
(12) |
Oct
(30) |
Nov
(27) |
Dec
(31) |
2010 |
Jan
(17) |
Feb
(2) |
Mar
(41) |
Apr
(8) |
May
(19) |
Jun
(11) |
Jul
(53) |
Aug
(1) |
Sep
(14) |
Oct
(31) |
Nov
(13) |
Dec
(10) |
2011 |
Jan
(10) |
Feb
(15) |
Mar
(6) |
Apr
(6) |
May
(4) |
Jun
|
Jul
(6) |
Aug
(5) |
Sep
(6) |
Oct
(9) |
Nov
(2) |
Dec
(3) |
2012 |
Jan
|
Feb
(10) |
Mar
(11) |
Apr
(3) |
May
(2) |
Jun
(6) |
Jul
(12) |
Aug
(1) |
Sep
(3) |
Oct
(23) |
Nov
(6) |
Dec
(11) |
2013 |
Jan
(9) |
Feb
(2) |
Mar
(8) |
Apr
(7) |
May
(40) |
Jun
(9) |
Jul
(47) |
Aug
(23) |
Sep
(52) |
Oct
(6) |
Nov
(9) |
Dec
(8) |
2014 |
Jan
(27) |
Feb
(15) |
Mar
(26) |
Apr
(36) |
May
(33) |
Jun
(4) |
Jul
(15) |
Aug
(2) |
Sep
(11) |
Oct
(120) |
Nov
(32) |
Dec
(27) |
2015 |
Jan
(30) |
Feb
(15) |
Mar
(7) |
Apr
(17) |
May
(27) |
Jun
(23) |
Jul
(15) |
Aug
(39) |
Sep
(19) |
Oct
(5) |
Nov
(26) |
Dec
(6) |
2016 |
Jan
(37) |
Feb
(35) |
Mar
(51) |
Apr
(18) |
May
(8) |
Jun
(11) |
Jul
(5) |
Aug
(7) |
Sep
(54) |
Oct
(6) |
Nov
(33) |
Dec
(11) |
2017 |
Jan
(15) |
Feb
(25) |
Mar
(25) |
Apr
(19) |
May
(17) |
Jun
(28) |
Jul
(11) |
Aug
(56) |
Sep
(53) |
Oct
(15) |
Nov
(19) |
Dec
(30) |
2018 |
Jan
(63) |
Feb
(44) |
Mar
(42) |
Apr
(41) |
May
(19) |
Jun
(22) |
Jul
(16) |
Aug
(38) |
Sep
(14) |
Oct
(6) |
Nov
(11) |
Dec
(12) |
2019 |
Jan
(44) |
Feb
(7) |
Mar
(11) |
Apr
(58) |
May
(10) |
Jun
(10) |
Jul
(42) |
Aug
(36) |
Sep
(3) |
Oct
(29) |
Nov
(29) |
Dec
(23) |
2020 |
Jan
(7) |
Feb
(22) |
Mar
(3) |
Apr
(38) |
May
(14) |
Jun
(7) |
Jul
(12) |
Aug
(48) |
Sep
(85) |
Oct
(71) |
Nov
(14) |
Dec
(4) |
2021 |
Jan
(11) |
Feb
(36) |
Mar
(65) |
Apr
(106) |
May
(73) |
Jun
(33) |
Jul
(25) |
Aug
(19) |
Sep
(19) |
Oct
(29) |
Nov
(95) |
Dec
(21) |
2022 |
Jan
(91) |
Feb
(30) |
Mar
(43) |
Apr
(95) |
May
(136) |
Jun
(47) |
Jul
(28) |
Aug
(36) |
Sep
(17) |
Oct
(46) |
Nov
(53) |
Dec
(15) |
2023 |
Jan
|
Feb
(15) |
Mar
(44) |
Apr
(9) |
May
(20) |
Jun
(18) |
Jul
(8) |
Aug
(18) |
Sep
(41) |
Oct
(67) |
Nov
(44) |
Dec
(2) |
2024 |
Jan
(4) |
Feb
(7) |
Mar
(45) |
Apr
(35) |
May
(4) |
Jun
(29) |
Jul
(4) |
Aug
(37) |
Sep
(13) |
Oct
|
Nov
|
Dec
|
From: Corey M. <tcm...@gm...> - 2012-07-04 17:41:24
|
On 07/04/2012 07:17 AM, Andi Kleen wrote: >> Rather than just have a static entry such as 'Linux' I could probably write the version number and more(distro name etc.. ) >> >> Thoughts.. ? > I still think "Linux" means nothing even to the management software. > What should it do with that? > > If you provide some way for a distro to fill in "foobar linux 1.2.3.4" > maybe. But just Linux or even Linux x.y.z would be wrong because the same kernel > version can behave very differently. > > But it would be better to define specific feature flags for > specific needs that actually mean something. I think the conclusion I have come to is that this really belongs as a small program that runs at startup. That's not significantly different than having it done in the driver. I can help you write it, if you like. I think Andi is right here. "Linux" may mean something to your management software. Some other management software may want "Linux x.y.z". Another may want "SuSE Enterprise Linux x.y". It's impossible to be general enough in the kernel. > >> I know there were some concerns with the security aspect, Can you please let me know what kind of security holes we could be looking at ? > I don't think there are any security problems. just forward/backward > compatibility problems, as the ACPI experience shows. I had mentioned possible security issues with having some daemon that identified the system directly over IP. I don't think there are any issues with IPMI, at least not any more than you already have with IPMI, and it's reasonably secure for what it does. -corey |
From: Andi K. <an...@fi...> - 2012-07-04 12:17:47
|
> The intend of this patch was not to really workaround any issues, the aim was for the management software to use this data. .. use it to do something "Linux" specific right? I think Corey summed it up pretty well in his previous comment. Management software associated with BMC usually use this information to let the users know about the OS that is using the BMC. Generally there are 3rd party application software that goes and pretty much fills up the OS env information and the management software uses this. My intend was why don't we get this information loaded by the driver itself. If there are applications that wants to rewrite it, so be it..! > Rather than just have a static entry such as 'Linux' I could probably write the version number and more(distro name etc.. ) > > Thoughts.. ? I still think "Linux" means nothing even to the management software. What should it do with that? If you provide some way for a distro to fill in "foobar linux 1.2.3.4" maybe. But just Linux or even Linux x.y.z would be wrong because the same kernel version can behave very differently. But it would be better to define specific feature flags for specific needs that actually mean something. > I know there were some concerns with the security aspect, Can you please let me know what kind of security holes we could be looking at ? I don't think there are any security problems. just forward/backward compatibility problems, as the ACPI experience shows. -Andi -- ak...@li... -- Speaking for myself only. |
From: <Srinivas_G_Gowda@Dell.com> - 2012-07-04 05:13:13
|
On 06/29/2012 07:57 PM, Corey Minyard wrote: > seems to me that it's better to directly query what is running on the > target to know what is running on it, but perhaps that's a security > problem waiting to happen. And perhaps it's better to have a small > program set this at startup, since this operation will currently fail on > the majority of systems out there. Hi All, The intend of this patch was not to really workaround any issues, the aim was for the management software to use this data. I think Corey summed it up pretty well in his previous comment. Management software associated with BMC usually use this information to let the users know about the OS that is using the BMC. Generally there are 3rd party application software that goes and pretty much fills up the OS env information and the management software uses this. My intend was why don't we get this information loaded by the driver itself. If there are applications that wants to rewrite it, so be it..! Rather than just have a static entry such as 'Linux' I could probably write the version number and more(distro name etc.. ) Thoughts.. ? I know there were some concerns with the security aspect, Can you please let me know what kind of security holes we could be looking at ? Thanks, G |
From: Corey M. <tcm...@gm...> - 2012-06-29 14:27:18
|
On 06/29/2012 07:30 AM, Matthew Garrett wrote: > On Thu, Jun 28, 2012 at 05:01:54PM -0700, Andi Kleen wrote: >> Not sure that's all that useful. I can just see BMC's making the ACPI >> mistake of trying to work around specific issues, by checking for >> Linux. I'm not sure I see that happening, but I suppose you never know. >> >> But since there are so many different Linux that will never work >> because "Linux" does not describe a fixed release or code base. >> >> Probably dangerous. > Agreed. Linux doesn't make interface guarantees to hardware, and where > we've implied that we do it's ended up breaking things. > This is not really about making interface guarantees to hardware. This is more of a management discovery thing, so that system management software talking to the BMC can know what is running on the target. Something where management software can say "Hey, why is Linux running on that box? It's supposed to be BSD." or "That box has booted Linux but hasn't started its maintenance software". According to the spec, the information is supposed to be cleared if the system powers down or resets. It seems to me that it's better to directly query what is running on the target to know what is running on it, but perhaps that's a security problem waiting to happen. And perhaps it's better to have a small program set this at startup, since this operation will currently fail on the majority of systems out there. -corey |
From: Corey M. <tcm...@gm...> - 2012-06-29 03:06:46
|
Srinivas, what is your use case? -corey On 06/28/2012 07:01 PM, Andi Kleen wrote: > <Srinivas_G_Gowda@Dell.com> writes: >> + >> + data[0] = param_select; >> + data[1] = set_selector; >> + data[2] = string_encode; >> + data[3] = str_len; >> + data[4] = 'L'; >> + data[5] = 'i'; >> + data[6] = 'n'; >> + data[7] = 'u'; >> + data[8] = 'x'; > Not sure that's all that useful. I can just see BMC's making the ACPI > mistake of trying to work around specific issues, by checking for > Linux. > > But since there are so many different Linux that will never work > because "Linux" does not describe a fixed release or code base. > > Probably dangerous. > > -Andi > > |
From: Corey M. <tcm...@gm...> - 2012-06-27 03:19:10
|
On 06/26/2012 09:33 PM, dbashaw wrote: > I have a question about using ipmi_watchdog, ipmitool, and another open > source program ipmisensors together > at the same time. > > ipmitool uses the /dev/ipmi0 char interface. So does (can) > ipmi_watchdog. ipmisensors uses ipmi_settime(..) to send > messages via KCS. > > Question: is there any type of file locking going on for /dev/ipmi0 > device when two programs attempt to use the > /dev/ipmi0 interface? The IPMI driver properly multiplexes all this, so it's not an issue. -corey |
From: dbashaw <db...@ri...> - 2012-06-27 02:33:19
|
I have a question about using ipmi_watchdog, ipmitool, and another open source program ipmisensors together at the same time. ipmitool uses the /dev/ipmi0 char interface. So does (can) ipmi_watchdog. ipmisensors uses ipmi_settime(..) to send messages via KCS. Question: is there any type of file locking going on for /dev/ipmi0 device when two programs attempt to use the /dev/ipmi0 interface? Thanks. Dave |
From: Andrew M. <ak...@li...> - 2012-06-27 00:19:58
|
On Wed, 30 May 2012 07:21:17 -0700 Naresh Bhat <nb...@mv...> wrote: > Hi, > > The code changes will fix the IPMI I/O error on NEC FC storage array. When fixing a bug, please fully describe that bug. This helps people to work out which kernel version(s) should be patched and helps others work out whether your patch will fix a problem which they or their customers are seeing. > Signed-off-by: Naresh Bhat <nb...@mv...> > --- linux-2.6.32.59/drivers/char/ipmi/ipmi_kcs_sm.c.orig 2012-05-31 > 06:11:54.753480863 -0700 > +++ linux-2.6.32.59/drivers/char/ipmi/ipmi_kcs_sm.c 2012-05-31 > 06:21:26.780977397 -0700 Your email client is wordwrapping the patches. > @@ -344,6 +344,24 @@ static int get_kcs_result(struct si_sm_d > */ > static enum si_sm_result kcs_event(struct si_sm_data *kcs, long time) > { > +#define READ_STATUS_MAX_LOOP 4 > +#define SI_SM_WAIT_STATE(_state_) \ > + do { \ > + int i; \ > + for (i = 0; (i < READ_STATUS_MAX_LOOP) \ > + && (state != _state_); i++) { \ > + state = GET_STATUS_STATE(read_status(kcs)); \ > + } \ > + } while (0); > + > +#define SI_SM_WAIT_STATE2(_state_, _state2_) \ > + do { \ > + int i; \ > + for (i = 0; (i < READ_STATUS_MAX_LOOP) && \ > + (state != _state_) && (state != _state2_); i++) { \ > + state = GET_STATUS_STATE(read_status(kcs)); \ > + } \ > + } while (0); > unsigned char status; > unsigned char state; > > @@ -370,6 +388,7 @@ static enum si_sm_result kcs_event(struc > return SI_SM_IDLE; > > case KCS_START_OP: > + SI_SM_WAIT_STATE(KCS_IDLE_STATE); This is really ugly :( Please convert the above two macros into regular C functions. Please ensure that those functions are suitably commented. Something like state = si_sm_wait_state(kcs, KCS_IDLE_STATE); note that we don't pass `state' into si_sm_wait_state(). let's remove the "Just about everything looks at the KCS stat" assignment and do the read within those switch cases which actually need it. Here, like this: --- a/drivers/char/ipmi/ipmi_kcs_sm.c~fix-ipmi-i-o-error-on-nec-fc-storage-array +++ a/drivers/char/ipmi/ipmi_kcs_sm.c @@ -337,6 +337,40 @@ static int get_kcs_result(struct si_sm_d return kcs->read_pos; } +#define READ_STATUS_MAX_LOOP 4 + +/* + * comment goes here + */ +static int si_sm_wait_state(struct si_sm_data *kcs, int state) +{ + int i; + int ret; + + for (i = 0; i < READ_STATUS_MAX_LOOP; i++) { + ret = GET_STATUS_STATE(read_status(kcs)); + if (ret == state) + break; + } + return ret; +} + +/* + * comment goes here + */ +static int si_sm_wait_state2(struct si_sm_data *kcs, int state1, int state2) +{ + int i; + int ret; + + for (i = 0; i < READ_STATUS_MAX_LOOP; i++) { + ret = GET_STATUS_STATE(read_status(kcs)); + if (ret == state1 || ret == state2) + break; + } + return ret; +} + /* * This implements the state machine defined in the IPMI manual, see * that for details on how this works. Divide that flowchart into @@ -356,9 +390,6 @@ static enum si_sm_result kcs_event(struc if (!check_ibf(kcs, status, time)) return SI_SM_CALL_WITH_DELAY; - /* Just about everything looks at the KCS state, so grab that, too. */ - state = GET_STATUS_STATE(status); - switch (kcs->state) { case KCS_IDLE: /* If there's and interrupt source, turn it off. */ @@ -370,6 +401,7 @@ static enum si_sm_result kcs_event(struc return SI_SM_IDLE; case KCS_START_OP: + state = si_sm_wait_state(kcs, KCS_IDLE_STATE); if (state != KCS_IDLE_STATE) { start_error_recovery(kcs, "State machine not idle at start"); @@ -382,6 +414,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_WAIT_WRITE_START: + state = si_sm_wait_state(kcs, KCS_WRITE_STATE); if (state != KCS_WRITE_STATE) { start_error_recovery( kcs, @@ -399,6 +432,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_WAIT_WRITE: + state = si_sm_wait_state(kcs, KCS_WRITE_STATE); if (state != KCS_WRITE_STATE) { start_error_recovery(kcs, "Not in write state for write"); @@ -414,6 +448,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_WAIT_WRITE_END: + state = si_sm_wait_state(kcs, KCS_WRITE_STATE); if (state != KCS_WRITE_STATE) { start_error_recovery(kcs, "Not in write state" @@ -426,6 +461,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_WAIT_READ: + state = si_sm_wait_state2(kcs, KCS_READ_STATE, KCS_IDLE_STATE); if ((state != KCS_READ_STATE) && (state != KCS_IDLE_STATE)) { start_error_recovery( kcs, @@ -472,6 +508,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_ERROR2: + state = si_sm_wait_state(kcs, KCS_READ_STATE); if (state != KCS_READ_STATE) { start_error_recovery(kcs, "Not in read state for error2"); @@ -486,6 +523,7 @@ static enum si_sm_result kcs_event(struc break; case KCS_ERROR3: + state = si_sm_wait_state(kcs, KCS_IDLE_STATE); if (state != KCS_IDLE_STATE) { start_error_recovery(kcs, "Not in idle state for error3"); _ |
From: Jan S. <jsa...@re...> - 2012-06-08 16:12:55
|
Add prefix to all binaries in /sbin to the init script, just in case someone would start it in unusual environment (like sudo /etc/init.d/ipmi). Signed-off-by: Jan Safranek <jsa...@re...> --- ipmi.init | 34 +++++++++++++++++----------------- 1 file changed, 17 insertions(+), 17 deletions(-) diff --git a/ipmi.init b/ipmi.init index 2ebcd94..945be6c 100644 --- a/ipmi.init +++ b/ipmi.init @@ -182,7 +182,7 @@ minimum_modules_loaded() load_si() { if [ "${IPMI_SI}" = "yes" ]; then - modprobe ${IPMI_SI_MODULE_NAME} > /dev/null 2>&1 + /sbin/modprobe ${IPMI_SI_MODULE_NAME} > /dev/null 2>&1 modules_loaded ${IPMI_SI_MODULE_NAME} [ ${OnePlusLoaded} -ne 1 ] && RETVAL=$((RETVAL | 1)) fi @@ -191,7 +191,7 @@ load_si() load_smb() { if [ "${IPMI_SMB}" = "yes" ]; then - modprobe ${IPMI_SMB_MODULE_NAME} > /dev/null 2>&1 + /sbin/modprobe ${IPMI_SMB_MODULE_NAME} > /dev/null 2>&1 modules_loaded ${IPMI_SMB_MODULE_NAME} [ ${OnePlusLoaded} -ne 1 ] && RETVAL=$((RETVAL | 1)) fi @@ -216,7 +216,7 @@ start_watchdog_common() return fi load_hw_modules - modprobe ipmi_watchdog ${IPMI_WATCHDOG_OPTIONS} > /dev/null 2>&1 + /sbin/modprobe ipmi_watchdog ${IPMI_WATCHDOG_OPTIONS} > /dev/null 2>&1 modules_loaded ipmi_watchdog [ ${OnePlusUnloaded} -ne 0 ] && RETVAL=$((RETVAL | 2)) && @@ -253,7 +253,7 @@ start_watchdog() stop_watchdog() { echo -n $"Stopping ipmi_watchdog driver: " - modprobe -q -r ipmi_watchdog > /dev/null 2>&1 + /sbin/modprobe -q -r ipmi_watchdog > /dev/null 2>&1 modules_loaded ipmi_watchdog if [ ${OnePlusLoaded} -ne 0 ]; then RETVAL=$((RETVAL | 32)) @@ -268,7 +268,7 @@ stop_watchdog() stop_watchdog_quiet() { - modprobe -q -r ipmi_watchdog > /dev/null 2>&1 + /sbin/modprobe -q -r ipmi_watchdog > /dev/null 2>&1 modules_loaded ipmi_watchdog if [ ${OnePlusLoaded} -ne 0 ]; then RETVAL=$((RETVAL | 32)) @@ -289,7 +289,7 @@ start_powercontrol_common() modinfo ipmi_poweroff 2>/dev/null | grep poweroff_powercycle > /dev/null 2>&1 && \ poweroff_opts="poweroff_powercycle=1" fi - modprobe ipmi_poweroff "${poweroff_opts}" > /dev/null 2>&1 + /sbin/modprobe ipmi_poweroff "${poweroff_opts}" > /dev/null 2>&1 modules_loaded ipmi_poweroff [ ${OnePlusUnloaded} -ne 0 ] && RETVAL=$((RETVAL | 2)) && @@ -319,7 +319,7 @@ start_powercontrol() stop_powercontrol() { echo -n $"Stopping ipmi_poweroff driver: " - modprobe -q -r ipmi_poweroff > /dev/null 2>&1 + /sbin/modprobe -q -r ipmi_poweroff > /dev/null 2>&1 modules_loaded ipmi_poweroff if [ ${OnePlusLoaded} -ne 0 ]; then RETVAL=$((RETVAL | 32)) @@ -331,7 +331,7 @@ stop_powercontrol() stop_powercontrol_quiet() { - modprobe -q -r ipmi_poweroff > /dev/null 2>&1 + /sbin/modprobe -q -r ipmi_poweroff > /dev/null 2>&1 modules_loaded ipmi_poweroff [ ${OnePlusLoaded} -ne 0 ] && RETVAL=$((RETVAL | 32)) } @@ -342,27 +342,27 @@ unload_all_ipmi_modules() stop_watchdog_quiet stop_powercontrol_quiet for m in ${MODULES}; do - modprobe -q -r ${m} > /dev/null 2>&1 + /sbin/modprobe -q -r ${m} > /dev/null 2>&1 done # delete interface node ONLY if ipmi_devintf is unloaded - [ `lsmod | grep -c "ipmi_devintf"` -eq 0 ] && + [ `/sbin/lsmod | grep -c "ipmi_devintf"` -eq 0 ] && rm -f "/dev/ipmi${INTF_NUM}" } unload_ipmi_modules_leave_features() { for m in ${MODULES_INTERFACES}; do - modprobe -q -r ${m} > /dev/null 2>&1 + /sbin/modprobe -q -r ${m} > /dev/null 2>&1 done # delete interface node ONLY if ipmi_devintf is unloaded - [ `lsmod | grep -c "ipmi_devintf"` -eq 0 ] && + [ `/sbin/lsmod | grep -c "ipmi_devintf"` -eq 0 ] && rm -f "/dev/ipmi${INTF_NUM}" - lsmod | egrep -q "ipmi_(poweroff|watchdog)" > /dev/null 2>&1 + /sbin/lsmod | egrep -q "ipmi_(poweroff|watchdog)" > /dev/null 2>&1 if [ "$?" -ne "0" ]; then stop_watchdog_quiet stop_powercontrol_quiet for m in ${MODULES}; do - modprobe -q -r ${m} > /dev/null 2>&1 + /sbin/modprobe -q -r ${m} > /dev/null 2>&1 done fi } @@ -371,14 +371,14 @@ unload_ipmi_modules_leave_features() load_ipmi_modules () { local locdelay - modprobe ipmi_msghandler > /dev/null 2>&1 + /sbin/modprobe ipmi_msghandler > /dev/null 2>&1 modules_loaded ipmi_msghandler [ ${OnePlusLoaded} -ne 1 ] && unload_all_ipmi_modules && RETVAL=$((RETVAL | 1)) && return load_hw_modules [ $((RETVAL & 1)) -eq 1 ] && unload_all_ipmi_modules && RETVAL=$((RETVAL | 1)) && return if [ "${DEV_IPMI}" = "yes" ]; then - modprobe ipmi_devintf > /dev/null 2>&1 + /sbin/modprobe ipmi_devintf > /dev/null 2>&1 modules_loaded ipmi_devintf RETVAL=$((RETVAL & ~2)) [ ${OnePlusLoaded} -eq 0 ] && RETVAL=$((RETVAL | 2)) @@ -400,7 +400,7 @@ load_ipmi_modules () fi if [ "${IPMI_IMB}" = "yes" ]; then - modprobe ipmi_imb > /dev/null 2>&1 + /sbin/modprobe ipmi_imb > /dev/null 2>&1 modules_loaded ipmi_imb RETVAL=$((RETVAL & ~2)) [ ${OnePlusLoaded} -eq 0 ] && RETVAL=$((RETVAL | 2)) |
From: Jan S. <jsa...@re...> - 2012-05-07 08:18:57
|
Hi, I noticed there is OpenIPMI-2.0.19.tar.gz on OpenIPMI's sf.net page, but I haven't seen any announcement. Is the release ready for use? Or is it some preliminary version for testing? Jan |
From: Andy C. <and...@us...> - 2012-05-01 13:08:50
|
Corey, I've seen this also, mostly on Red Hat kernels around the 2.6.32 vintage. It would be nice to leverage the fix you mention below. Can you point to it? Andy -----Original Message----- From: Corey Minyard [mailto:tcm...@gm...] Sent: Friday, April 06, 2012 4:53 PM To: dbashaw Cc: OpenIPMI Subject: Re: [Openipmi-developer] Multiple users sending messages via KCS interface You don't mention the kernel version or anything of that nature. There was a bug fixed a while back for something that looked like this. -corey On 04/05/2012 09:15 PM, dbashaw wrote: > I have two IPMI users created using ipmi_create_user(..). > Each user can send messages async with respect to each other. > I have also seen IPMI message handler: BMC returned incorrect response > errors some times. > > When I allow only one user to use the KCS interface this does not seem > to happen. > After reviewing ipmi_msghandler.c I am not able to figure out how a > response to a message > from a given user can be identified with that user and not some other. > > ipmi_msghandler() sends the message to the interface handler (KCS > state machine in this case) and is done, > free to handle the next user request since responses arrive > asynchronously at a later time. > > From the IPMI spec I see KCS message request format described as: > BYTE 1 BYTE 2 BYTE 3:N > NetFn/Lun Cmd Data > > KCS message response format: > BYTE 1 BYTE 2 BYTE 3 BYTE 4:N > NetFn/Lun Cmd Ccode Data > > Data is that which is required by the specific command being sent only. > > Request and response messages both have no user identifiable information > that I can find in the V1.5 or V2.0 spec. > > I'm beginning to think that this is the reason for the "incorrect > response" error above when two users are > sending requests. > > handle_new_recv_msg((ipmi_smi_t intf, > struct ipmi_smi_msg *msg) > } else if (((msg->rsp[0]>> 2) != ((msg->data[0]>> 2) | 1)) > || (msg->rsp[1] != msg->data[1])) { > /* > * The NetFN and Command in the response is not even > * marginally correct. > */ > printk(KERN_WARNING PFX "BMC returned incorrect response," > " expected netfn %x cmd %x, got netfn %x cmd %x\n", > (msg->data[0]>> 2) | 1, msg->data[1], > msg->rsp[0]>> 2, msg->rsp[1]); > > No information in msg->rsp or msg->data can be used to make sure this > response is associated with > the correct user. Higher level things like sequence numbers are > contained in the msg struct at some level > but don't address the issue of associating the response to a unique user. > > How to fix this? > > Dave > > > > > > ------------------------------------------------------------------------ ------ > For Developers, A Lot Can Happen In A Second. > Boundary is the first to Know...and Tell You. > Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! > http://p.sf.net/sfu/Boundary-d2dvs2 > _______________________________________________ > Openipmi-developer mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openipmi-developer ------------------------------------------------------------------------ ------ For Developers, A Lot Can Happen In A Second. Boundary is the first to Know...and Tell You. Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! http://p.sf.net/sfu/Boundary-d2dvs2 _______________________________________________ Openipmi-developer mailing list Ope...@li... https://lists.sourceforge.net/lists/listinfo/openipmi-developer |
From: Corey M. <tcm...@gm...> - 2012-04-06 20:52:59
|
You don't mention the kernel version or anything of that nature. There was a bug fixed a while back for something that looked like this. -corey On 04/05/2012 09:15 PM, dbashaw wrote: > I have two IPMI users created using ipmi_create_user(..). > Each user can send messages async with respect to each other. > I have also seen IPMI message handler: BMC returned incorrect response > errors some times. > > When I allow only one user to use the KCS interface this does not seem > to happen. > After reviewing ipmi_msghandler.c I am not able to figure out how a > response to a message > from a given user can be identified with that user and not some other. > > ipmi_msghandler() sends the message to the interface handler (KCS > state machine in this case) and is done, > free to handle the next user request since responses arrive > asynchronously at a later time. > > From the IPMI spec I see KCS message request format described as: > BYTE 1 BYTE 2 BYTE 3:N > NetFn/Lun Cmd Data > > KCS message response format: > BYTE 1 BYTE 2 BYTE 3 BYTE 4:N > NetFn/Lun Cmd Ccode Data > > Data is that which is required by the specific command being sent only. > > Request and response messages both have no user identifiable information > that I can find in the V1.5 or V2.0 spec. > > I'm beginning to think that this is the reason for the "incorrect > response" error above when two users are > sending requests. > > handle_new_recv_msg((ipmi_smi_t intf, > struct ipmi_smi_msg *msg) > } else if (((msg->rsp[0]>> 2) != ((msg->data[0]>> 2) | 1)) > || (msg->rsp[1] != msg->data[1])) { > /* > * The NetFN and Command in the response is not even > * marginally correct. > */ > printk(KERN_WARNING PFX "BMC returned incorrect response," > " expected netfn %x cmd %x, got netfn %x cmd %x\n", > (msg->data[0]>> 2) | 1, msg->data[1], > msg->rsp[0]>> 2, msg->rsp[1]); > > No information in msg->rsp or msg->data can be used to make sure this > response is associated with > the correct user. Higher level things like sequence numbers are > contained in the msg struct at some level > but don't address the issue of associating the response to a unique user. > > How to fix this? > > Dave > > > > > > ------------------------------------------------------------------------------ > For Developers, A Lot Can Happen In A Second. > Boundary is the first to Know...and Tell You. > Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! > http://p.sf.net/sfu/Boundary-d2dvs2 > _______________________________________________ > Openipmi-developer mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openipmi-developer |
From: sudhakar <sud...@fb...> - 2012-04-06 03:33:57
|
I am not an expert but won't the sender in ipmi_si_intf.c make sure only one message is active at one time? if (smi_info->si_state == SI_NORMAL && smi_info->curr_msg == NULL) start_next_msg(smi_info); Sudhakar On 04/05/2012 07:15 PM, dbashaw wrote: > I have two IPMI users created using ipmi_create_user(..). > Each user can send messages async with respect to each other. > I have also seen IPMI message handler: BMC returned incorrect response > errors some times. > > When I allow only one user to use the KCS interface this does not seem > to happen. > After reviewing ipmi_msghandler.c I am not able to figure out how a > response to a message > from a given user can be identified with that user and not some other. > > ipmi_msghandler() sends the message to the interface handler (KCS > state machine in this case) and is done, > free to handle the next user request since responses arrive > asynchronously at a later time. > > From the IPMI spec I see KCS message request format described as: > BYTE 1 BYTE 2 BYTE 3:N > NetFn/Lun Cmd Data > > KCS message response format: > BYTE 1 BYTE 2 BYTE 3 BYTE 4:N > NetFn/Lun Cmd Ccode Data > > Data is that which is required by the specific command being sent only. > > Request and response messages both have no user identifiable information > that I can find in the V1.5 or V2.0 spec. > > I'm beginning to think that this is the reason for the "incorrect > response" error above when two users are > sending requests. > > handle_new_recv_msg((ipmi_smi_t intf, > struct ipmi_smi_msg *msg) > } else if (((msg->rsp[0] >> 2) != ((msg->data[0] >> 2) | 1)) > || (msg->rsp[1] != msg->data[1])) { > /* > * The NetFN and Command in the response is not even > * marginally correct. > */ > printk(KERN_WARNING PFX "BMC returned incorrect response," > " expected netfn %x cmd %x, got netfn %x cmd %x\n", > (msg->data[0] >> 2) | 1, msg->data[1], > msg->rsp[0] >> 2, msg->rsp[1]); > > No information in msg->rsp or msg->data can be used to make sure this > response is associated with > the correct user. Higher level things like sequence numbers are > contained in the msg struct at some level > but don't address the issue of associating the response to a unique user. > > How to fix this? > > Dave > > > > > > ------------------------------------------------------------------------------ > For Developers, A Lot Can Happen In A Second. > Boundary is the first to Know...and Tell You. > Monitor Your Applications in Ultra-Fine Resolution. Try it FREE! > http://p.sf.net/sfu/Boundary-d2dvs2 > _______________________________________________ > Openipmi-developer mailing list > Ope...@li... > https://lists.sourceforge.net/lists/listinfo/openipmi-developer |
From: dbashaw <db...@ri...> - 2012-04-06 02:50:18
|
I have two IPMI users created using ipmi_create_user(..). Each user can send messages async with respect to each other. I have also seen IPMI message handler: BMC returned incorrect response errors some times. When I allow only one user to use the KCS interface this does not seem to happen. After reviewing ipmi_msghandler.c I am not able to figure out how a response to a message from a given user can be identified with that user and not some other. ipmi_msghandler() sends the message to the interface handler (KCS state machine in this case) and is done, free to handle the next user request since responses arrive asynchronously at a later time. From the IPMI spec I see KCS message request format described as: BYTE 1 BYTE 2 BYTE 3:N NetFn/Lun Cmd Data KCS message response format: BYTE 1 BYTE 2 BYTE 3 BYTE 4:N NetFn/Lun Cmd Ccode Data Data is that which is required by the specific command being sent only. Request and response messages both have no user identifiable information that I can find in the V1.5 or V2.0 spec. I'm beginning to think that this is the reason for the "incorrect response" error above when two users are sending requests. handle_new_recv_msg((ipmi_smi_t intf, struct ipmi_smi_msg *msg) } else if (((msg->rsp[0] >> 2) != ((msg->data[0] >> 2) | 1)) || (msg->rsp[1] != msg->data[1])) { /* * The NetFN and Command in the response is not even * marginally correct. */ printk(KERN_WARNING PFX "BMC returned incorrect response," " expected netfn %x cmd %x, got netfn %x cmd %x\n", (msg->data[0] >> 2) | 1, msg->data[1], msg->rsp[0] >> 2, msg->rsp[1]); No information in msg->rsp or msg->data can be used to make sure this response is associated with the correct user. Higher level things like sequence numbers are contained in the msg struct at some level but don't address the issue of associating the response to a unique user. How to fix this? Dave |
From: Li Z. <zh...@li...> - 2012-03-09 09:53:54
|
On Tue, 2012-03-06 at 11:27 +0100, Vegard Nossum wrote: > On 6 March 2012 11:09, Li Zhong <zh...@li...> wrote: > > This patch tries to fix the problem of page fault exception caused by > > accessing nmiaction structure in nmi if kmemcheck is enabled. > > > > If kmemcheck is enabled, the memory allocated through slab are in pages > > that are marked non-present, so that some checks could be done in the > > page fault handling code ( e.g. whether the memory is read before > > written to ). > > As nmiaction is allocated in this way, so it resides in a non-present > > page. Then there is a page fault while the nmi code accessing the > > nmiaction structure, which would then cause a warning by > > WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). > > > > v2: as Peter suggested, changed the nmiaction to use static storage. > > > > v3: as Peter suggested, use macro to shorten the codes. Also keep the > > original usage of register_nmi_handler, so users of this call doesn't > > need change. > > > > Signed-off-by: Li Zhong <zh...@li...> > > Looks like you've solved this now. Thanks. There is still one place [2/2] not solved ... and I guess it might need the way you suggested below. > > For the record, another way to prevent the page fault from happening > in the first place is to set up a new slab cache with the flag > SLAB_NOTRACK. This is different from the GFP_NOTRACK flag which, as > you noted, doesn't prevent page faults, just inhibits > checking/warnings for those memory areas. > > It's a bit of a hassle, I admit. Maybe we could create an additional, > separate set of slab caches (using SLAB_NOTRACK) and a new GFP flag > which selects this set of caches instead. This would allow anything > that takes a gfp_t to allocate memory that is guaranteed not to page > fault when using kmemcheck. Pekka, any thoughts? > I'm not sure whether I understand it correctly? If CONFIG_KMEMCHECK is enabled, create another two sets of malloc_sizes caches, one for cs_cachep, one for cs_dmacachep, both with SLAB_NOTRACK flag. Create a new GFP flag, like __GFP_NO_PF for those places where page fault is not allowed, and return memory from the caches created above. This new GFP flag is set to 0 if CONFIG_KMEMCHECK is not enabled. I think there shouldn't be many such cases, so most of these caches wouldn't actually be used ... Thanks, Zhong > > Vegard > |
From: Don Z. <dz...@re...> - 2012-03-06 15:01:05
|
On Tue, Mar 06, 2012 at 06:09:43PM +0800, Li Zhong wrote: > This patch tries to fix the problem of page fault exception caused by > accessing nmiaction structure in nmi if kmemcheck is enabled. > > If kmemcheck is enabled, the memory allocated through slab are in pages > that are marked non-present, so that some checks could be done in the > page fault handling code ( e.g. whether the memory is read before > written to ). > As nmiaction is allocated in this way, so it resides in a non-present > page. Then there is a page fault while the nmi code accessing the > nmiaction structure, which would then cause a warning by > WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). > > v2: as Peter suggested, changed the nmiaction to use static storage. > > v3: as Peter suggested, use macro to shorten the codes. Also keep the > original usage of register_nmi_handler, so users of this call doesn't > need change. Thanks Li! Looks fine to me. I'll run some tests to make sure things still work correctly. Cheers, Don |
From: Vegard N. <veg...@gm...> - 2012-03-06 10:27:31
|
On 6 March 2012 11:09, Li Zhong <zh...@li...> wrote: > This patch tries to fix the problem of page fault exception caused by > accessing nmiaction structure in nmi if kmemcheck is enabled. > > If kmemcheck is enabled, the memory allocated through slab are in pages > that are marked non-present, so that some checks could be done in the > page fault handling code ( e.g. whether the memory is read before > written to ). > As nmiaction is allocated in this way, so it resides in a non-present > page. Then there is a page fault while the nmi code accessing the > nmiaction structure, which would then cause a warning by > WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). > > v2: as Peter suggested, changed the nmiaction to use static storage. > > v3: as Peter suggested, use macro to shorten the codes. Also keep the > original usage of register_nmi_handler, so users of this call doesn't > need change. > > Signed-off-by: Li Zhong <zh...@li...> Looks like you've solved this now. Thanks. For the record, another way to prevent the page fault from happening in the first place is to set up a new slab cache with the flag SLAB_NOTRACK. This is different from the GFP_NOTRACK flag which, as you noted, doesn't prevent page faults, just inhibits checking/warnings for those memory areas. It's a bit of a hassle, I admit. Maybe we could create an additional, separate set of slab caches (using SLAB_NOTRACK) and a new GFP flag which selects this set of caches instead. This would allow anything that takes a gfp_t to allocate memory that is guaranteed not to page fault when using kmemcheck. Pekka, any thoughts? Vegard |
From: Li Z. <zh...@li...> - 2012-03-06 10:10:11
|
This patch tries to fix the problem of page fault exception caused by accessing nmiaction structure in nmi if kmemcheck is enabled. If kmemcheck is enabled, the memory allocated through slab are in pages that are marked non-present, so that some checks could be done in the page fault handling code ( e.g. whether the memory is read before written to ). As nmiaction is allocated in this way, so it resides in a non-present page. Then there is a page fault while the nmi code accessing the nmiaction structure, which would then cause a warning by WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). v2: as Peter suggested, changed the nmiaction to use static storage. v3: as Peter suggested, use macro to shorten the codes. Also keep the original usage of register_nmi_handler, so users of this call doesn't need change. Signed-off-by: Li Zhong <zh...@li...> --- arch/x86/include/asm/nmi.h | 19 ++++++++++++++- arch/x86/kernel/nmi.c | 52 ++++++-------------------------------------- 2 files changed, 24 insertions(+), 47 deletions(-) diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h index fd3f9f1..5a2b2c6 100644 --- a/arch/x86/include/asm/nmi.h +++ b/arch/x86/include/asm/nmi.h @@ -35,8 +35,23 @@ enum { typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *); -int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long, - const char *); +struct nmiaction { + struct list_head list; + nmi_handler_t handler; + unsigned int flags; + const char *name; +}; + +#define register_nmi_handler(t, fn, fg, n) \ +({ \ + static struct nmiaction fn##_na = { \ + .handler = (fn), \ + .name = (n), \ + }; \ + __register_nmi_handler((t), (fg), &fn##_na); \ +}) + +int __register_nmi_handler(unsigned int, unsigned int, struct nmiaction *); void unregister_nmi_handler(unsigned int, const char *); diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c index 47acaf3..a097559 100644 --- a/arch/x86/kernel/nmi.c +++ b/arch/x86/kernel/nmi.c @@ -31,14 +31,6 @@ #include <asm/nmi.h> #include <asm/x86_init.h> -#define NMI_MAX_NAMELEN 16 -struct nmiaction { - struct list_head list; - nmi_handler_t handler; - unsigned int flags; - char *name; -}; - struct nmi_desc { spinlock_t lock; struct list_head head; @@ -160,51 +152,21 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name) return (n); } -int register_nmi_handler(unsigned int type, nmi_handler_t handler, - unsigned long nmiflags, const char *devname) +int __register_nmi_handler(unsigned int type, unsigned int nmiflags, + struct nmiaction *na) { - struct nmiaction *action; - int retval = -ENOMEM; - - if (!handler) + if (!na->handler) return -EINVAL; - action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL); - if (!action) - goto fail_action; - - action->handler = handler; - action->flags = nmiflags; - action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL); - if (!action->name) - goto fail_action_name; - - retval = __setup_nmi(type, action); - - if (retval) - goto fail_setup_nmi; + na->flags = nmiflags; - return retval; - -fail_setup_nmi: - kfree(action->name); -fail_action_name: - kfree(action); -fail_action: - - return retval; + return __setup_nmi(type, na); } -EXPORT_SYMBOL_GPL(register_nmi_handler); +EXPORT_SYMBOL_GPL(__register_nmi_handler); void unregister_nmi_handler(unsigned int type, const char *name) { - struct nmiaction *a; - - a = __free_nmi(type, name); - if (a) { - kfree(a->name); - kfree(a); - } + __free_nmi(type, name); } EXPORT_SYMBOL_GPL(unregister_nmi_handler); -- 1.7.1 |
From: Li Z. <zh...@li...> - 2012-03-06 01:46:57
|
On Mon, 2012-03-05 at 11:29 +0100, Wim Van Sebroeck wrote: > Hi Li, > > > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > > index 8464ea1..e575e63 100644 > > --- a/drivers/watchdog/hpwdt.c > > +++ b/drivers/watchdog/hpwdt.c > > @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy) > > } > > } > > > > +static struct nmiaction hpwdt_nmiaction[] = { > > + { > > + .handler = hpwdt_pretimeout, > > + .name = "hpwdt", > > + }, > > + { > > + .handler = hpwdt_pretimeout, > > + .flags = NMI_FLAG_FIRST, > > + .name = "hpwdt", > > + }, > > +}; > > + > > static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > > { > > int retval; > > + struct nmiaction *na = hpwdt_nmiaction; > > > > /* > > * On typical CRU-based systems we need to map that service in > > @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > > * die notify list to handle a critical NMI. The default is to > > * be last so other users of the NMI signal can function. > > */ > > - retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, > > - (priority) ? NMI_FLAG_FIRST : 0, > > - "hpwdt"); > > + > > + if (priority) > > + na = &hpwdt_nmiaction[1]; > > + > > + retval = register_nmi_handler(NMI_UNKNOWN, na); > > if (retval != 0) { > > dev_warn(&dev->dev, > > "Unable to register a die notifier (err=%d).\n", > > Why not do something like; > > static struct nmiaction hpwdt_nmiaction = { > .handler = hpwdt_pretimeout, > .name = "hpwdt", > }; > > ... > if (priority) > hpwdt_nmiaction.flags = NMI_FLAG_FIRST; > ... > Thank you, Wim. I'll update it. Thanks, Zhong > Kind regards, > Wim. > |
From: Don Z. <dz...@re...> - 2012-03-05 21:46:28
|
On Mon, Mar 05, 2012 at 06:49:07PM +0100, Peter Zijlstra wrote: > On Mon, 2012-03-05 at 18:05 +0800, Li Zhong wrote: > > +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = { > > + .handler = arch_trigger_all_cpu_backtrace_handler, > > + .name = "arch_bt", > > +}; > > + > > static int __init register_trigger_all_cpu_backtrace(void) > > { > > - register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, > > - 0, "arch_bt"); > > + register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction); > > return 0; > > } > > If you look at things like cpu_notifier() you can shorten this still: > > #define nmi_notifier(t, fn, n) \ > do { \ > static struct nmiaction fn##_na = { \ > .handler = (fn), \ > .name = (n), \ > }; \ > register_nmi_handler((t), &fn##_na); \ > } while (0) > > The whole thing then becomes: > > nmi_notifier(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, "arch_bt"); Hi Li, I would be open to this suggestion. If you want to modify the patch for this approach. Cheers, Don |
From: Peter Z. <a.p...@ch...> - 2012-03-05 18:54:09
|
On Mon, 2012-03-05 at 10:54 -0500, Don Zickus wrote: > This is one way of doing this. I was trying to avoid this when I rewrote the > nmi handlers, because everyone kept screwing up the structs. I thought it > would be safer to have callers pass in data based on an api instead. Apparently kmemcheck marks pages as non-present and does magic in the fault handler. Having the action thing allocated meant kmemcheck also marks that thing as non-present in the page-tables, the list iteration from NMI context would then fault and things would go funny. There's two ways out, help kmemcheck with a new annotation (which of course starts with checking if there isn't already such a thing). Or this one, avoid the action things from being allocated, this side-steps kmemcheck and avoids the problem thusly. Sadly this patch doesn't at all mention the first possibility and why that isn't a feasible approach. A well... |
From: Peter Z. <a.p...@ch...> - 2012-03-05 18:24:36
|
On Mon, 2012-03-05 at 18:05 +0800, Li Zhong wrote: > +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = { > + .handler = arch_trigger_all_cpu_backtrace_handler, > + .name = "arch_bt", > +}; > + > static int __init register_trigger_all_cpu_backtrace(void) > { > - register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, > - 0, "arch_bt"); > + register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction); > return 0; > } If you look at things like cpu_notifier() you can shorten this still: #define nmi_notifier(t, fn, n) \ do { \ static struct nmiaction fn##_na = { \ .handler = (fn), \ .name = (n), \ }; \ register_nmi_handler((t), &fn##_na); \ } while (0) The whole thing then becomes: nmi_notifier(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, "arch_bt"); |
From: Don Z. <dz...@re...> - 2012-03-05 15:55:49
|
On Mon, Mar 05, 2012 at 06:05:17PM +0800, Li Zhong wrote: > This patch tries to fix the problem of page fault exception caused by > accessing nmiaction structure in nmi if kmemcheck is enabled. > > If kmemcheck is enabled, the memory allocated through slab are in pages > that are marked non-present, so that some checks could be done in the > page fault handling code ( e.g. whether the memory is read before > written to ). > As nmiaction is allocated in this way, so it resides in a non-present > page. Then there is a page fault while the nmi code accessing the > nmiaction structure, which would then cause a warning by > WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). This is one way of doing this. I was trying to avoid this when I rewrote the nmi handlers, because everyone kept screwing up the structs. I thought it would be safer to have callers pass in data based on an api instead. So I am not necessarily a big fan of this patch (nor is it entirely correct because you throwaway all the checks in register_nmi_handler()). Then again I am not a memory expert and may be misunderstanding something simple why it is safer to do static storage. Cheers, Don > > v2: as Peter suggested, changed the nmiaction to use static storage. > > Signed-off-by: Li Zhong <zh...@li...> > --- > arch/x86/include/asm/nmi.h | 10 +++++- > arch/x86/kernel/apic/hw_nmi.c | 8 ++++- > arch/x86/kernel/apic/x2apic_uv_x.c | 7 ++++- > arch/x86/kernel/cpu/mcheck/mce-inject.c | 8 ++++- > arch/x86/kernel/cpu/perf_event.c | 7 ++++- > arch/x86/kernel/kgdb.c | 11 ++++-- > arch/x86/kernel/nmi.c | 49 ++---------------------------- > arch/x86/kernel/nmi_selftest.c | 16 ++++++++-- > arch/x86/kernel/reboot.c | 9 ++++- > arch/x86/kernel/smp.c | 9 ++++- > arch/x86/oprofile/nmi_int.c | 8 ++++- > drivers/acpi/apei/ghes.c | 8 ++++- > drivers/char/ipmi/ipmi_watchdog.c | 10 +++++- > drivers/watchdog/hpwdt.c | 21 +++++++++++-- > 14 files changed, 108 insertions(+), 73 deletions(-) > > diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h > index fd3f9f1..08d464f 100644 > --- a/arch/x86/include/asm/nmi.h > +++ b/arch/x86/include/asm/nmi.h > @@ -35,8 +35,14 @@ enum { > > typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *); > > -int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long, > - const char *); > +struct nmiaction { > + struct list_head list; > + nmi_handler_t handler; > + unsigned int flags; > + const char *name; > +}; > + > +int register_nmi_handler(unsigned int, struct nmiaction *); > > void unregister_nmi_handler(unsigned int, const char *); > > diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c > index 31cb9ae..9baea77 100644 > --- a/arch/x86/kernel/apic/hw_nmi.c > +++ b/arch/x86/kernel/apic/hw_nmi.c > @@ -80,10 +80,14 @@ arch_trigger_all_cpu_backtrace_handler(unsigned int cmd, struct pt_regs *regs) > return NMI_DONE; > } > > +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = { > + .handler = arch_trigger_all_cpu_backtrace_handler, > + .name = "arch_bt", > +}; > + > static int __init register_trigger_all_cpu_backtrace(void) > { > - register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, > - 0, "arch_bt"); > + register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction); > return 0; > } > early_initcall(register_trigger_all_cpu_backtrace); > diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c > index 79b05b8..5739042 100644 > --- a/arch/x86/kernel/apic/x2apic_uv_x.c > +++ b/arch/x86/kernel/apic/x2apic_uv_x.c > @@ -715,9 +715,14 @@ int uv_handle_nmi(unsigned int reason, struct pt_regs *regs) > return NMI_HANDLED; > } > > +static struct nmiaction uv_nmiaction = { > + .handler = uv_handle_nmi, > + .name = "uv", > +}; > + > void uv_register_nmi_notifier(void) > { > - if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv")) > + if (register_nmi_handler(NMI_UNKNOWN, &uv_nmiaction)) > printk(KERN_WARNING "UV NMI handler failed to register\n"); > } > > diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c > index fc4beb3..f9ab41c 100644 > --- a/arch/x86/kernel/cpu/mcheck/mce-inject.c > +++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c > @@ -229,14 +229,18 @@ static ssize_t mce_write(struct file *filp, const char __user *ubuf, > return usize; > } > > +static struct nmiaction mce_nmiaction = { > + .handler = mce_raise_notify, > + .name = "mce_notify", > +}; > + > static int inject_init(void) > { > if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL)) > return -ENOMEM; > printk(KERN_INFO "Machine check injector initialized\n"); > register_mce_write_callback(mce_write); > - register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, > - "mce_notify"); > + register_nmi_handler(NMI_LOCAL, &mce_nmiaction); > return 0; > } > > diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c > index 5adce10..8bdff1b 100644 > --- a/arch/x86/kernel/cpu/perf_event.c > +++ b/arch/x86/kernel/cpu/perf_event.c > @@ -1246,6 +1246,11 @@ static void __init pmu_check_apic(void) > pr_info("no hardware sampling interrupt available.\n"); > } > > +static struct nmiaction perf_event_nmiaction = { > + .handler = perf_event_nmi_handler, > + .name = "PMI", > +}; > + > static int __init init_hw_perf_events(void) > { > struct x86_pmu_quirk *quirk; > @@ -1297,7 +1302,7 @@ static int __init init_hw_perf_events(void) > ((1LL << x86_pmu.num_counters_fixed)-1) << X86_PMC_IDX_FIXED; > > perf_events_lapic_init(); > - register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI"); > + register_nmi_handler(NMI_LOCAL, &perf_event_nmiaction); > > unconstrained = (struct event_constraint) > __EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1, > diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c > index faba577..d259d2a 100644 > --- a/arch/x86/kernel/kgdb.c > +++ b/arch/x86/kernel/kgdb.c > @@ -601,6 +601,11 @@ static struct notifier_block kgdb_notifier = { > .notifier_call = kgdb_notify, > }; > > +static struct nmiaction kgdb_nmiaction = { > + .handler = kgdb_nmi_handler, > + .name = "kgdb", > +}; > + > /** > * kgdb_arch_init - Perform any architecture specific initalization. > * > @@ -615,13 +620,11 @@ int kgdb_arch_init(void) > if (retval) > goto out; > > - retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler, > - 0, "kgdb"); > + retval = register_nmi_handler(NMI_LOCAL, &kgdb_nmiaction); > if (retval) > goto out1; > > - retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler, > - 0, "kgdb"); > + retval = register_nmi_handler(NMI_UNKNOWN, &kgdb_nmiaction); > > if (retval) > goto out2; > diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c > index 47acaf3..d844acc 100644 > --- a/arch/x86/kernel/nmi.c > +++ b/arch/x86/kernel/nmi.c > @@ -31,14 +31,6 @@ > #include <asm/nmi.h> > #include <asm/x86_init.h> > > -#define NMI_MAX_NAMELEN 16 > -struct nmiaction { > - struct list_head list; > - nmi_handler_t handler; > - unsigned int flags; > - char *name; > -}; > - > struct nmi_desc { > spinlock_t lock; > struct list_head head; > @@ -160,51 +152,18 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name) > return (n); > } > > -int register_nmi_handler(unsigned int type, nmi_handler_t handler, > - unsigned long nmiflags, const char *devname) > +int register_nmi_handler(unsigned int type, struct nmiaction *na) > { > - struct nmiaction *action; > - int retval = -ENOMEM; > - > - if (!handler) > + if (!na->handler) > return -EINVAL; > > - action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL); > - if (!action) > - goto fail_action; > - > - action->handler = handler; > - action->flags = nmiflags; > - action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL); > - if (!action->name) > - goto fail_action_name; > - > - retval = __setup_nmi(type, action); > - > - if (retval) > - goto fail_setup_nmi; > - > - return retval; > - > -fail_setup_nmi: > - kfree(action->name); > -fail_action_name: > - kfree(action); > -fail_action: > - > - return retval; > + return __setup_nmi(type, na); > } > EXPORT_SYMBOL_GPL(register_nmi_handler); > > void unregister_nmi_handler(unsigned int type, const char *name) > { > - struct nmiaction *a; > - > - a = __free_nmi(type, name); > - if (a) { > - kfree(a->name); > - kfree(a); > - } > + __free_nmi(type, name); > } > > EXPORT_SYMBOL_GPL(unregister_nmi_handler); > diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c > index 0d01a8e..40fd637 100644 > --- a/arch/x86/kernel/nmi_selftest.c > +++ b/arch/x86/kernel/nmi_selftest.c > @@ -37,10 +37,15 @@ static int nmi_unk_cb(unsigned int val, struct pt_regs *regs) > return NMI_HANDLED; > } > > +static struct nmiaction selftest_unk_nmiaction = { > + .handler = nmi_unk_cb, > + .name = "nmi_selftest_unk", > +}; > + > static void init_nmi_testsuite(void) > { > /* trap all the unknown NMIs we may generate */ > - register_nmi_handler(NMI_UNKNOWN, nmi_unk_cb, 0, "nmi_selftest_unk"); > + register_nmi_handler(NMI_UNKNOWN, &selftest_unk_nmiaction); > } > > static void cleanup_nmi_testsuite(void) > @@ -58,12 +63,17 @@ static int test_nmi_ipi_callback(unsigned int val, struct pt_regs *regs) > return NMI_DONE; > } > > +static struct nmiaction selftest_nmiaction = { > + .handler = test_nmi_ipi_callback, > + .flags = NMI_FLAG_FIRST, > + .name = "nmi_selftest", > +}; > + > static void test_nmi_ipi(struct cpumask *mask) > { > unsigned long timeout; > > - if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback, > - NMI_FLAG_FIRST, "nmi_selftest")) { > + if (register_nmi_handler(NMI_LOCAL, &selftest_nmiaction)) { > nmi_fail = FAILURE; > return; > } > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c > index d840e69..a0f8c15 100644 > --- a/arch/x86/kernel/reboot.c > +++ b/arch/x86/kernel/reboot.c > @@ -799,6 +799,12 @@ static void smp_send_nmi_allbutself(void) > apic->send_IPI_allbutself(NMI_VECTOR); > } > > +static struct nmiaction crash_nmiaction = { > + .handler = crash_nmi_callback, > + .flags = NMI_FLAG_FIRST, > + .name = "crash", > +}; > + > /* Halt all other CPUs, calling the specified function on each of them > * > * This function can be used to halt all other CPUs on crash > @@ -817,8 +823,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback) > > atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1); > /* Would it be better to replace the trap vector here? */ > - if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback, > - NMI_FLAG_FIRST, "crash")) > + if (register_nmi_handler(NMI_LOCAL, &crash_nmiaction)) > return; /* return what? */ > /* Ensure the new callback function is set before sending > * out the NMI > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c > index 66c74f4..135d0b2 100644 > --- a/arch/x86/kernel/smp.c > +++ b/arch/x86/kernel/smp.c > @@ -162,6 +162,12 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs) > return NMI_HANDLED; > } > > +static struct nmiaction smp_stop_nmiaction = { > + .handler = smp_stop_nmi_callback, > + .flags = NMI_FLAG_FIRST, > + .name = "smp_stop", > +}; > + > static void native_nmi_stop_other_cpus(int wait) > { > unsigned long flags; > @@ -179,8 +185,7 @@ static void native_nmi_stop_other_cpus(int wait) > if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1) > return; > > - if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback, > - NMI_FLAG_FIRST, "smp_stop")) > + if (register_nmi_handler(NMI_LOCAL, &smp_stop_nmiaction)) > /* Note: we ignore failures here */ > return; > > diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c > index 26b8a85..c3408f6 100644 > --- a/arch/x86/oprofile/nmi_int.c > +++ b/arch/x86/oprofile/nmi_int.c > @@ -453,6 +453,11 @@ static struct notifier_block oprofile_cpu_nb = { > .notifier_call = oprofile_cpu_notifier > }; > > +static struct nmiaction oprofile_nmiaction = { > + .handler = profile_exceptions_notify, > + .name = "oprofile", > +}; > + > static int nmi_setup(void) > { > int err = 0; > @@ -489,8 +494,7 @@ static int nmi_setup(void) > ctr_running = 0; > /* make variables visible to the nmi handler: */ > smp_mb(); > - err = register_nmi_handler(NMI_LOCAL, profile_exceptions_notify, > - 0, "oprofile"); > + err = register_nmi_handler(NMI_LOCAL, &oprofile_nmiaction); > if (err) > goto fail; > > diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c > index 9b3cac0..1d38f92 100644 > --- a/drivers/acpi/apei/ghes.c > +++ b/drivers/acpi/apei/ghes.c > @@ -901,6 +901,11 @@ static unsigned long ghes_esource_prealloc_size( > return prealloc_size; > } > > +static struct nmiaction ghes_nmiaction = { > + .handler = ghes_notify_nmi, > + .name = "ghes", > +}; > + > static int __devinit ghes_probe(struct platform_device *ghes_dev) > { > struct acpi_hest_generic *generic; > @@ -975,8 +980,7 @@ static int __devinit ghes_probe(struct platform_device *ghes_dev) > ghes_estatus_pool_expand(len); > mutex_lock(&ghes_list_mutex); > if (list_empty(&ghes_nmi)) > - register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, > - "ghes"); > + register_nmi_handler(NMI_LOCAL, &ghes_nmiaction); > list_add_rcu(&ghes->list, &ghes_nmi); > mutex_unlock(&ghes_list_mutex); > break; > diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c > index 34767a6..29a6e0a 100644 > --- a/drivers/char/ipmi/ipmi_watchdog.c > +++ b/drivers/char/ipmi/ipmi_watchdog.c > @@ -1296,6 +1296,13 @@ static int preop_op(const char *inval, char *outval) > return 0; > } > > +#ifdef HAVE_DIE_NMI > +static struct nmiaction ipmi_nmiaction = { > + .handler = ipmi_nmi, > + .name = "ipmi", > +}; > +#endif > + > static void check_parms(void) > { > #ifdef HAVE_DIE_NMI > @@ -1313,8 +1320,7 @@ static void check_parms(void) > } > } > if (do_nmi && !nmi_handler_registered) { > - rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0, > - "ipmi"); > + rv = register_nmi_handler(NMI_UNKNOWN, &ipmi_nmiaction); > if (rv) { > printk(KERN_WARNING PFX > "Can't register nmi handler\n"); > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index 8464ea1..e575e63 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy) > } > } > > +static struct nmiaction hpwdt_nmiaction[] = { > + { > + .handler = hpwdt_pretimeout, > + .name = "hpwdt", > + }, > + { > + .handler = hpwdt_pretimeout, > + .flags = NMI_FLAG_FIRST, > + .name = "hpwdt", > + }, > +}; > + > static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > { > int retval; > + struct nmiaction *na = hpwdt_nmiaction; > > /* > * On typical CRU-based systems we need to map that service in > @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > * die notify list to handle a critical NMI. The default is to > * be last so other users of the NMI signal can function. > */ > - retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, > - (priority) ? NMI_FLAG_FIRST : 0, > - "hpwdt"); > + > + if (priority) > + na = &hpwdt_nmiaction[1]; > + > + retval = register_nmi_handler(NMI_UNKNOWN, na); > if (retval != 0) { > dev_warn(&dev->dev, > "Unable to register a die notifier (err=%d).\n", > -- > 1.7.1 > > > > |
From: Wim V. S. <wi...@ig...> - 2012-03-05 10:57:18
|
Hi Li, > diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c > index 8464ea1..e575e63 100644 > --- a/drivers/watchdog/hpwdt.c > +++ b/drivers/watchdog/hpwdt.c > @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy) > } > } > > +static struct nmiaction hpwdt_nmiaction[] = { > + { > + .handler = hpwdt_pretimeout, > + .name = "hpwdt", > + }, > + { > + .handler = hpwdt_pretimeout, > + .flags = NMI_FLAG_FIRST, > + .name = "hpwdt", > + }, > +}; > + > static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > { > int retval; > + struct nmiaction *na = hpwdt_nmiaction; > > /* > * On typical CRU-based systems we need to map that service in > @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) > * die notify list to handle a critical NMI. The default is to > * be last so other users of the NMI signal can function. > */ > - retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, > - (priority) ? NMI_FLAG_FIRST : 0, > - "hpwdt"); > + > + if (priority) > + na = &hpwdt_nmiaction[1]; > + > + retval = register_nmi_handler(NMI_UNKNOWN, na); > if (retval != 0) { > dev_warn(&dev->dev, > "Unable to register a die notifier (err=%d).\n", Why not do something like; static struct nmiaction hpwdt_nmiaction = { .handler = hpwdt_pretimeout, .name = "hpwdt", }; ... if (priority) hpwdt_nmiaction.flags = NMI_FLAG_FIRST; ... Kind regards, Wim. |
From: Li Z. <zh...@li...> - 2012-03-05 10:39:25
|
This patch tries to fix the problem of page fault exception caused by accessing nmiaction structure in nmi if kmemcheck is enabled. If kmemcheck is enabled, the memory allocated through slab are in pages that are marked non-present, so that some checks could be done in the page fault handling code ( e.g. whether the memory is read before written to ). As nmiaction is allocated in this way, so it resides in a non-present page. Then there is a page fault while the nmi code accessing the nmiaction structure, which would then cause a warning by WARN_ON_ONCE(in_nmi()) in kmemcheck_fault(), called by do_page_fault(). v2: as Peter suggested, changed the nmiaction to use static storage. Signed-off-by: Li Zhong <zh...@li...> --- arch/x86/include/asm/nmi.h | 10 +++++- arch/x86/kernel/apic/hw_nmi.c | 8 ++++- arch/x86/kernel/apic/x2apic_uv_x.c | 7 ++++- arch/x86/kernel/cpu/mcheck/mce-inject.c | 8 ++++- arch/x86/kernel/cpu/perf_event.c | 7 ++++- arch/x86/kernel/kgdb.c | 11 ++++-- arch/x86/kernel/nmi.c | 49 ++---------------------------- arch/x86/kernel/nmi_selftest.c | 16 ++++++++-- arch/x86/kernel/reboot.c | 9 ++++- arch/x86/kernel/smp.c | 9 ++++- arch/x86/oprofile/nmi_int.c | 8 ++++- drivers/acpi/apei/ghes.c | 8 ++++- drivers/char/ipmi/ipmi_watchdog.c | 10 +++++- drivers/watchdog/hpwdt.c | 21 +++++++++++-- 14 files changed, 108 insertions(+), 73 deletions(-) diff --git a/arch/x86/include/asm/nmi.h b/arch/x86/include/asm/nmi.h index fd3f9f1..08d464f 100644 --- a/arch/x86/include/asm/nmi.h +++ b/arch/x86/include/asm/nmi.h @@ -35,8 +35,14 @@ enum { typedef int (*nmi_handler_t)(unsigned int, struct pt_regs *); -int register_nmi_handler(unsigned int, nmi_handler_t, unsigned long, - const char *); +struct nmiaction { + struct list_head list; + nmi_handler_t handler; + unsigned int flags; + const char *name; +}; + +int register_nmi_handler(unsigned int, struct nmiaction *); void unregister_nmi_handler(unsigned int, const char *); diff --git a/arch/x86/kernel/apic/hw_nmi.c b/arch/x86/kernel/apic/hw_nmi.c index 31cb9ae..9baea77 100644 --- a/arch/x86/kernel/apic/hw_nmi.c +++ b/arch/x86/kernel/apic/hw_nmi.c @@ -80,10 +80,14 @@ arch_trigger_all_cpu_backtrace_handler(unsigned int cmd, struct pt_regs *regs) return NMI_DONE; } +static struct nmiaction arch_trigger_all_cpu_bt_nmiaction = { + .handler = arch_trigger_all_cpu_backtrace_handler, + .name = "arch_bt", +}; + static int __init register_trigger_all_cpu_backtrace(void) { - register_nmi_handler(NMI_LOCAL, arch_trigger_all_cpu_backtrace_handler, - 0, "arch_bt"); + register_nmi_handler(NMI_LOCAL, &arch_trigger_all_cpu_bt_nmiaction); return 0; } early_initcall(register_trigger_all_cpu_backtrace); diff --git a/arch/x86/kernel/apic/x2apic_uv_x.c b/arch/x86/kernel/apic/x2apic_uv_x.c index 79b05b8..5739042 100644 --- a/arch/x86/kernel/apic/x2apic_uv_x.c +++ b/arch/x86/kernel/apic/x2apic_uv_x.c @@ -715,9 +715,14 @@ int uv_handle_nmi(unsigned int reason, struct pt_regs *regs) return NMI_HANDLED; } +static struct nmiaction uv_nmiaction = { + .handler = uv_handle_nmi, + .name = "uv", +}; + void uv_register_nmi_notifier(void) { - if (register_nmi_handler(NMI_UNKNOWN, uv_handle_nmi, 0, "uv")) + if (register_nmi_handler(NMI_UNKNOWN, &uv_nmiaction)) printk(KERN_WARNING "UV NMI handler failed to register\n"); } diff --git a/arch/x86/kernel/cpu/mcheck/mce-inject.c b/arch/x86/kernel/cpu/mcheck/mce-inject.c index fc4beb3..f9ab41c 100644 --- a/arch/x86/kernel/cpu/mcheck/mce-inject.c +++ b/arch/x86/kernel/cpu/mcheck/mce-inject.c @@ -229,14 +229,18 @@ static ssize_t mce_write(struct file *filp, const char __user *ubuf, return usize; } +static struct nmiaction mce_nmiaction = { + .handler = mce_raise_notify, + .name = "mce_notify", +}; + static int inject_init(void) { if (!alloc_cpumask_var(&mce_inject_cpumask, GFP_KERNEL)) return -ENOMEM; printk(KERN_INFO "Machine check injector initialized\n"); register_mce_write_callback(mce_write); - register_nmi_handler(NMI_LOCAL, mce_raise_notify, 0, - "mce_notify"); + register_nmi_handler(NMI_LOCAL, &mce_nmiaction); return 0; } diff --git a/arch/x86/kernel/cpu/perf_event.c b/arch/x86/kernel/cpu/perf_event.c index 5adce10..8bdff1b 100644 --- a/arch/x86/kernel/cpu/perf_event.c +++ b/arch/x86/kernel/cpu/perf_event.c @@ -1246,6 +1246,11 @@ static void __init pmu_check_apic(void) pr_info("no hardware sampling interrupt available.\n"); } +static struct nmiaction perf_event_nmiaction = { + .handler = perf_event_nmi_handler, + .name = "PMI", +}; + static int __init init_hw_perf_events(void) { struct x86_pmu_quirk *quirk; @@ -1297,7 +1302,7 @@ static int __init init_hw_perf_events(void) ((1LL << x86_pmu.num_counters_fixed)-1) << X86_PMC_IDX_FIXED; perf_events_lapic_init(); - register_nmi_handler(NMI_LOCAL, perf_event_nmi_handler, 0, "PMI"); + register_nmi_handler(NMI_LOCAL, &perf_event_nmiaction); unconstrained = (struct event_constraint) __EVENT_CONSTRAINT(0, (1ULL << x86_pmu.num_counters) - 1, diff --git a/arch/x86/kernel/kgdb.c b/arch/x86/kernel/kgdb.c index faba577..d259d2a 100644 --- a/arch/x86/kernel/kgdb.c +++ b/arch/x86/kernel/kgdb.c @@ -601,6 +601,11 @@ static struct notifier_block kgdb_notifier = { .notifier_call = kgdb_notify, }; +static struct nmiaction kgdb_nmiaction = { + .handler = kgdb_nmi_handler, + .name = "kgdb", +}; + /** * kgdb_arch_init - Perform any architecture specific initalization. * @@ -615,13 +620,11 @@ int kgdb_arch_init(void) if (retval) goto out; - retval = register_nmi_handler(NMI_LOCAL, kgdb_nmi_handler, - 0, "kgdb"); + retval = register_nmi_handler(NMI_LOCAL, &kgdb_nmiaction); if (retval) goto out1; - retval = register_nmi_handler(NMI_UNKNOWN, kgdb_nmi_handler, - 0, "kgdb"); + retval = register_nmi_handler(NMI_UNKNOWN, &kgdb_nmiaction); if (retval) goto out2; diff --git a/arch/x86/kernel/nmi.c b/arch/x86/kernel/nmi.c index 47acaf3..d844acc 100644 --- a/arch/x86/kernel/nmi.c +++ b/arch/x86/kernel/nmi.c @@ -31,14 +31,6 @@ #include <asm/nmi.h> #include <asm/x86_init.h> -#define NMI_MAX_NAMELEN 16 -struct nmiaction { - struct list_head list; - nmi_handler_t handler; - unsigned int flags; - char *name; -}; - struct nmi_desc { spinlock_t lock; struct list_head head; @@ -160,51 +152,18 @@ static struct nmiaction *__free_nmi(unsigned int type, const char *name) return (n); } -int register_nmi_handler(unsigned int type, nmi_handler_t handler, - unsigned long nmiflags, const char *devname) +int register_nmi_handler(unsigned int type, struct nmiaction *na) { - struct nmiaction *action; - int retval = -ENOMEM; - - if (!handler) + if (!na->handler) return -EINVAL; - action = kzalloc(sizeof(struct nmiaction), GFP_KERNEL); - if (!action) - goto fail_action; - - action->handler = handler; - action->flags = nmiflags; - action->name = kstrndup(devname, NMI_MAX_NAMELEN, GFP_KERNEL); - if (!action->name) - goto fail_action_name; - - retval = __setup_nmi(type, action); - - if (retval) - goto fail_setup_nmi; - - return retval; - -fail_setup_nmi: - kfree(action->name); -fail_action_name: - kfree(action); -fail_action: - - return retval; + return __setup_nmi(type, na); } EXPORT_SYMBOL_GPL(register_nmi_handler); void unregister_nmi_handler(unsigned int type, const char *name) { - struct nmiaction *a; - - a = __free_nmi(type, name); - if (a) { - kfree(a->name); - kfree(a); - } + __free_nmi(type, name); } EXPORT_SYMBOL_GPL(unregister_nmi_handler); diff --git a/arch/x86/kernel/nmi_selftest.c b/arch/x86/kernel/nmi_selftest.c index 0d01a8e..40fd637 100644 --- a/arch/x86/kernel/nmi_selftest.c +++ b/arch/x86/kernel/nmi_selftest.c @@ -37,10 +37,15 @@ static int nmi_unk_cb(unsigned int val, struct pt_regs *regs) return NMI_HANDLED; } +static struct nmiaction selftest_unk_nmiaction = { + .handler = nmi_unk_cb, + .name = "nmi_selftest_unk", +}; + static void init_nmi_testsuite(void) { /* trap all the unknown NMIs we may generate */ - register_nmi_handler(NMI_UNKNOWN, nmi_unk_cb, 0, "nmi_selftest_unk"); + register_nmi_handler(NMI_UNKNOWN, &selftest_unk_nmiaction); } static void cleanup_nmi_testsuite(void) @@ -58,12 +63,17 @@ static int test_nmi_ipi_callback(unsigned int val, struct pt_regs *regs) return NMI_DONE; } +static struct nmiaction selftest_nmiaction = { + .handler = test_nmi_ipi_callback, + .flags = NMI_FLAG_FIRST, + .name = "nmi_selftest", +}; + static void test_nmi_ipi(struct cpumask *mask) { unsigned long timeout; - if (register_nmi_handler(NMI_LOCAL, test_nmi_ipi_callback, - NMI_FLAG_FIRST, "nmi_selftest")) { + if (register_nmi_handler(NMI_LOCAL, &selftest_nmiaction)) { nmi_fail = FAILURE; return; } diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c index d840e69..a0f8c15 100644 --- a/arch/x86/kernel/reboot.c +++ b/arch/x86/kernel/reboot.c @@ -799,6 +799,12 @@ static void smp_send_nmi_allbutself(void) apic->send_IPI_allbutself(NMI_VECTOR); } +static struct nmiaction crash_nmiaction = { + .handler = crash_nmi_callback, + .flags = NMI_FLAG_FIRST, + .name = "crash", +}; + /* Halt all other CPUs, calling the specified function on each of them * * This function can be used to halt all other CPUs on crash @@ -817,8 +823,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback) atomic_set(&waiting_for_crash_ipi, num_online_cpus() - 1); /* Would it be better to replace the trap vector here? */ - if (register_nmi_handler(NMI_LOCAL, crash_nmi_callback, - NMI_FLAG_FIRST, "crash")) + if (register_nmi_handler(NMI_LOCAL, &crash_nmiaction)) return; /* return what? */ /* Ensure the new callback function is set before sending * out the NMI diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c index 66c74f4..135d0b2 100644 --- a/arch/x86/kernel/smp.c +++ b/arch/x86/kernel/smp.c @@ -162,6 +162,12 @@ static int smp_stop_nmi_callback(unsigned int val, struct pt_regs *regs) return NMI_HANDLED; } +static struct nmiaction smp_stop_nmiaction = { + .handler = smp_stop_nmi_callback, + .flags = NMI_FLAG_FIRST, + .name = "smp_stop", +}; + static void native_nmi_stop_other_cpus(int wait) { unsigned long flags; @@ -179,8 +185,7 @@ static void native_nmi_stop_other_cpus(int wait) if (atomic_cmpxchg(&stopping_cpu, -1, safe_smp_processor_id()) != -1) return; - if (register_nmi_handler(NMI_LOCAL, smp_stop_nmi_callback, - NMI_FLAG_FIRST, "smp_stop")) + if (register_nmi_handler(NMI_LOCAL, &smp_stop_nmiaction)) /* Note: we ignore failures here */ return; diff --git a/arch/x86/oprofile/nmi_int.c b/arch/x86/oprofile/nmi_int.c index 26b8a85..c3408f6 100644 --- a/arch/x86/oprofile/nmi_int.c +++ b/arch/x86/oprofile/nmi_int.c @@ -453,6 +453,11 @@ static struct notifier_block oprofile_cpu_nb = { .notifier_call = oprofile_cpu_notifier }; +static struct nmiaction oprofile_nmiaction = { + .handler = profile_exceptions_notify, + .name = "oprofile", +}; + static int nmi_setup(void) { int err = 0; @@ -489,8 +494,7 @@ static int nmi_setup(void) ctr_running = 0; /* make variables visible to the nmi handler: */ smp_mb(); - err = register_nmi_handler(NMI_LOCAL, profile_exceptions_notify, - 0, "oprofile"); + err = register_nmi_handler(NMI_LOCAL, &oprofile_nmiaction); if (err) goto fail; diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index 9b3cac0..1d38f92 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -901,6 +901,11 @@ static unsigned long ghes_esource_prealloc_size( return prealloc_size; } +static struct nmiaction ghes_nmiaction = { + .handler = ghes_notify_nmi, + .name = "ghes", +}; + static int __devinit ghes_probe(struct platform_device *ghes_dev) { struct acpi_hest_generic *generic; @@ -975,8 +980,7 @@ static int __devinit ghes_probe(struct platform_device *ghes_dev) ghes_estatus_pool_expand(len); mutex_lock(&ghes_list_mutex); if (list_empty(&ghes_nmi)) - register_nmi_handler(NMI_LOCAL, ghes_notify_nmi, 0, - "ghes"); + register_nmi_handler(NMI_LOCAL, &ghes_nmiaction); list_add_rcu(&ghes->list, &ghes_nmi); mutex_unlock(&ghes_list_mutex); break; diff --git a/drivers/char/ipmi/ipmi_watchdog.c b/drivers/char/ipmi/ipmi_watchdog.c index 34767a6..29a6e0a 100644 --- a/drivers/char/ipmi/ipmi_watchdog.c +++ b/drivers/char/ipmi/ipmi_watchdog.c @@ -1296,6 +1296,13 @@ static int preop_op(const char *inval, char *outval) return 0; } +#ifdef HAVE_DIE_NMI +static struct nmiaction ipmi_nmiaction = { + .handler = ipmi_nmi, + .name = "ipmi", +}; +#endif + static void check_parms(void) { #ifdef HAVE_DIE_NMI @@ -1313,8 +1320,7 @@ static void check_parms(void) } } if (do_nmi && !nmi_handler_registered) { - rv = register_nmi_handler(NMI_UNKNOWN, ipmi_nmi, 0, - "ipmi"); + rv = register_nmi_handler(NMI_UNKNOWN, &ipmi_nmiaction); if (rv) { printk(KERN_WARNING PFX "Can't register nmi handler\n"); diff --git a/drivers/watchdog/hpwdt.c b/drivers/watchdog/hpwdt.c index 8464ea1..e575e63 100644 --- a/drivers/watchdog/hpwdt.c +++ b/drivers/watchdog/hpwdt.c @@ -689,9 +689,22 @@ static void __devinit dmi_find_icru(const struct dmi_header *dm, void *dummy) } } +static struct nmiaction hpwdt_nmiaction[] = { + { + .handler = hpwdt_pretimeout, + .name = "hpwdt", + }, + { + .handler = hpwdt_pretimeout, + .flags = NMI_FLAG_FIRST, + .name = "hpwdt", + }, +}; + static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) { int retval; + struct nmiaction *na = hpwdt_nmiaction; /* * On typical CRU-based systems we need to map that service in @@ -733,9 +746,11 @@ static int __devinit hpwdt_init_nmi_decoding(struct pci_dev *dev) * die notify list to handle a critical NMI. The default is to * be last so other users of the NMI signal can function. */ - retval = register_nmi_handler(NMI_UNKNOWN, hpwdt_pretimeout, - (priority) ? NMI_FLAG_FIRST : 0, - "hpwdt"); + + if (priority) + na = &hpwdt_nmiaction[1]; + + retval = register_nmi_handler(NMI_UNKNOWN, na); if (retval != 0) { dev_warn(&dev->dev, "Unable to register a die notifier (err=%d).\n", -- 1.7.1 |