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

[Cyril Bur]

On Thu, 29 Oct 2015 12:10:42 +1100
Alistair Popple <alistair at popple.id.au> wrote:

> 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?
> 

Ah I never found a satisfactory way of dealing with the fact that its not
effortless because, by virtue of being here, we don't have a bt_msg...  I
thought about making a fake one on the stack but I decided against that in
favour of just not using my macros...

> >  		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?

Yeah would be nicer, will change.

> 
> > +	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) {
> >   
>