Re: [PATCH v2 1/3] hw/i2c: add mctp core

[Klaus Jensen]

On Apr 25 10:19, Corey Minyard wrote:
> On Tue, Apr 25, 2023 at 08:35:38AM +0200, Klaus Jensen wrote:
> > From: Klaus Jensen <k.jensen@samsung.com>
> > 
> > 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.
> > 
> > Parts of this implementation is inspired by code[1] previously posted by
> > Jonathan Cameron.
> 
> All in all this looks good.  Two comments:
> 
> I would like to see the buffer handling consolidated into one function
> and the length checked, even for (especially for) the outside users of
> this code, like the nvme code.  Best to avoid future issues with buffer
> overruns.  This will require reworking the get_message_types function,
> unfortunately.
> 

Right now the implementations (i.e. hw/nvme/nmi-i2c.c) writes directly
into the mctp core buffer for get_message_bytes(). The contract is that
it must not write more than the `maxlen` parameter. Is that bad? Would
it be better that get_message_bytes() returned a pointer to its own
buffer that hw/mctp can then copy from?

> You have one trace function on a bad receive message check, but lots of
> other bad receive message checks with no trace.  Just a suggestion, but
> it might be nice for tracking down issues to trace all the reasons a
> message is dropped.
> 

Sounds reasonable! :)

Thanks for the review!

signature.asc
Description: PGP signature