Message-ID: <20220211143815.55fb29e3@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 11 Feb 2022 14:38:15 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Matt Johnston <matt@...econstruct.com.au>,
        Wolfram Sang <wsa@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jeremy Kerr <jk@...econstruct.com.au>,
        linux-i2c@...r.kernel.org, netdev@...r.kernel.org,
        Zev Weiss <zev@...ilderbeest.net>
Subject: Re: [PATCH net-next v5 2/2] mctp i2c: MCTP I2C binding driver

On Thu, 10 Feb 2022 14:36:51 +0800 Matt Johnston wrote:
> Provides MCTP network transport over an I2C bus, as specified in
> DMTF DSP0237. All messages between nodes are sent as SMBus Block Writes.
> 
> Each I2C bus to be used for MCTP is flagged in devicetree by a
> 'mctp-controller' property on the bus node. Each flagged bus gets a
> mctpi2cX net device created based on the bus number. A
> 'mctp-i2c-controller' I2C client needs to be added under the adapter. In
> an I2C mux situation the mctp-i2c-controller node must be attached only
> to the root I2C bus. The I2C client will handle incoming I2C slave block
> write data for subordinate busses as well as its own bus.
> 
> In configurations without devicetree a driver instance can be attached
> to a bus using the I2C slave new_device mechanism.
> 
> The MCTP core will hold/release the MCTP I2C device while responses
> are pending (a 6 second timeout or once a socket is closed, response
> received etc). While held the MCTP I2C driver will lock the I2C bus so
> that the correct I2C mux remains selected while responses are received.
> 
> (Ideally we would just lock the mux to keep the current bus selected for
> the response rather than a full I2C bus lock, but that isn't exposed in
> the I2C mux API)
> 
> Signed-off-by: Matt Johnston <matt@...econstruct.com.au>
> Signed-off-by: Jeremy Kerr <jk@...econstruct.com.au>

The i2c stuff looks quite unfamiliar, can we can an ack for that?
Does it look sane to you, Wolfram?

>  menu "MCTP Device Drivers"
>  
> +

spurious

>  config MCTP_SERIAL
>  	tristate "MCTP serial transport"
>  	depends on TTY

> +static int mctp_i2c_add_netdev(struct mctp_i2c_client *mcli,
> +			       struct i2c_adapter *adap)
> +{
> +	unsigned long flags;
> +	struct mctp_i2c_dev *midev = NULL;
> +	struct net_device *ndev = NULL;
> +	struct i2c_adapter *root;
> +	char namebuf[30];
> +	int rc;
> +
> +	root = mux_root_adapter(adap);
> +	if (root != mcli->client->adapter) {
> +		dev_err(&mcli->client->dev,
> +			"I2C adapter %s is not a child bus of %s",
> +			mcli->client->adapter->name, root->name);
> +		return -EINVAL;
> +	}
> +
> +	WARN_ON(!mutex_is_locked(&mi_driver_state.lock));
> +	snprintf(namebuf, sizeof(namebuf), "mctpi2c%d", adap->nr);
> +	ndev = alloc_netdev(sizeof(*midev), namebuf, NET_NAME_ENUM, mctp_i2c_net_setup);
> +	if (!ndev) {
> +		dev_err(&mcli->client->dev, "%s alloc netdev failed\n", __func__);
> +		rc = -ENOMEM;
> +		goto err;
> +	}
> +	dev_net_set(ndev, current->nsproxy->net_ns);
> +	SET_NETDEV_DEV(ndev, &adap->dev);
> +	dev_addr_set(ndev, &mcli->lladdr);
> +
> +	midev = netdev_priv(ndev);
> +	skb_queue_head_init(&midev->tx_queue);
> +	INIT_LIST_HEAD(&midev->list);
> +	midev->adapter = adap;
> +	midev->client = mcli;
> +	spin_lock_init(&midev->flow_lock);
> +	midev->i2c_lock_count = 0;
> +	midev->release_count = 0;
> +	/* Hold references */
> +	get_device(&midev->adapter->dev);
> +	get_device(&midev->client->client->dev);
> +	midev->ndev = ndev;
> +	init_waitqueue_head(&midev->tx_wq);
> +	midev->tx_thread = kthread_create(mctp_i2c_tx_thread, midev,
> +					  "%s/tx", namebuf);
> +	if (IS_ERR_OR_NULL(midev->tx_thread)) {
> +		rc = -ENOMEM;
> +		goto err_free;
> +	}
> +
> +	rc = mctp_register_netdev(ndev, &mctp_i2c_mctp_ops);
> +	if (rc < 0) {
> +		dev_err(&mcli->client->dev,
> +			"%s register netdev \"%s\" failed %d\n", __func__,
> +			ndev->name, rc);
> +		goto err_stop_kthread;
> +	}
> +	spin_lock_irqsave(&mcli->curr_lock, flags);
> +	list_add(&midev->list, &mcli->devs);
> +	// Select a device by default
> +	if (!mcli->sel)
> +		__mctp_i2c_device_select(mcli, midev);
> +	spin_unlock_irqrestore(&mcli->curr_lock, flags);
> +
> +	wake_up_process(midev->tx_thread);

Simliar but inverse comment as below...

> +	return 0;
> +
> +err_stop_kthread:
> +	kthread_stop(midev->tx_thread);
> +
> +err_free:
> +	free_netdev(ndev);
> +
> +err:
> +	return rc;
> +}
> +
> +// Removes and unregisters a mctp-i2c netdev
> +static void mctp_i2c_free_netdev(struct mctp_i2c_dev *midev)
> +{
> +	struct mctp_i2c_client *mcli = midev->client;
> +	unsigned long flags;
> +
> +	netif_stop_queue(midev->ndev);
> +	kthread_stop(midev->tx_thread);
> +	skb_queue_purge(&midev->tx_queue);
> +
> +	/* Release references, used only for TX which has stopped */
> +	put_device(&midev->adapter->dev);
> +	put_device(&mcli->client->dev);
> +
> +	/* Remove it from the parent mcli */
> +	spin_lock_irqsave(&mcli->curr_lock, flags);
> +	list_del(&midev->list);
> +	if (mcli->sel == midev) {
> +		struct mctp_i2c_dev *first;
> +
> +		first = list_first_entry_or_null(&mcli->devs, struct mctp_i2c_dev, list);
> +		__mctp_i2c_device_select(mcli, first);
> +	}
> +	spin_unlock_irqrestore(&mcli->curr_lock, flags);

You're doing a lot before the unregister call, this is likely racy.
The usual flow is to unregister the netdev, then do uninit, then free.
For instance you purge the queue but someone may Tx afterwards.
needs_free_netdev is a footgun.

> +	/* Remove netdev. mctp_i2c_slave_cb() takes a dev_hold() so removing
> +	 * it now is safe. unregister_netdev() frees ndev and midev.
> +	 */
> +	mctp_unregister_netdev(midev->ndev);
> +}

> +static __init int mctp_i2c_init(void)
> +{
> +	int rc;
> +
> +	INIT_LIST_HEAD(&mi_driver_state.clients);
> +	mutex_init(&mi_driver_state.lock);

I think there are static initializers for these.

Powered by blists - more mailing lists