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