Re: [PATCH 1/2] hwtracing: hisi_ptt: Propagate DMA reset timeout in trace_start()

[Yicong Yang]

On 2026/4/9 09:07, Pradhan, Sanman wrote:
> From: Sanman Pradhan <psanman@xxxxxxxxxxx>
>
> hisi_ptt_wait_dma_reset_done() discards the return value of
> readl_poll_timeout_atomic(). If the DMA engine does not complete its
> reset within the timeout, hisi_ptt_trace_start() proceeds to start
> tracing regardless.
>
> Return the poll result from hisi_ptt_wait_dma_reset_done() and
> propagate it from hisi_ptt_trace_start(). Deassert the reset bit
> before returning on timeout, preserving the existing reset cleanup
> sequence. Move ctrl->started to the successful path so a failed start
> does not leave the trace marked as active.
>
> Fixes: ff0de066b463 ("hwtracing: hisi_ptt: Add trace function support for HiSilicon PCIe Tune and Trace device")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Sanman Pradhan <psanman@xxxxxxxxxxx>
> ---
>  drivers/hwtracing/ptt/hisi_ptt.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/hwtracing/ptt/hisi_ptt.c b/drivers/hwtracing/ptt/hisi_ptt.c
> index 94c371c491357..73b93df8504c4 100644
> --- a/drivers/hwtracing/ptt/hisi_ptt.c
> +++ b/drivers/hwtracing/ptt/hisi_ptt.c
> @@ -171,13 +171,13 @@ static bool hisi_ptt_wait_trace_hw_idle(struct hisi_ptt *hisi_ptt)
>  					  HISI_PTT_WAIT_TRACE_TIMEOUT_US);
>  }
>  
> -static void hisi_ptt_wait_dma_reset_done(struct hisi_ptt *hisi_ptt)
> +static int hisi_ptt_wait_dma_reset_done(struct hisi_ptt *hisi_ptt)

the other status wait functions in this driver return a boolean, it's better to keep consistence.

>  {
>  	u32 val;
>  
> -	readl_poll_timeout_atomic(hisi_ptt->iobase + HISI_PTT_TRACE_WR_STS,
> -				  val, !val, HISI_PTT_RESET_POLL_INTERVAL_US,
> -				  HISI_PTT_RESET_TIMEOUT_US);
> +	return readl_poll_timeout_atomic(hisi_ptt->iobase + HISI_PTT_TRACE_WR_STS,
> +					 val, !val, HISI_PTT_RESET_POLL_INTERVAL_US,
> +					 HISI_PTT_RESET_TIMEOUT_US);
>  }
>  
>  static void hisi_ptt_trace_end(struct hisi_ptt *hisi_ptt)
> @@ -194,6 +194,7 @@ static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt)
>  {
>  	struct hisi_ptt_trace_ctrl *ctrl = &hisi_ptt->trace_ctrl;
>  	u32 val;
> +	int ret;
>  	int i;
>  
>  	/* Check device idle before start trace */
> @@ -202,19 +203,21 @@ static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt)
>  		return -EBUSY;
>  	}
>  
> -	ctrl->started = true;
> -
>  	/* Reset the DMA before start tracing */
>  	val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>  	val |= HISI_PTT_TRACE_CTRL_RST;
>  	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>  
> -	hisi_ptt_wait_dma_reset_done(hisi_ptt);
> +	ret = hisi_ptt_wait_dma_reset_done(hisi_ptt);
>  
> +	/* De-assert reset regardless of whether the wait timed out */
>  	val = readl(hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>  	val &= ~HISI_PTT_TRACE_CTRL_RST;
>  	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);
>  
> +	if (ret)
> +		return ret;
> +

could add some error log here for better debug. otherwise looks good to me.

the timeout wasn't checked since the hardware reset will be finished in the limited time normally,
which is less than the HISI_PTT_RESET_TIMEOUT_US. It'll be better to add this check in case
there's something wrong with the device.

Thanks.

>  	/* Reset the index of current buffer */
>  	hisi_ptt->trace_ctrl.buf_index = 0;
>  
> @@ -234,6 +237,8 @@ static int hisi_ptt_trace_start(struct hisi_ptt *hisi_ptt)
>  	if (!hisi_ptt->trace_ctrl.is_port)
>  		val |= HISI_PTT_TRACE_CTRL_FILTER_MODE;
>  
> +	ctrl->started = true;
> +
>  	/* Start the Trace */
>  	val |= HISI_PTT_TRACE_CTRL_EN;
>  	writel(val, hisi_ptt->iobase + HISI_PTT_TRACE_CTRL);