Re: [PATCH 04/11] rvtrace: Add functions to start/stop tracing on a component path

[Mayuresh Chitale <mchitale@ventanamicro.com>]

Hi Bo,

On Mon, Oct 13, 2025 at 10:23 AM Bo Gan <ganboing@gmail.com> wrote:
>
> On 10/12/25 20:43, Anup Patel wrote:
> > On Wed, Oct 8, 2025 at 2:45 PM Bo Gan <ganboing@gmail.com> wrote:
> >>
> >> On 10/1/25 23:07, Anup Patel wrote:
> >>> From: Mayuresh Chitale <mchitale@ventanamicro.com>
> >>>
> >>> The perf driver framework needs to be able to start / stop all components
> >>> in a trace component path during its operation. Add rvtrace_path_start()
> >>> and rvtrace_path_stop() functions for this purpose.
> >>>
> >>> Co-developed-by: Anup Patel <apatel@ventanamicro.com>
> >>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> >>> Signed-off-by: Mayuresh Chitale <mchitale@ventanamicro.com>
> >>> ---
> >>>    drivers/hwtracing/rvtrace/rvtrace-core.c | 44 ++++++++++++++++++++++++
> >>>    include/linux/rvtrace.h                  |  6 ++++
> >>>    2 files changed, 50 insertions(+)
> >>>
> >>> diff --git a/drivers/hwtracing/rvtrace/rvtrace-core.c b/drivers/hwtracing/rvtrace/rvtrace-core.c
> >>> index 7013d50ca569..109be40d4b24 100644
> >>> --- a/drivers/hwtracing/rvtrace/rvtrace-core.c
> >>> +++ b/drivers/hwtracing/rvtrace/rvtrace-core.c
> >>> @@ -614,6 +614,50 @@ static void rvtrace_release_path_nodes(struct rvtrace_path *path)
> >>>        }
> >>>    }
> >>>
> >>> +int rvtrace_path_start(struct rvtrace_path *path)
> >>> +{
> >>> +     const struct rvtrace_driver *rtdrv;
> >>> +     struct rvtrace_component *comp;
> >>> +     struct rvtrace_path_node *node;
> >>> +     int ret;
> >>> +
> >>> +     list_for_each_entry(node, &path->comp_list, head) {
> >>> +             comp = node->comp;
> >>> +             rtdrv = to_rvtrace_driver(comp->dev.driver);
> >>> +             if (!rtdrv->start)
> >>> +                     continue;
> >>> +
> >>> +             ret = rtdrv->start(comp);
> >>> +             if (ret)
> >>> +                     return ret;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(rvtrace_path_start);
> >>> +
> >>> +int rvtrace_path_stop(struct rvtrace_path *path)
> >>> +{
> >>> +     const struct rvtrace_driver *rtdrv;
> >>> +     struct rvtrace_component *comp;
> >>> +     struct rvtrace_path_node *node;
> >>> +     int ret;
> >>> +
> >>> +     list_for_each_entry(node, &path->comp_list, head) {
> >>> +             comp = node->comp;
> >>> +             rtdrv = to_rvtrace_driver(comp->dev.driver);
> >>> +             if (!rtdrv->stop)
> >>> +                     continue;
> >>> +
> >>> +             ret = rtdrv->stop(comp);
> >>> +             if (ret)
> >>> +                     return ret;
> >>> +     }
> >>> +
> >>> +     return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(rvtrace_path_stop);
> >>> +
> >>>    struct rvtrace_path *rvtrace_create_path(struct rvtrace_component *source,
> >>>                                         struct rvtrace_component *sink,
> >>>                                         enum rvtrace_component_mode mode)
> >>> diff --git a/include/linux/rvtrace.h b/include/linux/rvtrace.h
> >>> index f2174f463a69..e7bd335d388f 100644
> >>> --- a/include/linux/rvtrace.h
> >>> +++ b/include/linux/rvtrace.h
> >>> @@ -273,10 +273,14 @@ struct rvtrace_path *rvtrace_create_path(struct rvtrace_component *source,
> >>>                                         struct rvtrace_component *sink,
> >>>                                         enum rvtrace_component_mode mode);
> >>>    void rvtrace_destroy_path(struct rvtrace_path *path);
> >>> +int rvtrace_path_start(struct rvtrace_path *path);
> >>> +int rvtrace_path_stop(struct rvtrace_path *path);
> >>>
> >>>    /**
> >>>     * struct rvtrace_driver - Representation of a RISC-V trace driver
> >>>     * id_table: Table to match components handled by the driver
> >>> + * start:        Callback to start tracing
> >>> + * stop:         Callback to stop tracing
> >>>     * probe:        Driver probe() function
> >>>     * remove:       Driver remove() function
> >>>     * get_trace_id: Get/allocate a trace ID
> >>> @@ -285,6 +289,8 @@ void rvtrace_destroy_path(struct rvtrace_path *path);
> >>>     */
> >>>    struct rvtrace_driver {
> >>>        const struct rvtrace_component_id *id_table;
> >>> +     int                     (*start)(struct rvtrace_component *comp);
> >>> +     int                     (*stop)(struct rvtrace_component *comp);
> >>>        int                     (*probe)(struct rvtrace_component *comp);
> >>>        void                    (*remove)(struct rvtrace_component *comp);
> >>>        int                     (*get_trace_id)(struct rvtrace_component *comp,
> >>
> >> I'd suggest add another function (*quiesce) or something like that. Trace
> >> components have a tr??Empty bit that indicates trace has been all flushed
> >> out. Also along the path when you do rvtrace_path_stop, you need to ensure
> >> the source has stopped and quiescent before beginning to stop the sink.
> >> Otherwise you'll get partial or corrupted trace. In essence, follow Control
> >> Interface Spec 11.3 Enabling and Disabling. FYI: my userspace driver:
> >> https://github.com/ganboing/riscv-trace-umd/blob/master/rvtrace/funnel.py#L223
> >
> > It's better to add functions on a need basis rather than adding
> > it now without any potential user.
> >
> > Regards,
> > Anu
>
> Hi Anup, my previous comment also applies to your current use case where you
> have encoder->RAM sink directly connected together. Having a longer path,
> e.g., funnels in between makes it worse. The driver needs to poll the empty
> bit tr??Empty (bit 3) of the control register to check if trace has been
> completely flushed. Otherwise, you get a partial trace, possibly with last
> few messages missing or truncated. So, yes, there's really a need to do so.

I think this can also be implemented in the component's 'stop' callback.
>
> Bo