RE: [PATCH v7 03/22] hw/i3c: Add bus support

Jamin Lin
2026-03-02T03:36:59Z

Hi Jithu Joseph

> Subject: Re: [PATCH v7 03/22] hw/i3c: Add bus support
>
>
> Another minor comment which too can be addressed later
>
> On 2/24/2026 6:12 PM, Jamin Lin wrote:
> > Adds an I3C bus and a target class.
> > The bus supports:
> > - I3C data transmission and reception
> > - CCCs (including ENTDAA)
> > - IBIs
> > - legacy I2C transactions
> >
> > General usage of the bus is similar to I2C. Users are expected to
> > initialize a bus via i3c_init_bus, and use the bus returned from the
> > init function to do transactions on the bus.
> >
> > In order to handle IBIs, the controller provides callbacks to handle
> > receiving an IBI from a target, receiving (optional) additional IBI
> > bytes from a target, and handling when a target is done with its IBI.
> >
> > Similarly, target creation is done via i3c_target_create_simple and
> > users use the provided I3CTarget to handle transactions.
> > The target has functions provided that it can use to invoke an IBI and
> > send additional bytes.
> >
> > Along with the send, recv, and event callbacks that are expected of an
> > I3C target, which are similar to I2C, there is a separate callback for
> > CCC handling.
> > This is to help encapsulate CCC handling and keep it separate from
> > target-specific read/write functionality.
> >
> > To avoid repition for required CCCs among I3C targets, there is some
> > class-level CCC handling added. The CCC is then passed to the target
> > in case it needs to handle it in some way.
> >
> > Signed-off-by: Joe Komlodi <komlodi@google.com>
> > Reviewed-by: Patrick Venture <venture@google.com>
> > Reviewed-by: Titus Rwantare <titusr@google.com>
> > Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> > include/hw/i3c/i3c.h | 277 ++++++++++++++++++
> > hw/i3c/core.c | 647
> +++++++++++++++++++++++++++++++++++++++++++
> > hw/i3c/meson.build | 1 +
> > hw/i3c/trace-events | 16 ++
> > 4 files changed, 941 insertions(+)
> > create mode 100644 include/hw/i3c/i3c.h create mode 100644
> > hw/i3c/core.c
> >
>
> ...
>
> > +
> > +int i3c_send_byte(I3CBus *bus, uint8_t data) {
> > + /*
> > + * Ignored, the caller can determine how many were sent based on if
> this was
> > + * ACKed/NACKed.
> > + */
> > + uint32_t num_sent;
>
> num_sent is uninitialized here. Even though i3c_send_byte ignores it after the
> call, it gets passed by pointer into i3c_send() where it is used in the
> trace_i3c_send() call.
> If the send callback does not write *num_sent (as is the case with
> mock_i3c_target_tx, refer my prev comment for patch 18/22), the trace might
> show garbage. Initializing it to zero might reduce the confusion
>

Thanks for your review and suggestion.
I send v1 here, https://patchwork.kernel.org/project/qemu-devel/list/?series=1059757 and ready for review.

Thanks,
Jamin

> > + return i3c_send(bus, &data, 1, &num_sent); }
> > +
> > +int i3c_send(I3CBus *bus, const uint8_t *data, uint32_t num_to_send,
> > + uint32_t *num_sent)
> > +{
> > + I3CTargetClass *tc;
> > + I3CTarget *t;
> > + I3CNode *node;
> > + int ret = 0;
> > +
> > + /* If this message is a broadcast and no CCC has been found, grab it. */
> > + if (bus->broadcast && !bus->in_ccc) {
> > + bus->ccc = *data;
> > + bus->in_ccc = true;
> > + /*
> > + * We need to keep track if we're currently in ENTDAA.
> > + * On any other CCC, the CCC is over on a RESTART or STOP, but
> ENTDAA
> > + * is only over on a STOP.
> > + */
> > + if (bus->ccc == I3C_CCC_ENTDAA) {
> > + bus->in_entdaa = true;
> > + }
> > + }
> > +
> > + QLIST_FOREACH(node, &bus->current_devs, next) {
> > + t = node->target;
> > + tc = I3C_TARGET_GET_CLASS(t);
> > + if (bus->in_ccc) {
> > + if (!tc->handle_ccc_write) {
> > + ret = -1;
> > + continue;
> > + }
> > + ret = i3c_target_handle_ccc_write(t, data, num_to_send,
> num_sent);
> > + /* Targets should only NACK on a direct CCC. */
> > + if (ret && !CCC_IS_DIRECT(bus->ccc)) {
> > + ret = 0;
> > + }
> > + } else {
> > + if (tc->send) {
> > + ret = ret || tc->send(t, data, num_to_send, num_sent);
> > + } else {
> > + ret = -1;
> > + }
> > + }
> > + }
> > +
> > + trace_i3c_send(*num_sent, num_to_send, ret == 0);
>
> This trace might print garbage if the target send callback doesnt set it
>
> > +
> > + return ret ? -1 : 0;
> > +}
> > +
>
> Thanks
> Jithu