Re: [PATCH net-next v2 1/3] netdevsim: allow two netdevsim ports to be connected

[Jakub Kicinski <kuba@kernel.org>]

On Sat,  9 Dec 2023 17:04:46 -0800 David Wei wrote:
> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index bcbc1e19edde..3e4378e9dbee 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -364,3 +364,13 @@ void nsim_bus_exit(void)
>  	driver_unregister(&nsim_driver);
>  	bus_unregister(&nsim_bus);
>  }
> +
> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id)

nit: s/get/find/ get sometimes implied taking a reference 

> +{
> +	struct nsim_bus_dev *nsim_bus_dev;

new line here, please checkpatch --strict

> +	list_for_each_entry(nsim_bus_dev, &nsim_bus_dev_list, list) {
> +		if (nsim_bus_dev->dev.id == id)
> +			return nsim_bus_dev;

You must assume some lock is being held so that you can walk the list
and return a meaningful value? :) Please figure out what caller has to
hold and add an appropriate lockdep assert here.

> +	}
> +	return NULL;
> +}

> +static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
> +				  size_t count, loff_t *ppos)
> +{
> +	struct nsim_dev_port *nsim_dev_port;
> +	struct netdevsim *peer;
> +	unsigned int id, port;
> +	char buf[23];
> +	ssize_t len;
> +
> +	nsim_dev_port = file->private_data;
> +	rcu_read_lock();
> +	peer = rcu_dereference(nsim_dev_port->ns->peer);
> +	if (!peer) {
> +		len = scnprintf(buf, sizeof(buf), "\n");

Why not return 0?

> +		goto out;
> +	}
> +
> +	id = peer->nsim_bus_dev->dev.id;
> +	port = peer->nsim_dev_port->port_index;
> +	len = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
> +
> +out:
> +	rcu_read_unlock();
> +	return simple_read_from_buffer(data, count, ppos, buf, len);
> +}

> @@ -417,3 +418,5 @@ struct nsim_bus_dev {
>  
>  int nsim_bus_init(void);
>  void nsim_bus_exit(void);
> +
> +struct nsim_bus_dev *nsim_bus_dev_get(unsigned int id);

nit: let this go before the module init/exit funcs, 3 lines up