Re: [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing component disable (resource leak) on error paths in rvtrace_encoder_start() and rvtrace_encoder_stop().
- [High] Hardware state corruption due to setting the multi-bit INSTMODE field without clearing existing bits.
- [Medium] gtrace_poll_bit() evaluates incorrectly and fails immediately when timeout is 0.
--

--- Patch [6]: [PATCH v5 06/14] gtrace: Add RISC-V Trace encoder driver ---
commit a4adb209cac509aff6293c009b2c4a5f828df5f5
Author: Mayuresh Chitale <mayuresh.chitale@oss.qualcomm.com>

gtrace: Add RISC-V Trace encoder driver

Add initial implementation of RISC-V E-Trace encoder driver. The encoder
is defined in the RISC-V Trace Control Interface specification.

> diff --git a/drivers/hwtracing/gtrace/rvtrace-encoder.c b/drivers/hwtracing/gtrace/rvtrace-encoder.c
> new file mode 100644
> index 0000000000000..9ff3413d1aa41
> --- /dev/null
> +++ b/drivers/hwtracing/gtrace/rvtrace-encoder.c

[ ... ]

> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2026 Qualcomm Technologies, Inc.
> + */
> +
> +#include <linux/device.h>
> +#include <linux/gtrace.h>
> +#include <linux/types.h>
> +#include "rvtrace.h"
> +
> +#define RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT	2
> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT	4
> +#define RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT	0x6
> +
> +static int rvtrace_encoder_start(struct gtrace_component *comp)
> +{
> +	struct gtrace_platform_data *pdata = comp->pdata;
> +	int ret;
> +	u32 val;
> +
> +	ret = gtrace_enable_component(comp);
> +	if (ret) {
> +		dev_err(&comp->dev, "failed to enable encoder.\n");
> +		return ret;
> +	}
> +
> +	/* set mode */
> +	val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	val |= (RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT << RVTRACE_COMPONENT_CTRL_INSTMODE_SHIFT);

[Severity: High]
Is a bitmask needed before ORing the new mode here?

Since RVTRACE_COMPONENT_CTRL_INSTMODE_OPIT is 0x6 (spanning 3 bits), if the
hardware register already contains non-zero bits from a previous state or the
bootloader (e.g., 0x1), won't the bitwise OR result in a corrupted state
(0x1 | 0x6 = 0x7)?

> +	gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> +
> +	val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	val |= BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
> +	gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> +			      RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 1,
> +			      pdata->control_poll_timeout_usecs);

[Severity: Medium]
Does the gtrace_poll_bit() implementation in gtrace-core.c handle a 0 timeout
correctly?

Looking at gtrace_poll_bit():

    while (timeout--) {
        val = gtrace_read32(pdata, offset);

If a caller passes a timeout of 0, the postfix decrement causes it to bypass
the loop entirely and immediately return -ETIMEDOUT without reading the
register even once. Should standard polling semantics perform at least one
non-blocking read when timeout is 0?

> +	if (ret)
> +		dev_err(&comp->dev, "failed to enable tracing.\n");
> +
> +	return ret;

[Severity: High]
If gtrace_poll_bit() times out, this returns the error but skips disabling the
component. Will this leak the hardware resources (like clocks or power
domains) that were enabled earlier in gtrace_enable_component()?

> +}
> +
> +static int rvtrace_encoder_stop(struct gtrace_component *comp)
> +{
> +	struct gtrace_platform_data *pdata = comp->pdata;
> +	int ret;
> +	u32 val;
> +
> +	val = gtrace_read32(pdata, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	val &= ~BIT(RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT);
> +	gtrace_write32(pdata, val, RVTRACE_COMPONENT_CTRL_OFFSET);
> +	ret = gtrace_poll_bit(pdata, RVTRACE_COMPONENT_CTRL_OFFSET,
> +			      RVTRACE_COMPONENT_CTRL_ITRACE_SHIFT, 0,
> +			      pdata->control_poll_timeout_usecs);
> +	if (ret) {
> +		dev_err(&comp->dev, "failed to stop tracing.\n");
> +		return ret;
> +	}

[Severity: High]
If stopping the trace times out, this returns early. Does this intentionally
bypass gtrace_disable_component() and leak the resource, or should it still
attempt to disable the encoder even on a timeout?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260810152223.3946743-1-mayuresh.chitale@oss.qualcomm.com?part=6