Chris,
On 7/18/07, Kottaridis, Chris <[EMAIL PROTECTED]> wrote:
> Thanks for the response
>
> >The ipmi_serial_alert() function should be just discarding the
> character because either it
> >is the alert char, or it is some unexpected character (the reason for
> the "concern" comment
> >is because the baseboard management controller, BMC, has violated the
> specification if it
> >is sending anything other than the alert char at this time).
>
> So, what we want to have happen is for ipmi_serial_event() to return
> IDLE if the character is not the alert character and ATN if it is the
> alert character.
Correct
>
> >> Since the ipmi_serial_alert() routine doesn't do anything with the
> return
> >> value of ipmi_serial_read_data() the alert_char gets effectively
> lost.
> >
> >It is recognized because the return value is set to "SI_SM_ATTN"
>
> In the original version of the code I have, since
> ipmi_serial_read_data() doesn't put the character in the enc_read_msg[]
> buffer, the alert chracter really wasn't getting recognized since the
> check for it was in the enc_read_msg[] buffer:
Ok, I think I finally see what you are talking about :-) You are
correct, the value that the comparison is being done against is wrong.
>
> static int
> ipmi_serial_alert(struct si_sm_data *serial) {
> int error;
> ipmi_serial_read_data(serial,&error);
> if(error)
> return 0;
> if(serial->enc_read_msg[serial->read_pos] !=
> serial->codec->alert_char)
> return 1;
> return 0;
> }
>
> So, it does sound like ipmi_serial_alert() should be checking the return
> value of ipmi_serial_read_data():
>
> static int
> ipmi_serial_alert(struct si_sm_data *serial)
> {
> unsigned char byte;
> int error;
> byte = ipmi_serial_read_data(serial,&error);
> if(error)
> return 0;
> if(byte == serial->codec->alert_char)
> return 1;
> return 0;
> }
Yes, I think this is the correct way to handle this :-)
>
> Thanks for the response it was helpful.
>
> Do you know of any pointers to any documentation that may describe the
> state machine and how the code is supposed to move from one state to the
> next ?
This is a non-standard interface (it was not specified in the IPMI
spec), but it has been designed to be similar to the KCS interface
defined in the IPMI specification version 1.5. which is free to
download from Intel's website.
>
> Thanks
>
> Chris Kottaridis
> Senior Engineer
> Wind River Systems
> 719-522-9786
>
> -----Original Message-----
> From: dagriego [mailto:[EMAIL PROTECTED]
> Sent: Tuesday, July 17, 2007 11:13 PM
> To: Kottaridis, Chris
> Cc: [email protected]
> Subject: Re: [Openipmi-developer] Serial alert missing
>
> Sorry Chris I've been on vacation last week an only got a chance to look
> at this today :-)
>
> The ipmi_serial_alert() function should be just discarding the character
> because either it is the alert char, or it is some unexpected character
> (the reason for the "concern" comment is because the baseboard
> management controller, BMC, has violated the specification if it is
> sending anything other than the alert char at this time).
>
> See further comments below:
>
> On 7/11/07, Kottaridis, Chris <[EMAIL PROTECTED]> wrote:
> >
> >
> > I am having a problem with the IPMI serial alert mechanism. I am
> > running on a 2.6.10 linux kernel.
> >
> > The report I got goes like this;
> >
> > >it was discovered that the reason we were not following down the ipmi
>
> > >alert code path was due to the msg type being set incorrectly. It
> > >seems that the
> > msg type being
> > >sent by the IPMC is correct and being received by the IPMI serial
> > >interface
> > driver is 0x7. Somewhere
> > >in the msg passing to the ipmi_serial_alert() function the msg type
> > >is set
> > to 0x5b which is incorrect.
> >
> > I am a bit unfamilair with the IPMI protocols and would appreciate any
>
> > help or advice on this issue.
> >
> > In looking at the ipmi_serial_alert() code it does seem that indeed
> > the character is probably getting lost:
> >
> >
> > static int
> > ipmi_serial_alert(struct si_sm_data *serial) {
> > int error;
> > ipmi_serial_read_data(serial,&error);
> > if(error)
> > return 0;
> > if(serial->enc_read_msg[serial->read_pos] !=
> > serial->codec->alert_char)
> > return 1;
> > return 0;
> > }
> >
> >
> > The ipmi_serial_read_data() routine does NOT move the character from
> > the tty buffer to the enc_read_msg[] buffer, but just returns the next
>
> > character in the tty buffer:
>
> It is suppose to discard the character because it is unexpected. No
> need to copy it to the buffer.
>
> >
> > static unsigned char
> > ipmi_serial_read_data(struct si_sm_data *serial,int *error) {
> > unsigned char retval;
> > struct tty_struct *tty = serial->port->info->tty;
> > unsigned long flags;
> > *error = 0;
> > retval = tty->read_buf[tty->read_tail];
> > spin_lock_irqsave(&tty->read_lock, flags);
> > tty->read_tail = (tty->read_tail + 1) & (N_TTY_BUF_SIZE-1);
> > tty->read_cnt -= 1;
> > spin_unlock_irqrestore(&tty->read_lock, flags);
> > return retval;
> > }
> >
> >
> > Since the ipmi_serial_alert() routine doesn't do anything with the
> return
> > value of ipmi_serial_read_data() the alert_char gets effectively lost.
>
> It is recognized because the return value is set to "SI_SM_ATTN"
>
> >
> > There is a different routine called ipmi_serial_read_next_byte that
> does
> > move data from the tty buffer into the enc_read_msg[] buffer. You can
> see
> > that if there is room in the enc_read_msg[] buffer that it uses
> > ipmi_serial_read_data() and saves the return value into the
> enc_read_msg[]
> > buffer:
> >
> > } else {
> > serial->enc_read_msg[serial->read_pos] =
> > ipmi_serial_read_data(serial,&error);
> >
> >
> > It then starts processing that character comparing it against various
> > control characters. The alert_char is one of them it looks for:
> >
> > /* See if this is an alert char */
> > if((serial->codec->alert_char) &&
> >
> > (serial->enc_read_msg[serial->read_pos] ==
> >
> > serial->codec->alert_char)){
> > atomic_set(&serial->alert,1);
> > serial->rx_timeout -= time;
> > return;
> > }
>
> This is needed because we can not process the alert until after the
> current message is complete.
>
> >
> > Notice that if the character just read is the alert_char it sets the
> > serial->alert flag and then returns WITHOUT incrementing the value
> > serial->readpos. Since read_pos was not incremented the alert_char is
> > effectively not part of the enc_read_msg[] buffer.
> >
> > So, I think there are two things wrong with ipmi_serial_alert(). First
> it
> > uses ipmi_serial_read_data() which effectively throws the alert_char
> away.
> > Secondly, it looks for the alert_char to be in the enc_read_msg[]
> buffer. It
> > so happens that if ipmi_serial_read_next_byte() is called the
> character at
> > read_pos in the enc_read_msg[] buffer is the alert_char. But, if the
> > character is not the alert_char, or one of the other control
> characters,
> > then read_pos will be incremented and we'll be looking at some random
> value
> > located where the next incoming character is to be placed. So, we'll
> be
> > making the decision as to wether the alert_char was sent off of some
> random
> > bogus data, if we were to just use ipmi_serial_read_next_byte()
> instead of
> > ipmi_serial_read_data().
> >
> > It seems the ipmi_serial_alert() routine should look more like this:
> >
> > static int
> > ipmi_serial_alert(struct si_sm_data *serial)
> > {
> > int error;
> > ipmi_serial_read_next_byte(serial,&error);
> > if(error)
> > return 0;
> > /* check if the alert character was sent */
> > if(atomic_read(serial->alert) == 1) {
> > /* Reset alert if it was set so we don't think we got
> two
> > alert characters */
> > atomic_set(&serial->alert, 0);
> > return 1;
> > }
> > return 0;
> > }
> >
> >
> > This assumes that if the character is not the alert character we want
> it to
> > go into the enc_read_msg[]. If we should just toss the value in the
> tty
> > buffer if it's not the alert character then we can do this:
> >
> > static int
> > ipmi_serial_alert(struct si_sm_data *serial)
> > {
> > unsigned char byte;
> > int error;
> > byte = ipmi_serial_read_data(serial,&error);
> > if(error)
> > return 0;
> > if(byte == serial->codec->alert_char)
> > return 1;
> > return 0;
> > }
> >
> >
> >
> > Now, ipmi_serial_alert() only gets called from ipmi_serial_event() if
> the
> > state machine is either SERIAL_IDLE or SERIAL_ASYNC_MSG and if
> queue_msg
> > field isn't set:
> >
> > switch (serial->state) {
> > case SERIAL_IDLE:
> > case SERIAL_ASYNC_MSG:
> >
> > if(atomic_read(&serial->alert)){
> > atomic_set(&serial->alert,0);
> > return SI_SM_ATTN;
> > }
> > if
> > (ipmi_serial_check_rx_ready(serial,(SERIAL_IDLE?0:time)))
> > {
> > /* Something in RX FIFO */
> > if(serial->codec->queue_msg){
> > /* BMC does not support msg queue
> > * Must queue on host
> > */
> > do{
> >
> > ipmi_serial_read_next_byte(serial,time);
> >
> > if(serial->end_of_rx){
> >
> > serial->codec->queue_msg(
> >
> > serial->codec_data,
> >
> > serial->enc_read_msg,
> >
> > serial->read_pos);
> >
> > ipmi_serial_clear_transaction(serial);
> > return
> > SI_SM_ATTN;
> > }
> >
> > }while(ipmi_serial_check_rx_ready(serial,0));
> > serial->state = SERIAL_ASYNC_MSG;
> > return SI_SM_CALL_WITH_DELAY;
> > }else{
> > /* if there is an attention charactor
> we
> > * should check for it
> > here, otherwise
> > * anything at the serial
> > port should be of
> > * some concern
> > */
> > if(serial->codec->alert_char){
> >
> > if(!ipmi_serial_alert(serial))
> >
> > return SI_SM_IDLE;
> > return SI_SM_ATTN;
> > }
> > return SI_SM_IDLE;
> > }
> >
> > I am not real sure what the serial->codec->queue_msg is getting set to
> nor
> > the comment about "BMC" there. I also am not real sure about the
> comment
> > that if the character is not the alert_char there should be "some
> concern".
> > If this means we should only expect an alert_char then I guess I'd go
> for
> > the second implementation.
>
> BMC = baseboard management controller
>
> One of the supported BMCs did not follow the specification and could
> not queue messages in the BMC from other devices such as the shelf
> controller, so we provided this workaround to allow these messages to
> be queued on the host and simulate receiving the alert char.
>
> >
> > I am not sure wether we should preserve any character that
> > ipmi_serial_alert() might find in the tty buffer that isn't the
> alert_char
> > or not.
>
> These characters are unexpected, so currently they are discarded.
>
> >
> > Any advice or suggestions would be appreciated.
> >
>
> Hope that clarifies things for you.
>
> -David
>
-------------------------------------------------------------------------
This SF.net email is sponsored by DB2 Express
Download DB2 Express C - the FREE version of DB2 express and take
control of your XML. No limits. Just data. Click to get it now.
http://sourceforge.net/powerbar/db2/
_______________________________________________
Openipmi-developer mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/openipmi-developer
