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

[Kenny Hsieh]

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.

> - [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@xxxxxxxxx/

> - [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