[GIT PULL 0/9] intel_th: Updates for v5.3

[GIT PULL 0/9] intel_th: Updates for v5.3

[Alexander Shishkin]

Hi Greg,

There are the updates for the Intel TH driver that I have for the next
cycle. These are multi-window mode buffer management changes, mainly to
add support for software trace sinks.

One patch at the top is a dependency, from a pull request I sent last week
with fixes for v5.2 [1].

Individual patches are in follow-up emails, a signed tag is also pushed
out and available at the URL below. Please consider pulling or applying.
Thanks!

[1] https://marc.info/?l=linux-kernel&m=156113401304637

The following changes since commit a188339ca5a396acc588e5851ed7e19f66b0ebd9:

  Linux 5.2-rc1 (2019-05-19 15:47:09 -0700)

are available in the Git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/ash/stm.git tags/intel_th-for-greg-20190627

for you to fetch changes up to 4fe4659624b7cf6fa7c2197eb5588f1be1123e17:

  intel_th: msu: Preserve pre-existing buffer configuration (2019-06-27 15:21:25 +0300)

----------------------------------------------------------------
intel_th: Updates for v5.3

These are:
  * Support for software trace sinks / buffer drivers
  * Various reworks in buffer management code

----------------------------------------------------------------
Alexander Shishkin (8):
      intel_th: msu: Support multipage blocks
      intel_th: msu: Split sgt array and pointer in multiwindow mode
      intel_th: msu: Start read iterator from a non-empty window
      intel_th: msu: Introduce buffer driver interface
      intel_th: msu: Prevent freeing buffers while locked windows exist
      intel_th: msu: Get rid of the window size limit
      intel_th: msu-sink: An example msu buffer driver
      intel_th: msu: Preserve pre-existing buffer configuration

Shaokun Zhang (1):
      intel_th: msu: Fix unused variable warning on arm64 platform

 .../ABI/testing/sysfs-bus-intel_th-devices-msc     |   3 +-
 MAINTAINERS                                        |   1 +
 drivers/hwtracing/intel_th/Makefile                |   3 +
 drivers/hwtracing/intel_th/msu-sink.c              | 127 +++++
 drivers/hwtracing/intel_th/msu.c                   | 609 +++++++++++++++++----
 drivers/hwtracing/intel_th/msu.h                   |  23 +-
 include/linux/intel_th.h                           |  67 +++
 7 files changed, 710 insertions(+), 123 deletions(-)
 create mode 100644 drivers/hwtracing/intel_th/msu-sink.c
 create mode 100644 include/linux/intel_th.h

-- 
2.20.1

[GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Alexander Shishkin]

From: Shaokun Zhang <zhangshaokun@hisilicon.com>

Commit ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
introduced the following warnings on non-x86 architectures, as a result
of reordering the multi mode buffer allocation sequence:

> drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_alloc’:
> drivers/hwtracing/intel_th/msu.c:783:21: warning: unused variable ‘i’
> [-Wunused-variable]
> int ret = -ENOMEM, i;
>                    ^
> drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_free’:
> drivers/hwtracing/intel_th/msu.c:863:6: warning: unused variable ‘i’
> [-Wunused-variable]
> int i;
>     ^

Fix this compiler warning by factoring out set_memory sequences and making
them x86-only.

Suggested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
Fixes: ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 40 +++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 13 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 81bb54fa3ce8..8c568b5c8920 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -767,6 +767,30 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 	return -ENOMEM;
 }
 
+#ifdef CONFIG_X86
+static void msc_buffer_set_uc(struct msc_window *win, unsigned int nr_blocks)
+{
+	int i;
+
+	for (i = 0; i < nr_blocks; i++)
+		/* Set the page as uncached */
+		set_memory_uc((unsigned long)msc_win_block(win, i), 1);
+}
+
+static void msc_buffer_set_wb(struct msc_window *win)
+{
+	int i;
+
+	for (i = 0; i < win->nr_blocks; i++)
+		/* Reset the page to write-back */
+		set_memory_wb((unsigned long)msc_win_block(win, i), 1);
+}
+#else /* !X86 */
+static inline void
+msc_buffer_set_uc(struct msc_window *win, unsigned int nr_blocks) {}
+static inline void msc_buffer_set_wb(struct msc_window *win) {}
+#endif /* CONFIG_X86 */
+
 /**
  * msc_buffer_win_alloc() - alloc a window for a multiblock mode
  * @msc:	MSC device
@@ -780,7 +804,7 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 {
 	struct msc_window *win;
-	int ret = -ENOMEM, i;
+	int ret = -ENOMEM;
 
 	if (!nr_blocks)
 		return 0;
@@ -811,11 +835,7 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 	if (ret < 0)
 		goto err_nomem;
 
-#ifdef CONFIG_X86
-	for (i = 0; i < ret; i++)
-		/* Set the page as uncached */
-		set_memory_uc((unsigned long)msc_win_block(win, i), 1);
-#endif
+	msc_buffer_set_uc(win, ret);
 
 	win->nr_blocks = ret;
 
@@ -860,8 +880,6 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
  */
 static void msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 {
-	int i;
-
 	msc->nr_pages -= win->nr_blocks;
 
 	list_del(&win->entry);
@@ -870,11 +888,7 @@ static void msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 		msc->base_addr = 0;
 	}
 
-#ifdef CONFIG_X86
-	for (i = 0; i < win->nr_blocks; i++)
-		/* Reset the page to write-back */
-		set_memory_wb((unsigned long)msc_win_block(win, i), 1);
-#endif
+	msc_buffer_set_wb(win);
 
 	__msc_buffer_win_free(msc, win);
 
-- 
2.20.1

Re: [GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Greg Kroah-Hartman]

On Thu, Jun 27, 2019 at 03:51:44PM +0300, Alexander Shishkin wrote:
> From: Shaokun Zhang <zhangshaokun@hisilicon.com>
> 
> Commit ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> introduced the following warnings on non-x86 architectures, as a result
> of reordering the multi mode buffer allocation sequence:
> 
> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_alloc’:
> > drivers/hwtracing/intel_th/msu.c:783:21: warning: unused variable ‘i’
> > [-Wunused-variable]
> > int ret = -ENOMEM, i;
> >                    ^
> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_free’:
> > drivers/hwtracing/intel_th/msu.c:863:6: warning: unused variable ‘i’
> > [-Wunused-variable]
> > int i;
> >     ^
> 
> Fix this compiler warning by factoring out set_memory sequences and making
> them x86-only.
> 
> Suggested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Fixes: ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/intel_th/msu.c | 40 +++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 13 deletions(-)

Should be backported to stable trees, I'll add the tag...

Re: [GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Greg Kroah-Hartman]

On Thu, Jun 27, 2019 at 03:51:44PM +0300, Alexander Shishkin wrote:
> From: Shaokun Zhang <zhangshaokun@hisilicon.com>
> 
> Commit ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> introduced the following warnings on non-x86 architectures, as a result
> of reordering the multi mode buffer allocation sequence:
> 
> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_alloc’:
> > drivers/hwtracing/intel_th/msu.c:783:21: warning: unused variable ‘i’
> > [-Wunused-variable]
> > int ret = -ENOMEM, i;
> >                    ^
> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_free’:
> > drivers/hwtracing/intel_th/msu.c:863:6: warning: unused variable ‘i’
> > [-Wunused-variable]
> > int i;
> >     ^
> 
> Fix this compiler warning by factoring out set_memory sequences and making
> them x86-only.
> 
> Suggested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> Fixes: ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> ---
>  drivers/hwtracing/intel_th/msu.c | 40 +++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 13 deletions(-)

Does not apply to my tree :(

Re: [GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

> On Thu, Jun 27, 2019 at 03:51:44PM +0300, Alexander Shishkin wrote:
>> From: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> 
>> Commit ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
>> introduced the following warnings on non-x86 architectures, as a result
>> of reordering the multi mode buffer allocation sequence:
>> 
>> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_alloc’:
>> > drivers/hwtracing/intel_th/msu.c:783:21: warning: unused variable ‘i’
>> > [-Wunused-variable]
>> > int ret = -ENOMEM, i;
>> >                    ^
>> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_free’:
>> > drivers/hwtracing/intel_th/msu.c:863:6: warning: unused variable ‘i’
>> > [-Wunused-variable]
>> > int i;
>> >     ^
>> 
>> Fix this compiler warning by factoring out set_memory sequences and making
>> them x86-only.
>> 
>> Suggested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
>> Fixes: ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
>> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
>> ---
>>  drivers/hwtracing/intel_th/msu.c | 40 +++++++++++++++++++++-----------
>>  1 file changed, 27 insertions(+), 13 deletions(-)
>
> Does not apply to my tree :(

It's the same one as the one in the fixes series. I just put it here for
completeness.

Regards,
--
Alex

Re: [GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Greg Kroah-Hartman]

On Wed, Jul 03, 2019 at 06:54:05PM +0300, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> > On Thu, Jun 27, 2019 at 03:51:44PM +0300, Alexander Shishkin wrote:
> >> From: Shaokun Zhang <zhangshaokun@hisilicon.com>
> >> 
> >> Commit ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> >> introduced the following warnings on non-x86 architectures, as a result
> >> of reordering the multi mode buffer allocation sequence:
> >> 
> >> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_alloc’:
> >> > drivers/hwtracing/intel_th/msu.c:783:21: warning: unused variable ‘i’
> >> > [-Wunused-variable]
> >> > int ret = -ENOMEM, i;
> >> >                    ^
> >> > drivers/hwtracing/intel_th/msu.c: In function ‘msc_buffer_win_free’:
> >> > drivers/hwtracing/intel_th/msu.c:863:6: warning: unused variable ‘i’
> >> > [-Wunused-variable]
> >> > int i;
> >> >     ^
> >> 
> >> Fix this compiler warning by factoring out set_memory sequences and making
> >> them x86-only.
> >> 
> >> Suggested-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> Signed-off-by: Shaokun Zhang <zhangshaokun@hisilicon.com>
> >> Fixes: ba39bd8306057 ("intel_th: msu: Switch over to scatterlist")
> >> Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> >> Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
> >> ---
> >>  drivers/hwtracing/intel_th/msu.c | 40 +++++++++++++++++++++-----------
> >>  1 file changed, 27 insertions(+), 13 deletions(-)
> >
> > Does not apply to my tree :(
> 
> It's the same one as the one in the fixes series. I just put it here for
> completeness.

to be extra sure it was really applied?  that's funny...

Re: [GIT PULL 1/9] intel_th: msu: Fix unused variable warning on arm64 platform

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

>> > Does not apply to my tree :(
>> 
>> It's the same one as the one in the fixes series. I just put it here for
>> completeness.
>
> to be extra sure it was really applied?  that's funny...

It's also a pull request based on 5.2-rc1. I don't know any other ways
to do it. Oh, yes, it's also explicitly mentioned in the cover letter.

Thanks,
--
Alex

[GIT PULL 2/9] intel_th: msu: Support multipage blocks

[Alexander Shishkin]

Now that the MSU is using scatterlist, we can support multipage blocks.
At the moment, the code assumes that all blocks are page-sized, but in
larger buffers it may make sense to chunk together larger blocks of
memory. One place where one-to-many relationship needs to be handled is
the MSU buffer's mmap path.

Get rid of the implicit assumption that all blocks are page-sized.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 56 ++++++++++++++++++++++----------
 1 file changed, 38 insertions(+), 18 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 8c568b5c8920..45af29af2473 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -33,12 +33,14 @@
  * @entry:	window list linkage (msc::win_list)
  * @pgoff:	page offset into the buffer that this window starts at
  * @nr_blocks:	number of blocks (pages) in this window
+ * @nr_segs:	number of segments in this window (<= @nr_blocks)
  * @sgt:	array of block descriptors
  */
 struct msc_window {
 	struct list_head	entry;
 	unsigned long		pgoff;
 	unsigned int		nr_blocks;
+	unsigned int		nr_segs;
 	struct msc		*msc;
 	struct sg_table		sgt;
 };
@@ -141,6 +143,12 @@ msc_win_block(struct msc_window *win, unsigned int block)
 	return sg_virt(&win->sgt.sgl[block]);
 }
 
+static inline size_t
+msc_win_actual_bsz(struct msc_window *win, unsigned int block)
+{
+	return win->sgt.sgl[block].length;
+}
+
 static inline dma_addr_t
 msc_win_baddr(struct msc_window *win, unsigned int block)
 {
@@ -234,7 +242,7 @@ static unsigned int msc_win_oldest_block(struct msc_window *win)
 	 * with wrapping, last written block contains both the newest and the
 	 * oldest data for this window.
 	 */
-	for (blk = 0; blk < win->nr_blocks; blk++) {
+	for (blk = 0; blk < win->nr_segs; blk++) {
 		bdesc = msc_win_block(win, blk);
 
 		if (msc_block_last_written(bdesc))
@@ -366,7 +374,7 @@ static int msc_iter_block_advance(struct msc_iter *iter)
 		return msc_iter_win_advance(iter);
 
 	/* block advance */
-	if (++iter->block == iter->win->nr_blocks)
+	if (++iter->block == iter->win->nr_segs)
 		iter->block = 0;
 
 	/* no wrapping, sanity check in case there is no last written block */
@@ -478,7 +486,7 @@ static void msc_buffer_clear_hw_header(struct msc *msc)
 		size_t hw_sz = sizeof(struct msc_block_desc) -
 			offsetof(struct msc_block_desc, hw_tag);
 
-		for (blk = 0; blk < win->nr_blocks; blk++) {
+		for (blk = 0; blk < win->nr_segs; blk++) {
 			struct msc_block_desc *bdesc = msc_win_block(win, blk);
 
 			memset(&bdesc->hw_tag, 0, hw_sz);
@@ -734,17 +742,17 @@ static struct page *msc_buffer_contig_get_page(struct msc *msc,
 }
 
 static int __msc_buffer_win_alloc(struct msc_window *win,
-				  unsigned int nr_blocks)
+				  unsigned int nr_segs)
 {
 	struct scatterlist *sg_ptr;
 	void *block;
 	int i, ret;
 
-	ret = sg_alloc_table(&win->sgt, nr_blocks, GFP_KERNEL);
+	ret = sg_alloc_table(&win->sgt, nr_segs, GFP_KERNEL);
 	if (ret)
 		return -ENOMEM;
 
-	for_each_sg(win->sgt.sgl, sg_ptr, nr_blocks, i) {
+	for_each_sg(win->sgt.sgl, sg_ptr, nr_segs, i) {
 		block = dma_alloc_coherent(msc_dev(win->msc)->parent->parent,
 					  PAGE_SIZE, &sg_dma_address(sg_ptr),
 					  GFP_KERNEL);
@@ -754,7 +762,7 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 		sg_set_buf(sg_ptr, block, PAGE_SIZE);
 	}
 
-	return nr_blocks;
+	return nr_segs;
 
 err_nomem:
 	for (i--; i >= 0; i--)
@@ -768,11 +776,11 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 }
 
 #ifdef CONFIG_X86
-static void msc_buffer_set_uc(struct msc_window *win, unsigned int nr_blocks)
+static void msc_buffer_set_uc(struct msc_window *win, unsigned int nr_segs)
 {
 	int i;
 
-	for (i = 0; i < nr_blocks; i++)
+	for (i = 0; i < nr_segs; i++)
 		/* Set the page as uncached */
 		set_memory_uc((unsigned long)msc_win_block(win, i), 1);
 }
@@ -781,13 +789,13 @@ static void msc_buffer_set_wb(struct msc_window *win)
 {
 	int i;
 
-	for (i = 0; i < win->nr_blocks; i++)
+	for (i = 0; i < win->nr_segs; i++)
 		/* Reset the page to write-back */
 		set_memory_wb((unsigned long)msc_win_block(win, i), 1);
 }
 #else /* !X86 */
 static inline void
-msc_buffer_set_uc(struct msc_window *win, unsigned int nr_blocks) {}
+msc_buffer_set_uc(struct msc_window *win, unsigned int nr_segs) {}
 static inline void msc_buffer_set_wb(struct msc_window *win) {}
 #endif /* CONFIG_X86 */
 
@@ -827,7 +835,6 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 							  struct msc_window,
 							  entry);
 
-		/* This works as long as blocks are page-sized */
 		win->pgoff = prev->pgoff + prev->nr_blocks;
 	}
 
@@ -837,7 +844,8 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 
 	msc_buffer_set_uc(win, ret);
 
-	win->nr_blocks = ret;
+	win->nr_segs = ret;
+	win->nr_blocks = nr_blocks;
 
 	if (list_empty(&msc->win_list)) {
 		msc->base = msc_win_block(win, 0);
@@ -860,7 +868,7 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 {
 	int i;
 
-	for (i = 0; i < win->nr_blocks; i++) {
+	for (i = 0; i < win->nr_segs; i++) {
 		struct page *page = sg_page(&win->sgt.sgl[i]);
 
 		page->mapping = NULL;
@@ -923,7 +931,7 @@ static void msc_buffer_relink(struct msc *msc)
 			next_win = list_next_entry(win, entry);
 		}
 
-		for (blk = 0; blk < win->nr_blocks; blk++) {
+		for (blk = 0; blk < win->nr_segs; blk++) {
 			struct msc_block_desc *bdesc = msc_win_block(win, blk);
 
 			memset(bdesc, 0, sizeof(*bdesc));
@@ -934,7 +942,7 @@ static void msc_buffer_relink(struct msc *msc)
 			 * Similarly to last window, last block should point
 			 * to the first one.
 			 */
-			if (blk == win->nr_blocks - 1) {
+			if (blk == win->nr_segs - 1) {
 				sw_tag |= MSC_SW_TAG_LASTBLK;
 				bdesc->next_blk = msc_win_bpfn(win, 0);
 			} else {
@@ -942,7 +950,7 @@ static void msc_buffer_relink(struct msc *msc)
 			}
 
 			bdesc->sw_tag = sw_tag;
-			bdesc->block_sz = PAGE_SIZE / 64;
+			bdesc->block_sz = msc_win_actual_bsz(win, blk) / 64;
 		}
 	}
 
@@ -1101,6 +1109,7 @@ static int msc_buffer_free_unless_used(struct msc *msc)
 static struct page *msc_buffer_get_page(struct msc *msc, unsigned long pgoff)
 {
 	struct msc_window *win;
+	unsigned int blk;
 
 	if (msc->mode == MSC_MODE_SINGLE)
 		return msc_buffer_contig_get_page(msc, pgoff);
@@ -1113,7 +1122,18 @@ static struct page *msc_buffer_get_page(struct msc *msc, unsigned long pgoff)
 
 found:
 	pgoff -= win->pgoff;
-	return sg_page(&win->sgt.sgl[pgoff]);
+
+	for (blk = 0; blk < win->nr_segs; blk++) {
+		struct page *page = sg_page(&win->sgt.sgl[blk]);
+		size_t pgsz = PFN_DOWN(msc_win_actual_bsz(win, blk));
+
+		if (pgoff < pgsz)
+			return page + pgoff;
+
+		pgoff -= pgsz;
+	}
+
+	return NULL;
 }
 
 /**
-- 
2.20.1

[GIT PULL 3/9] intel_th: msu: Split sgt array and pointer in multiwindow mode

[Alexander Shishkin]

To allow the use of externally allocated SG tables further down the line,
change the code to reference the table via a pointer and make it point to
the locally allocated table by default.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 45af29af2473..8efd2510192f 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -34,6 +34,7 @@
  * @pgoff:	page offset into the buffer that this window starts at
  * @nr_blocks:	number of blocks (pages) in this window
  * @nr_segs:	number of segments in this window (<= @nr_blocks)
+ * @_sgt:	array of block descriptors
  * @sgt:	array of block descriptors
  */
 struct msc_window {
@@ -42,7 +43,8 @@ struct msc_window {
 	unsigned int		nr_blocks;
 	unsigned int		nr_segs;
 	struct msc		*msc;
-	struct sg_table		sgt;
+	struct sg_table		_sgt;
+	struct sg_table		*sgt;
 };
 
 /**
@@ -140,19 +142,19 @@ static inline bool msc_block_is_empty(struct msc_block_desc *bdesc)
 static inline struct msc_block_desc *
 msc_win_block(struct msc_window *win, unsigned int block)
 {
-	return sg_virt(&win->sgt.sgl[block]);
+	return sg_virt(&win->sgt->sgl[block]);
 }
 
 static inline size_t
 msc_win_actual_bsz(struct msc_window *win, unsigned int block)
 {
-	return win->sgt.sgl[block].length;
+	return win->sgt->sgl[block].length;
 }
 
 static inline dma_addr_t
 msc_win_baddr(struct msc_window *win, unsigned int block)
 {
-	return sg_dma_address(&win->sgt.sgl[block]);
+	return sg_dma_address(&win->sgt->sgl[block]);
 }
 
 static inline unsigned long
@@ -748,11 +750,11 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 	void *block;
 	int i, ret;
 
-	ret = sg_alloc_table(&win->sgt, nr_segs, GFP_KERNEL);
+	ret = sg_alloc_table(win->sgt, nr_segs, GFP_KERNEL);
 	if (ret)
 		return -ENOMEM;
 
-	for_each_sg(win->sgt.sgl, sg_ptr, nr_segs, i) {
+	for_each_sg(win->sgt->sgl, sg_ptr, nr_segs, i) {
 		block = dma_alloc_coherent(msc_dev(win->msc)->parent->parent,
 					  PAGE_SIZE, &sg_dma_address(sg_ptr),
 					  GFP_KERNEL);
@@ -770,7 +772,7 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 				  msc_win_block(win, i),
 				  msc_win_baddr(win, i));
 
-	sg_free_table(&win->sgt);
+	sg_free_table(win->sgt);
 
 	return -ENOMEM;
 }
@@ -829,6 +831,7 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 		return -ENOMEM;
 
 	win->msc = msc;
+	win->sgt = &win->_sgt;
 
 	if (!list_empty(&msc->win_list)) {
 		struct msc_window *prev = list_last_entry(&msc->win_list,
@@ -869,13 +872,13 @@ static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 	int i;
 
 	for (i = 0; i < win->nr_segs; i++) {
-		struct page *page = sg_page(&win->sgt.sgl[i]);
+		struct page *page = sg_page(&win->sgt->sgl[i]);
 
 		page->mapping = NULL;
 		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
 				  msc_win_block(win, i), msc_win_baddr(win, i));
 	}
-	sg_free_table(&win->sgt);
+	sg_free_table(win->sgt);
 }
 
 /**
@@ -1124,7 +1127,7 @@ static struct page *msc_buffer_get_page(struct msc *msc, unsigned long pgoff)
 	pgoff -= win->pgoff;
 
 	for (blk = 0; blk < win->nr_segs; blk++) {
-		struct page *page = sg_page(&win->sgt.sgl[blk]);
+		struct page *page = sg_page(&win->sgt->sgl[blk]);
 		size_t pgsz = PFN_DOWN(msc_win_actual_bsz(win, blk));
 
 		if (pgoff < pgsz)
-- 
2.20.1

[GIT PULL 4/9] intel_th: msu: Start read iterator from a non-empty window

[Alexander Shishkin]

In multi-window mode, the read iterator is supposed to start from the
window with the oldest data, which is, chronologically, the next window
after the one with the newest data. This, however, fails to take into
account the potentially empty windows, so in short trace sessions it's
possible to have a lot of zeroes read from the character device first.

Fix this by skipping over the empty windows in initialization of the
read iterator.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 42 +++++++++++++++++++++++++-------
 1 file changed, 33 insertions(+), 9 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 8efd2510192f..59a596911e54 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -189,17 +189,18 @@ static struct msc_window *msc_next_window(struct msc_window *win)
 }
 
 /**
- * msc_oldest_window() - locate the window with oldest data
+ * msc_find_window() - find a window matching a given sg_table
  * @msc:	MSC device
+ * @sgt:	SG table of the window
+ * @nonempty:	skip over empty windows
  *
- * This should only be used in multiblock mode. Caller should hold the
- * msc::user_count reference.
- *
- * Return:	the oldest window with valid data
+ * Return:	MSC window structure pointer or NULL if the window
+ *		could not be found.
  */
-static struct msc_window *msc_oldest_window(struct msc *msc)
+static struct msc_window *
+msc_find_window(struct msc *msc, struct sg_table *sgt, bool nonempty)
 {
-	struct msc_window *win, *next = msc_next_window(msc->cur_win);
+	struct msc_window *win;
 	unsigned int found = 0;
 
 	if (list_empty(&msc->win_list))
@@ -211,17 +212,40 @@ static struct msc_window *msc_oldest_window(struct msc *msc)
 	 * something like 2, in which case we're good
 	 */
 	list_for_each_entry(win, &msc->win_list, entry) {
-		if (win == next)
+		if (win->sgt == sgt)
 			found++;
 
 		/* skip the empty ones */
-		if (msc_block_is_empty(msc_win_block(win, 0)))
+		if (nonempty && msc_block_is_empty(msc_win_block(win, 0)))
 			continue;
 
 		if (found)
 			return win;
 	}
 
+	return NULL;
+}
+
+/**
+ * msc_oldest_window() - locate the window with oldest data
+ * @msc:	MSC device
+ *
+ * This should only be used in multiblock mode. Caller should hold the
+ * msc::user_count reference.
+ *
+ * Return:	the oldest window with valid data
+ */
+static struct msc_window *msc_oldest_window(struct msc *msc)
+{
+	struct msc_window *win;
+
+	if (list_empty(&msc->win_list))
+		return NULL;
+
+	win = msc_find_window(msc, msc_next_window(msc->cur_win)->sgt, true);
+	if (win)
+		return win;
+
 	return list_first_entry(&msc->win_list, struct msc_window, entry);
 }
 
-- 
2.20.1

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

[Alexander Shishkin]

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.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 .../testing/sysfs-bus-intel_th-devices-msc    |   3 +-
 MAINTAINERS                                   |   1 +
 drivers/hwtracing/intel_th/msu.c              | 355 +++++++++++++++++-
 drivers/hwtracing/intel_th/msu.h              |  23 +-
 include/linux/intel_th.h                      |  67 ++++
 5 files changed, 419 insertions(+), 30 deletions(-)
 create mode 100644 include/linux/intel_th.h

diff --git a/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc b/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
index f54ae244f3f1..7da00601afdc 100644
--- a/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
+++ b/Documentation/ABI/testing/sysfs-bus-intel_th-devices-msc
@@ -12,7 +12,8 @@ Description:	(RW) Configure MSC operating mode:
 		  - "single", for contiguous buffer mode (high-order alloc);
 		  - "multi", for multiblock mode;
 		  - "ExI", for DCI handler mode;
-		  - "debug", for debug mode.
+		  - "debug", for debug mode;
+		  - any of the currently loaded buffer drivers.
 		If operating mode changes, existing buffer is deallocated,
 		provided there are no active users and tracing is not enabled,
 		otherwise the write will fail.
diff --git a/MAINTAINERS b/MAINTAINERS
index 5cfbea4ce575..9fba97fbac64 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8152,6 +8152,7 @@ M:	Alexander Shishkin <alexander.shishkin@linux.intel.com>
 S:	Supported
 F:	Documentation/trace/intel_th.rst
 F:	drivers/hwtracing/intel_th/
+F:	include/linux/intel_th.h
 
 INTEL(R) TRUSTED EXECUTION TECHNOLOGY (TXT)
 M:	Ning Sun <ning.sun@intel.com>
diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 59a596911e54..fc9890e56e5b 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -17,12 +17,14 @@
 #include <linux/mm.h>
 #include <linux/fs.h>
 #include <linux/io.h>
+#include <linux/workqueue.h>
 #include <linux/dma-mapping.h>
 
 #ifdef CONFIG_X86
 #include <asm/set_memory.h>
 #endif
 
+#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
+};
+
+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;
+}
+
+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))
+		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 (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);
+
+	if (msc->bdrv && msc->bdrv->deactivate)
+		msc->bdrv->deactivate(msc->bdrv_priv);
+	intel_th_msu_deinit(msc);
 	intel_th_trace_disable(msc->thdev);
 
 	if (msc->mode == MSC_MODE_SINGLE) {
@@ -630,6 +804,11 @@ static void msc_disable(struct msc *msc)
 	reg = ioread32(msc->reg_base + REG_MSU_MSC0CTL);
 	reg &= ~MSC_EN;
 	iowrite32(reg, msc->reg_base + REG_MSU_MSC0CTL);
+
+	if (msc->bdrv && msc->bdrv->ready)
+		msc->bdrv->ready(msc->bdrv_priv, win->sgt,
+				 msc_win_total_sz(win));
+
 	msc->enabled = 0;
 
 	iowrite32(0, msc->reg_base + REG_MSU_MSC0BAR);
@@ -640,6 +819,10 @@ static void msc_disable(struct msc *msc)
 
 	reg = ioread32(msc->reg_base + REG_MSU_MSC0STS);
 	dev_dbg(msc_dev(msc), "MSCnSTS: %08x\n", reg);
+
+	reg = ioread32(msc->reg_base + REG_MSU_MSUSTS);
+	reg &= msc->index ? MSUSTS_MSC1BLAST : MSUSTS_MSC0BLAST;
+	iowrite32(reg, msc->reg_base + REG_MSU_MSUSTS);
 }
 
 static int intel_th_msc_activate(struct intel_th_device *thdev)
@@ -865,14 +1048,20 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 		win->pgoff = prev->pgoff + prev->nr_blocks;
 	}
 
-	ret = __msc_buffer_win_alloc(win, nr_blocks);
-	if (ret < 0)
+	if (msc->bdrv && msc->bdrv->alloc_window)
+		ret = msc->bdrv->alloc_window(msc->bdrv_priv, &win->sgt,
+					      nr_blocks << PAGE_SHIFT);
+	else
+		ret = __msc_buffer_win_alloc(win, nr_blocks);
+
+	if (ret <= 0)
 		goto err_nomem;
 
 	msc_buffer_set_uc(win, ret);
 
 	win->nr_segs = ret;
 	win->nr_blocks = nr_blocks;
+	atomic_set(&win->lockout, WIN_READY);
 
 	if (list_empty(&msc->win_list)) {
 		msc->base = msc_win_block(win, 0);
@@ -925,7 +1114,10 @@ static void msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 
 	msc_buffer_set_wb(win);
 
-	__msc_buffer_win_free(msc, win);
+	if (msc->bdrv && msc->bdrv->free_window)
+		msc->bdrv->free_window(msc->bdrv_priv, win->sgt);
+	else
+		__msc_buffer_win_free(msc, win);
 
 	kfree(win);
 }
@@ -1463,18 +1655,76 @@ static void msc_win_switch(struct msc *msc)
 	intel_th_trace_switch(msc->thdev);
 }
 
+/*
+ * @dev:	MSC device to which this relates
+ * @sgt:	buffer driver's sg_table for the window, can't be NULL
+ */
+void intel_th_msc_window_unlock(struct device *dev, struct sg_table *sgt)
+{
+	struct msc *msc = dev_get_drvdata(dev);
+	struct msc_window *win;
+
+	if (WARN_ON_ONCE(!sgt))
+		return;
+
+	win = msc_find_window(msc, sgt, false);
+	if (!win)
+		return;
+
+	msc_win_set_lockout(win, WIN_LOCKED, WIN_READY);
+}
+EXPORT_SYMBOL_GPL(intel_th_msc_window_unlock);
+
+static void msc_work(struct work_struct *work)
+{
+	struct msc *msc = container_of(work, struct msc, work);
+
+	intel_th_msc_deactivate(msc->thdev);
+}
+
 static irqreturn_t intel_th_msc_interrupt(struct intel_th_device *thdev)
 {
 	struct msc *msc = dev_get_drvdata(&thdev->dev);
 	u32 msusts = ioread32(msc->msu_base + REG_MSU_MSUSTS);
 	u32 mask = msc->index ? MSUSTS_MSC1BLAST : MSUSTS_MSC0BLAST;
+	struct msc_window *win, *next_win;
 
-	if (!(msusts & mask)) {
-		if (msc->enabled)
-			return IRQ_HANDLED;
+	if (!msc->do_irq || !msc->bdrv)
 		return IRQ_NONE;
+
+	msusts &= mask;
+
+	if (!msusts)
+		return msc->enabled ? IRQ_HANDLED : IRQ_NONE;
+
+	iowrite32(msusts, msc->msu_base + REG_MSU_MSUSTS);
+
+	if (!msc->enabled)
+		return IRQ_NONE;
+
+	/* grab the window before we do the switch */
+	win = msc->cur_win;
+	if (WARN_ON_ONCE(!win))
+		return IRQ_HANDLED;
+	next_win = msc_next_window(win);
+	if (WARN_ON_ONCE(!next_win))
+		return IRQ_HANDLED;
+
+	/* next window: if READY, proceed, if LOCKED, stop the trace */
+	if (msc_win_set_lockout(next_win, WIN_READY, WIN_INUSE)) {
+		schedule_work(&msc->work);
+		return IRQ_HANDLED;
 	}
 
+	/* current window: INUSE -> LOCKED */
+	msc_win_set_lockout(win, WIN_INUSE, WIN_LOCKED);
+
+	msc_win_switch(msc);
+
+	if (msc->bdrv && msc->bdrv->ready)
+		msc->bdrv->ready(msc->bdrv_priv, win->sgt,
+				 msc_win_total_sz(win));
+
 	return IRQ_HANDLED;
 }
 
@@ -1512,21 +1762,43 @@ wrap_store(struct device *dev, struct device_attribute *attr, const char *buf,
 
 static DEVICE_ATTR_RW(wrap);
 
+static void msc_buffer_unassign(struct msc *msc)
+{
+	lockdep_assert_held(&msc->buf_mutex);
+
+	if (!msc->bdrv)
+		return;
+
+	msc->bdrv->unassign(msc->bdrv_priv);
+	module_put(msc->bdrv->owner);
+	msc->bdrv_priv = NULL;
+	msc->bdrv = NULL;
+}
+
 static ssize_t
 mode_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
 	struct msc *msc = dev_get_drvdata(dev);
+	const char *mode = msc_mode[msc->mode];
+	ssize_t ret;
 
-	return scnprintf(buf, PAGE_SIZE, "%s\n", msc_mode[msc->mode]);
+	mutex_lock(&msc->buf_mutex);
+	if (msc->bdrv)
+		mode = msc->bdrv->name;
+	ret = scnprintf(buf, PAGE_SIZE, "%s\n", mode);
+	mutex_unlock(&msc->buf_mutex);
+
+	return ret;
 }
 
 static ssize_t
 mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
 	   size_t size)
 {
+	const struct msu_buffer_driver *bdrv = NULL;
 	struct msc *msc = dev_get_drvdata(dev);
 	size_t len = size;
-	char *cp;
+	char *cp, *mode;
 	int i, ret;
 
 	if (!capable(CAP_SYS_RAWIO))
@@ -1536,17 +1808,59 @@ mode_store(struct device *dev, struct device_attribute *attr, const char *buf,
 	if (cp)
 		len = cp - buf;
 
-	for (i = 0; i < ARRAY_SIZE(msc_mode); i++)
-		if (!strncmp(msc_mode[i], buf, len))
-			goto found;
+	mode = kstrndup(buf, len, GFP_KERNEL);
+	i = match_string(msc_mode, ARRAY_SIZE(msc_mode), mode);
+	if (i >= 0)
+		goto found;
+
+	/* Buffer drivers only work with a usable IRQ */
+	if (!msc->do_irq) {
+		kfree(mode);
+		return -EINVAL;
+	}
+
+	bdrv = msu_buffer_driver_get(mode);
+	kfree(mode);
+	if (bdrv)
+		goto found;
 
 	return -EINVAL;
 
 found:
 	mutex_lock(&msc->buf_mutex);
+	ret = 0;
+
+	/* Same buffer driver: do nothing */
+	if (bdrv && bdrv == msc->bdrv) {
+		/* matches try_module_get() in  msu_buffer_driver_get() */
+		module_put(bdrv->owner);
+		goto unlock;
+	}
+
 	ret = msc_buffer_unlocked_free_unless_used(msc);
-	if (!ret)
-		msc->mode = i;
+	if (ret)
+		goto unlock;
+
+	if (bdrv) {
+		void *bdrv_priv = bdrv->assign(dev, &i);
+
+		if (!bdrv_priv) {
+			ret = -ENOMEM;
+			goto unlock;
+		}
+
+		msc_buffer_unassign(msc);
+		msc->bdrv_priv = bdrv_priv;
+		msc->bdrv = bdrv;
+	} else {
+		msc_buffer_unassign(msc);
+	}
+
+	msc->mode = i;
+
+unlock:
+	if (ret && bdrv)
+		module_put(bdrv->owner);
 	mutex_unlock(&msc->buf_mutex);
 
 	return ret ? ret : size;
@@ -1668,7 +1982,12 @@ win_switch_store(struct device *dev, struct device_attribute *attr,
 		return -EINVAL;
 
 	mutex_lock(&msc->buf_mutex);
-	if (msc->mode != MSC_MODE_MULTI)
+	/*
+	 * Window switch can only happen in the "multi" mode.
+	 * If a buffer driver is engaged, they have the full
+	 * control over window switching.
+	 */
+	if (msc->mode != MSC_MODE_MULTI || msc->bdrv)
 		ret = -ENOTSUPP;
 	else
 		msc_win_switch(msc);
@@ -1721,10 +2040,7 @@ static int intel_th_msc_probe(struct intel_th_device *thdev)
 	msc->reg_base = base + msc->index * 0x100;
 	msc->msu_base = base;
 
-	err = intel_th_msu_init(msc);
-	if (err)
-		return err;
-
+	INIT_WORK(&msc->work, msc_work);
 	err = intel_th_msc_init(msc);
 	if (err)
 		return err;
@@ -1740,7 +2056,6 @@ static void intel_th_msc_remove(struct intel_th_device *thdev)
 	int ret;
 
 	intel_th_msc_deactivate(thdev);
-	intel_th_msu_deinit(msc);
 
 	/*
 	 * Buffers should not be used at this point except if the
diff --git a/drivers/hwtracing/intel_th/msu.h b/drivers/hwtracing/intel_th/msu.h
index 574c16004cb2..acd808e316f9 100644
--- a/drivers/hwtracing/intel_th/msu.h
+++ b/drivers/hwtracing/intel_th/msu.h
@@ -44,14 +44,6 @@ enum {
 #define M0BLIE		BIT(16)
 #define M1BLIE		BIT(24)
 
-/* MSC operating modes (MSC_MODE) */
-enum {
-	MSC_MODE_SINGLE	= 0,
-	MSC_MODE_MULTI,
-	MSC_MODE_EXI,
-	MSC_MODE_DEBUG,
-};
-
 /* MSCnSTS bits */
 #define MSCSTS_WRAPSTAT	BIT(1)	/* Wrap occurred */
 #define MSCSTS_PLE	BIT(2)	/* Pipeline Empty */
@@ -93,6 +85,19 @@ static inline unsigned long msc_data_sz(struct msc_block_desc *bdesc)
 	return bdesc->valid_dw * 4 - MSC_BDESC;
 }
 
+static inline unsigned long msc_total_sz(struct msc_block_desc *bdesc)
+{
+	if (!bdesc->valid_dw)
+		return 0;
+
+	return bdesc->valid_dw * 4;
+}
+
+static inline unsigned long msc_block_sz(struct msc_block_desc *bdesc)
+{
+	return bdesc->block_sz * 64 - MSC_BDESC;
+}
+
 static inline bool msc_block_wrapped(struct msc_block_desc *bdesc)
 {
 	if (bdesc->hw_tag & (MSC_HW_TAG_BLOCKWRAP | MSC_HW_TAG_WINWRAP))
@@ -104,7 +109,7 @@ static inline bool msc_block_wrapped(struct msc_block_desc *bdesc)
 static inline bool msc_block_last_written(struct msc_block_desc *bdesc)
 {
 	if ((bdesc->hw_tag & MSC_HW_TAG_ENDBIT) ||
-	    (msc_data_sz(bdesc) != DATA_IN_PAGE))
+	    (msc_data_sz(bdesc) != msc_block_sz(bdesc)))
 		return true;
 
 	return false;
diff --git a/include/linux/intel_th.h b/include/linux/intel_th.h
new file mode 100644
index 000000000000..b00b7e1254d9
--- /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;
+	/*
+	 * ->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);
+};
+
+int intel_th_msu_buffer_register(const struct msu_buffer_driver *bdrv);
+void intel_th_msu_buffer_unregister(const struct msu_buffer_driver *bdrv);
+void intel_th_msc_window_unlock(struct device *dev, struct sg_table *sgt);
+
+#endif /* _INTEL_TH_H_ */
-- 
2.20.1

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

[Greg Kroah-Hartman]

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

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

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

>> +	/*
>> +	 * ->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'?

It's not a real driver, in a sense that there's no underlying
device. None of the usual driver stuff applies. It's still a set of
callbacks, though. Should this be an elaborate comment, should I replace
the word "driver" with something else?

I'd really like to avoid shoehorning the whole 'struct device' + 'struct
driver' here.

Thanks,
--
Alex

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

[Greg Kroah-Hartman]

On Wed, Jul 03, 2019 at 07:33:58PM +0300, Alexander Shishkin wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> 
> >> +	/*
> >> +	 * ->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'?
> 
> It's not a real driver, in a sense that there's no underlying
> device. None of the usual driver stuff applies.

Then do not call it a "driver", as in the kernel we have a very
well-defined and known definition of a driver.  Call it something else
please.  Yes, naming is hard, but don't try to overload onto an already
existing name.

> It's still a set of callbacks, though. Should this be an elaborate
> comment, should I replace the word "driver" with something else?

Yes.

> I'd really like to avoid shoehorning the whole 'struct device' +
> 'struct driver' here.

Why not?  If you have a driver, just make it a real one.  It not take
all that much boiler-plate code to do so and then you get all of the
things you will want in the end anyway (sysfs representation,
attributes, auto-loading of modules, etc.)

Try doing it "for real" and see what happens.

thanks,

greg k-h

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

[Alexander Shishkin]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:

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

Fixed & updated version [1] should be in your inbox about now.

[1] https://marc.info/?l=linux-kernel&m=156233609417236

Thanks,
--
Alex

[GIT PULL 6/9] intel_th: msu: Prevent freeing buffers while locked windows exist

[Alexander Shishkin]

We already prevent freeing buffers via sysfs interface in case there are
existing users or if trace is active. Treat the existence of locked windows
similarly and return -EBUSY on attempts to free the buffer. When the last
window is unlocked, the freeing will succeed.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index fc9890e56e5b..f2d52c025528 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -703,6 +703,11 @@ static int msc_win_set_lockout(struct msc_window *win, int expect, int new)
 	if (expect == WIN_LOCKED && old == new)
 		return 0;
 
+	if (old == expect && new == WIN_LOCKED)
+		atomic_inc(&win->msc->user_count);
+	else if (old == expect && old == WIN_LOCKED)
+		atomic_dec(&win->msc->user_count);
+
 	if (WARN_ONCE(old != expect, "expected lockout state %d, got %d\n",
 		      expect, old))
 		return -EINVAL;
-- 
2.20.1

[GIT PULL 7/9] intel_th: msu: Get rid of the window size limit

[Alexander Shishkin]

Currently, the window size is limited to the maximum number of sg entries
in one table. This is because the code addresses individual blocks within
the window by their numeric index. In reality, though, the blocks most
often are iterated through sequentially. By rewriting the logic to use sg
pointers instead of block indices we loose the necessity to dereference
them directly and gain the ability to use multiple chained tables if
necessary.

Get rid of the limitation by replacing index-based block accesses with
sequential block accesses.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 139 +++++++++++++++----------------
 1 file changed, 68 insertions(+), 71 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index f2d52c025528..58dd381debd2 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -70,8 +70,8 @@ struct msc_iter {
 	struct msc_window	*start_win;
 	struct msc_window	*win;
 	unsigned long		offset;
-	int			start_block;
-	int			block;
+	struct scatterlist	*start_block;
+	struct scatterlist	*block;
 	unsigned int		block_off;
 	unsigned int		wrap_count;
 	unsigned int		eof;
@@ -251,28 +251,25 @@ static inline bool msc_block_is_empty(struct msc_block_desc *bdesc)
 	return false;
 }
 
-static inline struct msc_block_desc *
-msc_win_block(struct msc_window *win, unsigned int block)
+static inline struct scatterlist *msc_win_base_sg(struct msc_window *win)
 {
-	return sg_virt(&win->sgt->sgl[block]);
+	return win->sgt->sgl;
 }
 
-static inline size_t
-msc_win_actual_bsz(struct msc_window *win, unsigned int block)
+static inline struct msc_block_desc *msc_win_base(struct msc_window *win)
 {
-	return win->sgt->sgl[block].length;
+	return sg_virt(msc_win_base_sg(win));
 }
 
-static inline dma_addr_t
-msc_win_baddr(struct msc_window *win, unsigned int block)
+static inline dma_addr_t msc_win_base_dma(struct msc_window *win)
 {
-	return sg_dma_address(&win->sgt->sgl[block]);
+	return sg_dma_address(msc_win_base_sg(win));
 }
 
 static inline unsigned long
-msc_win_bpfn(struct msc_window *win, unsigned int block)
+msc_win_base_pfn(struct msc_window *win)
 {
-	return msc_win_baddr(win, block) >> PAGE_SHIFT;
+	return PFN_DOWN(msc_win_base_dma(win));
 }
 
 /**
@@ -302,11 +299,12 @@ static struct msc_window *msc_next_window(struct msc_window *win)
 
 static size_t msc_win_total_sz(struct msc_window *win)
 {
+	struct scatterlist *sg;
 	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);
+	for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
+		struct msc_block_desc *bdesc = sg_virt(sg);
 
 		if (msc_block_wrapped(bdesc))
 			return win->nr_blocks << PAGE_SHIFT;
@@ -347,7 +345,7 @@ msc_find_window(struct msc *msc, struct sg_table *sgt, bool nonempty)
 			found++;
 
 		/* skip the empty ones */
-		if (nonempty && msc_block_is_empty(msc_win_block(win, 0)))
+		if (nonempty && msc_block_is_empty(msc_win_base(win)))
 			continue;
 
 		if (found)
@@ -381,44 +379,38 @@ static struct msc_window *msc_oldest_window(struct msc *msc)
 }
 
 /**
- * msc_win_oldest_block() - locate the oldest block in a given window
+ * msc_win_oldest_sg() - locate the oldest block in a given window
  * @win:	window to look at
  *
  * Return:	index of the block with the oldest data
  */
-static unsigned int msc_win_oldest_block(struct msc_window *win)
+static struct scatterlist *msc_win_oldest_sg(struct msc_window *win)
 {
 	unsigned int blk;
-	struct msc_block_desc *bdesc = msc_win_block(win, 0);
+	struct scatterlist *sg;
+	struct msc_block_desc *bdesc = msc_win_base(win);
 
 	/* without wrapping, first block is the oldest */
 	if (!msc_block_wrapped(bdesc))
-		return 0;
+		return msc_win_base_sg(win);
 
 	/*
 	 * with wrapping, last written block contains both the newest and the
 	 * oldest data for this window.
 	 */
-	for (blk = 0; blk < win->nr_segs; blk++) {
-		bdesc = msc_win_block(win, blk);
+	for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
+		struct msc_block_desc *bdesc = sg_virt(sg);
 
 		if (msc_block_last_written(bdesc))
-			return blk;
+			return sg;
 	}
 
-	return 0;
+	return msc_win_base_sg(win);
 }
 
 static struct msc_block_desc *msc_iter_bdesc(struct msc_iter *iter)
 {
-	return msc_win_block(iter->win, iter->block);
-}
-
-static void msc_iter_init(struct msc_iter *iter)
-{
-	memset(iter, 0, sizeof(*iter));
-	iter->start_block = -1;
-	iter->block = -1;
+	return sg_virt(iter->block);
 }
 
 static struct msc_iter *msc_iter_install(struct msc *msc)
@@ -443,7 +435,6 @@ static struct msc_iter *msc_iter_install(struct msc *msc)
 		goto unlock;
 	}
 
-	msc_iter_init(iter);
 	iter->msc = msc;
 
 	list_add_tail(&iter->entry, &msc->iter_list);
@@ -464,10 +455,10 @@ static void msc_iter_remove(struct msc_iter *iter, struct msc *msc)
 
 static void msc_iter_block_start(struct msc_iter *iter)
 {
-	if (iter->start_block != -1)
+	if (iter->start_block)
 		return;
 
-	iter->start_block = msc_win_oldest_block(iter->win);
+	iter->start_block = msc_win_oldest_sg(iter->win);
 	iter->block = iter->start_block;
 	iter->wrap_count = 0;
 
@@ -491,7 +482,7 @@ static int msc_iter_win_start(struct msc_iter *iter, struct msc *msc)
 		return -EINVAL;
 
 	iter->win = iter->start_win;
-	iter->start_block = -1;
+	iter->start_block = NULL;
 
 	msc_iter_block_start(iter);
 
@@ -501,7 +492,7 @@ static int msc_iter_win_start(struct msc_iter *iter, struct msc *msc)
 static int msc_iter_win_advance(struct msc_iter *iter)
 {
 	iter->win = msc_next_window(iter->win);
-	iter->start_block = -1;
+	iter->start_block = NULL;
 
 	if (iter->win == iter->start_win) {
 		iter->eof++;
@@ -531,8 +522,10 @@ static int msc_iter_block_advance(struct msc_iter *iter)
 		return msc_iter_win_advance(iter);
 
 	/* block advance */
-	if (++iter->block == iter->win->nr_segs)
-		iter->block = 0;
+	if (sg_is_last(iter->block))
+		iter->block = msc_win_base_sg(iter->win);
+	else
+		iter->block = sg_next(iter->block);
 
 	/* no wrapping, sanity check in case there is no last written block */
 	if (!iter->wrap_count && iter->block == iter->start_block)
@@ -637,14 +630,15 @@ msc_buffer_iterate(struct msc_iter *iter, size_t size, void *data,
 static void msc_buffer_clear_hw_header(struct msc *msc)
 {
 	struct msc_window *win;
+	struct scatterlist *sg;
 
 	list_for_each_entry(win, &msc->win_list, entry) {
 		unsigned int blk;
 		size_t hw_sz = sizeof(struct msc_block_desc) -
 			offsetof(struct msc_block_desc, hw_tag);
 
-		for (blk = 0; blk < win->nr_segs; blk++) {
-			struct msc_block_desc *bdesc = msc_win_block(win, blk);
+		for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
+			struct msc_block_desc *bdesc = sg_virt(sg);
 
 			memset(&bdesc->hw_tag, 0, hw_sz);
 		}
@@ -979,10 +973,9 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 	return nr_segs;
 
 err_nomem:
-	for (i--; i >= 0; i--)
+	for_each_sg(win->sgt->sgl, sg_ptr, i, ret)
 		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
-				  msc_win_block(win, i),
-				  msc_win_baddr(win, i));
+				  sg_virt(sg_ptr), sg_dma_address(sg_ptr));
 
 	sg_free_table(win->sgt);
 
@@ -992,20 +985,26 @@ static int __msc_buffer_win_alloc(struct msc_window *win,
 #ifdef CONFIG_X86
 static void msc_buffer_set_uc(struct msc_window *win, unsigned int nr_segs)
 {
+	struct scatterlist *sg_ptr;
 	int i;
 
-	for (i = 0; i < nr_segs; i++)
+	for_each_sg(win->sgt->sgl, sg_ptr, nr_segs, i) {
 		/* Set the page as uncached */
-		set_memory_uc((unsigned long)msc_win_block(win, i), 1);
+		set_memory_uc((unsigned long)sg_virt(sg_ptr),
+			      PFN_DOWN(sg_ptr->length));
+	}
 }
 
 static void msc_buffer_set_wb(struct msc_window *win)
 {
+	struct scatterlist *sg_ptr;
 	int i;
 
-	for (i = 0; i < win->nr_segs; i++)
+	for_each_sg(win->sgt->sgl, sg_ptr, win->nr_segs, i) {
 		/* Reset the page to write-back */
-		set_memory_wb((unsigned long)msc_win_block(win, i), 1);
+		set_memory_wb((unsigned long)sg_virt(sg_ptr),
+			      PFN_DOWN(sg_ptr->length));
+	}
 }
 #else /* !X86 */
 static inline void
@@ -1031,13 +1030,6 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 	if (!nr_blocks)
 		return 0;
 
-	/*
-	 * This limitation hold as long as we need random access to the
-	 * block. When that changes, this can go away.
-	 */
-	if (nr_blocks > SG_MAX_SINGLE_ALLOC)
-		return -EINVAL;
-
 	win = kzalloc(sizeof(*win), GFP_KERNEL);
 	if (!win)
 		return -ENOMEM;
@@ -1069,8 +1061,8 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 	atomic_set(&win->lockout, WIN_READY);
 
 	if (list_empty(&msc->win_list)) {
-		msc->base = msc_win_block(win, 0);
-		msc->base_addr = msc_win_baddr(win, 0);
+		msc->base = msc_win_base(win);
+		msc->base_addr = msc_win_base_dma(win);
 		msc->cur_win = win;
 	}
 
@@ -1087,14 +1079,15 @@ static int msc_buffer_win_alloc(struct msc *msc, unsigned int nr_blocks)
 
 static void __msc_buffer_win_free(struct msc *msc, struct msc_window *win)
 {
+	struct scatterlist *sg;
 	int i;
 
-	for (i = 0; i < win->nr_segs; i++) {
-		struct page *page = sg_page(&win->sgt->sgl[i]);
+	for_each_sg(win->sgt->sgl, sg, win->nr_segs, i) {
+		struct page *page = sg_page(sg);
 
 		page->mapping = NULL;
 		dma_free_coherent(msc_dev(win->msc)->parent->parent, PAGE_SIZE,
-				  msc_win_block(win, i), msc_win_baddr(win, i));
+				  sg_virt(sg), sg_dma_address(sg));
 	}
 	sg_free_table(win->sgt);
 }
@@ -1140,6 +1133,7 @@ static void msc_buffer_relink(struct msc *msc)
 
 	/* call with msc::mutex locked */
 	list_for_each_entry(win, &msc->win_list, entry) {
+		struct scatterlist *sg;
 		unsigned int blk;
 		u32 sw_tag = 0;
 
@@ -1155,12 +1149,12 @@ static void msc_buffer_relink(struct msc *msc)
 			next_win = list_next_entry(win, entry);
 		}
 
-		for (blk = 0; blk < win->nr_segs; blk++) {
-			struct msc_block_desc *bdesc = msc_win_block(win, blk);
+		for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
+			struct msc_block_desc *bdesc = sg_virt(sg);
 
 			memset(bdesc, 0, sizeof(*bdesc));
 
-			bdesc->next_win = msc_win_bpfn(next_win, 0);
+			bdesc->next_win = msc_win_base_pfn(next_win);
 
 			/*
 			 * Similarly to last window, last block should point
@@ -1168,13 +1162,15 @@ static void msc_buffer_relink(struct msc *msc)
 			 */
 			if (blk == win->nr_segs - 1) {
 				sw_tag |= MSC_SW_TAG_LASTBLK;
-				bdesc->next_blk = msc_win_bpfn(win, 0);
+				bdesc->next_blk = msc_win_base_pfn(win);
 			} else {
-				bdesc->next_blk = msc_win_bpfn(win, blk + 1);
+				dma_addr_t addr = sg_dma_address(sg_next(sg));
+
+				bdesc->next_blk = PFN_DOWN(addr);
 			}
 
 			bdesc->sw_tag = sw_tag;
-			bdesc->block_sz = msc_win_actual_bsz(win, blk) / 64;
+			bdesc->block_sz = sg->length / 64;
 		}
 	}
 
@@ -1333,6 +1329,7 @@ static int msc_buffer_free_unless_used(struct msc *msc)
 static struct page *msc_buffer_get_page(struct msc *msc, unsigned long pgoff)
 {
 	struct msc_window *win;
+	struct scatterlist *sg;
 	unsigned int blk;
 
 	if (msc->mode == MSC_MODE_SINGLE)
@@ -1347,9 +1344,9 @@ static struct page *msc_buffer_get_page(struct msc *msc, unsigned long pgoff)
 found:
 	pgoff -= win->pgoff;
 
-	for (blk = 0; blk < win->nr_segs; blk++) {
-		struct page *page = sg_page(&win->sgt->sgl[blk]);
-		size_t pgsz = PFN_DOWN(msc_win_actual_bsz(win, blk));
+	for_each_sg(win->sgt->sgl, sg, win->nr_segs, blk) {
+		struct page *page = sg_page(sg);
+		size_t pgsz = PFN_DOWN(sg->length);
 
 		if (pgoff < pgsz)
 			return page + pgoff;
@@ -1654,8 +1651,8 @@ static void msc_win_switch(struct msc *msc)
 	else
 		msc->cur_win = list_next_entry(msc->cur_win, entry);
 
-	msc->base = msc_win_block(msc->cur_win, 0);
-	msc->base_addr = msc_win_baddr(msc->cur_win, 0);
+	msc->base = msc_win_base(msc->cur_win);
+	msc->base_addr = msc_win_base_dma(msc->cur_win);
 
 	intel_th_trace_switch(msc->thdev);
 }
-- 
2.20.1

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

[Alexander Shishkin]

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,
+	.assign		= msu_sink_assign,
+	.unassign	= msu_sink_unassign,
+	.alloc_window	= msu_sink_alloc_window,
+	.free_window	= msu_sink_free_window,
+	.ready		= msu_sink_ready,
+};
+
+static int msu_sink_init(void)
+{
+	return intel_th_msu_buffer_register(&sink_bdrv);
+}
+module_init(msu_sink_init);
+
+static void msu_sink_exit(void)
+{
+	intel_th_msu_buffer_unregister(&sink_bdrv);
+}
+module_exit(msu_sink_exit);
+
+MODULE_LICENSE("GPL v2");
-- 
2.20.1

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

[Greg Kroah-Hartman]

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

[GIT PULL 9/9] intel_th: msu: Preserve pre-existing buffer configuration

[Alexander Shishkin]

The MSU configuration registers may contain buffer address/size set by
the BIOS or an external hardware debugger, which may want to take over
tracing from the driver when the driver is not actively tracing.

Preserve these settings when not actively tracing.

Signed-off-by: Alexander Shishkin <alexander.shishkin@linux.intel.com>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/hwtracing/intel_th/msu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/intel_th/msu.c b/drivers/hwtracing/intel_th/msu.c
index 58dd381debd2..9b6ef4edf09e 100644
--- a/drivers/hwtracing/intel_th/msu.c
+++ b/drivers/hwtracing/intel_th/msu.c
@@ -116,6 +116,8 @@ struct msc {
 	unsigned int		single_wrap : 1;
 	void			*base;
 	dma_addr_t		base_addr;
+	u32			orig_addr;
+	u32			orig_sz;
 
 	/* <0: no buffer, 0: no users, >0: active users */
 	atomic_t		user_count;
@@ -736,6 +738,9 @@ static int msc_configure(struct msc *msc)
 		msc_buffer_clear_hw_header(msc);
 	}
 
+	msc->orig_addr = ioread32(msc->reg_base + REG_MSU_MSC0BAR);
+	msc->orig_sz   = ioread32(msc->reg_base + REG_MSU_MSC0SIZE);
+
 	reg = msc->base_addr >> PAGE_SHIFT;
 	iowrite32(reg, msc->reg_base + REG_MSU_MSC0BAR);
 
@@ -810,8 +815,8 @@ static void msc_disable(struct msc *msc)
 
 	msc->enabled = 0;
 
-	iowrite32(0, msc->reg_base + REG_MSU_MSC0BAR);
-	iowrite32(0, msc->reg_base + REG_MSU_MSC0SIZE);
+	iowrite32(msc->orig_addr, msc->reg_base + REG_MSU_MSC0BAR);
+	iowrite32(msc->orig_sz, msc->reg_base + REG_MSU_MSC0SIZE);
 
 	dev_dbg(msc_dev(msc), "MSCnNWSA: %08x\n",
 		ioread32(msc->reg_base + REG_MSU_MSC0NWSA));
-- 
2.20.1