[Skiboot] [PATCH 1/5] hw/bt: Improve debug printing

[Alistair Popple]

Hey,

On Thu, 29 Oct 2015 11:24:05 Cyril Bur wrote:
> Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> ---
>  hw/bt.c | 41 ++++++++++++++++++++++++++++++++---------
>  1 file changed, 32 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/bt.c b/hw/bt.c
> index 6d559d0..15e494f 100644
> --- a/hw/bt.c
> +++ b/hw/bt.c
> @@ -71,11 +71,32 @@
>  
>  #define BT_QUEUE_DEBUG 0
>  
> -#define BT_ERR(msg, fmt, args...) \
> -	do { prerror("BT seq 0x%02x netfn 0x%02x cmd 0x%02x: " fmt "\n", \
> -	     (msg)->seq, (msg)->ipmi_msg.netfn, (msg)->ipmi_msg.cmd, ##args); 
\
> +#define _BT_LOG(level, msg, fmt, args...) \
> +	do { if (msg) \
> +			prlog(level, "BT seq 0x%02x netfn 0x%02x cmd 0x%02x: " 
fmt "\n", \
> +			(msg)->seq, (msg)->ipmi_msg.netfn, (msg)-
>ipmi_msg.cmd, ##args); \
> +		else \
> +			prlog(level, "BT seq 0x?? netfn 0x?? cmd 0x??: " fmt 
"\n", ##args); \
>  	} while(0)
>  
> +/*
> + * takes a struct bt_msg *
> + */
> +#define BT_ERR(msg, fmt, args...) \
> +	_BT_LOG(PR_ERR, msg, fmt, ##args)
> +
> +/*
> + * takes a struct bt_msg *
> + */
> +#define BT_DBG(msg, fmt, args...) \
> +	_BT_LOG(PR_DEBUG, msg, fmt, ##args)
> +
> +/*
> + * takes a struct bt_msg *
> + */
> +#define BT_INF(msg, fmt, args...) \
> +	_BT_LOG(PR_INFO, msg, fmt, ##args)
> +
>  enum bt_states {
>  	BT_STATE_IDLE = 0,
>  	BT_STATE_RESP_WAIT,
> @@ -191,6 +212,7 @@ static void bt_send_msg(struct bt_msg *bt_msg)
>  	for (i = 0; i < ipmi_msg->req_size; i++)
>  		bt_outb(ipmi_msg->data[i], BT_HOST2BMC);
>  
> +	BT_DBG(bt_msg, "Message sent to host");
>  	bt_outb(BT_CTRL_H2B_ATN, BT_CTRL);
>  	bt_set_state(BT_STATE_RESP_WAIT);
>  
> @@ -244,7 +266,8 @@ static void bt_get_resp(void)
>  	}
>  	if (!bt_msg) {
>  		/* A response to a message we no longer care about. */
> -		prlog(PR_INFO, "BT: Nobody cared about a response to an 
BT/IPMI message\n");
> +		prlog(PR_INFO, "BT: Nobody cared about a response to an 
BT/IPMI message"
> +			"(seq 0x%02x netfn 0x%02x cmd 0x%02x)", seq, netfn, 
cmd);

Why don't you use those lovely macros you define here?

>  		bt_flush_msg();
>  		bt_set_state(BT_STATE_IDLE);
>  		return;
> @@ -271,6 +294,10 @@ static void bt_get_resp(void)
>  
>  	bt_set_state(BT_STATE_IDLE);
>  
> +#if BT_QUEUE_DEBUG

Wouldn't it be clearer if we removed this and just have an empty definition 
for BT_DBG if BT_QUEUE_DEBUG is not set?

> +	BT_DBG(bt_msg, "IPMI MSG done");
> +#endif
> +
>
>  	list_del(&bt_msg->link);
>  	bt.queue_len--;
>  	unlock(&bt.lock);
> @@ -278,10 +305,6 @@ static void bt_get_resp(void)
>  	/*
>  	 * Call the IPMI layer to finish processing the message.
>  	 */
> -#if BT_QUEUE_DEBUG
> -	prlog(PR_DEBUG, "cmd 0x%02x done\n", seq);
> -#endif
> -
>  	ipmi_cmd_done(cmd, netfn, cc, ipmi_msg);
>  	lock(&bt.lock);
>  
> @@ -327,7 +350,7 @@ static void print_debug_queue_info(void)
>  		printed = false;
>  		prlog(PR_DEBUG, "-------- BT Msg Queue --------\n");
>  		list_for_each(&bt.msgq, msg, link) {
> -			prlog(PR_DEBUG, "Seq: 0x%02x Cmd: 0x%02x\n", msg->seq, 
msg->ipmi_msg.cmd);
> +			BT_DBG(msg, "[]");
>  		}
>  		prlog(PR_DEBUG, "-----------------------------\n");
>  	} else if (!printed) {
>