On Wed, 30 May 2012 07:21:17 -0700 Naresh Bhat <[email protected]> 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 <[email protected]> > --- 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"); _ ------------------------------------------------------------------------------ Live Security Virtual Conference Exclusive live event will cover all the ways today's security and threat landscape has changed and how IT managers can respond. Discussions will include endpoint security, mobile security and the latest in malware threats. http://www.accelacomm.com/jaw/sfrnl04242012/114/50122263/ _______________________________________________ Openipmi-developer mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/openipmi-developer
