[Skiboot] [PATCH] hw/bt.c: Fix message timeout

[Cédric Le Goater]

Hello Alistair,

On 02/23/2016 05:34 AM, Alistair Popple wrote:
> Commit fb457c9 (hw/bt: Ask the BMC for its BT interface capabilities)
> sets the BT message timeout based on the IPMI get capabilities
> command. This adds a new structure containing a uint8_t to store the
> message timeout in seconds (msg_timeout).
> 
> This introduces two problems:
> 
> 1) The code that checks for timeouts assumes msg_timeout is in units
> of the timebase (tb_hz).
> 
> 2) If get_bt_caps() fails msg_timeout is set to BT_MSG_TIMEOUT which
> is in units of tb_hz and easily overflows a uint8_t.
> 
> This patch solves the above two problems by changing the definition of
> BT_MSG_TIMEOUT to seconds and using secs_to_tb() when comparing
> timebases. It also converts the timebase comparison to use
> tb_compare().
> 
> Signed-off-by: Alistair Popple <alistair at popple.id.au>
> Cc: Cyril Bur <cyril.bur at au1.ibm.com>
> Cc: Stewart Smith <stewart at linux.vnet.ibm.com>
> Cc: Jeremy Kerr <jk at ozlabs.org>
> Cc: Mamatha Inamdar <mamatha4 at linux.vnet.ibm.com>
> ---
>  hw/bt.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/bt.c b/hw/bt.c
> index df4a4f0..3325f69 100644
> --- a/hw/bt.c
> +++ b/hw/bt.c
> @@ -24,6 +24,7 @@
>  #include <bt.h>
>  #include <timer.h>
>  #include <ipmi.h>
> +#include <timebase.h>
>  
>  /* BT registers */
>  #define BT_CTRL			0
> @@ -65,9 +66,9 @@
>  #define BT_MAX_QUEUE_LEN 10
>  
>  /*
> - * How long (in TB ticks) before a message is timed out.
> + * How long (in seconds) before a message is timed out.
>   */
> -#define BT_MSG_TIMEOUT (secs_to_tb(3))
> +#define BT_MSG_TIMEOUT 3
>  
>  /*
>   * Maximum number of times to attempt sending a message before giving up.
> @@ -390,7 +391,8 @@ static void bt_expire_old_msg(uint64_t tb)
>  
>  	bt_msg = list_top(&bt.msgq, struct bt_msg, link);
>  
> -	if (bt_msg && bt_msg->tb > 0 && (bt_msg->tb + bt.caps.msg_timeout) < tb) {
> +	if (bt_msg && bt_msg->tb > 0 &&
> +	    (tb_compare(tb, bt_msg->tb + secs_to_tb(bt.caps.msg_timeout)) == TB_AAFTERB)) {
>  		if (bt_msg->send_count < BT_MAX_SEND_COUNT) {

may be we should use bt.caps.num_retries now instead of BT_MAX_SEND_COUNT : 

  		if (bt_msg->send_count < bt.caps.num_retries + 1 ) {



When the BMC advertises for its capabilities, it says : 

	[1015653747,7] BT: BMC BT capabilities received:
	[1015655700,7] BT: buffer sizes: 65 input 65 output
	[1015657936,7] BT: number of requests: 1
	[1015659716,7] BT: msg timeout: 10 max retries: 1


C.


>  			/* A message timeout is usually due to the BMC
>  			clearing the H2B_ATN flag without actually
>