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

[Bo Gan <ganboing@gmail.com>]

Hi Mayuresh,

On 10/14/25 01:10, Mayuresh Chitale wrote:
> 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.

Sure, that also works.

>>
>> Bo

Bo