Re: [PATCH 1/5] coresight: Fix buffer size in snapshot mode

On Tue, 7 May 2019 at 01:39, Leo Yan leo.yan@linaro.org wrote:

On Wed, May 01, 2019 at 11:50:48AM -0600, Mathieu Poirier wrote:

...

In snapshot mode the buffer used by the sink devices need to be equal to the ring buffer size in order for the user space mechanic to work properly.

Signed-off-by: Mathieu Poirier mathieu.poirier@linaro.org

drivers/hwtracing/coresight/coresight-etb10.c | 23 +++++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etf.c | 20 ++++++++++++++++ .../hwtracing/coresight/coresight-tmc-etr.c | 8 +++++-- 3 files changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/hwtracing/coresight/coresight-etb10.c b/drivers/hwtracing/coresight/coresight-etb10.c index 4ee4c80a4354..0764647b92bc 100644 --- a/drivers/hwtracing/coresight/coresight-etb10.c +++ b/drivers/hwtracing/coresight/coresight-etb10.c @@ -374,7 +374,30 @@ static void *etb_alloc_buffer(struct coresight_device *csdev, int nr_pages, bool overwrite) { int node, cpu = event->cpu;

• u32 capacity;
  struct cs_buffers *buf;
  

• struct etb_drvdata *drvdata;
  

• /*
  

•  * In snapsot mode the size of the perf ring buffer needs to be equal
  

•  * to the size of the device's internal memory if we want to reuse the
  

•  * generic AUX buffer management mechanic.
  

•  *
  

•  * For example (assuming 4096 byte page size):
  

Here is delibrately to write as '4096 byte'? Though I think should be '4096 bytes' but I am not confident which is right ...

Well, English isn't my first language either but I think it is correct since I am referring to "the" page size and "4096 byte" is an adjective of it.

...

•  *
  

•  *    # cat /sys/bus/coresight/devices/20010000.etb/mgmt/rdp
  

•  *    0x2000
  

•  *    # perf record -e cs_etm/@20010000.etf/ -S -m,8 --per-thread $APP
  

•  *
  

•  */
  

• drvdata = dev_get_drvdata(csdev->dev.parent);
  

• capacity = drvdata->buffer_depth * ETB_FRAME_SIZE_WORDS;
  

• if (overwrite &&
  

•     ((nr_pages << PAGE_SHIFT) != capacity)) {
  

•         dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
  

•         return NULL;
  

• }
  
  if (cpu == -1)
          cpu = smp_processor_id();
  

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etf.c b/drivers/hwtracing/coresight/coresight-tmc-etf.c index 2527b5d3b65e..7694833b13cb 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etf.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etf.c @@ -380,6 +380,26 @@ static void *tmc_alloc_etf_buffer(struct coresight_device *csdev, { int node, cpu = event->cpu; struct cs_buffers *buf;

• struct tmc_drvdata *drvdata;
  

• /*
  

•  * In snapsot mode the size of the perf ring buffer needs to be equal
  

•  * to the size of the device's internal memory if we want to reuse the
  

•  * generic AUX buffer management mechanic.
  

•  *
  

•  * For example (assuming 4096 byte page size):
  

•  *
  

•  *    # cat /sys/bus/coresight/devices/20010000.etf/buffer_size
  

•  *    0x10000
  

•  *    # perf record -e cs_etm/@20010000.etf/ -S -m,16 --per-thread $APP
  

•  *
  

•  */
  

• drvdata = dev_get_drvdata(csdev->dev.parent);
  

• if (overwrite &&
  

•     ((nr_pages << PAGE_SHIFT) != drvdata->size)) {
  

•         dev_err(&csdev->dev, "Ring buffer not equal to device buffer");
  

•         return NULL;
  

• }
  
  if (cpu == -1)
          cpu = smp_processor_id();
  

diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c index df6e4b0b84e9..b9881d6d41ba 100644 --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c @@ -1188,9 +1188,13 @@ alloc_etr_buf(struct tmc_drvdata *drvdata, struct perf_event *event,

  /*
   * Try to match the perf ring buffer size if it is larger

•  * than the size requested via sysfs.
  

•  * than the size requested via sysfs.  In snapsot mode the size
  

•  * of the perf ring buffer needs to be equal to the allocated
  

•  * size if we want to reuse the generic AUX buffer management
  

•  * mechanic.
   */
  

• if ((nr_pages << PAGE_SHIFT) > drvdata->size) {
  

• if (snapshot ||
  

•     (nr_pages << PAGE_SHIFT) > drvdata->size) {
          etr_buf = tmc_alloc_etr_buf(drvdata, (nr_pages << PAGE_SHIFT),
                                      0, node, NULL);
          if (!IS_ERR(etr_buf))
  

If tmc_alloc_etr_buf() returns failure then it's possible to run into the below sequence to allocate smaller buffer size for snapshot mode; which is not expected for snapshot mode.

So here if tmc_alloc_etr_buf() fails to allocate buffer for snapshot mode, should directly bail out with error code.

You are quite right - I need to fix this.

Thanks, Leo Yan