Re: [PATCH net-next v2] mctp i2c: add sysfs control for mux child netdevs

[Matt Johnston <matt@codeconstruct.com.au>]

Hi Kenny,

On Tue, 2026-10-27 at 08:58 +0000, Kenny Hsieh wrote:
> Hi Andi, Peter, Matt, Jeremy,
> 
> > - [High] The store-side guard against adapters being deleted does not
> >   close the race it claims to close.
> 
> > - [High] mctp_i2c_remove() runs mctp_i2c_remove_sysfs_all() while the
> >   client is still on driver_clients.
> 
> > - [Medium] mctp_i2c_probe() now releases driver_clients_lock after
> >   list_add() and before i2c_for_each_dev(mcli,...
> 
> > - [Medium] The new ABI lives on i2c adapter devices, which the I2C core
> >   owns, but the mctp-i2c driver attaches it with sysfs_update_group() on...
> 
> These races come back to the last point: mctp-i2c bolts the attribute
> onto adapters the i2c core owns. Before I respin I'd like to check which
> way to go.
> 
> I can keep it in mctp-i2c and fix the races there, mostly by reordering
> notify_del() and remove(), and adding the attributes after the probe
> walk so it can hold driver_clients_lock again. That works, but it's more
> bookkeeping and there's still no uevent for the attribute.
> 
> The other option is to declare it on i2c_adapter_type in the core and
> have mctp-i2c register the show/store callbacks. That's cleaner, the
> driver core handles the lifetime and most of the races go away, but it
> adds an i2c API with mctp-i2c as its only user.
> 
> I prefer to keep it in mctp-i2c, but happy to go either way.

I agree it would be cleaner to avoid having mctp-i2c create items in i2c core
sysfs space.

An alternative could be to create sysfs attributes under the mctp-i2c device
instance itself (the i2c slave).

Mux buses could have mctp-i2c netdevs instantiated with

echo "i2c-6" > /sys/bus/i2c/devices/2-1060/add_mctp_controller

(where i2c-6 is a child mux bus of i2c-2, and i2c-2 0x60 is a mctp-i2c i2c
device).

"echo i2c-6 > .../2-1060/remove_mctp_controller" would remove the mctp-i2c
netdev.
Setting a root node as an mctp controller would involve writing its own name
to the attribute file.

How does that look to you?

Cheers,
Matt


> > - [High] When mctp_register_netdev() fails, mctp_i2c_add_netdev()
> >   (drivers/net/mctp/mctp-i2c.c) now always sets `midev = NULL`, so the...
> 
> > - [Medium] In the baseline, mctp_i2c_add_netdev() has a real double
> >   teardown.
> 
> This is the known double free, tracked in a separate patch on net, and
> v3 drops the midev = NULL change:
> https://lore.kernel.org/netdev/20261027053715.1623087-1-kenny_hsieh@jabil.com/
> 
> > - [Medium] The new ABI document Documentation/ABI/testing/sysfs-
> >   bus-i2c-devices-mctp says "The attribute appears only on mux child...
> 
> > - [Medium] mctp_i2c_new_client() now calls `write_pnet(&mcli->net,
> >   get_net(current->nsproxy->net_ns))` before i2c_slave_register().
> 
> > - [Low] The commit message says "a rebind reattaches it reading 0".
> 
> > - [Low] The commit message gives "an adapter that the devicetree cannot
> >   describe at all, such as a USB-attached one" as a use case.
> 
> Will fix all four in v3.
> 
> > - [Medium] Writing 0 to mctp_controller removes a channel's netdev
> >   through mctp_i2c_remove_netdev() -> mctp_i2c_unregister() ->...
> 
> The sel reselection on removal isn't new, removing a mux adapter already
> goes down that path. I'd rather leave that for a separate patch and just
> fix the ABI wording here.
> 
> pw-bot: cr