Re: [Freeipmi-devel] KCS Driver & SMS_ATN Register

[Al Chu]

Hey Matt,

Looks pretty good.  A few comments:

+int
+ipmi_kcs_wait (ipmi_kcs_ctx_t ctx)
+{
+  /* Sit in loop waiting for the SMS_ATN register to be set.
+     This can also exit by invoking ipmi_kcs_wait_cancel */
+  ctx->event_exit = 0;
+  while (ctx->event_exit == 0)
+    {
+      if (_ipmi_kcs_get_status (ctx) & IPMI_KCS_STATUS_REG_SMS_ATN)
+        return(0);
+      sleep(1);
+    }
+   
+   KCS_SET_ERRNUM(ctx, IPMI_KCS_ERR_DRIVER_TIMEOUT);
+   return (-1);
+}

A sleep of 1 second is probably too big for most people.  Perhaps we can
pass in a parameter to determine the usleep time??

Looking through the spec, I think not locking the inband sempahore is
probably safe.  Although there's a tiny part of me that says maybe we
should lock it just to be on the safe side.  A.B., what do you think?

+int
+ipmi_kcs_clear_wait (ipmi_kcs_ctx_t ctx)
+{
+  /* Allow the event thread to exit the wait loop */
+  ctx->event_exit = 1;
+}

This isn't thread safe.  Perhaps we can add back that extra semaphore
(like in your first patch) as the "flag" to break out of the loop?

BTW, a few commenting nits for when I integrate the patch, you don't
follow GNU coding style.  The biggest thing you're missing is a space
between a function call and the parentheses.  So for example:

> KCS_SET_ERRNUM(ctx, IPMI_KCS_ERR_DRIVER_TIMEOUT);

should be

> KCS_SET_ERRNUM (ctx, IPMI_KCS_ERR_DRIVER_TIMEOUT);

and

+  if (ctx->type == IPMI_DEVICE_KCS)
+    {
+      return(ipmi_kcs_cmd_wait_event(ctx));
+    }

there shouldn't be any braces with one line after the if.

> but I didn't run it on hardware yet because my customer has not yet 
> sent me the KCS hardware.

No problem.  Once we settle on a patch, we'll wait for you to test.
Once it's good to go, we can add it into FreeIPMI.

Al

On Mon, 2010-03-01 at 06:34 -0800, Matt Jerdonek wrote:
> 
> Al,
> 
> Thanks for the clarification.  I see your point and I updated the
> patch to address it.  As with the previous patch, this compiles, but I
> didn't run it on hardware yet because my customer has not yet sent me
> the KCS hardware.
> 
> Thanks again,
> -Matt
> 
> 
> --- On Thu, 2/25/10, Al Chu <address@hidden> wrote:
>         
>         From: Al Chu <address@hidden>
>         Subject: Re: [Freeipmi-devel] KCS Driver & SMS_ATN Register
>         To: "Matt Jerdonek" <address@hidden>
>         Cc: "Anand Babu Periasamy" <address@hidden>,
>         address@hidden
>         Date: Thursday, February 25, 2010, 10:33 AM
>         
>         Hi Matt,
>         
>         I don't see it that way.  I could see someone programming a
>         single
>         thread and only wanting to poll the SMS_ATN bit, and process
>         events as
>         they occur.  Not doing any other KCS. e.g.
>         
>         main()
>         {
>            setup_kcs();
>         
>            while (1) {
>               kcs_wait_for_sms()
>               get_message_flags()
>               process_event()
>            }
>         }
>         
>         Maybe I didn't describe it well.  The concern I have with your
>         patch (if
>         I'm reading it correctly, correct me if I'm wrong) is that the
>         only time
>         the SMS ATN bit is checked is in _ipmi_kcs_get_status().
>         _ipmi_kcs_get_status() will only be called through other KCS
>         functions
>         like ipmi_kcs_read() and ipmi_kcs_write().
>         
>         So in order for the SMS ATN bit to be checked, ipmi_kcs_read()
>         and
>         ipmi_kcs_write() have to be called, either by your application
>         or other
>         IPMI going on in the system, otherwise the SMS_ATN bit will
>         never be
>         checked.  Correct?  Under your patch, in the above code
>         snippet,
>         kcs_wait_for_sms() will never return, b/c no other KCS calls
>         are going
>         on (unless they are other KCS IPMI going on in the system
>         elsewhere). 
>         
>         Perhaps within your patch, you assumed other IPMI going on in
>         other
>         parts of the system?
>         
>         Al
>         
>         -- 
>         Albert Chu
>         address@hidden
>         Computer Scientist
>         High Performance Systems Division
>         Lawrence Livermore National Laboratory
>         
>         
> 
-- 
Albert Chu
address@hidden
Computer Scientist
High Performance Systems Division
Lawrence Livermore National Laboratory