[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:04:26 AEDT 2015
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...
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