Re: [PATCH 08/11] coresight: Add generic TMC sg table framework

[Mathieu Poirier <mathieu.poirier@linaro.org>]

On Fri, May 25, 2018 at 05:07:07PM +0100, Suzuki K Poulose wrote:
> On 23/05/18 21:25, Mathieu Poirier wrote:
> >On Fri, May 18, 2018 at 05:39:24PM +0100, Suzuki K Poulose wrote:
> >>This patch introduces a generic sg table data structure and
> >>associated operations. An SG table can be used to map a set
> >>of Data pages where the trace data could be stored by the TMC
> >>ETR. The information about the data pages could be stored in
> >>different formats, depending on the type of the underlying
> >>SG mechanism (e.g, TMC ETR SG vs Coresight CATU). The generic
> >>structure provides book keeping of the pages used for the data
> >>as well as the table contents. The table should be filled by
> >>the user of the infrastructure.
> >>
> >>A table can be created by specifying the number of data pages
> >>as well as the number of table pages required to hold the
> >>pointers, where the latter could be different for different
> >>types of tables. The pages are mapped in the appropriate dma
> >>data direction mode (i.e, DMA_TO_DEVICE for table pages
> >>and DMA_FROM_DEVICE for data pages).  The framework can optionally
> >>accept a set of allocated data pages (e.g, perf ring buffer) and
> >>map them accordingly. The table and data pages are vmap'ed to allow
> >>easier access by the drivers. The framework also provides helpers to
> >>sync the data written to the pages with appropriate directions.
> >>
> >>This will be later used by the TMC ETR SG unit and CATU.
> >>
> >>Cc: Mathieu Poirier <mathieu.poirier@linaro.org>
> >>Signed-off-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>---
> >>Changes since v1:
> >>  - Address code style issues, more comments
> >>---
> >>  drivers/hwtracing/coresight/coresight-tmc-etr.c | 290 ++++++++++++++++++++++++
> >>  drivers/hwtracing/coresight/coresight-tmc.h     |  50 ++++
> >>  2 files changed, 340 insertions(+)
> >>
> >>diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>index 9780798..1e844f8 100644
> >>--- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>+++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> >>@@ -17,9 +17,299 @@
> 
> 
> >>+static inline dma_addr_t tmc_sg_table_base_paddr(struct tmc_sg_table *sg_table)
> >>+{
> >>+	if (WARN_ON(!sg_table->data_pages.pages[0]))
> >>+		return 0;
> >>+	return sg_table->table_daddr;
> >>+}
> >>+
> >>+static inline void *tmc_sg_table_base_vaddr(struct tmc_sg_table *sg_table)
> >>+{
> >>+	if (WARN_ON(!sg_table->data_pages.pages[0]))
> >>+		return NULL;
> >>+	return sg_table->table_vaddr;
> >>+}
> >
> >The above two functions deal with DMA'able and virtual addresses for the table
> >page buffer.  Yet the test in the WARN_ON is done on the data page array.
> >Shouldn't this be sg_table->table_pages.pages[0] instead?
> 
> The table is as good as empty if there are no data pages associated with
> the table. Hence the data_pages check.

That is correct.  On the flip side you can't have data_pages without table_pages
and vice versa, hence my comment.

> 
> >
> >If not please add a comment justifying your position so that someone else
> >looking at the code does't end up thinking the same in a year from now.
> 
> I will add a comment to reflect the above.
> 
> >
> >>+
> >>+static inline void *
> >>+tmc_sg_table_data_vaddr(struct tmc_sg_table *sg_table)
> >>+{
> >>+	if (WARN_ON(!sg_table->data_pages.nr_pages))
> >>+		return 0;
> >>+	return sg_table->data_vaddr;
> >>+}
> >
> >I see that tmc_sg_table_base_vaddr() and tmc_sg_table_data_vaddr() are both
> >returning the virtual address of the contiguous buffer for table and data
> >respectively.  Yet there is a discrepency in the naming convention.  I would
> >have expected tmc_sg_table_base_vaddr() and tmc_sg_data_base_vaddr() so that
> >there is a little symmetry between them.
> 
> Agree. I will fix it.
> 
> Suzuki