On Thu, Sep 20, 2018 at 03:45:41PM +0300, Alexander Shishkin wrote:
> At the moment, the stm class applies a certain STP framing pattern to
> the data as it is written to the underlying STM device. In order to
> allow different framing patterns (aka protocols), this patch introduces
> the concept of STP protocol drivers, defines data structures and APIs
> for the protocol drivers to use.
>
> Signed-off-by: Alexander Shishkin <[email protected]>
> ---
> drivers/hwtracing/stm/core.c | 142 ++++++++++++++++++++++++++++++++
> drivers/hwtracing/stm/policy.c | 145 ++++++++++++++++++++++++++++++---
> drivers/hwtracing/stm/stm.h | 52 ++++++++++--
> 3 files changed, 321 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/hwtracing/stm/core.c b/drivers/hwtracing/stm/core.c
> index 7b1c549d6320..c869a30661ac 100644
> --- a/drivers/hwtracing/stm/core.c
> +++ b/drivers/hwtracing/stm/core.c
> @@ -316,11 +316,26 @@ static int stm_output_assign(struct stm_device *stm,
> unsigned int width,
> output->master = midx;
> output->channel = cidx;
> output->nr_chans = width;
> + if (stm->pdrv->output_open) {
> + void *priv = stp_policy_node_priv(policy_node);
> +
> + if (WARN_ON_ONCE(!priv))
> + goto unlock;
> +
> + /* configfs subsys mutex is held by the caller */
> + ret = stm->pdrv->output_open(priv, output);
> + if (ret)
> + goto unlock;
> + }
> +
> stm_output_claim(stm, output);
> dev_dbg(&stm->dev, "assigned %u:%u (+%u)\n", midx, cidx, width);
>
> ret = 0;
> unlock:
> + if (ret)
> + output->nr_chans = 0;
> +
> spin_unlock(&output->lock);
> spin_unlock(&stm->mc_lock);
>
> @@ -333,6 +348,8 @@ static void stm_output_free(struct stm_device *stm,
> struct stm_output *output)
> spin_lock(&output->lock);
> if (output->nr_chans)
> stm_output_disclaim(stm, output);
> + if (stm->pdrv->output_close)
> + stm->pdrv->output_close(output);
> spin_unlock(&output->lock);
> spin_unlock(&stm->mc_lock);
> }
> @@ -349,6 +366,122 @@ static int major_match(struct device *dev, const void
> *data)
> return MAJOR(dev->devt) == major;
> }
>
> +/*
> + * Framing protocol management
> + * Modules can implement STM protocol drivers and (un-)register them
> + * with the STM class framework.
> + */
> +static struct list_head stm_pdrv_head;
> +static struct mutex stm_pdrv_mutex;
> +
> +struct stm_pdrv_entry {
> + struct list_head entry;
> + const struct stm_protocol_driver *pdrv;
> + const struct config_item_type *node_type;
> +};
> +
> +static const struct stm_pdrv_entry *
> +__stm_lookup_protocol(const char *name)
> +{
> + struct stm_pdrv_entry *pe;
> +
> + list_for_each_entry(pe, &stm_pdrv_head, entry) {
> + if (!name || !*name || !strcmp(name, pe->pdrv->name))
> + return pe;
> + }
Please add a comment asserting your intentions with this loop. I had to look at
it for quite a while before understanding all the implications it conveys.
> +
> + return NULL;
> +}
> +
> +int stm_register_protocol(const struct stm_protocol_driver *pdrv)
> +{
> + struct stm_pdrv_entry *pe = NULL;
> + int ret = -ENOMEM;
> +
> + mutex_lock(&stm_pdrv_mutex);
> +
> + if (__stm_lookup_protocol(pdrv->name)) {
> + ret = -EEXIST;
> + goto unlock;
> + }
> +
> + pe = kzalloc(sizeof(*pe), GFP_KERNEL);
> + if (!pe)
> + goto unlock;
> +
> + if (pdrv->policy_attr) {
> + pe->node_type = get_policy_node_type(pdrv->policy_attr);
> + if (!pe->node_type)
> + goto unlock;
> + }
> +
> + list_add_tail(&pe->entry, &stm_pdrv_head);
> + pe->pdrv = pdrv;
> +
> + ret = 0;
> +unlock:
> + mutex_unlock(&stm_pdrv_mutex);
> +
> + if (ret)
> + kfree(pe);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(stm_register_protocol);
> +
> +void stm_unregister_protocol(const struct stm_protocol_driver *pdrv)
> +{
> + struct stm_pdrv_entry *pe, *iter;
> +
> + mutex_lock(&stm_pdrv_mutex);
> +
> + list_for_each_entry_safe(pe, iter, &stm_pdrv_head, entry) {
> + if (pe->pdrv == pdrv) {
> + list_del(&pe->entry);
> +
> + /* XXX: factor out */
> + if (pe->node_type) {
> + kfree(pe->node_type->ct_attrs);
> + kfree(pe->node_type);
> + }
> + kfree(pe);
> + break;
> + }
> + }
> +
> + mutex_unlock(&stm_pdrv_mutex);
> +}
> +EXPORT_SYMBOL_GPL(stm_unregister_protocol);
> +
> +static bool stm_get_protocol(const struct stm_protocol_driver *pdrv)
> +{
> + return try_module_get(pdrv->owner);
> +}
> +
> +void stm_put_protocol(const struct stm_protocol_driver *pdrv)
> +{
> + module_put(pdrv->owner);
> +}
> +
> +int stm_lookup_protocol(const char *name,
> + const struct stm_protocol_driver **pdrv,
> + const struct config_item_type **node_type)
> +{
> + const struct stm_pdrv_entry *pe;
> +
> + mutex_lock(&stm_pdrv_mutex);
> +
> + pe = __stm_lookup_protocol(name);
> + if (pe && pe->pdrv && stm_get_protocol(pe->pdrv)) {
> + *pdrv = pe->pdrv;
> + *node_type = pe->node_type;
> + }
> +
> + mutex_unlock(&stm_pdrv_mutex);
> +
> + return pe ? 0 : -ENOENT;
> +}
> +
> static int stm_char_open(struct inode *inode, struct file *file)
> {
> struct stm_file *stmf;
> @@ -1178,6 +1311,15 @@ static int __init stm_core_init(void)
> goto err_src;
>
> init_srcu_struct(&stm_source_srcu);
> + INIT_LIST_HEAD(&stm_pdrv_head);
> + mutex_init(&stm_pdrv_mutex);
> +
> + /*
> + * So as to not confuse existing users with a requirement
> + * to load yet another module, do it here.
> + */
> + if (IS_ENABLED(CONFIG_STM_PROTO_BASIC))
> + (void)request_module_nowait("stm_p_basic");
>
> stm_core_up++;
>
> diff --git a/drivers/hwtracing/stm/policy.c b/drivers/hwtracing/stm/policy.c
> index a505f055f464..894598cfe2b7 100644
> --- a/drivers/hwtracing/stm/policy.c
> +++ b/drivers/hwtracing/stm/policy.c
> @@ -33,8 +33,18 @@ struct stp_policy_node {
> unsigned int last_master;
> unsigned int first_channel;
> unsigned int last_channel;
> + /* this is the one that's exposed to the attributes */
> + unsigned char priv[0];
> };
>
> +void *stp_policy_node_priv(struct stp_policy_node *pn)
> +{
> + if (!pn)
> + return NULL;
> +
> + return pn->priv;
> +}
> +
> static struct configfs_subsystem stp_policy_subsys;
>
> void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> @@ -68,6 +78,14 @@ to_stp_policy_node(struct config_item *item)
> NULL;
> }
>
> +void *to_pdrv_policy_node(struct config_item *item)
> +{
> + struct stp_policy_node *node = to_stp_policy_node(item);
> +
> + return stp_policy_node_priv(node);
> +}
> +EXPORT_SYMBOL_GPL(to_pdrv_policy_node);
> +
> static ssize_t
> stp_policy_node_masters_show(struct config_item *item, char *page)
> {
> @@ -163,7 +181,9 @@ stp_policy_node_channels_store(struct config_item *item,
> const char *page,
>
> static void stp_policy_node_release(struct config_item *item)
> {
> - kfree(to_stp_policy_node(item));
> + struct stp_policy_node *node = to_stp_policy_node(item);
> +
> + kfree(node);
> }
>
> static struct configfs_item_operations stp_policy_node_item_ops = {
> @@ -182,10 +202,61 @@ static struct configfs_attribute
> *stp_policy_node_attrs[] = {
> static const struct config_item_type stp_policy_type;
> static const struct config_item_type stp_policy_node_type;
>
> +/* lifted from arch/x86/events/core.c */
> +static struct configfs_attribute **merge_attr(struct configfs_attribute **a,
> struct configfs_attribute **b)
> +{
> + struct configfs_attribute **new;
> + int j, i;
> +
> + for (j = 0; a[j]; j++)
> + ;
> + for (i = 0; b[i]; i++)
> + j++;
> + j++;
> +
> + new = kmalloc_array(j, sizeof(struct configfs_attribute *),
> + GFP_KERNEL);
> + if (!new)
> + return NULL;
> +
> + j = 0;
> + for (i = 0; a[i]; i++)
> + new[j++] = a[i];
> + for (i = 0; b[i]; i++)
> + new[j++] = b[i];
> + new[j] = NULL;
> +
> + return new;
> +}
> +
> +const struct config_item_type *
> +get_policy_node_type(struct configfs_attribute **attrs)
> +{
> + struct config_item_type *type;
> + struct configfs_attribute **merged;
> +
> + type = kmemdup(&stp_policy_node_type, sizeof(stp_policy_node_type),
> + GFP_KERNEL);
> + if (!type)
> + return NULL;
> +
> + merged = merge_attr(stp_policy_node_attrs, attrs);
> + if (!merged) {
> + kfree(type);
> + return NULL;
> + }
> +
> + type->ct_attrs = merged;
> +
> + return type;
> +}
> +
> static struct config_group *
> stp_policy_node_make(struct config_group *group, const char *name)
> {
> + const struct config_item_type *type = &stp_policy_node_type;
> struct stp_policy_node *policy_node, *parent_node;
> + const struct stm_protocol_driver *pdrv;
> struct stp_policy *policy;
>
> if (group->cg_item.ci_type == &stp_policy_type) {
> @@ -199,12 +270,20 @@ stp_policy_node_make(struct config_group *group, const
> char *name)
> if (!policy->stm)
> return ERR_PTR(-ENODEV);
>
> - policy_node = kzalloc(sizeof(struct stp_policy_node), GFP_KERNEL);
> + pdrv = policy->stm->pdrv;
> + policy_node =
> + kzalloc(offsetof(struct stp_policy_node, priv[pdrv->priv_sz]),
> + GFP_KERNEL);
> if (!policy_node)
> return ERR_PTR(-ENOMEM);
>
> - config_group_init_type_name(&policy_node->group, name,
> - &stp_policy_node_type);
> + if (pdrv->policy_node_init)
> + pdrv->policy_node_init((void *)policy_node->priv);
> +
> + if (policy->stm->pdrv_node_type)
> + type = policy->stm->pdrv_node_type;
> +
> + config_group_init_type_name(&policy_node->group, name, type);
>
> policy_node->policy = policy;
>
> @@ -254,8 +333,26 @@ static ssize_t stp_policy_device_show(struct config_item
> *item,
>
> CONFIGFS_ATTR_RO(stp_policy_, device);
>
> +static ssize_t stp_policy_protocol_show(struct config_item *item,
> + char *page)
> +{
> + struct stp_policy *policy = to_stp_policy(item);
> + ssize_t count;
> +
> + /* XXX: there shouldn't be 'none', ever */
> + count = sprintf(page, "%s\n",
> + (policy && policy->stm) ?
> + policy->stm->pdrv->name :
> + "<none>");
> +
> + return count;
> +}
> +
> +CONFIGFS_ATTR_RO(stp_policy_, protocol);
> +
> static struct configfs_attribute *stp_policy_attrs[] = {
> &stp_policy_attr_device,
> + &stp_policy_attr_protocol,
> NULL,
> };
>
> @@ -276,6 +373,7 @@ void stp_policy_unbind(struct stp_policy *policy)
> stm->policy = NULL;
> policy->stm = NULL;
>
> + stm_put_protocol(stm->pdrv);
> stm_put_device(stm);
> }
>
> @@ -313,9 +411,12 @@ static const struct config_item_type stp_policy_type = {
> static struct config_group *
> stp_policy_make(struct config_group *group, const char *name)
> {
> + const struct config_item_type *pdrv_node_type;
> + const struct stm_protocol_driver *pdrv;
> + char *devname, *proto, *p;
> struct config_group *ret;
> struct stm_device *stm;
> - char *devname, *p;
> + int err;
>
> devname = kasprintf(GFP_KERNEL, "%s", name);
> if (!devname)
> @@ -326,6 +427,7 @@ stp_policy_make(struct config_group *group, const char
> *name)
> * <device_name> is the name of an existing stm device; may
> * contain dots;
> * <policy_name> is an arbitrary string; may not contain dots
> + * <device_name>:<protocol_name>.<policy_name>
> */
> p = strrchr(devname, '.');
> if (!p) {
> @@ -335,11 +437,28 @@ stp_policy_make(struct config_group *group, const char
> *name)
>
> *p = '\0';
>
> + /*
> + * look for ":<protocol_name>":
> + * + no protocol suffix: fall back to whatever is available;
> + * + unknown protocol: fail the whole thing
> + */
> + proto = strrchr(devname, ':');
> + if (proto)
> + *proto++ = '\0';
> +
> stm = stm_find_device(devname);
> + if (!stm) {
> + kfree(devname);
> + return ERR_PTR(-ENODEV);
> + }
> +
> + err = stm_lookup_protocol(proto, &pdrv, &pdrv_node_type);
> kfree(devname);
>
> - if (!stm)
> + if (err) {
> + stm_put_device(stm);
> return ERR_PTR(-ENODEV);
> + }
This condition prevent the subsystem from being used until patch 06/16 is added.
I would also suggest automatically compiling in the functionality provided by
p_basic if p_sys-t is not selected. That way we preserve the original
behaviour. I would also use p_basic if no protocol driver is selected rather
than leaving it to the insertion order to make things more deterministic.
>
> mutex_lock(&stm->policy_mutex);
> if (stm->policy) {
> @@ -349,21 +468,27 @@ stp_policy_make(struct config_group *group, const char
> *name)
>
> stm->policy = kzalloc(sizeof(*stm->policy), GFP_KERNEL);
> if (!stm->policy) {
> - ret = ERR_PTR(-ENOMEM);
> - goto unlock_policy;
> + mutex_unlock(&stm->policy_mutex);
> + stm_put_protocol(pdrv);
> + stm_put_device(stm);
> + return ERR_PTR(-ENOMEM);
> }
>
> config_group_init_type_name(&stm->policy->group, name,
> &stp_policy_type);
> - stm->policy->stm = stm;
>
> + stm->pdrv = pdrv;
> + stm->pdrv_node_type = pdrv_node_type;
> + stm->policy->stm = stm;
> ret = &stm->policy->group;
>
> unlock_policy:
> mutex_unlock(&stm->policy_mutex);
>
> - if (IS_ERR(ret))
> + if (IS_ERR(ret)) {
> + stm_put_protocol(stm->pdrv);
> stm_put_device(stm);
> + }
>
> return ret;
> }
> diff --git a/drivers/hwtracing/stm/stm.h b/drivers/hwtracing/stm/stm.h
> index e5df08ae59cf..921ebd9fd3bd 100644
> --- a/drivers/hwtracing/stm/stm.h
> +++ b/drivers/hwtracing/stm/stm.h
> @@ -10,20 +10,17 @@
> #ifndef _STM_STM_H_
> #define _STM_STM_H_
>
> +#include <linux/configfs.h>
> +
> struct stp_policy;
> struct stp_policy_node;
> +struct stm_protocol_driver;
>
> -struct stp_policy_node *
> -stp_policy_node_lookup(struct stm_device *stm, char *s);
> -void stp_policy_node_put(struct stp_policy_node *policy_node);
> -void stp_policy_unbind(struct stp_policy *policy);
> -
> -void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> - unsigned int *mstart, unsigned int *mend,
> - unsigned int *cstart, unsigned int *cend);
> int stp_configfs_init(void);
> void stp_configfs_exit(void);
>
> +void *stp_policy_node_priv(struct stp_policy_node *pn);
> +
> struct stp_master {
> unsigned int nr_free;
> unsigned long chan_map[0];
> @@ -40,6 +37,9 @@ struct stm_device {
> struct mutex link_mutex;
> spinlock_t link_lock;
> struct list_head link_list;
> + /* framing protocol in use */
> + const struct stm_protocol_driver *pdrv;
> + const struct config_item_type *pdrv_node_type;
> /* master allocation */
> spinlock_t mc_lock;
> struct stp_master *masters[0];
> @@ -48,11 +48,25 @@ struct stm_device {
> #define to_stm_device(_d) \
> container_of((_d), struct stm_device, dev)
>
> +struct stp_policy_node *
> +stp_policy_node_lookup(struct stm_device *stm, char *s);
> +void stp_policy_node_put(struct stp_policy_node *policy_node);
> +void stp_policy_unbind(struct stp_policy *policy);
> +
> +void stp_policy_node_get_ranges(struct stp_policy_node *policy_node,
> + unsigned int *mstart, unsigned int *mend,
> + unsigned int *cstart, unsigned int *cend);
> +
> +/* XXX: unXXX this! */
> +const struct config_item_type *
> +get_policy_node_type(struct configfs_attribute **attrs);
> +
> struct stm_output {
> spinlock_t lock;
> unsigned int master;
> unsigned int channel;
> unsigned int nr_chans;
> + void *pdrv_private;
> };
>
> struct stm_file {
> @@ -76,4 +90,26 @@ struct stm_source_device {
> #define to_stm_source_device(_d) \
> container_of((_d), struct stm_source_device, dev)
>
> +void *to_pdrv_policy_node(struct config_item *item);
> +
> +struct stm_protocol_driver {
> + struct module *owner;
> + const char *name;
> + ssize_t (*write)(struct stm_data *data,
> + struct stm_output *output, unsigned int chan,
> + const char *buf, size_t count);
> + void (*policy_node_init)(void *arg);
> + int (*output_open)(void *priv, struct stm_output *output);
> + void (*output_close)(struct stm_output *output);
> + ssize_t priv_sz;
> + struct configfs_attribute **policy_attr;
> +};
> +
> +int stm_register_protocol(const struct stm_protocol_driver *pdrv);
> +void stm_unregister_protocol(const struct stm_protocol_driver *pdrv);
> +int stm_lookup_protocol(const char *name,
> + const struct stm_protocol_driver **pdrv,
> + const struct config_item_type **type);
> +void stm_put_protocol(const struct stm_protocol_driver *pdrv);
> +
> #endif /* _STM_STM_H_ */
> --
> 2.18.0
>
