Re: [GIT PULL 5/9] intel_th: msu: Introduce buffer driver interface

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

On Thu, Jun 27, 2019 at 03:51:48PM +0300, Alexander Shishkin wrote:
> Introduces a concept of buffer drivers, which is a mechanism for creating
> trace sinks that would receive trace data from MSC buffers and transfer it
> elsewhere.
> 
> A buffer driver can implement its own window allocation/deallocation if
> it has to. It must provide a callback that's used to notify it when a
> window fills up, so that it can then start a DMA transaction from that
> window 'elsewhere'. This window remains in a 'locked' state and won't be
> used for storing new trace data until the buffer driver 'unlocks' it with
> a provided API call, at which point the window can be used again for
> storing trace data.
> 
> This relies on a functional "last block" interrupt, so not all versions of
> Trace Hub can use this feature.

So you are breaking userspace?  Or just that older userspace tools will
not have this new functionality?

> +#include <linux/intel_th.h>
>  #include "intel_th.h"
>  #include "msu.h"
>  
> @@ -32,6 +34,7 @@
>   * struct msc_window - multiblock mode window descriptor
>   * @entry:	window list linkage (msc::win_list)
>   * @pgoff:	page offset into the buffer that this window starts at
> + * @lockout:	lockout state, see comment below
>   * @nr_blocks:	number of blocks (pages) in this window
>   * @nr_segs:	number of segments in this window (<= @nr_blocks)
>   * @_sgt:	array of block descriptors
> @@ -40,6 +43,7 @@
>  struct msc_window {
>  	struct list_head	entry;
>  	unsigned long		pgoff;
> +	atomic_t		lockout;
>  	unsigned int		nr_blocks;
>  	unsigned int		nr_segs;
>  	struct msc		*msc;
> @@ -100,6 +104,10 @@ struct msc {
>  	void __iomem		*msu_base;
>  	struct intel_th_device	*thdev;
>  
> +	const struct msu_buffer_driver	*bdrv;
> +	void				*bdrv_priv;
> +
> +	struct work_struct	work;
>  	struct list_head	win_list;
>  	struct sg_table		single_sgt;
>  	struct msc_window	*cur_win;
> @@ -126,6 +134,110 @@ struct msc {
>  	unsigned int		index;
>  };
>  
> +/*
> + * Lockout state transitions:
> + *   READY -> INUSE -+-> LOCKED -+-> READY -> etc.
> + *                   \-----------/
> + * WIN_READY:	window can be used by HW
> + * WIN_INUSE:	window is in use
> + * WIN_LOCKED:	window is filled up and is being processed by the buffer driver
> + *
> + * All state transitions happen automatically, except for the LOCKED->READY,
> + * which needs to be signalled by the buffer driver by calling
> + * intel_th_msc_window_unlock().
> + *
> + * When the interrupt handler has to switch to the next window, it checks
> + * whether it's READY, and if it is, it performs the switch and tracing
> + * continues. If it's LOCKED, it stops the trace.
> + */
> +enum {
> +	WIN_READY = 0,
> +	WIN_INUSE,
> +	WIN_LOCKED
> +};

Why use an atomic for a state?  What's wrong with a "normal" lock?  Are
you _sure_ you are using it correctly?  If so, what is the benefit?

> +static LIST_HEAD(msu_buffer_list);
> +static struct mutex msu_buffer_mutex;
> +
> +struct msu_buffer {
> +	struct list_head		entry;
> +	const struct msu_buffer_driver	*bdrv;
> +};
> +
> +static struct msu_buffer *__msu_buffer_find(const char *name)
> +{
> +	struct msu_buffer *buf;
> +
> +	list_for_each_entry(buf, &msu_buffer_list, entry) {
> +		if (!strcmp(buf->bdrv->name, name))
> +			return buf;
> +	}
> +
> +	return NULL;

No locking of your list?  Shouldn't you annotate that the lock must be
held here for this to work?  Putting a lock outside of a function is a
sure way to make people reading the code go crazy.

> +}
> +
> +static const struct msu_buffer_driver *
> +__msu_buffer_driver_find(const char *name)
> +{
> +	struct msu_buffer *buf = __msu_buffer_find(name);
> +
> +	return buf ? buf->bdrv : NULL;
> +}
> +
> +static const struct msu_buffer_driver *
> +msu_buffer_driver_get(const char *name)
> +{
> +	const struct msu_buffer_driver *bdrv;
> +
> +	mutex_lock(&msu_buffer_mutex);
> +	bdrv = __msu_buffer_driver_find(name);
> +	if (bdrv && !try_module_get(bdrv->owner))
> +		bdrv = NULL;
> +	mutex_unlock(&msu_buffer_mutex);
> +
> +	return bdrv;
> +}
> +
> +int intel_th_msu_buffer_register(const struct msu_buffer_driver *bdrv)
> +{
> +	struct msu_buffer *buf;
> +	int ret = -EEXIST;
> +
> +	buf = kzalloc(sizeof(*buf), GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	mutex_lock(&msu_buffer_mutex);
> +	if (__msu_buffer_driver_find(bdrv->name))
> +		goto out;
> +
> +	buf->bdrv = bdrv;
> +	list_add_tail(&buf->entry, &msu_buffer_list);
> +	ret = 0;
> +out:
> +	mutex_unlock(&msu_buffer_mutex);
> +
> +	if (ret)
> +		kfree(buf);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(intel_th_msu_buffer_register);
> +
> +void intel_th_msu_buffer_unregister(const struct msu_buffer_driver *bdrv)
> +{
> +	struct msu_buffer *buf;
> +
> +	mutex_lock(&msu_buffer_mutex);
> +	buf = __msu_buffer_find(bdrv->name);
> +	if (buf) {
> +		list_del(&buf->entry);
> +		kfree(buf);
> +	}
> +	mutex_unlock(&msu_buffer_mutex);
> +}
> +EXPORT_SYMBOL_GPL(intel_th_msu_buffer_unregister);
> +
>  static inline bool msc_block_is_empty(struct msc_block_desc *bdesc)
>  {
>  	/* header hasn't been written */
> @@ -188,6 +300,25 @@ static struct msc_window *msc_next_window(struct msc_window *win)
>  	return list_next_entry(win, entry);
>  }
>  
> +static size_t msc_win_total_sz(struct msc_window *win)
> +{
> +	unsigned int blk;
> +	size_t size = 0;
> +
> +	for (blk = 0; blk < win->nr_segs; blk++) {
> +		struct msc_block_desc *bdesc = msc_win_block(win, blk);
> +
> +		if (msc_block_wrapped(bdesc))
> +			return win->nr_blocks << PAGE_SHIFT;
> +
> +		size += msc_total_sz(bdesc);
> +		if (msc_block_last_written(bdesc))
> +			break;
> +	}
> +
> +	return size;
> +}
> +
>  /**
>   * msc_find_window() - find a window matching a given sg_table
>   * @msc:	MSC device
> @@ -527,6 +658,9 @@ static int intel_th_msu_init(struct msc *msc)
>  	if (!msc->do_irq)
>  		return 0;
>  
> +	if (!msc->bdrv)
> +		return 0;
> +
>  	mintctl = ioread32(msc->msu_base + REG_MSU_MINTCTL);
>  	mintctl |= msc->index ? M1BLIE : M0BLIE;
>  	iowrite32(mintctl, msc->msu_base + REG_MSU_MINTCTL);
> @@ -554,6 +688,27 @@ static void intel_th_msu_deinit(struct msc *msc)
>  	iowrite32(mintctl, msc->msu_base + REG_MSU_MINTCTL);
>  }
>  
> +static int msc_win_set_lockout(struct msc_window *win, int expect, int new)
> +{
> +	int old;
> +
> +	if (!win->msc->bdrv)
> +		return 0;
> +
> +	old = atomic_cmpxchg(&win->lockout, expect, new);
> +	if (expect == WIN_READY && old == WIN_LOCKED)
> +		return -EBUSY;
> +
> +	/* from intel_th_msc_window_unlock(), don't warn if not locked */
> +	if (expect == WIN_LOCKED && old == new)
> +		return 0;
> +
> +	if (WARN_ONCE(old != expect, "expected lockout state %d, got %d\n",
> +		      expect, old))

How can this be hit?  If it is it, do you want people's machines to
crash (panic on warn)?

Anyway, this fucntion is crazy, why not use a normal enum and a lock?


> +		return -EINVAL;
> +
> +	return 0;
> +}
>  /**
>   * msc_configure() - set up MSC hardware
>   * @msc:	the MSC device to configure
> @@ -571,8 +726,16 @@ static int msc_configure(struct msc *msc)
>  	if (msc->mode > MSC_MODE_MULTI)
>  		return -ENOTSUPP;
>  
> -	if (msc->mode == MSC_MODE_MULTI)
> +	if (msc->mode == MSC_MODE_MULTI) {
> +		/* Window allocation path makes sure this doesn't happen */
> +		if (WARN_ON_ONCE(!msc->cur_win))
> +			return -EINVAL;

If this can never happen, don't test for it.  If it can, test and handle
it properly.  Don't crash.

> +
> +		if (msc_win_set_lockout(msc->cur_win, WIN_READY, WIN_INUSE))
> +			return -EBUSY;
> +
>  		msc_buffer_clear_hw_header(msc);
> +	}
>  
>  	reg = msc->base_addr >> PAGE_SHIFT;
>  	iowrite32(reg, msc->reg_base + REG_MSU_MSC0BAR);
> @@ -594,10 +757,14 @@ static int msc_configure(struct msc *msc)
>  
>  	iowrite32(reg, msc->reg_base + REG_MSU_MSC0CTL);
>  
> +	intel_th_msu_init(msc);
> +
>  	msc->thdev->output.multiblock = msc->mode == MSC_MODE_MULTI;
>  	intel_th_trace_enable(msc->thdev);
>  	msc->enabled = 1;
>  
> +	if (msc->bdrv && msc->bdrv->activate)
> +		msc->bdrv->activate(msc->bdrv_priv);
>  
>  	return 0;
>  }
> @@ -611,10 +778,17 @@ static int msc_configure(struct msc *msc)
>   */
>  static void msc_disable(struct msc *msc)
>  {
> +	struct msc_window *win = msc->cur_win;
>  	u32 reg;
>  
>  	lockdep_assert_held(&msc->buf_mutex);
>  
> +	if (msc->mode == MSC_MODE_MULTI)
> +		msc_win_set_lockout(win, WIN_INUSE, WIN_LOCKED);

Look, the lock is held!  Use it!

Anyway, this patch is odd, please re-review it.

> --- /dev/null
> +++ b/include/linux/intel_th.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Intel(R) Trace Hub data structures for implementing buffer drivers.
> + *
> + * Copyright (C) 2019 Intel Corporation.
> + */
> +
> +#ifndef _INTEL_TH_H_
> +#define _INTEL_TH_H_
> +
> +#include <linux/scatterlist.h>
> +
> +/* MSC operating modes (MSC_MODE) */
> +enum {
> +	MSC_MODE_SINGLE	= 0,
> +	MSC_MODE_MULTI,
> +	MSC_MODE_EXI,
> +	MSC_MODE_DEBUG,
> +};
> +
> +struct msu_buffer_driver {
> +	const char	*name;
> +	struct module	*owner;

Why does a driver have a module pointer?  And then not use it?

> +	/*
> +	 * ->assign() called when buffer 'mode' is set to this driver
> +	 *   (aka mode_store())
> +	 * @device:	struct device * of the msc
> +	 * @mode:	allows the driver to set HW mode (see the enum above)
> +	 * Returns:	a pointer to a private structure associated with this
> +	 *		msc or NULL in case of error. This private structure
> +	 *		will then be passed into all other callbacks.
> +	 */
> +	void	*(*assign)(struct device *dev, int *mode);
> +	/* ->unassign():	some other mode is selected, clean up */
> +	void	(*unassign)(void *priv);
> +	/*
> +	 * ->alloc_window(): allocate memory for the window of a given
> +	 *		size
> +	 * @sgt:	pointer to sg_table, can be overridden by the buffer
> +	 *		driver, or kept intact
> +	 * Returns:	number of sg table entries <= number of pages;
> +	 *		0 is treated as an allocation failure.
> +	 */
> +	int	(*alloc_window)(void *priv, struct sg_table **sgt,
> +				size_t size);
> +	void	(*free_window)(void *priv, struct sg_table *sgt);
> +	/* ->activate():	trace has started */
> +	void	(*activate)(void *priv);
> +	/* ->deactivate():	trace is about to stop */
> +	void	(*deactivate)(void *priv);
> +	/*
> +	 * ->ready():	window @sgt is filled up to the last block OR
> +	 *		tracing is stopped by the user; this window contains
> +	 *		@bytes data. The window in question transitions into
> +	 *		the "LOCKED" state, indicating that it can't be used
> +	 *		by hardware. To clear this state and make the window
> +	 *		available to the hardware again, call
> +	 *		intel_th_msc_window_unlock().
> +	 */
> +	int	(*ready)(void *priv, struct sg_table *sgt, size_t bytes);
> +};

Why isn't this based off of 'struct driver'?

thanks,

greg k-h