| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 26 Jun 2012 17:19:49 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: Naresh Bhat <nbhat@...sta.com>
Cc: openipmi-developer@...ts.sourceforge.net,
linux-kernel@...r.kernel.org
Subject: Re: fix IPMI I/O error on NEC FC storage array
On Wed, 30 May 2012 07:21:17 -0700
Naresh Bhat <nbhat@...sta.com> 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 <nbhat@...sta.com>
> --- 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");
_
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists