On Thu, Mar 21, 2019 at 02:26:02PM +0100, Christian Gromm wrote:
> This patch adds the file configfs.c to the driver directory. The file
> registers the necessary subsystems with configfs in order to move the
> driver configuration from sysfs to configfs.
>
> Signed-off-by: Christian Gromm <christian.gromm@xxxxxxxxxxxxx>
> ---
> drivers/staging/most/configfs.c | 659 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 659 insertions(+)
> create mode 100644 drivers/staging/most/configfs.c
>
> diff --git a/drivers/staging/most/configfs.c b/drivers/staging/most/configfs.c
> new file mode 100644
> index 0000000..cefce69
> --- /dev/null
> +++ b/drivers/staging/most/configfs.c
> @@ -0,0 +1,659 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * configfs.c - Implementation of configfs interface to the driver stack
> + *
> + * Copyright (C) 2013-2015 Microchip Technology Germany II GmbH & Co. KG
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/init.h>
> +#include <linux/configfs.h>
> +#include <most/core.h>
> +
> +#define MAX_LEN 30
> +
> +struct mdev_link {
> + struct config_item item;
> + int create;
> + u16 num_buffers;
> + u16 buffer_size;
> + u16 subbuffer_size;
> + u16 packets_per_xact;
> + u16 dbr_size;
> + char datatype[MAX_LEN];
> + char direction[MAX_LEN];
> + char name[MAX_LEN];
> + char mdev[MAX_LEN];
> + char channel[MAX_LEN];
> + char comp[MAX_LEN];
> + char param[MAX_LEN];
> +};
> +
> +int set_cfg_buffer_size(struct mdev_link *link)
^^^^^^^^^^^^^^^^^^^^^^^
Please run Sparse over your code. This should be static.
> +{
> + if (!link->buffer_size)
> + return -ENODATA;
> + return most_set_cfg_buffer_size(link->mdev, link->channel,
> + link->buffer_size);
> +}
> +
> +int set_cfg_subbuffer_size(struct mdev_link *link)
> +{
> + if (!link->subbuffer_size)
> + return -ENODATA;
> + return most_set_cfg_subbuffer_size(link->mdev, link->channel,
> + link->subbuffer_size);
> +}
> +
> +int set_cfg_dbr_size(struct mdev_link *link)
> +{
> + if (!link->dbr_size)
> + return -ENODATA;
> + return most_set_cfg_dbr_size(link->mdev, link->channel,
> + link->dbr_size);
> +}
> +
> +int set_cfg_num_buffers(struct mdev_link *link)
> +{
> + if (!link->num_buffers)
> + return -ENODATA;
> + return most_set_cfg_num_buffers(link->mdev, link->channel,
> + link->num_buffers);
> +}
> +
> +int set_cfg_packets_xact(struct mdev_link *link)
> +{
> + if (!link->packets_per_xact)
> + return -ENODATA;
> + return most_set_cfg_packets_xact(link->mdev, link->channel,
> + link->packets_per_xact);
> +}
> +
> +int set_cfg_direction(struct mdev_link *link)
> +{
> + if (!strlen(link->direction))
> + return -ENODATA;
> + return most_set_cfg_direction(link->mdev, link->channel,
> + link->direction);
> +}
> +
> +int set_cfg_datatype(struct mdev_link *link)
> +{
> + if (!strlen(link->datatype))
> + return -ENODATA;
> + return most_set_cfg_datatype(link->mdev, link->channel,
> + link->datatype);
> +}
> +
> +static int (*set_config_val[])(struct mdev_link *link) = {
> + set_cfg_buffer_size,
> + set_cfg_subbuffer_size,
> + set_cfg_dbr_size,
> + set_cfg_num_buffers,
> + set_cfg_packets_xact,
> + set_cfg_direction,
> + set_cfg_datatype,
> +};
> +
> +static inline struct mdev_link *to_mdev_link(struct config_item *item)
^^^^^^
No need to mark this as inline. The compiler is smart enough to do it
automatically.
> +{
> + return item ? container_of(item, struct mdev_link, item) : NULL;
None of the callers check the return... Just do:
return container_of(item, struct mdev_link, item);
> +}
> +
> +static ssize_t mdev_link_create_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->create);
> +}
> +
> +static ssize_t mdev_link_create_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + u16 tmp;
> + int ret;
> + int i;
> + char *p = (char *)page;
> +
> + ret = kstrtou16(p, 0, &tmp);
> + if (ret)
> + return ret;
> + if (tmp > 1)
> + return -ERANGE;
> +
> + for (i = 0; i < ARRAY_SIZE(set_config_val); i++) {
> + ret = set_config_val[i](mdev_link);
> + if (ret < 0) {
> + pr_err("Config failed\n");
This error message is not very useful.
> + return ret;
> + }
> + }
> +
> + if (!mdev_link->mdev || !mdev_link->channel || !mdev_link->comp) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
These are arrays, not pointers. Remove. We checked already.
> + pr_err("Config parameters incomplete\n");
> + return -EIO;
> + }
> + if (tmp)
> + ret = most_add_link(mdev_link->mdev, mdev_link->channel,
> + mdev_link->comp, mdev_link->name,
> + mdev_link->param);
> + else
> + ret = most_remove_link(mdev_link->mdev, mdev_link->channel,
> + mdev_link->comp);
> + if (ret)
> + return ret;
> + mdev_link->create = tmp;
> + return count;
> +}
> +
> +static ssize_t mdev_link_direction_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->direction);
Can you use snprintf()? It's slightly tricky for static analysis tools
to verify that ->direction is NUL terminated properly.
> +}
> +
> +static ssize_t mdev_link_direction_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *buf = (char *)page;
^^^^^^^^^^^^^^^^^^^^^^^^
No need for the "buf" variable. Just rename page, or use page as-is.
> +
> + if (strcmp(buf, "dir_rx\n") && strcmp(buf, "rx\n") &&
> + strcmp(buf, "dir_tx\n") && strcmp(buf, "tx\n"))
> + return -EINVAL;
> + strcpy(mdev_link->direction, buf);
> + return count;
> +}
> +
> +static ssize_t mdev_link_datatype_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->datatype);
%s goes to snprintf(), please.
> +}
> +
> +static ssize_t mdev_link_datatype_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *buf = (char *)page;
> +
> + if (strcmp(buf, "control\n") && strcmp(buf, "async\n") &&
> + strcmp(buf, "sync\n") && strcmp(buf, "isoc\n") &&
> + strcmp(buf, "isoc_avp\n"))
> + return -EINVAL;
> + strcpy(mdev_link->datatype, buf);
> + return count;
> +}
> +
> +static ssize_t mdev_link_mdev_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->mdev);
> +}
> +
> +static ssize_t mdev_link_mdev_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> +
> + strcpy(mdev_link->mdev, p);
^^^^^^^^^^^^^^^^^^^^^^^^^^
Buffer overflow. PAGE_SIZE of text can't fit in 30 chars.
> + return count;
> +}
> +
> +static ssize_t mdev_link_channel_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->channel);
> +}
> +
> +static ssize_t mdev_link_channel_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> +
> + strcpy(mdev_link->channel, p);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Buffer overflow.
> + return count;
> +}
> +
> +static ssize_t mdev_link_comp_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->comp);
> +}
> +
> +static ssize_t mdev_link_comp_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> +
> + strcpy(mdev_link->comp, p);
^^^^^^^^^^^^^^^^^^^^^^^^^^
Buffer overflow.
> +
> + return count;
> +}
> +
> +static ssize_t mdev_link_param_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%s\n", to_mdev_link(item)->param);
^^^^^^^ ^^
> +}
> +
> +static ssize_t mdev_link_param_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> +
> + strcpy(mdev_link->param, p);
^^^^^^^^^^^^^^^^^^^^^^^^^^
> +
> + return count;
> +}
> +
> +static ssize_t mdev_link_num_buffers_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->num_buffers);
> +}
> +
> +static ssize_t mdev_link_num_buffers_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
^^^^^^^
No need.
> + int ret;
> +
> + ret = kstrtou16(p, 0, &mdev_link->num_buffers);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +static ssize_t mdev_link_buffer_size_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->buffer_size);
> +}
> +
> +static ssize_t mdev_link_buffer_size_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> + int ret;
> +
> + ret = kstrtou16(p, 0, &mdev_link->buffer_size);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +static ssize_t mdev_link_subbuffer_size_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->subbuffer_size);
> +}
> +
> +static ssize_t mdev_link_subbuffer_size_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> + int ret;
> +
> + ret = kstrtou16(p, 0, &mdev_link->subbuffer_size);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +static ssize_t mdev_link_packets_per_xact_show(struct config_item *item,
> + char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->packets_per_xact);
> +}
> +
> +static ssize_t mdev_link_packets_per_xact_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> + int ret;
> +
> + ret = kstrtou16(p, 0, &mdev_link->packets_per_xact);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +static ssize_t mdev_link_dbr_size_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%d\n", to_mdev_link(item)->dbr_size);
> +}
> +
> +static ssize_t mdev_link_dbr_size_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct mdev_link *mdev_link = to_mdev_link(item);
> + char *p = (char *)page;
> + int ret;
> +
> + ret = kstrtou16(p, 0, &mdev_link->dbr_size);
> + if (ret)
> + return ret;
> + return count;
> +}
> +
> +CONFIGFS_ATTR(mdev_link_, create);
> +CONFIGFS_ATTR(mdev_link_, mdev);
> +CONFIGFS_ATTR(mdev_link_, channel);
> +CONFIGFS_ATTR(mdev_link_, comp);
> +CONFIGFS_ATTR(mdev_link_, param);
> +CONFIGFS_ATTR(mdev_link_, num_buffers);
> +CONFIGFS_ATTR(mdev_link_, buffer_size);
> +CONFIGFS_ATTR(mdev_link_, subbuffer_size);
> +CONFIGFS_ATTR(mdev_link_, packets_per_xact);
> +CONFIGFS_ATTR(mdev_link_, datatype);
> +CONFIGFS_ATTR(mdev_link_, direction);
> +CONFIGFS_ATTR(mdev_link_, dbr_size);
> +
> +static struct configfs_attribute *mdev_link_attrs[] = {
> + &mdev_link_attr_create,
> + &mdev_link_attr_mdev,
> + &mdev_link_attr_channel,
> + &mdev_link_attr_comp,
> + &mdev_link_attr_param,
> + &mdev_link_attr_num_buffers,
> + &mdev_link_attr_buffer_size,
> + &mdev_link_attr_subbuffer_size,
> + &mdev_link_attr_packets_per_xact,
> + &mdev_link_attr_datatype,
> + &mdev_link_attr_direction,
> + &mdev_link_attr_dbr_size,
> + NULL,
> +};
> +
> +static void mdev_link_release(struct config_item *item)
> +{
> + kfree(to_mdev_link(item));
> +}
> +
> +static struct configfs_item_operations mdev_link_item_ops = {
> + .release = mdev_link_release,
> +};
> +
> +static const struct config_item_type mdev_link_type = {
> + .ct_item_ops = &mdev_link_item_ops,
> + .ct_attrs = mdev_link_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +struct most_common {
> + struct config_group group;
> +};
> +
> +static inline struct most_common *to_most_common(struct config_item *item)
> +{
> + return item ? container_of(to_config_group(item),
> + struct most_common, group) : NULL;
The caller does check for NULLs, but I don't know if checking is really
required here. I suspect not.
> +}
> +
> +static struct config_item *most_common_make_item(struct config_group *group,
> + const char *name)
> +{
> + struct mdev_link *mdev_link;
> +
> + mdev_link = kzalloc(sizeof(*mdev_link), GFP_KERNEL);
> + if (!mdev_link)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&mdev_link->item, name,
> + &mdev_link_type);
> +
> + if (!strcmp(group->cg_item.ci_namebuf, "most_cdev"))
> + strcpy(mdev_link->comp, "cdev");
> + else if (!strcmp(group->cg_item.ci_namebuf, "most_net"))
> + strcpy(mdev_link->comp, "net");
> + else if (!strcmp(group->cg_item.ci_namebuf, "most_video"))
> + strcpy(mdev_link->comp, "video");
> + strcpy(mdev_link->name, name);
> + return &mdev_link->item;
> +}
> +
> +static void most_common_release(struct config_item *item)
> +{
> + kfree(to_most_common(item));
> +}
> +
> +static struct configfs_item_operations most_common_item_ops = {
> + .release = most_common_release,
> +};
> +
> +static struct configfs_group_operations most_common_group_ops = {
> + .make_item = most_common_make_item,
> +};
> +
> +static const struct config_item_type most_common_type = {
> + .ct_item_ops = &most_common_item_ops,
> + .ct_group_ops = &most_common_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct configfs_subsystem most_cdev_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "most_cdev",
> + .ci_type = &most_common_type,
> + },
> + },
> +};
> +
> +static struct configfs_subsystem most_net_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "most_net",
> + .ci_type = &most_common_type,
> + },
> + },
> +};
> +
> +static struct configfs_subsystem most_video_subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "most_video",
> + .ci_type = &most_common_type,
> + },
> + },
> +};
> +
> +struct most_snd_grp {
> + struct config_group group;
> + int create;
> + struct list_head list;
> +};
> +
> +static inline struct most_snd_grp *to_most_snd_grp(struct config_item *item)
> +{
> + return item ? container_of(to_config_group(item),
> + struct most_snd_grp, group) : NULL;
The callers don't check for NULL returns.
> +}
> +
> +static struct config_item *most_snd_grp_make_item(struct config_group *group,
> + const char *name)
> +{
> + struct mdev_link *mdev_link;
> +
> + mdev_link = kzalloc(sizeof(*mdev_link), GFP_KERNEL);
> + if (!mdev_link)
> + return ERR_PTR(-ENOMEM);
> +
> + config_item_init_type_name(&mdev_link->item, name, &mdev_link_type);
> + mdev_link->create = 0;
> + strcpy(mdev_link->name, name);
> + strcpy(mdev_link->comp, "sound");
> + return &mdev_link->item;
> +}
> +
> +static ssize_t most_snd_grp_create_show(struct config_item *item, char *page)
> +{
> + return sprintf(page, "%d\n", to_most_snd_grp(item)->create);
> +}
> +
> +static ssize_t most_snd_grp_create_store(struct config_item *item,
> + const char *page, size_t count)
> +{
> + struct most_snd_grp *snd_grp = to_most_snd_grp(item);
> + int ret = 0;
^^^^
No need to initialize.
> + u16 tmp;
> + char *p = (char *)page;
^^^^^^^
No need.
> +
> + ret = kstrtou16(p, 0, &tmp);
I would be tempting to recomend kstrtobool() which allows "Yes/no" and
on/off inputs as well.
> + if (ret)
> + return ret;
> + if (tmp > 1)
> + return -ERANGE;
> + if (tmp) {
> + ret = most_cfg_complete("sound");
> + if (ret)
> + return ret;
> + }
> + snd_grp->create = tmp;
> + return count;
> +}
> +
> +CONFIGFS_ATTR(most_snd_grp_, create);
> +
> +static struct configfs_attribute *most_snd_grp_attrs[] = {
> + &most_snd_grp_attr_create,
> + NULL,
> +};
> +
> +static void most_snd_grp_release(struct config_item *item)
> +{
> + struct most_snd_grp *group = to_most_snd_grp(item);
> +
> + list_del(&group->list);
> + kfree(group);
> +}
> +
> +static struct configfs_item_operations most_snd_grp_item_ops = {
> + .release = most_snd_grp_release,
> +};
> +
> +static struct configfs_group_operations most_snd_grp_group_ops = {
> + .make_item = most_snd_grp_make_item,
> +};
> +
> +static const struct config_item_type most_snd_grp_type = {
> + .ct_item_ops = &most_snd_grp_item_ops,
> + .ct_group_ops = &most_snd_grp_group_ops,
> + .ct_attrs = most_snd_grp_attrs,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +struct most_sound {
> + struct configfs_subsystem subsys;
> + struct list_head soundcard_list;
> +};
> +
> +static inline struct most_sound *to_most_sound(struct config_item *item)
I can't find any callers. Weird that the compiler doesn't complain.
> +{
> + return item ? container_of(to_configfs_subsystem(to_config_group(item)),
> + struct most_sound, subsys) : NULL;
> +}
> +
> +static struct config_group *most_sound_make_group(struct config_group *group,
> + const char *name)
> +{
> + struct most_snd_grp *most;
> + struct most_sound *ms = container_of(to_configfs_subsystem(group),
> + struct most_sound, subsys);
> +
> + list_for_each_entry(most, &ms->soundcard_list, list) {
> + if (!most->create) {
> + pr_info("adapter configuration still in progress.\n");
This error message is slightly off. The adapter could be disabled.
> + return ERR_PTR(-EPROTO);
> + }
> + }
> + most = kzalloc(sizeof(*most), GFP_KERNEL);
> + if (!most)
> + return ERR_PTR(-ENOMEM);
> +
> + config_group_init_type_name(&most->group, name, &most_snd_grp_type);
> + list_add_tail(&most->list, &ms->soundcard_list);
> + return &most->group;
> +}
> +
> +static struct configfs_group_operations most_sound_group_ops = {
> + .make_group = most_sound_make_group,
> +};
> +
> +static const struct config_item_type most_sound_type = {
> + .ct_group_ops = &most_sound_group_ops,
> + .ct_owner = THIS_MODULE,
> +};
> +
> +static struct most_sound most_sound_subsys = {
> + .subsys = {
> + .su_group = {
> + .cg_item = {
> + .ci_namebuf = "most_sound",
> + .ci_type = &most_sound_type,
> + },
> + },
> + },
> +};
> +
> +int most_register_configfs_subsys(struct core_component *c)
> +{
> + int ret;
> +
> + if (!strcmp(c->name, "cdev"))
> + ret = configfs_register_subsystem(&most_cdev_subsys);
> + else if (!strcmp(c->name, "net"))
> + ret = configfs_register_subsystem(&most_net_subsys);
> + else if (!strcmp(c->name, "video"))
> + ret = configfs_register_subsystem(&most_video_subsys);
> + else if (!strcmp(c->name, "sound"))
> + ret = configfs_register_subsystem(&most_sound_subsys.subsys);
> + else
> + return -ENODEV;
> +
> + if (ret) {
> + pr_err("Error %d while registering subsystem %s\n",
> + ret,
> + most_sound_subsys.subsys.su_group.cg_item.ci_namebuf);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Could you use "c->name" instead?
> + }
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(most_register_configfs_subsys);
> +
> +void most_deregister_configfs_subsys(struct core_component *c)
> +{
> + if (!strcmp(c->name, "cdev"))
> + configfs_unregister_subsystem(&most_cdev_subsys);
> + else if (!strcmp(c->name, "net"))
> + configfs_unregister_subsystem(&most_net_subsys);
> + else if (!strcmp(c->name, "video"))
> + configfs_unregister_subsystem(&most_video_subsys);
> + else if (!strcmp(c->name, "sound"))
> + configfs_unregister_subsystem(&most_sound_subsys.subsys);
> +}
> +EXPORT_SYMBOL_GPL(most_deregister_configfs_subsys);
> +
> +int __init configfs_init(void)
> +{
> + config_group_init(&most_cdev_subsys.su_group);
> + mutex_init(&most_cdev_subsys.su_mutex);
> +
> + config_group_init(&most_net_subsys.su_group);
> + mutex_init(&most_net_subsys.su_mutex);
> +
> + config_group_init(&most_video_subsys.su_group);
> + mutex_init(&most_video_subsys.su_mutex);
> +
> + config_group_init(&most_sound_subsys.subsys.su_group);
> + mutex_init(&most_sound_subsys.subsys.su_mutex);
> +
> + INIT_LIST_HEAD(&most_sound_subsys.soundcard_list);
> +
> + return 0;
> +}
> +
> +void __exit configfs_exit(void)
> +{
> +}
You shouldn't need empty functions...
regards,
dan carpenter
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
