[Skiboot] [PATCH v2 1/3] hw/bt: Fix bt_add_ipmi_msg_head() logic

[Oliver]

On Fri, Feb 22, 2019 at 11:34 AM Andrew Jeffery <andrew at aj.id.au> wrote:
>
>
>
> On Fri, 22 Feb 2019, at 02:37, Vasant Hegde wrote:
> > BT send logic always sends top of bt message list to BMC. Once BMC reads the
> > message, it clears the interrupt and bt_idle() becomes true.
> >
> > bt_add_ipmi_msg_head() adds message to top of the list. If bt message list
> > is not empty then:
> >   - if bt_idle() is true then we will endup sending message to BMC before
> >     getting response from BMC for inflight message. Looks like on some
> >     BMC implementation this results in message timeout.
> >   - else we endup starting message timer without actually sending message
> >     to BMC.. which is not correct.
> >
> > This patch fixes above described issue by adding message to right place
> > in the list.
> >
> > Signed-off-by: Vasant Hegde <hegdevasant at linux.vnet.ibm.com>
> > ---
> > Stewart,
> >   All I'm doing is list_pop/list_add. May be we can have new function
> >   (list_add_after()>. Let me know whether you are ok with this patch
> >    -OR- want me to add list_add_after()>
> >
> > -Vasant
> >
> >  hw/bt.c | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/bt.c b/hw/bt.c
> > index 5a6bc3cde..a3561953b 100644
> > --- a/hw/bt.c
> > +++ b/hw/bt.c
> > @@ -533,11 +533,20 @@ static void bt_add_msg(struct bt_msg *bt_msg)
> >
> >  static int bt_add_ipmi_msg_head(struct ipmi_msg *ipmi_msg)
> >  {
> > +     struct bt_msg *tmp_bt_msg;
> >       struct bt_msg *bt_msg = container_of(ipmi_msg, struct bt_msg, ipmi_msg);
> >
> >       lock(&bt.lock);
> >       bt_add_msg(bt_msg);
> > -     list_add(&bt.msgq, &bt_msg->link);
> > +
> > +     if (list_empty(&bt.msgq)) {
> > +             list_add(&bt.msgq, &bt_msg->link);
> > +     } else {
> > +             /* Add message after top message */
> > +             tmp_bt_msg = list_pop(&bt.msgq, struct bt_msg, link);
> > +             list_add(&bt.msgq, &bt_msg->link);
> > +             list_add(&bt.msgq, &tmp_bt_msg->link);
>
> This looks a bit contorted. Is it worth having a separate pointer for the
> in-flight message so we're not storing it at the head of the list?

Considering this is implementing a crude priority mechanism it might
be worth having a seperate queue for high priority messages. The
current behaviour results in first-in-last-out behaviour if we have
multiple high priority messages are queued which doesn't sound right.
The comment above ipmi_elog_poll() also points out that inserting at
the top of the list is sort of broken anyway, so we might as well fix
it properly.

> > +     }
> >       bt_send_and_unlock();
> >
> >       return 0;
> > --
> > 2.14.3
> >
> > _______________________________________________
> > Skiboot mailing list
> > Skiboot at lists.ozlabs.org
> > https://lists.ozlabs.org/listinfo/skiboot
> >
> _______________________________________________
> Skiboot mailing list
> Skiboot at lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/skiboot