Date:   Wed, 9 Oct 2019 20:53:51 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jiri Pirko <jiri@...nulli.us>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, ayal@...lanox.com,
        moshe@...lanox.com, eranbe@...lanox.com, mlxsw@...lanox.com
Subject: Re: [patch net-next 3/4] netdevsim: implement couple of testing
 devlink health reporters

On Wed,  9 Oct 2019 13:04:44 +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@...lanox.com>
> 
> Implement "empty" and "dummy" reporters. The first one is really simple
> and does nothing. The other one has debugfs files to trigger breakage
> and it is able to do recovery. The ops also implement dummy fmsg
> content.
> 
> Signed-off-by: Jiri Pirko <jiri@...lanox.com>

> diff --git a/drivers/net/netdevsim/health.c b/drivers/net/netdevsim/health.c
> new file mode 100644
> index 000000000000..088ae8fd89fc
> --- /dev/null
> +++ b/drivers/net/netdevsim/health.c

> +static int
> +nsim_dev_dummy_reporter_recover(struct devlink_health_reporter *reporter,
> +				void *priv_ctx,
> +				struct netlink_ext_ack *extack)
> +{
> +	struct nsim_dev_health *health = devlink_health_reporter_priv(reporter);
> +	struct nsim_dev_dummy_reporter_ctx *ctx = priv_ctx;
> +
> +	if (health->fail_recover) {
> +		/* For testing purposes, user set debugfs fail_recover
> +		 * value to true. Fail right away.
> +		 */
> +		NL_SET_ERR_MSG_MOD(extack, "User setup the recover to fail for testing purposes");
> +		return -EINVAL;
> +	}
> +	if (ctx) {
> +		health->recovered_break_msg = kstrdup(ctx->break_msg,

Can't there already be a message there? wouldn't this leak it?

> +						      GFP_KERNEL);
> +		if (!health->recovered_break_msg)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}

> +static ssize_t nsim_dev_health_break_write(struct file *file,
> +					   const char __user *data,
> +					   size_t count, loff_t *ppos)
> +{
> +	struct nsim_dev_health *health = file->private_data;
> +	struct nsim_dev_dummy_reporter_ctx ctx;
> +	char *break_msg;
> +	int err;
> +
> +	break_msg = kmalloc(count + 1, GFP_KERNEL);
> +	if (!break_msg)
> +		return -ENOMEM;
> +
> +	if (copy_from_user(break_msg, data, count)) {
> +		err = -EFAULT;
> +		goto out;
> +	}
> +	break_msg[count] = '\0';
> +	if (break_msg[count - 1] == '\n')
> +		break_msg[count - 1] = '\0';
> +
> +	ctx.break_msg = break_msg;
> +	err = devlink_health_report(health->dummy_reporter, break_msg, &ctx);
> +	if (err)
> +		goto out;
> +
> +out:
> +	kfree(break_msg);
> +	return err ? err : count;

nit: ?: works here, like you used below

> +}
> +
> +static const struct file_operations nsim_dev_health_break_fops = {
> +	.open = simple_open,
> +	.write = nsim_dev_health_break_write,
> +	.llseek = generic_file_llseek,
> +};
> +
> +int nsim_dev_health_init(struct nsim_dev *nsim_dev, struct devlink *devlink)
> +{
> +	struct nsim_dev_health *health = &nsim_dev->health;
> +	int err;
> +
> +	health->empty_reporter =
> +		devlink_health_reporter_create(devlink,
> +					       &nsim_dev_empty_reporter_ops,
> +					       0, false, health);
> +	if (IS_ERR(health->empty_reporter))
> +		return PTR_ERR(health->empty_reporter);
> +
> +	health->dummy_reporter =
> +		devlink_health_reporter_create(devlink,
> +					       &nsim_dev_dummy_reporter_ops,
> +					       0, false, health);
> +	if (IS_ERR(health->dummy_reporter)) {
> +		err = PTR_ERR(health->dummy_reporter);
> +		goto err_empty_reporter_destroy;
> +	}
> +
> +	health->ddir = debugfs_create_dir("health", nsim_dev->ddir);
> +	if (IS_ERR_OR_NULL(health->ddir))
> +		return PTR_ERR_OR_ZERO(health->ddir) ?: -EINVAL;

goto err_dummy_reporter_destroy?

> +	health->recovered_break_msg = NULL;
> +	debugfs_create_file("break_health", 0200, health->ddir, health,
> +			    &nsim_dev_health_break_fops);
> +	health->binary_len = 16;
> +	debugfs_create_u32("binary_len", 0600, health->ddir,
> +			   &health->binary_len);
> +	health->fail_recover = false;
> +	debugfs_create_bool("fail_recover", 0600, health->ddir,
> +			    &health->fail_recover);
> +	return 0;
> +
> +err_empty_reporter_destroy:
> +	devlink_health_reporter_destroy(health->empty_reporter);
> +	return err;
> +}
> +
> +void nsim_dev_health_exit(struct nsim_dev *nsim_dev)
> +{
> +	struct nsim_dev_health *health = &nsim_dev->health;
> +
> +	debugfs_remove_recursive(health->ddir);
> +	kfree(health->recovered_break_msg);
> +	devlink_health_reporter_destroy(health->dummy_reporter);
> +	devlink_health_reporter_destroy(health->empty_reporter);
> +}
> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
> index 24358385d869..657cbae50293 100644
> --- a/drivers/net/netdevsim/netdevsim.h
> +++ b/drivers/net/netdevsim/netdevsim.h
> @@ -18,6 +18,7 @@
>  #include <linux/list.h>
>  #include <linux/netdevice.h>
>  #include <linux/u64_stats_sync.h>
> +#include <linux/debugfs.h>

I don't think this include is needed? Forward declaration of 
struct dentry, should be sufficient, if needed, no?

>  #include <net/devlink.h>
>  #include <net/xdp.h>
>  
> @@ -134,6 +135,18 @@ enum nsim_resource_id {
>  	NSIM_RESOURCE_IPV6_FIB_RULES,
>  };
>  
> +struct nsim_dev_health {
> +	struct devlink_health_reporter *empty_reporter;
> +	struct devlink_health_reporter *dummy_reporter;
> +	struct dentry *ddir;
> +	char *recovered_break_msg;
> +	u32 binary_len;
> +	bool fail_recover;
> +};
> +
> +int nsim_dev_health_init(struct nsim_dev *nsim_dev, struct devlink *devlink);
> +void nsim_dev_health_exit(struct nsim_dev *nsim_dev);
> +
>  struct nsim_dev_port {
>  	struct list_head list;
>  	struct devlink_port devlink_port;
> @@ -164,6 +177,7 @@ struct nsim_dev {
>  	bool dont_allow_reload;
>  	bool fail_reload;
>  	struct devlink_region *dummy_region;
> +	struct nsim_dev_health health;
>  };
>  
>  static inline struct net *nsim_dev_net(struct nsim_dev *nsim_dev)

Powered by blists - more mailing lists