CoreSight

coresight@lists.linaro.org

6 participants
2589 discussions

[PATCH v1 0/4] coresight: etm: Correct (virtual) contextID tracing for namespace

by Leo Yan

If a profiling program runs in a non-root PID namespace, if CoreSight driver enables contextID tracing, it can lead to mismatching issue between the context ID in hardware trace data and the allocated PID in the non-root namespace. CoreSight driver has tried to address this issue for the contextID related interfaces under sysfs, but it misses other parts: it doesn't prevent user to set VMID (virtual contextID) for kernel runs in EL2 with VHE, and furthermore, it misses to handle the perf mode when the profiling tool (e.g. perf) doesn't run in root PID namespace. For this reason, this patch series is to correct contextID tracing for non-root namespace. Patch 01 is to use spinlock to protect reading virtual context ID comparator. Patch 02 corrects the virtual contextID tracing for non-root PID namespace. Patch 03/04 are used to fix the contextID tracing for perf mode. I only verified this patch series on Juno board in the root PID namespace and confirmed the patches don't introduce any regression for root PID namespace. Leo Yan (4): coresight: etm4x: Add lock for reading virtual context ID comparator coresight: etm4x: Don't use virtual contextID for non-root PID namespace coresight: etm4x: Don't trace contextID for non-root namespace in perf mode coresight: etm3x: Don't trace contextID for non-root namespace in perf mode .../coresight/coresight-etm3x-core.c | 4 +++ .../coresight/coresight-etm4x-core.c | 10 +++++-- .../coresight/coresight-etm4x-sysfs.c | 30 +++++++++++++++++++ 3 files changed, 42 insertions(+), 2 deletions(-) -- 2.25.1

4 years, 1 month

Re: [PATCH 05/10] Coresight: Add interface for TPDM BC subunit

by Mathieu Poirier

On Fri, Nov 12, 2021 at 05:10:38PM +0800, Jinlong wrote: > On Fri, Nov 12, 2021 at 04:42:31PM +0800, Jinlong wrote: > > [....] > > > > > > &tpdm_attr_grp, > > > > > + &tpdm_bc_attr_grp, > > > > > > > > It is quite tedious to review all these options at the same time as the core > > > > drivers. I suggest to concentrate on the base functionality for now. When that > > > > is merged we can add configuration options such as these. > > > > > > > > I am out of time for this patchset and as such will not review the remaining > > > > patches - those will have to wait for another iteration. > > > > > > > > Thanks, > > > > Mathieu > > > > > > > > > Thank your for all the comments. > > > We will update the patches. > > > > > > Thanks > > > Jinlong Mao > > > > > > > Correct the typo. > > Hi Mathieu, > > We will reduce the code changes. > There will be code changes below for next version: > > 1. coresight: add support to enable more coresight paths > 2. Basic function for TPDM(only enable/disable functions) > 3. TPDA driver > > Is that ok for you ? Yes, that sounds much better. Regards, Mathieu > > > > > > > > > > > NULL, > > > > > }; > > > > > > > > > > -- > > > > > 2.17.1 > > > > >

4 years, 1 month

Re: [PATCH] perf cs-etm: use swap() to make code cleaner

by Leo Yan

Hi Yihao, On Thu, Nov 11, 2021 at 06:33:44PM -0800, Yihao Han wrote: > Use the macro 'swap()' defined in 'include/linux/minmax.h' to avoid > opencoding it. The header 'include/linux/minmax.h' is used for kernel but it's not provided for user space program? > Signed-off-by: Yihao Han <hanyihao(a)vivo.com> > --- > tools/perf/util/cs-etm.c | 6 +----- > 1 file changed, 1 insertion(+), 5 deletions(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index f323adb1af85..aaa3ec725002 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -408,17 +408,13 @@ struct cs_etm_packet_queue > static void cs_etm__packet_swap(struct cs_etm_auxtrace *etm, > struct cs_etm_traceid_queue *tidq) > { > - struct cs_etm_packet *tmp; > - > if (etm->sample_branches || etm->synth_opts.last_branch || > etm->sample_instructions) { > /* > * Swap PACKET with PREV_PACKET: PACKET becomes PREV_PACKET for > * the next incoming packet. > */ > - tmp = tidq->packet; > - tidq->packet = tidq->prev_packet; > - tidq->prev_packet = tmp; > + swap(tidq->packet, tidq->prev_packet); NAK - This patch will introduce compilation error: util/cs-etm.c: In function ‘cs_etm__packet_swap’: util/cs-etm.c:417:3: error: implicit declaration of function ‘swap’; did you mean ‘swab’? [-Werror=implicit-function-declaration] swap(tidq->packet, tidq->prev_packet); ^~~~ swab Thanks, Leo > } > } > > -- > 2.17.1 >

4 years, 1 month

Re: [PATCH v2] coresight: Use devm_bitmap_zalloc when applicable

by Mathieu Poirier

Hi Christophe, On Wed, 3 Nov 2021 at 12:39, Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> wrote: > > 'drvdata->chs.guaranteed' is a bitmap. So use 'devm_bitmap_kzalloc()' to > simplify code, improve the semantic and avoid some open-coded arithmetic > in allocator arguments. > > Signed-off-by: Christophe JAILLET <christophe.jaillet(a)wanadoo.fr> > --- > v1 --> v2: remove the 'guaranteed' variable to be even less verbose > --- > drivers/hwtracing/coresight/coresight-stm.c | 10 +++------- > 1 file changed, 3 insertions(+), 7 deletions(-) > > diff --git a/drivers/hwtracing/coresight/coresight-stm.c b/drivers/hwtracing/coresight/coresight-stm.c > index 58062a5a8238..bb14a3a8a921 100644 > --- a/drivers/hwtracing/coresight/coresight-stm.c > +++ b/drivers/hwtracing/coresight/coresight-stm.c > @@ -856,13 +856,11 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) > { > int ret; > void __iomem *base; > - unsigned long *guaranteed; > struct device *dev = &adev->dev; > struct coresight_platform_data *pdata = NULL; > struct stm_drvdata *drvdata; > struct resource *res = &adev->res; > struct resource ch_res; > - size_t bitmap_size; > struct coresight_desc desc = { 0 }; > > desc.name = coresight_alloc_device_name(&stm_devs, dev); > @@ -904,12 +902,10 @@ static int stm_probe(struct amba_device *adev, const struct amba_id *id) > else > drvdata->numsp = stm_num_stimulus_port(drvdata); > > - bitmap_size = BITS_TO_LONGS(drvdata->numsp) * sizeof(long); > - > - guaranteed = devm_kzalloc(dev, bitmap_size, GFP_KERNEL); > - if (!guaranteed) > + drvdata->chs.guaranteed = devm_bitmap_zalloc(dev, drvdata->numsp, > + GFP_KERNEL); > + if (!drvdata->chs.guaranteed) > return -ENOMEM; > - drvdata->chs.guaranteed = guaranteed; I will pick up this patch when -rc1 gets released. Thanks, Mathieu > > spin_lock_init(&drvdata->spinlock); > > -- > 2.30.2 >

4 years, 1 month

[PATCH v2 0/2] arm-spe/cs-etm: Print size using consistent format

by German Gomez

Sent as a splitted re-roll of patchset [1] to keep the patches more semantically grouped. The patches themselves have been tested and reviewed in the previous submission already. Changes since v1: - Sent as a separate patchset. - Added acme to the recipients list as I forgot to include him in [1]. [1] https://lore.kernel.org/all/20210916154635.1525-1-german.gomez@arm.com/ Andrew Kilroy (2): perf cs-etm: Print size using consistent format perf arm-spe: Print size using consistent format tools/perf/util/arm-spe.c | 2 +- tools/perf/util/cs-etm.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) -- 2.25.1

4 years, 1 month

[PATCH 1/5] perf cs-etm: Print size using consistent format

by German Gomez

From: Andrew Kilroy <andrew.kilroy(a)arm.com> Since the size is already printed earlier in hex, print the same data using the same format, in hex. Reviewed-by: James Clark <james.clark(a)arm.com> Signed-off-by: Andrew Kilroy <andrew.kilroy(a)arm.com> Signed-off-by: German Gomez <german.gomez(a)arm.com> --- tools/perf/util/cs-etm.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index f323adb1af85..4f672f7d008c 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -537,7 +537,7 @@ static void cs_etm__dump_event(struct cs_etm_queue *etmq, fprintf(stdout, "\n"); color_fprintf(stdout, color, - ". ... CoreSight %s Trace data: size %zu bytes\n", + ". ... CoreSight %s Trace data: size %#zx bytes\n", cs_etm_decoder__get_name(etmq->decoder), buffer->size); do { -- 2.17.1

4 years, 1 month

Re: [PATCH v2] drivers/coresight: ultrasoc: Add System Memory Buffer driver

by liuqi (BA)

On 2021/11/5 9:36, Randy Dunlap wrote: > On 11/4/21 6:20 PM, Qi Liu wrote: >> diff --git a/drivers/hwtracing/coresight/ultrasoc/Kconfig >> b/drivers/hwtracing/coresight/ultrasoc/Kconfig >> new file mode 100644 >> index 000000000000..c18c25bf9df3 >> --- /dev/null >> +++ b/drivers/hwtracing/coresight/ultrasoc/Kconfig >> @@ -0,0 +1,12 @@ >> +# SPDX-License-Identifier: MIT >> +# >> +# ultrasoc configuration >> +# >> +config ULTRASOC_SMB >> + tristate "Ultrasoc system memory buffer drivers" >> + depends on ARM64 && CORESIGHT > > The "depends" line should be indented with a tab instead of spaces... > ah, yes, my editor might be set incorrectly, thanks for the reminder, I'll correct it next time. Qi >> + help >> + This enables support for the Ultrasoc system memory buffer >> + driver that is responsible for receiving the trace data from >> + Coresight ETM devices and storing them to a system buffer >> + respectively. > >

4 years, 1 month

[GIT PULL] Coresight fix for v5.16

by Mathieu Poirier

The following changes since commit 561ced0bb90a4be298b7db5fb54f29731d74a3f6: arm64: errata: Enable TRBE workaround for write to out-of-range address (2021-10-27 11:46:06 -0600) are available in the Git repository at: git@gitolite.kernel.org:pub/scm/linux/kernel/git/coresight/linux.git tags/coresight-fixes-v5.16 for you to fetch changes up to 20b02fe36530a1e48dde73c80b882ae276346ea3: arm64: cpufeature: Export this_cpu_has_cap helper (2021-11-04 10:46:21 -0600) --------------------------------------------------------------- coresight: Fix for v5.16 This fix enables to compile the TRBE driver as a module by exporting function this_cpu_has_cap(). Signed-off-by: Mathieu Poirier <mathieu.poirier(a)linaro.org> ---------------------------------------------------------------- Arnd Bergmann (1): arm64: cpufeature: Export this_cpu_has_cap helper arch/arm64/kernel/cpufeature.c | 1 + 1 file changed, 1 insertion(+)

4 years, 1 month

Re: [PATCH 04/10] Coresight: Enable BC and GPR for TPDM driver

by Mathieu Poirier

On Fri, Nov 05, 2021 at 04:17:54PM +0800, Jinlong wrote: > On Thu, Nov 04, 2021 at 11:02:24AM -0600, Mathieu Poirier wrote: > > [...] > > > > > > > + > > > > > +static ssize_t reset_store(struct device *dev, > > > > > + struct device_attribute *attr, > > > > > + const char *buf, > > > > > + size_t size) > > > > > +{ > > > > > + int ret = 0; > > > > > + unsigned long val; > > > > > + struct mcmb_dataset *mcmb_temp = NULL; > > > > > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > > > > > + > > > > > + ret = kstrtoul(buf, 10, &val); > > > > > > > > The coresight subsystem normally uses the hexadecimal base. > > > > > > > > > > We will address you comments. > > > > > > > > + if (ret) > > > > > + return ret; > > > > > > > > Shouldn't this be "if (!ret)" ? > > > > > > > > > > When ret is not 0, it need to return. > > > > I would expect something like this: > > > > $ echo 1 > /sys/path/to/tpdm/device/reset > > > > and not > > > > $ echo 0 > /sys/path/to/tpdm/device/reset > > > > The latter is what the code does. > > > > Thanks, > > Mathieu > > > > Hi Mathieu, > > The ret is the result of kstrtoul not the val. > Ah yes, you are correct.

4 years, 1 month

Re: [PATCH 05/10] Coresight: Add interface for TPDM BC subunit

by Mathieu Poirier

On Thu, Oct 21, 2021 at 03:38:51PM +0800, Tao Zhang wrote: > The BC(Basic Counters) interface has RW, WO and RO fields for > controlling BC dataset elements transmitted on ATB flush. > The BC data set subunit supports from 1-32 counter instances > allowing for collection of BC data sets. > > Signed-off-by: Tao Zhang <quic_taozha(a)quicinc.com> > --- > drivers/hwtracing/coresight/coresight-tpdm.c | 873 +++++++++++++++++++ > 1 file changed, 873 insertions(+) > > diff --git a/drivers/hwtracing/coresight/coresight-tpdm.c b/drivers/hwtracing/coresight/coresight-tpdm.c > index c0a01979e42f..0970c69ac8e2 100644 > --- a/drivers/hwtracing/coresight/coresight-tpdm.c > +++ b/drivers/hwtracing/coresight/coresight-tpdm.c > @@ -668,6 +668,878 @@ static ssize_t gp_regs_store(struct device *dev, > } > static DEVICE_ATTR_RW(gp_regs); > > +static ssize_t bc_capture_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) Indentation. I won't repeat this comment but please make sure it is fixed for the entire patchset. > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%s\n", > + drvdata->bc->capture_mode == TPDM_MODE_ATB ? > + "ATB" : "APB"); > +} > + > +static ssize_t bc_capture_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + char str[20] = ""; char str[4]; > + uint32_t val; > + > + if (size >= 20) > + return -EINVAL; > + if (sscanf(buf, "%s", str) != 1) if (sscanf(buf, "%3s", str) != 1) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; return -EINVAL; Please make sure this is fixed everywhere, except when -EINVAL is really the right error code. > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; Why does the device need to be enabled for this operation? Again no comments... > + } > + > + if (!strcmp(str, "ATB")) { > + drvdata->bc->capture_mode = TPDM_MODE_ATB; > + } else if (!strcmp(str, "APB") && > + drvdata->bc->retrieval_mode == TPDM_MODE_APB) { > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + val = val | BIT(3); > + tpdm_writel(drvdata, val, TPDM_BC_CR); > + TPDM_LOCK(drvdata); > + > + drvdata->bc->capture_mode = TPDM_MODE_APB; > + } else { > + mutex_unlock(&drvdata->lock); > + return -EINVAL; > + } > + > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_capture_mode); > + > +static ssize_t bc_retrieval_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%s\n", > + drvdata->bc->retrieval_mode == TPDM_MODE_ATB ? > + "ATB" : "APB"); > +} > + > +static ssize_t bc_retrieval_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + char str[20] = ""; > + > + if (size >= 20) > + return -EINVAL; > + if (sscanf(buf, "%s", str) != 1) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; Same here, I don't know why the device needs to be enabled for this to success. Please fix everywhere. > + } > + > + if (!strcmp(str, "ATB")) { > + drvdata->bc->retrieval_mode = TPDM_MODE_ATB; > + } else if (!strcmp(str, "APB")) { > + drvdata->bc->retrieval_mode = TPDM_MODE_APB; > + } else { > + mutex_unlock(&drvdata->lock); > + return -EINVAL; > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_retrieval_mode); > + > +static ssize_t bc_reset_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CR); > + val = val | BIT(1); > + tpdm_writel(drvdata, val, TPDM_BC_CR); > + TPDM_LOCK(drvdata); > + } > + > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_WO(bc_reset_counters); > + > +static ssize_t bc_sat_mode_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->sat_mode); return scnprintf(buf, PAGE_SIZE, "%#x\n", drvdata->bc->sat_mode); And everywhere casting in used... > +} > + > +static ssize_t bc_sat_mode_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->sat_mode = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_sat_mode); > + > +static ssize_t bc_enable_counters_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_counters); > +} > + > +static ssize_t bc_enable_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_counters = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_counters); > + > +static ssize_t bc_clear_counters_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->clear_counters); > +} > + > +static ssize_t bc_clear_counters_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->clear_counters = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_clear_counters); > + > +static ssize_t bc_enable_irq_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_irq); > +} > + > +static ssize_t bc_enable_irq_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_irq = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_irq); > + > +static ssize_t bc_clear_irq_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->clear_irq); > +} > + > +static ssize_t bc_clear_irq_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->clear_irq = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_clear_irq); > + > +static ssize_t bc_trig_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->trig_val_lo[i]); As previously stated, the sysfs interface should output single line and single values. I won't comment on this again, please fix everywhere. > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_trig_val_lo_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= drvdata->bc_counters_avail || > + drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO || > + (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0)) > + return -EPERM; > This is hard to read and maintain. Please break it up in multiple if() statements. > + mutex_lock(&drvdata->lock); > + drvdata->bc->trig_val_lo[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_trig_val_lo); > + > +static ssize_t bc_trig_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_COUNTERS; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->trig_val_hi[i]); > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_trig_val_hi_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= drvdata->bc_counters_avail || > + drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_NO || > + (drvdata->bc_trig_type == TPDM_SUPPORT_TYPE_PARTIAL && index > 0)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->trig_val_hi[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_trig_val_hi); > + > +static ssize_t bc_enable_ganging_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + return scnprintf(buf, PAGE_SIZE, "%lx\n", > + (unsigned long)drvdata->bc->enable_ganging); > +} > + > +static ssize_t bc_enable_ganging_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->enable_ganging = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_enable_ganging); > + > +static ssize_t bc_overflow_val_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + for (i = 0; i < TPDM_BC_MAX_OVERFLOW; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + drvdata->bc->overflow_val[i]); > + mutex_unlock(&drvdata->lock); > + return size; > +} > + > +static ssize_t bc_overflow_val_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long index, val; > + > + if (sscanf(buf, "%lx %lx", &index, &val) != 2) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->datasets) || > + index >= TPDM_BC_MAX_OVERFLOW) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->overflow_val[index] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_overflow_val); > + > +static ssize_t bc_ovsr_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_OVSR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_ovsr_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_OVSR); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_ovsr); > + > +static ssize_t bc_counter_sel_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_SELR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_counter_sel_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable || val >= drvdata->bc_counters_avail) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_SELR); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_counter_sel); > + > +static ssize_t bc_count_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CNTR_LO); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_count_val_lo_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val, select; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { if (!val) { mutex_unlock(&drvdata->lock); return -EINVAL; } > + TPDM_UNLOCK(drvdata); > + select = tpdm_readl(drvdata, TPDM_BC_SELR); > + > + /* Check if selected counter is disabled */ > + if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + tpdm_writel(drvdata, val, TPDM_BC_CNTR_LO); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_count_val_lo); > + > +static ssize_t bc_count_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_CNTR_HI); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_count_val_hi_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val, select; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { Same > + TPDM_UNLOCK(drvdata); > + select = tpdm_readl(drvdata, TPDM_BC_SELR); > + > + /* Check if selected counter is disabled */ > + if (BMVAL(tpdm_readl(drvdata, TPDM_BC_CNTENSET), select, select)) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + tpdm_writel(drvdata, val, TPDM_BC_CNTR_HI); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_count_val_hi); > + > +static ssize_t bc_shadow_val_lo_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + for (i = 0; i < drvdata->bc_counters_avail; i++) { > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + tpdm_readl(drvdata, TPDM_BC_SHADOW_LO(i))); > + } > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RO(bc_shadow_val_lo); > + > +static ssize_t bc_shadow_val_hi_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + ssize_t size = 0; > + int i = 0; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + for (i = 0; i < drvdata->bc_counters_avail; i++) > + size += scnprintf(buf + size, PAGE_SIZE - size, > + "Index: 0x%x Value: 0x%x\n", i, > + tpdm_readl(drvdata, TPDM_BC_SHADOW_HI(i))); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RO(bc_shadow_val_hi); > + > +static ssize_t bc_sw_inc_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + TPDM_UNLOCK(drvdata); > + val = tpdm_readl(drvdata, TPDM_BC_SWINC); > + TPDM_LOCK(drvdata); > + mutex_unlock(&drvdata->lock); > + return scnprintf(buf, PAGE_SIZE, "%lx\n", val); > +} > + > +static ssize_t bc_sw_inc_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned long val; > + > + if (kstrtoul(buf, 16, &val)) > + return -EINVAL; > + if (!test_bit(TPDM_DS_BC, drvdata->enable_ds)) > + return -EPERM; > + > + mutex_lock(&drvdata->lock); > + if (!drvdata->enable) { > + mutex_unlock(&drvdata->lock); > + return -EPERM; > + } > + > + if (val) { > + TPDM_UNLOCK(drvdata); > + tpdm_writel(drvdata, val, TPDM_BC_SWINC); > + TPDM_LOCK(drvdata); > + } > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_sw_inc); > + > +static ssize_t bc_msr_show(struct device *dev, > + struct device_attribute *attr, > + char *buf) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int i; > + ssize_t len = 0; > + > + if (!drvdata->msr_support) > + return -EINVAL; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + for (i = 0; i < TPDM_BC_MAX_MSR; i++) > + len += scnprintf(buf + len, PAGE_SIZE - len, "%u 0x%x\n", > + i, drvdata->bc->msr[i]); > + > + return len; > +} > + > +static ssize_t bc_msr_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, > + size_t size) > +{ > + struct tpdm_drvdata *drvdata = dev_get_drvdata(dev->parent); > + unsigned int num, val; > + int nval; > + > + if (!drvdata->msr_support) > + return -EINVAL; > + > + if (!test_bit(TPDM_DS_BC, drvdata->datasets)) > + return -EPERM; > + > + nval = sscanf(buf, "%u %x", &num, &val); > + if (nval != 2) > + return -EINVAL; > + > + if (num >= TPDM_BC_MAX_MSR) > + return -EINVAL; > + > + mutex_lock(&drvdata->lock); > + drvdata->bc->msr[num] = val; > + mutex_unlock(&drvdata->lock); > + return size; > +} > +static DEVICE_ATTR_RW(bc_msr); > + > +static struct attribute *tpdm_bc_attrs[] = { > + &dev_attr_bc_capture_mode.attr, > + &dev_attr_bc_retrieval_mode.attr, > + &dev_attr_bc_reset_counters.attr, > + &dev_attr_bc_sat_mode.attr, > + &dev_attr_bc_enable_counters.attr, > + &dev_attr_bc_clear_counters.attr, > + &dev_attr_bc_enable_irq.attr, > + &dev_attr_bc_clear_irq.attr, > + &dev_attr_bc_trig_val_lo.attr, > + &dev_attr_bc_trig_val_hi.attr, > + &dev_attr_bc_enable_ganging.attr, > + &dev_attr_bc_overflow_val.attr, > + &dev_attr_bc_ovsr.attr, > + &dev_attr_bc_counter_sel.attr, > + &dev_attr_bc_count_val_lo.attr, > + &dev_attr_bc_count_val_hi.attr, > + &dev_attr_bc_shadow_val_lo.attr, > + &dev_attr_bc_shadow_val_hi.attr, > + &dev_attr_bc_sw_inc.attr, > + &dev_attr_bc_msr.attr, > + NULL, This will result in a very crowded directory. Please move under a "bc" subdirectory. And as I commented before, all sysfs entries need to be documented under Documentation/ABI/testing. > +}; > + > +static struct attribute_group tpdm_bc_attr_grp = { > + .attrs = tpdm_bc_attrs, > +}; > + > static struct attribute *tpdm_attrs[] = { > &dev_attr_available_datasets.attr, > &dev_attr_enable_datasets.attr, > @@ -682,6 +1554,7 @@ static struct attribute_group tpdm_attr_grp = { > }; > static const struct attribute_group *tpdm_attr_grps[] = { > &tpdm_attr_grp, > + &tpdm_bc_attr_grp, It is quite tedious to review all these options at the same time as the core drivers. I suggest to concentrate on the base functionality for now. When that is merged we can add configuration options such as these. I am out of time for this patchset and as such will not review the remaining patches - those will have to wait for another iteration. Thanks, Mathieu > NULL, > }; > > -- > 2.17.1 >

4 years, 1 month

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

CoreSight