linux-kernel - Re: [PATCH v3 RESEND 08/10] coresight: tmc: add a switch buffer function for byte-cntr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAJ9a7VhLXrdP_CPQPgAYTAGWJfsVUa9SG9Bzv9dLtFzR4nFROg@mail.gmail.com>
Date: Tue, 22 Jul 2025 15:09:30 +0100
From: Mike Leach <mike.leach@...aro.org>
To: Jie Gan <jie.gan@....qualcomm.com>
Cc: Suzuki K Poulose <suzuki.poulose@....com>, James Clark <james.clark@...aro.org>, 
	Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Tingwei Zhang <quic_tingweiz@...cinc.com>, Yuanfang Zhang <quic_yuanfang@...cinc.com>, 
	Mao Jinlong <quic_jinlmao@...cinc.com>, Jie Gan <quic_jiegan@...cinc.com>, 
	coresight@...ts.linaro.org, linux-arm-kernel@...ts.infradead.org, 
	linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org, 
	devicetree@...r.kernel.org
Subject: Re: [PATCH v3 RESEND 08/10] coresight: tmc: add a switch buffer
 function for byte-cntr

Hi,

This buffer swap code looks OK in principle. The ETR is stopped,
memory synced and set to be read.
See other comments inline.

On Mon, 14 Jul 2025 at 07:31, Jie Gan <jie.gan@....qualcomm.com> wrote:
>
> Switching the sysfs_buf when current buffer is full or the timeout is
> triggered and resets rrp and rwp registers after switched the buffer.
> Disable the ETR device if it cannot find an available buffer to switch.
>
> Signed-off-by: Jie Gan <jie.gan@....qualcomm.com>
> ---
>  .../hwtracing/coresight/coresight-tmc-etr.c   | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
>
> diff --git a/drivers/hwtracing/coresight/coresight-tmc-etr.c b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> index 2b73bd8074bb..3e3e1b5e78ca 100644
> --- a/drivers/hwtracing/coresight/coresight-tmc-etr.c
> +++ b/drivers/hwtracing/coresight/coresight-tmc-etr.c
> @@ -1287,6 +1287,58 @@ static struct etr_buf *tmc_etr_get_sysfs_buffer(struct coresight_device *csdev)
>         return ret ? ERR_PTR(ret) : drvdata->sysfs_buf;
>  }
>
> +static bool tmc_byte_cntr_switch_buffer(struct tmc_drvdata *drvdata,
> +                                       struct ctcu_byte_cntr *byte_cntr_data)
> +{

This entire function should be in one of the byte_cntr source files,
not in the main etr files. Please keep byte cntr code separate as far
as possible

> +       struct etr_buf_node *nd, *next, *curr_node, *picked_node;
> +       struct etr_buf *curr_buf = drvdata->sysfs_buf;
> +       bool found_free_buf = false;
> +
> +       if (WARN_ON(!drvdata || !byte_cntr_data))
> +               return found_free_buf;
> +
> +       /* Stop the ETR before we start the switching process */
> +       if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)

Can this function be called if the mode is not CS_MODE_SYSFS?

> +               __tmc_etr_disable_hw(drvdata);
> +
> +       list_for_each_entry_safe(nd, next, &drvdata->etr_buf_list, node) {
> +               /* curr_buf is free for next round */
> +               if (nd->sysfs_buf == curr_buf) {
> +                       nd->is_free = true;
> +                       curr_node = nd;
> +               }
> +
> +               if (!found_free_buf && nd->is_free && nd->sysfs_buf != curr_buf) {
> +                       if (nd->reading)
> +                               continue;
> +
> +                       picked_node = nd;
> +                       found_free_buf = true;
> +               }
> +       }
> +
> +       if (found_free_buf) {
> +               curr_node->reading = true;
> +               curr_node->pos = 0;
> +               drvdata->reading_node = curr_node;
> +               drvdata->sysfs_buf = picked_node->sysfs_buf;
> +               drvdata->etr_buf = picked_node->sysfs_buf;
> +               picked_node->is_free = false;
> +               /* Reset irq_cnt for next etr_buf */
> +               atomic_set(&byte_cntr_data->irq_cnt, 0);
> +               /* Reset rrp and rwp when the system has switched the buffer*/
> +               CS_UNLOCK(drvdata->base);
> +               tmc_write_rrp(drvdata, 0);
> +               tmc_write_rwp(drvdata, 0);

This cannot possibly be correct. RWP / RRP are pointers into the
system memory where the ETR stores data.

> +               CS_LOCK(drvdata->base);
> +               /* Restart the ETR when we find a free buffer */
> +               if (coresight_get_mode(drvdata->csdev) == CS_MODE_SYSFS)
> +                       __tmc_etr_enable_hw(drvdata);

What happens if the ETR is not restarted? Using __tmc_etr_disable_hw()
is correct for this use case, but if you do not restart then the extra
shutdown that would ordinarily happen in tmc_etr_disable_hw() does not
occur. How is this handled in the rest of the update?

> +       }
> +
> +       return found_free_buf;
> +}
> +
>  static int tmc_enable_etr_sink_sysfs(struct coresight_device *csdev)
>  {
>         int ret = 0;
> --
> 2.34.1
>

Regards

Mike

--
Mike Leach
Principal Engineer, ARM Ltd.
Manchester Design Centre. UK