Re: [PATCH RFC 2/3] hw/i2c: add mctp core

[Klaus Jensen]

On Nov 18 13:56, Jeremy Kerr wrote:
> Hi Klaus,
> 
> > Add an abstract MCTP over I2C endpoint model. This implements MCTP
> > control message handling as well as handling the actual I2C transport
> > (packetization).
> > 
> > Devices are intended to derive from this and implement the class
> > methods.
> 
> Looks good, nice to see how it's used by the nmi device later too.
> 
> A couple of issues with the state machine though, comments inline, and
> a bit of a patch below.
> 
> > +static void i2c_mctp_tx(void *opaque)
> > +{
> > +    DeviceState *dev = DEVICE(opaque);
> > +    I2CBus *i2c = I2C_BUS(qdev_get_parent_bus(dev));
> > +    I2CSlave *slave = I2C_SLAVE(dev);
> > +    MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(dev);
> > +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > +    MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > +    uint8_t flags = 0;
> > +
> > +    switch (mctp->tx.state) {
> > +    case I2C_MCTP_STATE_TX_SEND_BYTE:
> > +        if (mctp->pos < mctp->len) {
> > +            uint8_t byte = mctp->buffer[mctp->pos];
> > +
> > +            trace_i2c_mctp_tx_send_byte(mctp->pos, byte);
> > +
> > +            /* send next byte */
> > +            i2c_send_async(i2c, byte);
> > +
> > +            mctp->pos++;
> > +
> > +            break;
> > +        }
> > +
> > +        /* packet sent */
> > +        i2c_end_transfer(i2c);
> 
> If we're sending a control message, mctp->len will be set to the control
> msg len here, then:
> 
> > +
> > +        /* fall through */
> > +
> > +    case I2C_MCTP_STATE_TX_START_SEND:
> > +        if (mctp->tx.is_control) {
> > +            /* packet payload is already in buffer */
> > +            flags |= MCTP_H_FLAGS_SOM | MCTP_H_FLAGS_EOM;
> > +        } else {
> > +            /* get message bytes from derived device */
> > +            mctp->len = mc->get_message_bytes(mctp, pkt->mctp.payload,
> > +                                              I2C_MCTP_MAXMTU, &flags);
> > +        }
> 
> ... it doesn't get cleared above, so:
> 
> > +
> > +        if (!mctp->len) {
> 
> ... we don't hit this conditional, and hence keep sending unlimited
> bytes. This presents as continuous interrupts to the aspeed i2c driver
> when replying to any control message.
> 
> I think we need a mctp->len = 0 with the i2c_end_transfer(). With that,
> I can get control protocol communication working with mctpd.
> 
> > +static int i2c_mctp_event_cb(I2CSlave *i2c, enum i2c_event event)
> > +{
> > +    MCTPI2CEndpoint *mctp = MCTP_I2C_ENDPOINT(i2c);
> > +    MCTPI2CEndpointClass *mc = MCTP_I2C_ENDPOINT_GET_CLASS(mctp);
> > +    MCTPI2CPacket *pkt = (MCTPI2CPacket *)mctp->buffer;
> > +    size_t payload_len;
> > +    uint8_t pec;
> > +
> > +    switch (event) {
> > +    case I2C_START_SEND:
> > +        if (mctp->state != I2C_MCTP_STATE_IDLE) {
> > +            return -1;
> 
> mctp->state may (validly) be I2C_MCTP_STATE_RX here, if we're receiving
> the start event for the second packet of a multi-packet message.
> 
> > +        }
> > +
> > +        /* the i2c core eats the slave address, so put it back in */
> > +        pkt->i2c.dest = i2c->address << 1;
> > +        mctp->len = 1;
> > +
> > +        mctp->state = I2C_MCTP_STATE_RX_STARTED;
> > +
> > +        return 0;
> > +
> > +    case I2C_FINISH:
> > +        payload_len = mctp->len - (1 + offsetof(MCTPI2CPacket, 
> > mctp.payload));
> > +
> > +        if (pkt->i2c.byte_count + 3 != mctp->len - 1) {
> > +            trace_i2c_mctp_drop_invalid_length(pkt->i2c.byte_count +
> > 3,
> > +                                               mctp->len - 1);
> > +            goto drop;
> > +        }
> > +
> > +        pec = i2c_smbus_pec(0, mctp->buffer, mctp->len - 1);
> > +        if (mctp->buffer[mctp->len - 1] != pec) {
> > +            trace_i2c_mctp_drop_invalid_pec(mctp->buffer[mctp->len - 1], 
> > pec);
> > +            goto drop;
> > +        }
> > +
> > +        if (pkt->mctp.hdr.eid.dest != mctp->my_eid) {
> > +            trace_i2c_mctp_drop_invalid_eid(pkt->mctp.hdr.eid.dest,
> > +                                            mctp->my_eid);
> > +            goto drop;
> > +        }
> > +
> > +        if (pkt->mctp.hdr.flags & MCTP_H_FLAGS_SOM) {
> > +            mctp->tx.is_control = false;
> > +
> > +            if (mctp->state == I2C_MCTP_STATE_RX) {
> > +                mc->reset_message(mctp);
> > +            }
> > +
> > +            mctp->state = I2C_MCTP_STATE_RX;
> > +
> > +            mctp->tx.addr = pkt->i2c.source;
> > +            mctp->tx.eid = pkt->mctp.hdr.eid.source;
> > +            mctp->tx.flags = pkt->mctp.hdr.flags & 0x7;
> > +            mctp->tx.pktseq = (pkt->mctp.hdr.flags >> 4) & 0x3;
> > +
> > +            if ((pkt->mctp.payload[0] & 0x7f) == 
> > MCTP_MESSAGE_TYPE_CONTROL) {
> > +                mctp->tx.is_control = true;
> > +
> > +                i2c_mctp_handle_control(mctp);
> > +
> > +                return 0;
> > +            }
> > +        } else if (mctp->state == I2C_MCTP_STATE_RX_STARTED) {
> > +            trace_i2c_mctp_drop("expected SOM");
> > +            goto drop;
> > +        } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != 
> > (mctp->tx.pktseq++ & 0x3)) {
> 
> The pktseq is the sequence number of the last packet seen, so you want a
> pre-increment there: ++mctp->tx.pktseq & 0x3
> 
>         } else if (((pkt->mctp.hdr.flags >> 4) & 0x3) != (++mctp->tx.pktseq & 
> 0x3)) {
> 
> With those changes, I can get control protocol going, and multi-packet
> messages work. There's a couple of failures from unsupported commands,
> but otherwise looks good:
> 
>   # mctp addr add 8 dev mctpi2c6
>   # mctp link set mctpi2c6 up
>   # mctp link set mctpi2c6 mtu 254
>   # systemctl restart mctpd
>   # busctl --no-pager call xyz.openbmc_project.MCTP \
>     /xyz/openbmc_project/mctp au.com.CodeConstruct.MCTP \
>     SetupEndpoint say mctpi2c6 1 0x1d
>   yisb 9 1 "/xyz/openbmc_project/mctp/1/9" true
>   # mctp route del 9 via mctpi2c6
>   # mctp route add 9 via mctpi2c6 mtu 68
>   # mi-mctp 1 9 info
>   nmi message type 0x2 not handled
>   Identify Controller failed, no quirks applied
>   NVMe MI subsys info:
>    num ports: 1
>    major ver: 1
>    minor ver: 1
>   NVMe MI port info:
>     port 0
>       type SMBus[2]
>       MCTP MTU: 64
>       MEB size: 0
>       SMBus address: 0x00
>       VPD access freq: 0x00
>       MCTP address: 0x1d
>       MCTP access freq: 0x01
>       NVMe basic management: disabled
>   nmi command 0x1 not handled
>   mi-mctp: can't perform Health Status Poll operation: Success
>   # mi-mctp 1 9 get-config 0
>   nmi message type 0x2 not handled
>   Identify Controller failed, no quirks applied
>   SMBus access frequency (port 0): 100k [0x1]
>   MCTP MTU (port 0): 64
> 
> I've included a patch below, with some fixes for the above, in case that
> helps.
> 

Thanks for the review and patch,

Definitely helps!

signature.asc
Description: PGP signature