[Skiboot] [PATCH V2 5/5] hw/bt: Ask the BMC for its BT interface capabilities

Cyril Bur cyril.bur at au1.ibm.com
Thu Nov 26 09:06:29 AEDT 2015


On Thu, 26 Nov 2015 09:04:26 +1100
Cyril Bur <cyril.bur at au1.ibm.com> wrote:

> On Wed, 25 Nov 2015 18:57:25 +0100
> C├ędric Le Goater <clg at fr.ibm.com> wrote:
> 
> > Hello Cyril,
> > 
> > A few comments below,
> > 
> > On 11/12/2015 07:22 AM, Cyril Bur wrote:  
> > > Signed-off-by: Cyril Bur <cyril.bur at au1.ibm.com>
> > > ---
> > >  hw/bt.c        | 86 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
> > >  include/ipmi.h |  1 +
> > >  2 files changed, 84 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/hw/bt.c b/hw/bt.c
> > > index 6d46367..58e6a37 100644
> > > --- a/hw/bt.c
> > > +++ b/hw/bt.c
> > > @@ -22,6 +22,7 @@
> > >  #include <ipmi.h>
> > >  #include <bt.h>
> > >  #include <timer.h>
> > > +#include <ipmi.h>
> > >  
> > >  /* BT registers */
> > >  #define BT_CTRL			0
> > > @@ -112,6 +113,14 @@ struct bt_msg {
> > >  	struct ipmi_msg ipmi_msg;
> > >  };
> > >  
> > > +struct bt_caps {
> > > +	uint8_t num_requests;
> > > +	uint8_t input_buf_len;
> > > +	uint8_t output_buf_len;
> > > +	uint8_t msg_timeout;
> > > +	uint8_t num_retries;
> > > +};
> > > +
> > >  struct bt {
> > >  	uint32_t base_addr;
> > >  	struct lock lock;
> > > @@ -119,7 +128,9 @@ struct bt {
> > >  	struct timer poller;
> > >  	bool irq_ok;
> > >  	int queue_len;
> > > +	struct bt_caps caps;
> > >  };
> > > +
> > >  static struct bt bt;
> > >  
> > >  static int ipmi_seq;
> > > @@ -150,6 +161,66 @@ static inline void bt_assert_h_busy(void)
> > >  	assert(rval & BT_CTRL_H_BUSY);
> > >  }
> > >  
> > > +static void get_bt_caps_complete(struct ipmi_msg *msg)
> > > +{
> > > +	/* Ignore errors, we'll fallback to using the defaults, no big deal */
> > > +
> > > +	if (msg->data[0] == 0) {
> > > +		BT_DBG("Got illegal BMC BT capability\n");
> > > +		goto out;
> > > +	}
> > > +
> > > +	if (msg->data[3] + 1 != BT_FIFO_LEN) {
> > > +		BT_DBG("Got a input buffer len (%d) cap which differs from the default\n",
> > > +				msg->data[3]);
> > > +		goto out;
> > > +	}    
> > 
> > It should be msg->data[1].  
> > 
> > why discard this value if it is different from the default value ? It should 
> > work and who knows what to expect in the future for such values. 
> >   
> Ah yep my bad, will send fixup
> >   
> > > +
> > > +	if (msg->data[4] + 1 != BT_FIFO_LEN) {
> > > +		BT_DBG("Got a output buffer len (%d) cap which differs from the default\n",
> > > +				msg->data[4]);
> > > +		goto out;
> > > +	}    
> > 
> > it should be  msg->data[2] here
> >   
> 
> Yep
> 
> > > +
> > > +	memcpy(&bt.caps, msg->data, sizeof(struct bt_caps));
> > > +
> > > +	/*
> > > +	 * IPMI Spec says that the value for buffer sizes are:
> > > +	 * "the largest value allowed in first byte"
> > > +	 * Therefore we want to add one to what we get
> > > +	 */
> > > +	bt.caps.input_buf_len++;
> > > +	bt.caps.output_buf_len++;    
> > 
> > If we don't discard the values different from BT_FIFO_LEN, there is 
> > an issue with the uint8_t above. One could receive a 0xff in the 
> > message and 255 + 1 == 0 !
> > 
> > I would change the type to uint16_t and get rid of the memcpy().   
> 
> If we're ditching the memcpy, which I'm in favour of anyway because it will be
> more clear whats happening, we could just check the value, in theory the same
> problem will happen with a uint16_t...
> 

Sorry no coffee, can't happen with uint16_t. You're correct.

> Thanks,
> 
> Cyril
> > 
> > Cheers,
> > 
> > C. 
> > 
> >   
> > > +	BT_DBG("BMC BT capabilities received:\n");
> > > +	BT_DBG("buffer sizes: %d input %d output\n",
> > > +			bt.caps.input_buf_len, bt.caps.output_buf_len);
> > > +	BT_DBG("number of requests: %d\n", bt.caps.num_requests);
> > > +	BT_DBG( "msg timeout: %d max retries: %d\n",
> > > +			bt.caps.msg_timeout, bt.caps.num_retries);
> > > +
> > > +out:
> > > +	ipmi_free_msg(msg);
> > > +}
> > > +
> > > +static void get_bt_caps(void)
> > > +{
> > > +
> > > +	struct ipmi_msg *bmc_caps;
> > > +	/*
> > > +	 * Didn't sent a message, now is a good time to ask the BMC for its
> > > +	 * capabilities.
> > > +	 */
> > > +	bmc_caps = ipmi_mkmsg(IPMI_DEFAULT_INTERFACE, IPMI_GET_BT_CAPS,
> > > +			get_bt_caps_complete, NULL, NULL, 0, sizeof(struct bt_caps));
> > > +	if (!bmc_caps)
> > > +		BT_ERR("Couldn't create BMC BT capabilities msg\n");
> > > +
> > > +	if (bmc_caps && ipmi_queue_msg(bmc_caps))
> > > +		BT_ERR("Couldn't enqueue request for BMC BT capabilities\n");
> > > +
> > > +	/* Ignore errors, we'll fallback to using the defaults, no big deal */
> > > +}
> > > +
> > >  static inline bool bt_idle(void)
> > >  {
> > >  	uint8_t bt_ctrl = bt_inb(BT_CTRL);
> > > @@ -225,7 +296,7 @@ static void bt_clear_fifo(void)
> > >  {
> > >  	int i;
> > >  
> > > -	for (i = 0; i < BT_FIFO_LEN; i++)
> > > +	for (i = 0; i < bt.caps.input_buf_len; i++)
> > >  		bt_outb(0xff, BT_HOST2BMC);
> > >  }
> > >  
> > > @@ -326,7 +397,7 @@ 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_MSG_TIMEOUT) < tb) {
> > > +	if (bt_msg && bt_msg->tb > 0 && (bt_msg->tb + bt.caps.msg_timeout) < tb) {
> > >  		if (bt_msg->send_count < BT_MAX_SEND_COUNT) {
> > >  			/* A message timeout is usually due to the BMC
> > >  			clearing the H2B_ATN flag without actually
> > > @@ -397,7 +468,6 @@ static void bt_send_and_unlock(void)
> > >  	}
> > >  
> > >  	unlock(&bt.lock);
> > > -	return;
> > >  }
> > >  
> > >  static void bt_poll(struct timer *t __unused, void *data __unused,
> > > @@ -560,6 +630,13 @@ void bt_init(void)
> > >  	const struct dt_property *prop;
> > >  	uint32_t irq;
> > >  
> > > +	/* Set sane capability defaults */
> > > +	bt.caps.num_requests = 1;
> > > +	bt.caps.input_buf_len = BT_FIFO_LEN;
> > > +	bt.caps.output_buf_len = BT_FIFO_LEN;
> > > +	bt.caps.msg_timeout = BT_MSG_TIMEOUT;
> > > +	bt.caps.num_retries = 1;
> > > +
> > >  	/* We support only one */
> > >  	n = dt_find_compatible_node(dt_root, NULL, "ipmi-bt");
> > >  	if (!n)
> > > @@ -602,5 +679,8 @@ void bt_init(void)
> > >  	bt_lpc_client.interrupts = LPC_IRQ(irq);
> > >  	lpc_register_client(dt_get_chip_id(n), &bt_lpc_client);
> > >  
> > > +	/* Enqueue an IPMI message to ask the BMC about its BT capabilities */
> > > +	get_bt_caps();
> > > +
> > >  	BT_DBG("BT: Using LPC IRQ %d\n", irq);
> > >  }
> > > diff --git a/include/ipmi.h b/include/ipmi.h
> > > index 99a9094..6aeacb0 100644
> > > --- a/include/ipmi.h
> > > +++ b/include/ipmi.h
> > > @@ -117,6 +117,7 @@
> > >  #define IPMI_GET_MESSAGE_FLAGS		IPMI_CODE(IPMI_NETFN_APP, 0x31)
> > >  #define IPMI_GET_MESSAGE		IPMI_CODE(IPMI_NETFN_APP, 0x33)
> > >  #define IPMI_READ_EVENT			IPMI_CODE(IPMI_NETFN_APP, 0x35)
> > > +#define IPMI_GET_BT_CAPS		IPMI_CODE(IPMI_NETFN_APP, 0x56)
> > >  #define IPMI_SET_SENSOR_READING		IPMI_CODE(IPMI_NETFN_SE, 0x30)
> > >  
> > >  /* AMI OEM comamnds. AMI uses NETFN 0x3a and 0x32 */
> > >     
> >   
> 



More information about the Skiboot mailing list