Re: [PATCH v5 0/5] i3c: add i3cdev module to expose i3c dev in /dev

[Sam Agazaryan]

> > [...] production BMC firmware update
> > and recovery daemons running in OpenBMC server fleets (managing Caliptra
> > Root-of-Trust and SoC recovery targets over I3C), as well as production
> > server accelerator management daemons.
>
> Okay, but they are not (obviously) use the upstream solutions for that.

Right, they run on vendor kernels carrying out-of-tree i3cdev patches
based on an earlier version of this series, so there is no upstream ABI
to stay compatible with. This series already departs from that ABI
and v6 will change it further based on the review in this thread.

> But it's still unclear to me why this can't be moved to the kernel space?
> There is no user space that communicates with upstream kernel in the way
> you described, they should have used a custom (out-of-tree) solutions, right?

It could be, and the closest in kernel fit would be an OCP recovery
i3c_driver exposing fw_upload. But pushing an image is only one part of
the spec, and an in kernel driver would have to cover all of it, not
just the subset one recovery agent happens to use:

- status and identity: PROT_CAP, DEVICE_ID, DEVICE_STATUS (with
  recovery reason codes), RECOVERY_STATUS and HW_STATUS
- reset and boot control: DEVICE_RESET with forced recovery and
  flashless boot, and multi-stage activation through RECOVERY_CTRL
- memory spaces: code, critical logging and vendor-defined CMS
  regions, through either the indirect window or the FIFO interface
- the VENDOR command and vendor-specific status, which differ per
  SoC vendor

Which of these an agent uses, when to trigger recovery, and which image
goes with each stage all vary by platform, and the BMC's userspace
already makes those decisions.

An in-kernel driver would need new sysfs ABI for this, and that ABI
would have to grow as the spec evolves: v1.1 alone added multi-stage
activation, flashless boot and the indirect FIFO interface. With a
target-scoped transfer interface, none of that needs kernel changes.
fw_upload and lib/pldmfw fit well where a kernel driver already owns
the device; here the only consumer is the BMC's management stack.

The kernel already splits things this way on I3C for MCTP: mctp-i3c
binds by DCR and provides the transport, while PLDM firmware update
runs in userspace. i3cdev could be made to bind the same way, by the
OCP recovery DCR, instead of binding to every unbound target. Outside
I3C it is the same pattern: nvme-cli does NVMe firmware download/commit
through the passthrough ioctls, and fwupd updates devices over hidraw,
usbfs and drm_dp_aux_dev.

So the kernel side stays a small mechanism consisting of target-scoped
transfers to explicitly bound devices, and the recovery policy stays
in userspace, where it can follow new spec revisions without new kernel
ABI.

> > If you mean electrical bus recovery, i3cdev doesn't perform
> > bus-level electrical recovery. It only submits normal target-addressed
> > transfers via i3c_device_do_xfers().
>
> But how it will do if the i2c device blocks the i3c bus in mixed mode?
> I mean, how will it affect the i3c transfers on the wire?

Electrically speaking, SDA and SCL are shared, so an I2C target that
holds SDA low blocks every transfer on that bus, I3C and I2C alike,
regardless of which driver issued it. The controller reports the failure
as a NACK, arbitration loss or timeout, depending on the controller.
i3c_device_do_xfers() returns the error, and i3cdev passes it back to
userspace from the ioctl.

So i3cdev behaves the same as any in kernel I3C driver on that bus
since it uses the same transfer path. Getting a stuck bus released is
up to the controller driver rather than i3cdev. For what it's worth,
section 8.4.1 of the OCP recovery spec recommends a pure I3C bus for
critical recovery paths, and for mixed buses requires validating that
I2C targets have spike filters and considering isolating or
disabling the ones that can't. I2C devices on a mixed bus are also not
allowed to stretch the clock.

> > On a mixed bus, the Linux I3C core already
> > configures the bus mode/SCL rate according to the I2C targets' LVR
> > spike-filter capabilities and serializes both paths via
> > i3c_bus_normaluse_lock(&master->bus).
>
> Good to know, thanks!

One correction to my previous statements: i3c_bus_normaluse_lock() is
a shared lock that only excludes maintenance operations such as DAA.
The I2C and I3C transfers themselves are serialized by the master
driver's own locking, which is what keeps them from being interleaved
on the wire.


Thanks,
Sam