Re: [PATCH net-next v5 5/7] ptp: ocp: Add profile-driven I2C topology support

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
ptp: ocp: Add profile-driven I2C topology support

This adds software-node based construction of a PCA9546 mux topology plus
per-device i2c and platform bus notifiers and a delayed work item that
waits for the root adapter and instantiates the selected profile.  The
platform bus unbind notifier is meant to stop the work before the i2c
controller driver deletes its root adapter.

> diff --git a/drivers/ptp/ptp_ocp.c b/drivers/ptp/ptp_ocp.c
> index 31a4c9dfe391c..444b26a5e2a9f 100644
> --- a/drivers/ptp/ptp_ocp.c
> +++ b/drivers/ptp/ptp_ocp.c

[ ... ]

> @@ -2059,6 +2141,465 @@ ptp_ocp_read_eeprom(struct ptp_ocp *bp, unsigned int fields)
>  	return ret;
>  }
>  

[ ... ]

> +static int
> +ptp_ocp_i2c_populate_channel(struct ptp_ocp *bp, unsigned int channel)
> +{
> +	struct ptp_ocp_i2c_topology *topology = bp->i2c_topology;
> +	const struct ptp_ocp_i2c_profile *profile = topology->profile;
> +	const struct software_node *node;
> +	struct fwnode_handle *fwnode;
> +	struct i2c_adapter *adapter;
> +	unsigned int i;
> +	int err, ret = 0;
> +
> +	fwnode = software_node_fwnode(&topology->channel_nodes[channel]);
> +	adapter = i2c_get_adapter_by_fwnode(fwnode);
> +	if (!adapter)
> +		return -EAGAIN;
> +
> +	for (i = 0; i < profile->sensor_count; i++) {
> +		const struct ptp_ocp_i2c_device *sensor;
> +
> +		sensor = &profile->sensors[i];
> +		if (sensor->channel != channel)
> +			continue;
> +
> +		node = &topology->sensor_nodes[i];
> +		err = ptp_ocp_i2c_add_device(adapter, node, sensor->type,
> +					     sensor->address);
> +		if (err && !ret)
> +			ret = err;
> +	}
> +
> +	if (channel == profile->led_mux_channel) {
> +		err = ptp_ocp_i2c_add_device(adapter, &topology->led_node,
> +					     "is32fl3207",
> +					     profile->led_address);
> +		if (err && !ret)
> +			ret = err;
> +	}
> +
> +	i2c_put_adapter(adapter);
> +	return ret;
> +}

[ ... ]

> +static void
> +ptp_ocp_i2c_kick(struct ptp_ocp *bp)
> +{
> +	if (!ptp_ocp_i2c_supported(bp))
> +		return;
> +	if (!READ_ONCE(bp->i2c_resources_ready))
> +		return;
> +	if (!READ_ONCE(bp->i2c_root_present))
> +		return;
> +
> +	mod_delayed_work(system_wq, &bp->i2c_work, 1);
> +}

[Severity: High]
The commit message says the platform bus unbind notifier "prevents a new
client from being created after i2c_del_adapter() has swept the adapter
children".  Does that barrier cover the channel adapters too?

ptp_ocp_i2c_populate_channel() creates clients on the mux channel
adapters, and those adapters are deleted by the mux client's own remove
path, which never touches bp->i2c_root_present, bp->i2c_work or
bp->i2c_topology_lock:

pca954x_remove()
  pca954x_cleanup()
    i2c_mux_del_adapters()
      i2c_del_adapter()      /* channel adapter */

In i2c_del_adapter() the child sweep happens before the unbounded wait:

drivers/i2c/i2c-core-base.c:i2c_del_adapter() {
	...
	i2c_deregister_clients(adap);
	...
	init_completion(&adap->dev_released);
	device_unregister(&adap->dev);
	wait_for_completion(&adap->dev_released);
	...
}

So if the work is between i2c_get_adapter_by_fwnode() and
i2c_new_client_device() while the sweep runs, can the added client keep
the reference that i2c_adapter_dev_release() is waiting for, leaving the
unbind thread stuck in wait_for_completion() forever?

The i2c bus notifier added here only handles ADD_DEVICE, DEL_DEVICE and
BOUND_DRIVER.  BUS_NOTIFY_DEL_DEVICE for an adapter is emitted from
device_del(), which runs after i2c_deregister_clients(), so can it act as
a barrier at all?  Would handling BUS_NOTIFY_UNBIND_DRIVER for the mux
client (and taking bp->i2c_topology_lock there) close that window?

Two related windows look open as well:

The ADD_DEVICE branch calls ptp_ocp_i2c_kick(bp) from inside device_add()
of the root adapter, i.e. from the middle of i2c_register_adapter().  With
a one jiffy delay, can the work call i2c_new_client_device() on that
adapter before registration finished?  If i2c_setup_smbus_alert() then
fails, i2c_register_adapter() takes its error path which also does
put_device() plus wait_for_completion(&adap->dev_released), so a
straggling child would block it the same way.

ptp_ocp_i2c_ctrl_notifier_call() reacts only to BUS_NOTIFY_UNBIND_DRIVER.
When the i2c controller's probe fails, the driver core tears the adapter
down and emits BUS_NOTIFY_DRIVER_NOT_BOUND instead.  Should that action be
handled too?

Since ptp_ocp_i2c_retry() re-arms the work every 30 seconds indefinitely
on persistent failure, this coincidence window keeps reopening for the
lifetime of the device.

[ ... ]

> @@ -4922,8 +5465,19 @@ ptp_ocp_detach_sysfs(struct ptp_ocp *bp)
>  static void
>  ptp_ocp_detach(struct ptp_ocp *bp)
>  {
> +	struct platform_device *i2c_ctrl;
>  	int i;
>  
> +	if (bp->i2c_ctrl_notifier_registered) {
> +		bus_unregister_notifier(&platform_bus_type,
> +					&bp->i2c_ctrl_notifier);
> +		bp->i2c_ctrl_notifier_registered = false;
> +	}
> +	if (bp->i2c_notifier_registered) {
> +		bus_unregister_notifier(&i2c_bus_type, &bp->i2c_notifier);
> +		bp->i2c_notifier_registered = false;
> +	}
> +	ptp_ocp_i2c_unregister(bp);

[Severity: Medium]
Is the ordering here inverted?  The platform bus notifier is dropped
first, and the work is only quiesced later inside
ptp_ocp_i2c_unregister(), which clears i2c_root_present and calls
disable_delayed_work_sync().

Between those two points bp->i2c_resources_ready and
bp->i2c_root_present are still true and bp->i2c_work is still armed:

ptp_ocp_detach()
  bus_unregister_notifier(&platform_bus_type, &bp->i2c_ctrl_notifier);
  /* preempted here */
  ...
  ptp_ocp_i2c_unregister(bp);   /* only now is the work stopped */

A concurrent unbind of the child i2c controller takes
device_lock(&i2c_ctrl->dev), not the PCI device lock held by the detach
path, so can it reach i2c_del_adapter() in that window without
ptp_ocp_i2c_ctrl_notifier_call() ever running, which is the callback the
commit message describes as providing the required ordering?

Would calling ptp_ocp_i2c_unregister(bp) before dropping the notifiers
give the intended sequence?

[ ... ]

> +static int
> +ptp_ocp_i2c_ctrl_notifier_call(struct notifier_block *nb,
> +			       unsigned long action, void *data)
> +{
> +	struct ptp_ocp *bp = container_of(nb, struct ptp_ocp,
> +					  i2c_ctrl_notifier);
> +	struct platform_device *i2c_ctrl = READ_ONCE(bp->i2c_ctrl);
> +	struct device *dev = data;
> +
> +	if (action != BUS_NOTIFY_UNBIND_DRIVER || !i2c_ctrl ||
> +	    dev != &i2c_ctrl->dev)
> +		return 0;
> +
> +	/*
> +	 * Stop topology creation before the controller driver's remove path
> +	 * deletes its root adapter. Otherwise a newly-created mux client can
> +	 * pin that adapter after i2c_del_adapter() has swept its children.
> +	 */
> +	WRITE_ONCE(bp->i2c_root_present, false);
> +	cancel_delayed_work_sync(&bp->i2c_work);
> +	mutex_lock(&bp->i2c_topology_lock);
> +	ptp_ocp_i2c_remove_mux(bp);
> +	bp->i2c_retry_count = 0;
> +	mutex_unlock(&bp->i2c_topology_lock);
> +	sysfs_remove_link(&bp->dev.kobj, "i2c");
> +
> +	return 0;
> +}

[ ... ]