Re: [PATCH V2 1/6] coresight: etb10: Properly set AUX buffer head in snapshot mode

From: Suzuki K Poulose <suzuki.poulose@arm.com>
To: mathieu.poirier@linaro.org
Cc: alexander.shishkin@linux.intel.com, coresight@lists.linaro.org,
	acme@kernel.org, peterz@infradead.org, mingo@redhat.com,
	leo.yan@linaro.org, linux-arm-kernel@lists.infradead.org,
	mike.leach@linaro.org
Subject: Re: [PATCH V2 1/6] coresight: etb10: Properly set AUX buffer head in snapshot mode
Date: Thu, 16 May 2019 10:41:22 +0100	[thread overview]
Message-ID: <d7f7ae43-4c73-724f-7080-c54279a05045@arm.com> (raw)
In-Reply-To: <CANLsYkwGyazUAB5Xs1PFn+sgT78c7DBqc6VDDPr4iBYx_Os+Ng@mail.gmail.com>

Hi Mathieu,

On 15/05/2019 15:28, Mathieu Poirier wrote:
> Good day Suzuki,
> 
> On Wed, 15 May 2019 at 03:45, Suzuki K Poulose <suzuki.poulose@arm.com> wrote:
>>
>> Hi Mathieu,
>>
>> On 14/05/2019 20:40, Mathieu Poirier wrote:
>>> Unify amongst sink drivers how the AUX ring buffer head is communicated
>>> to user space.  That way the same algorithm in cs_etm_find_snapshot()
>>
>> I would leave the userspace tool's function name out of the commit description
>> and the comment below. We could instead say: "That way the same algorithm can be
>> used by the userspace tool to determine the position and the size of the latest
>> snapshot data."
> 
> I purposely added the name of the function there so that people can
> quickly find it and avoid any misunderstanding about the code in
> question.  But I also have the same information as a comment in the
> code, which should be sufficient.  I'll fix it.
> 

No need to resend the series as it is just the comment and description.
You may fix up both before committing.

>>> diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c
>>> index 4ee4c80a4354..60e753b1768d 100644
>>> --- a/drivers/hwtracing/coresight/coresight-etb10.c
>>> +++ b/drivers/hwtracing/coresight/coresight-etb10.c
>>> @@ -548,13 +548,14 @@ static unsigned long etb_update_buffer(struct coresight_device *csdev,
>>>        writel_relaxed(0x0, drvdata->base + ETB_RAM_WRITE_POINTER);
>>>
>>>        /*
>>> -      * In snapshot mode we have to update the handle->head to point
>>> -      * to the new location.
>>> +      * In snapshot mode we simply increment the head by the number of byte
>>> +      * that were written.  User space function  cs_etm_find_snapshot() will
>>> +      * figure out how many bytes to get from the AUX buffer based on the
>>> +      * position of the head.
>>>         */
>>> -     if (buf->snapshot) {
>>> -             handle->head = (cur * PAGE_SIZE) + offset;
>>> -             to_read = buf->nr_pages << PAGE_SHIFT;
>>> -     }
>>> +     if (buf->snapshot)
>>> +             handle->head += to_read;
>>> +
>>>        __etb_enable_hw(drvdata);
>>>        CS_LOCK(drvdata->base);
>>>    out:
>>
>> Otherwise:
>>
>> Reviewed-by: Suzuki K Poulose <suzuki.poulose@arm.com>
> 
> Is this for all the kernel space patches or just this one?

It was initially for the first patch. But now I realize that
all the other patches are of similar approach. I will add a
different tag for better tracking.

Cheers
Suzuki

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel