From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jiri Pirko Subject: Re: [PATCH] net: team: expose sysfs attributes for each team option Date: Wed, 19 Nov 2014 13:41:49 +0100 Message-ID: <20141119124149.GD1926@nanopsycho.orion> References: <93856491220bda9aede41a9571fb1ac1.squirrel@www.codeaurora.org> <20141117130201.GB1872@nanopsycho.orion> <03067d01f18e264b30f86d0307d11b07.squirrel@www.codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org To: Hamad Kadmany Return-path: Received: from mail-wi0-f182.google.com ([209.85.212.182]:61328 "EHLO mail-wi0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754717AbaKSMlw (ORCPT ); Wed, 19 Nov 2014 07:41:52 -0500 Received: by mail-wi0-f182.google.com with SMTP id h11so1698892wiw.3 for ; Wed, 19 Nov 2014 04:41:51 -0800 (PST) Content-Disposition: inline In-Reply-To: <03067d01f18e264b30f86d0307d11b07.squirrel@www.codeaurora.org> Sender: netdev-owner@vger.kernel.org List-ID: Wed, Nov 19, 2014 at 01:28:19PM CET, hkadmany@codeaurora.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. btw how about libteam? Does that poses the same constraints? > >This is the reason for the this change, to have at least the team options exposed through sysfs. > > >> >>> >>>Signed-off-by: Hamad Kadmany >>>--- >>> 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 >>> >> > >