Re: [GIT PULL 8/9] intel_th: msu-sink: An example msu buffer driver

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

On Thu, Jun 27, 2019 at 03:51:51PM +0300, Alexander Shishkin wrote:
> This patch adds an example "sink" MSU buffer driver, which consumes trace
> data from MSC buffers.
> 
> Functionally, it acts similarly to "multi" mode with automatic window
> switching.
> 
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/hwtracing/intel_th/Makefile   |   3 +
>  drivers/hwtracing/intel_th/msu-sink.c | 127 ++++++++++++++++++++++++++
>  2 files changed, 130 insertions(+)
>  create mode 100644 drivers/hwtracing/intel_th/msu-sink.c
> 
> diff --git a/drivers/hwtracing/intel_th/Makefile b/drivers/hwtracing/intel_th/Makefile
> index d9252fa8d9ca..b63eb8f309ad 100644
> --- a/drivers/hwtracing/intel_th/Makefile
> +++ b/drivers/hwtracing/intel_th/Makefile
> @@ -20,3 +20,6 @@ intel_th_msu-y			:= msu.o
>  
>  obj-$(CONFIG_INTEL_TH_PTI)	+= intel_th_pti.o
>  intel_th_pti-y			:= pti.o
> +
> +obj-$(CONFIG_INTEL_TH_MSU)	+= intel_th_msu_sink.o
> +intel_th_msu_sink-y		:= msu-sink.o
> diff --git a/drivers/hwtracing/intel_th/msu-sink.c b/drivers/hwtracing/intel_th/msu-sink.c
> new file mode 100644
> index 000000000000..d2bdd4da6f14
> --- /dev/null
> +++ b/drivers/hwtracing/intel_th/msu-sink.c
> @@ -0,0 +1,127 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * An example software sink buffer driver for Intel TH MSU.
> + *
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#include <linux/intel_th.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/device.h>
> +#include <linux/dma-mapping.h>
> +
> +#define MAX_SGTS 16
> +
> +struct msu_sink_private {
> +	struct device	*dev;
> +	struct sg_table **sgts;
> +	unsigned int	nr_sgts;
> +};
> +
> +static void *msu_sink_assign(struct device *dev, int *mode)
> +{
> +	struct msu_sink_private *priv;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return NULL;
> +
> +	priv->sgts = kcalloc(MAX_SGTS, sizeof(void *), GFP_KERNEL);
> +	if (!priv->sgts) {
> +		kfree(priv);
> +		return NULL;
> +	}
> +
> +	priv->dev = dev;
> +	*mode = MSC_MODE_MULTI;
> +
> +	return priv;
> +}
> +
> +static void msu_sink_unassign(void *data)
> +{
> +	struct msu_sink_private *priv = data;
> +
> +	kfree(priv->sgts);
> +	kfree(priv);
> +}
> +
> +/* See also: msc.c: __msc_buffer_win_alloc() */
> +static int msu_sink_alloc_window(void *data, struct sg_table **sgt, size_t size)
> +{
> +	struct msu_sink_private *priv = data;
> +	unsigned int nents;
> +	struct scatterlist *sg_ptr;
> +	void *block;
> +	int ret, i;
> +
> +	if (priv->nr_sgts == MAX_SGTS)
> +		return -ENOMEM;
> +
> +	nents = DIV_ROUND_UP(size, PAGE_SIZE);
> +
> +	ret = sg_alloc_table(*sgt, nents, GFP_KERNEL);
> +	if (ret)
> +		return -ENOMEM;
> +
> +	priv->sgts[priv->nr_sgts++] = *sgt;
> +
> +	for_each_sg((*sgt)->sgl, sg_ptr, nents, i) {
> +		block = dma_alloc_coherent(priv->dev->parent->parent,
> +					   PAGE_SIZE, &sg_dma_address(sg_ptr),
> +					   GFP_KERNEL);
> +		sg_set_buf(sg_ptr, block, PAGE_SIZE);
> +	}
> +
> +	return nents;
> +}
> +
> +/* See also: msc.c: __msc_buffer_win_free() */
> +static void msu_sink_free_window(void *data, struct sg_table *sgt)
> +{
> +	struct msu_sink_private *priv = data;
> +	struct scatterlist *sg_ptr;
> +	int i;
> +
> +	for_each_sg(sgt->sgl, sg_ptr, sgt->nents, i) {
> +		dma_free_coherent(priv->dev->parent->parent, PAGE_SIZE,
> +				  sg_virt(sg_ptr), sg_dma_address(sg_ptr));
> +	}
> +
> +	sg_free_table(sgt);
> +	priv->nr_sgts--;
> +}
> +
> +static int msu_sink_ready(void *data, struct sg_table *sgt, size_t bytes)
> +{
> +	struct msu_sink_private *priv = data;
> +
> +	intel_th_msc_window_unlock(priv->dev, sgt);
> +
> +	return 0;
> +}
> +
> +static const struct msu_buffer_driver sink_bdrv = {
> +	.name		= "sink",
> +	.owner		= THIS_MODULE,

Ah, there you use it.

No, don't.  Use the "modern" way of not requiring code to ever set this
and use the compiler to do it for you.  Look at any of the normal bus
registering driver functions for how this is done.

thanks,

greg k-h