diff mbox series

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

Message ID 20190514194018.23420-2-mathieu.poirier@linaro.org (mailing list archive)
State New, archived
Headers show
Series coresight: Fix snapshot mode | expand

Commit Message

Mathieu Poirier May 14, 2019, 7:40 p.m. UTC
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()
can be used to determine where the latest data is and how much of it
to access.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/hwtracing/coresight/coresight-etb10.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

Comments

Suzuki K Poulose May 15, 2019, 9:45 a.m. UTC | #1
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."

> can be used to determine where the latest data is and how much of it
> to access.
> 
> Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> ---
>   drivers/hwtracing/coresight/coresight-etb10.c | 13 +++++++------
>   1 file changed, 7 insertions(+), 6 deletions(-)
> 
> 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>
Mathieu Poirier May 15, 2019, 2:28 p.m. UTC | #2
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.

>
> > can be used to determine where the latest data is and how much of it
> > to access.
> >
> > Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
> > ---
> >   drivers/hwtracing/coresight/coresight-etb10.c | 13 +++++++------
> >   1 file changed, 7 insertions(+), 6 deletions(-)
> >
> > 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?
Suzuki K Poulose May 16, 2019, 9:41 a.m. UTC | #3
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
diff mbox series

Patch

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: