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
