[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