[Skiboot] [PATCH v2 1/3] hw/bt: Fix bt_add_ipmi_msg_head() logic
Stewart Smith
stewart at linux.ibm.com
Mon Feb 25 11:42:35 AEDT 2019
Oliver <oohall at gmail.com> writes:
> 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.
Yeah, I'm inclined to agree.
Let's have a high priority list that's processed first.
--
Stewart Smith
OPAL Architect, IBM.
More information about the Skiboot
mailing list