Re: [patch net-next 3/4] netdevsim: implement couple of testing devlink health reporters

[Jiri Pirko <jiri@resnulli.us>]

Thu, Oct 10, 2019 at 05:53:51AM CEST, jakub.kicinski@netronome.com wrote:
>On Wed,  9 Oct 2019 13:04:44 +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@mellanox.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@mellanox.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?

It would. Will fix.

>
>> +						      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

Ok.


>
>> +}
>> +
>> +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?

Right.


>
>> +	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?

Right.


>
>>  #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)
>