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

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

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.

> 
> 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