Date:	Mon, 17 Nov 2014 14:02:01 +0100
From:	Jiri Pirko <jiri@...nulli.us>
To:	Hamad Kadmany <hkadmany@...eaurora.org>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH] net: team: expose sysfs attributes for each team option

Mon, Nov 17, 2014 at 11:11:31AM CET, hkadmany@...eaurora.org wrote:
>Current code provides only netlink API for user space
>to read/write options. Exposing sysfs API is useful for
>systems that don't have the required netlink
>user space support.
>
>Upon registration of team option, corresponding
>sysfs attribute is created.

Nak.

I don't like this patch. The plan for team was keep things simple and to
have single userspace api using netlink, as that is the correct way to
deal with things. Sysfs for this use-case is not. Please, use netlink api.

>
>Signed-off-by: Hamad Kadmany <hkadmany@...eaurora.org>
>---
> drivers/net/team/team.c | 282 ++++++++++++++++++++++++++++++++++++++++++++++--
> include/linux/if_team.h |   3 +
> 2 files changed, 278 insertions(+), 7 deletions(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index 1222229..afd2f8f 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -110,8 +110,198 @@ struct team_option_inst { /* One for each option instance */
> 	struct team_option_inst_info info;
> 	bool changed;
> 	bool removed;
>+	bool dev_attr_file_exist;
>+	struct device_attribute dev_attr; /* corresponding sysfs attribute */
> };
>
>+static struct attribute *team_sysfs_attrs[] = {
>+	NULL,
>+};
>+
>+static struct attribute_group team_sysfs_attr_group = {
>+	.attrs = team_sysfs_attrs,
>+};
>+
>+/* Device attributes (sysfs) */
>+
>+static ssize_t show_attribute(struct device *dev,
>+			      struct device_attribute *attr,
>+			      char *buf)
>+{
>+	struct team *team = dev_get_drvdata(dev);
>+	struct team_option_inst *opt_inst;
>+	ssize_t ret;
>+	struct team_option *option;
>+	struct team_gsetter_ctx ctx;
>+
>+	if (mutex_lock_interruptible(&team->lock))
>+		return -ERESTARTSYS;
>+
>+	opt_inst = container_of(attr, struct team_option_inst, dev_attr);
>+	option = opt_inst->option;
>+	if (!option->getter) {
>+		ret = -EOPNOTSUPP;
>+		netdev_err(team->dev,
>+			   "Option %s is write only\n", attr->attr.name);
>+		goto exit;
>+	}
>+
>+	ctx.info = &opt_inst->info;
>+	/* let the option getter do its job */
>+	ret = option->getter(team, &ctx);
>+	if (ret)
>+		goto exit;
>+
>+	/* translate option's output into sysfs output */
>+	switch (option->type) {
>+	case TEAM_OPTION_TYPE_U32:
>+		ret = scnprintf(buf, PAGE_SIZE, "%u\n", ctx.data.u32_val);
>+		break;
>+	case TEAM_OPTION_TYPE_STRING:
>+		ret = scnprintf(buf, PAGE_SIZE, "%s\n", ctx.data.str_val);
>+		break;
>+	case TEAM_OPTION_TYPE_BINARY:
>+		if (ctx.data.bin_val.len > PAGE_SIZE) {
>+			netdev_err(team->dev,
>+				   "Option output is too long (%d)\n",
>+				   ctx.data.bin_val.len);
>+			break;
>+		}
>+
>+		memcpy(buf, ctx.data.bin_val.ptr, ctx.data.bin_val.len);
>+		ret = ctx.data.bin_val.len;
>+		break;
>+	case TEAM_OPTION_TYPE_BOOL:
>+		ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.bool_val);
>+		break;
>+	case TEAM_OPTION_TYPE_S32:
>+		ret = scnprintf(buf, PAGE_SIZE, "%d\n", ctx.data.s32_val);
>+		break;
>+	default:
>+		BUG();
>+	}
>+
>+exit:
>+	mutex_unlock(&team->lock);
>+
>+	return ret;
>+}
>+
>+static int team_nl_send_event_options_get(struct team *team,
>+					  struct list_head *sel_opt_inst_list);
>+
>+static ssize_t set_attribute(struct device *dev,
>+			     struct device_attribute *attr,
>+			     const char *buf, size_t count)
>+{
>+	struct team_option_inst *opt_inst;
>+	struct team *team = dev_get_drvdata(dev);
>+	int err = 0;
>+	struct team_option *option = NULL;
>+	struct team_gsetter_ctx ctx;
>+
>+	LIST_HEAD(opt_inst_list);
>+
>+	if (mutex_lock_interruptible(&team->lock))
>+		return -ERESTARTSYS;
>+
>+	opt_inst = container_of(attr, struct team_option_inst, dev_attr);
>+	option = opt_inst->option;
>+	if (!option->setter) {
>+		netdev_err(team->dev,
>+			   "Option %s is read only\n", attr->attr.name);
>+		err = -EOPNOTSUPP;
>+		goto exit;
>+	}
>+
>+	ctx.info = &opt_inst->info;
>+
>+	/* translate sysfs input into option's input */
>+	switch (option->type) {
>+	case TEAM_OPTION_TYPE_U32:
>+		err = kstrtoint(buf, 0, &ctx.data.u32_val);
>+		break;
>+	case TEAM_OPTION_TYPE_STRING:
>+		if (count > TEAM_STRING_MAX_LEN) {
>+			netdev_err(team->dev,
>+				   "Input buffer too long (%zu)\n", count);
>+			err = -EINVAL;
>+			break;
>+		}
>+		ctx.data.str_val = buf;
>+		break;
>+	case TEAM_OPTION_TYPE_BINARY:
>+		ctx.data.bin_val.len = count;
>+		ctx.data.bin_val.ptr = buf;
>+		break;
>+	case TEAM_OPTION_TYPE_BOOL:
>+		err = strtobool(buf, &ctx.data.bool_val);
>+		break;
>+	case TEAM_OPTION_TYPE_S32:
>+		err = kstrtoint(buf, 0, &ctx.data.s32_val);
>+		break;
>+	default:
>+		BUG();
>+	}
>+
>+	if (err) {
>+		netdev_err(team->dev, "Failed to translate input buffer\n");
>+		goto exit;
>+	}
>+
>+	/* let the option setter do its job */
>+	err = option->setter(team, &ctx);
>+	if (err)
>+		goto exit;
>+
>+	/* propagate option changed event */
>+	opt_inst->changed = true;
>+	list_add(&opt_inst->tmp_list, &opt_inst_list);
>+	err = team_nl_send_event_options_get(team, &opt_inst_list);
>+	if (err == -ESRCH) /* no listeners, not a real error */
>+		err = 0;
>+
>+exit:
>+	mutex_unlock(&team->lock);
>+
>+	if (!err)
>+		err = count;
>+	return err;
>+}
>+
>+/* create sysfs attribute for each option that is being registered */
>+static int __team_option_add_sysfs_attr(struct team *team,
>+					struct team_option_inst *opt_inst,
>+					bool create_sysfs_file)
>+{
>+	int err = 0;
>+	struct device_attribute *new_dev_attr = &opt_inst->dev_attr;
>+
>+	new_dev_attr->attr.name = opt_inst->option->name;
>+	new_dev_attr->attr.mode = S_IRUGO | S_IWUSR;
>+	new_dev_attr->show = show_attribute;
>+	new_dev_attr->store = set_attribute;
>+
>+	if (create_sysfs_file) {
>+		err = sysfs_create_file(&team->dev->dev.kobj,
>+					&new_dev_attr->attr);
>+		if (err)
>+			netdev_err(team->dev,
>+				   "Failed to create sysfs attribute %s\n",
>+				   new_dev_attr->attr.name);
>+	}
>+
>+	return err;
>+}
>+
>+static void __team_option_del_sysfs_attr(struct team *team,
>+					 struct team_option_inst *opt_inst)
>+{
>+	if (opt_inst->dev_attr_file_exist)
>+		sysfs_remove_file(&team->dev->dev.kobj,
>+				  &opt_inst->dev_attr.attr);
>+}
>+
> static struct team_option *__team_find_option(struct team *team,
> 					      const char *opt_name)
> {
>@@ -124,8 +314,10 @@ static struct team_option *__team_find_option(struct team *team,
> 	return NULL;
> }
>
>-static void __team_option_inst_del(struct team_option_inst *opt_inst)
>+static void __team_option_inst_del(struct team *team,
>+				   struct team_option_inst *opt_inst)
> {
>+	__team_option_del_sysfs_attr(team, opt_inst);
> 	list_del(&opt_inst->list);
> 	kfree(opt_inst);
> }
>@@ -137,7 +329,7 @@ static void __team_option_inst_del_option(struct team *team,
>
> 	list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) {
> 		if (opt_inst->option == option)
>-			__team_option_inst_del(opt_inst);
>+			__team_option_inst_del(team, opt_inst);
> 	}
> }
>
>@@ -162,6 +354,7 @@ static int __team_option_inst_add(struct team *team, struct team_option *option,
> 		opt_inst->info.array_index = i;
> 		opt_inst->changed = true;
> 		opt_inst->removed = false;
>+		opt_inst->dev_attr_file_exist = false;
> 		list_add_tail(&opt_inst->list, &team->option_inst_list);
> 		if (option->init) {
> 			err = option->init(team, &opt_inst->info);
>@@ -170,6 +363,20 @@ static int __team_option_inst_add(struct team *team, struct team_option *option,
> 		}
>
> 	}
>+
>+	/* add sysfs attribute. per-port and array options are skipped */
>+	if (!option->per_port && !option->array_size) {
>+		/* create the sysfs file only if our state allows it */
>+		bool create_sysfs_file = device_is_registered(&team->dev->dev);
>+
>+		err = __team_option_add_sysfs_attr(team, opt_inst,
>+						   create_sysfs_file);
>+		if (err)
>+			return err;
>+
>+		opt_inst->dev_attr_file_exist = true;
>+	}
>+
> 	return 0;
> }
>
>@@ -218,7 +425,7 @@ static void __team_option_inst_del_port(struct team *team,
> 	list_for_each_entry_safe(opt_inst, tmp, &team->option_inst_list, list) {
> 		if (opt_inst->option->per_port &&
> 		    opt_inst->info.port == port)
>-			__team_option_inst_del(opt_inst);
>+			__team_option_inst_del(team, opt_inst);
> 	}
> }
>
>@@ -337,6 +544,51 @@ static void __team_options_unregister(struct team *team,
>
> static void __team_options_change_check(struct team *team);
>
>+static void team_attr_grp_free(struct team *team)
>+{
>+	kfree(team->attr_grp.attrs);
>+}
>+
>+/* allocate attribute group for creating sysfs for team's own options */
>+static int team_attr_grp_alloc(struct team *team)
>+{
>+	struct attribute **attributes;
>+	struct team_option_inst *opt_inst;
>+	int num_attr = 0;
>+	struct team_option *option;
>+
>+	list_for_each_entry(opt_inst, &team->option_inst_list, list) {
>+		option = opt_inst->option;
>+		/* per-port and array options currently not supported as
>+		 * sysfs attributes
>+		 */
>+		if (option->per_port || option->array_size)
>+			continue;
>+
>+		num_attr++;
>+	}
>+
>+	/* +1 for having NULL as last item in the array */
>+	attributes = kzalloc((num_attr + 1) * sizeof(*attributes), GFP_KERNEL);
>+	if (!attributes)
>+		return -ENOMEM;
>+	team->attr_grp.attrs = attributes;
>+
>+	num_attr = 0;
>+	list_for_each_entry(opt_inst, &team->option_inst_list, list) {
>+		option = opt_inst->option;
>+		/* per-port and array options currently not supported as
>+		 * sysfs attributes
>+		 */
>+		if (option->per_port || option->array_size)
>+			continue;
>+
>+		attributes[num_attr++] = &opt_inst->dev_attr.attr;
>+	}
>+
>+	return 0;
>+}
>+
> int team_options_register(struct team *team,
> 			  const struct team_option *option,
> 			  size_t option_count)
>@@ -1380,15 +1632,28 @@ static int team_init(struct net_device *dev)
>
> 	INIT_LIST_HEAD(&team->option_list);
> 	INIT_LIST_HEAD(&team->option_inst_list);
>-	err = team_options_register(team, team_options, ARRAY_SIZE(team_options));
>+
>+	err = team_options_register(team, team_options,
>+				    ARRAY_SIZE(team_options));
> 	if (err)
> 		goto err_options_register;
> 	netif_carrier_off(dev);
>
> 	team_set_lockdep_class(dev);
>
>+	/* store team context, to be used by Device attributes getter/setter */
>+	dev_set_drvdata(&dev->dev, team);
>+
>+	/* allocate and register sysfs attributes for team's own options */
>+	err = team_attr_grp_alloc(team);
>+	if (err)
>+		goto err_grp_alloc;
>+	dev->sysfs_groups[0] = &team->attr_grp;
>+
> 	return 0;
>
>+err_grp_alloc:
>+	team_options_unregister(team, team_options, ARRAY_SIZE(team_options));
> err_options_register:
> 	team_queue_override_fini(team);
> err_team_queue_override_init:
>@@ -1407,9 +1672,15 @@ static void team_uninit(struct net_device *dev)
> 	list_for_each_entry_safe(port, tmp, &team->port_list, list)
> 		team_port_del(team, port->dev);
>
>+	sysfs_remove_group(&team->dev->dev.kobj, &team->attr_grp);
>+	team_attr_grp_free(team);
>+	/* set to dummy group to avoid double free by core */
>+	dev->sysfs_groups[0] = &team_sysfs_attr_group;
>+
> 	__team_change_mode(team, NULL); /* cleanup */
> 	__team_options_unregister(team, team_options, ARRAY_SIZE(team_options));
> 	team_queue_override_fini(team);
>+
> 	mutex_unlock(&team->lock);
> }
>
>@@ -2194,9 +2465,6 @@ static int team_nl_cmd_options_get(struct sk_buff *skb, struct genl_info *info)
> 	return err;
> }
>
>-static int team_nl_send_event_options_get(struct team *team,
>-					  struct list_head *sel_opt_inst_list);
>-
> static int team_nl_cmd_options_set(struct sk_buff *skb, struct genl_info *info)
> {
> 	struct team *team;
>diff --git a/include/linux/if_team.h b/include/linux/if_team.h
>index 25b8b15..2e9fb2a 100644
>--- a/include/linux/if_team.h
>+++ b/include/linux/if_team.h
>@@ -188,6 +188,9 @@ struct team {
> 	struct list_head option_list;
> 	struct list_head option_inst_list; /* list of option instances */
>
>+	/* attribute group for registering team's own options at init */
>+	struct attribute_group attr_grp;
>+
> 	const struct team_mode *mode;
> 	struct team_mode_ops ops;
> 	bool user_carrier_enabled;
>-- 
>1.8.5.2
>-- 
>Qualcomm Israel, on behalf of Qualcomm Innovation Center, Inc.
>The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
>a Linux Foundation Collaborative Project
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists