Re: [patch net-next RFC 08/10] netdevsim: create devlink line card object and implement provisioning

[Ido Schimmel <idosch@idosch.org>]

On Wed, Jan 13, 2021 at 01:12:20PM +0100, Jiri Pirko wrote:
> @@ -977,6 +1012,9 @@ static int __nsim_dev_port_add(struct nsim_dev *nsim_dev,
>  	memcpy(attrs.switch_id.id, nsim_dev->switch_id.id, nsim_dev->switch_id.id_len);
>  	attrs.switch_id.id_len = nsim_dev->switch_id.id_len;
>  	devlink_port_attrs_set(devlink_port, &attrs);
> +	if (nsim_dev_linecard)
> +		devlink_port_linecard_set(devlink_port,
> +					  nsim_dev_linecard->devlink_linecard);

Should be fold into devlink_port_attrs_set()

>  	err = devlink_port_register(priv_to_devlink(nsim_dev), devlink_port,
>  				    nsim_dev_port->port_index);
>  	if (err)
> @@ -1053,10 +1091,88 @@ static int nsim_dev_port_add_all(struct nsim_dev *nsim_dev,
>  	return err;
>  }
>  
> +static void nsim_dev_linecard_provision_work(struct work_struct *work)
> +{
> +	struct nsim_dev_linecard *nsim_dev_linecard;
> +	struct nsim_bus_dev *nsim_bus_dev;
> +	int err;
> +	int i;
> +
> +	nsim_dev_linecard = container_of(work, struct nsim_dev_linecard,
> +					 provision_work);
> +
> +	nsim_bus_dev = nsim_dev_linecard->nsim_dev->nsim_bus_dev;
> +	for (i = 0; i < nsim_dev_linecard_port_count(nsim_dev_linecard); i++) {
> +		err = nsim_dev_port_add(nsim_bus_dev, nsim_dev_linecard, i);
> +		if (err)
> +			goto err_port_del_all;
> +	}
> +	nsim_dev_linecard->provisioned = true;
> +	devlink_linecard_provision_set(nsim_dev_linecard->devlink_linecard,
> +				       nsim_dev_linecard->type_index);
> +	return;
> +
> +err_port_del_all:
> +	for (i--; i >= 0; i--)
> +		nsim_dev_port_del(nsim_bus_dev, nsim_dev_linecard, i);
> +	devlink_linecard_provision_clear(nsim_dev_linecard->devlink_linecard);
> +}
> +
> +static int nsim_dev_linecard_provision(struct devlink_linecard *linecard,
> +				       void *priv, u32 type_index,
> +				       struct netlink_ext_ack *extack)
> +{
> +	struct nsim_dev_linecard *nsim_dev_linecard = priv;
> +
> +	nsim_dev_linecard->type_index = type_index;
> +	INIT_WORK(&nsim_dev_linecard->provision_work,
> +		  nsim_dev_linecard_provision_work);
> +	schedule_work(&nsim_dev_linecard->provision_work);
> +
> +	return 0;
> +}
> +
> +static void nsim_dev_linecard_unprovision_work(struct work_struct *work)
> +{
> +	struct nsim_dev_linecard *nsim_dev_linecard;
> +	struct nsim_bus_dev *nsim_bus_dev;
> +	int i;
> +
> +	nsim_dev_linecard = container_of(work, struct nsim_dev_linecard,
> +					 provision_work);
> +
> +	nsim_bus_dev = nsim_dev_linecard->nsim_dev->nsim_bus_dev;
> +	nsim_dev_linecard->provisioned = false;
> +	devlink_linecard_provision_clear(nsim_dev_linecard->devlink_linecard);
> +	for (i = 0; i < nsim_dev_linecard_port_count(nsim_dev_linecard); i++)
> +		nsim_dev_port_del(nsim_bus_dev, nsim_dev_linecard, i);
> +}
> +
> +static int nsim_dev_linecard_unprovision(struct devlink_linecard *linecard,
> +					 void *priv,
> +					 struct netlink_ext_ack *extack)
> +{
> +	struct nsim_dev_linecard *nsim_dev_linecard = priv;
> +
> +	INIT_WORK(&nsim_dev_linecard->provision_work,
> +		  nsim_dev_linecard_unprovision_work);
> +	schedule_work(&nsim_dev_linecard->provision_work);
> +
> +	return 0;
> +}
> +
> +static const struct devlink_linecard_ops nsim_dev_linecard_ops = {
> +	.supported_types = nsim_dev_linecard_supported_types,
> +	.supported_types_count = ARRAY_SIZE(nsim_dev_linecard_supported_types),
> +	.provision = nsim_dev_linecard_provision,
> +	.unprovision = nsim_dev_linecard_unprovision,
> +};
> +
>  static int __nsim_dev_linecard_add(struct nsim_dev *nsim_dev,
>  				   unsigned int linecard_index)
>  {
>  	struct nsim_dev_linecard *nsim_dev_linecard;
> +	struct devlink_linecard *devlink_linecard;
>  	int err;
>  
>  	nsim_dev_linecard = kzalloc(sizeof(*nsim_dev_linecard), GFP_KERNEL);
> @@ -1066,14 +1182,27 @@ static int __nsim_dev_linecard_add(struct nsim_dev *nsim_dev,
>  	nsim_dev_linecard->linecard_index = linecard_index;
>  	INIT_LIST_HEAD(&nsim_dev_linecard->port_list);
>  
> +	devlink_linecard = devlink_linecard_create(priv_to_devlink(nsim_dev),
> +						   linecard_index,
> +						   &nsim_dev_linecard_ops,
> +						   nsim_dev_linecard);
> +	if (IS_ERR(devlink_linecard)) {
> +		err = PTR_ERR(devlink_linecard);
> +		goto err_linecard_free;
> +	}
> +
> +	nsim_dev_linecard->devlink_linecard = devlink_linecard;
> +
>  	err = nsim_dev_linecard_debugfs_init(nsim_dev, nsim_dev_linecard);
>  	if (err)
> -		goto err_linecard_free;
> +		goto err_dl_linecard_destroy;
>  
>  	list_add(&nsim_dev_linecard->list, &nsim_dev->linecard_list);
>  
>  	return 0;
>  
> +err_dl_linecard_destroy:
> +	devlink_linecard_destroy(devlink_linecard);
>  err_linecard_free:
>  	kfree(nsim_dev_linecard);
>  	return err;
> @@ -1081,8 +1210,12 @@ static int __nsim_dev_linecard_add(struct nsim_dev *nsim_dev,
>  
>  static void __nsim_dev_linecard_del(struct nsim_dev_linecard *nsim_dev_linecard)
>  {
> +	struct devlink_linecard *devlink_linecard =
> +					nsim_dev_linecard->devlink_linecard;
> +
>  	list_del(&nsim_dev_linecard->list);
>  	nsim_dev_linecard_debugfs_exit(nsim_dev_linecard);

What about the delayed work? I believe it can run while you are
destroying the linecard, so cancel_delayed_work_sync() is needed

> +	devlink_linecard_destroy(devlink_linecard);
>  	kfree(nsim_dev_linecard);
>  }
>  
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 88b61b9390bf..ab217b361416 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -196,10 +196,14 @@ struct nsim_dev;
>  
>  struct nsim_dev_linecard {
>  	struct list_head list;
> +	struct devlink_linecard *devlink_linecard;
>  	struct nsim_dev *nsim_dev;
>  	struct list_head port_list;
>  	unsigned int linecard_index;
>  	struct dentry *ddir;
> +	bool provisioned;
> +	u32 type_index;
> +	struct work_struct provision_work;
>  };
>  
>  struct nsim_dev {
> -- 
> 2.26.2
>