Message-ID: <20141125153723.GG1971@nanopsycho.orion>
Date:	Tue, 25 Nov 2014 16:37:23 +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

Wed, Nov 19, 2014 at 01:40:43PM CET, jiri@...nulli.us wrote:
>Wed, Nov 19, 2014 at 01:28:19PM CET, hkadmany@...eaurora.org wrote:
>>On Mon, November 17, 2014 1:02 pm, Jiri Pirko wrote:
>>>
>>> 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.
>>
>>Using team driver requires using libnl3. In Android based platforms, libnl3 is not supported, and libnl3 license poses constraints in Android's user-space.
>>
>>This is the reason for the this change, to have at least the team options exposed through sysfs.
>
>No, that is certainly not the reason. If libnl3 does not suit you, use a
>different lib. It is in my opinion unacceptaptable to work around
>certain licence issues by adding parallel sysfs api (not to mention that
>sysfs is totally unsuitable for that).
>
>You should use existing Netlink API. That's it.

Also, using Netlink API, you get all the events to your userspace
daemon. Sysfs is just not suitable for that.

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