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

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

On 25/05/18 17:43, Mathieu Poirier wrote:
> 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.

Agree. On a second thought, those helpers are not used anywhere now. Also,
we only use the base addresses just after creation of the table and
we have necessary guards to make sure the table is actually created.
I suspect this was a left over from my original code. I am tempted to
rather remove them.

Suzuki