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

C├ędric Le Goater clg at fr.ibm.com
Thu Nov 26 04:57:25 AEDT 2015


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. 


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

> +
> +	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(). 

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