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
