| Message ID | 20250401014210.2576993-1-jie.gan@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: fix the wrong type of the trace_id in coresight_path | expand |
Hi, On Wed, 2 Apr 2025 at 01:50, Jie Gan <quic_jiegan@quicinc.com> wrote: > > > > On 4/1/2025 5:56 PM, Mike Leach wrote: > > Hi, > > > > On Tue, 1 Apr 2025 at 07:11, Anshuman Khandual > > <anshuman.khandual@arm.com> wrote: > >> > >> On 4/1/25 07:12, Jie Gan wrote: > >>> The trace_id in coresight_path may contain an error number which means a > >>> negative integer, but the current type of the trace_id is u8. Change the > >>> type to int to fix it. > >>> > >>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> > >>> Fixes: 3c03c49b2fa5 ("Coresight: Introduce a new struct coresight_path") > >>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> > >> > >> LGTM > >> > >> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> > >> > >>> --- > >>> include/linux/coresight.h | 2 +- > >>> 1 file changed, 1 insertion(+), 1 deletion(-) > >>> > >>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h > >>> index d79a242b271d..c2bf10c43e7c 100644 > >>> --- a/include/linux/coresight.h > >>> +++ b/include/linux/coresight.h > >>> @@ -337,7 +337,7 @@ static struct coresight_dev_list (var) = { \ > >>> */ > >>> struct coresight_path { > >>> struct list_head path_list; > >>> - u8 trace_id; > >>> + int trace_id; > >>> }; > >>> > >>> enum cs_mode { > > > > There are many places in the Coresight drivers that assign a u8 > > traceid from the path trace ID. > > > > e.g. > > In coresight-etm4x-core.c : etm4_enable_perf() > > > > drvdata->trcid = path->trace_id; > > > > drvdata->trcid is defined as a u8 - the reason being trace IDs are > > 128 bits wide with some reserved values. > > > > Will this not just trigger the same issue if path->trace_id is changed > > to an int? Even if not it is inconsistent handling of the trace ID > > values. > > > > Trace ID errors should be handled by returning an invalid trace ID > > value - were the trace ID value will fail the macro > > IS_VALID_CS_TRACE_ID(), or separate the return of a trace ID from an > > error return in a function. > > > > Hi Mike, > > The path->trace_id is verified after it has been assigned with the logic > you mentioned: > > if (!IS_VALID_CS_TRACE_ID(path->trace_id)) > goto err_path; > > So it should be safe to assign to another u8 parameter, like you mentioned: > > In coresight-etm4x-core.c : etm4_enable_perf() > > drvdata->trcid = path->trace_id; > It is safe but will it not trigger a warning just like the one you are trying to fix as the types are mismatched? Mike > Thanks, > Jie > > > > Regards > > > > Mike > > > > > > > > -- > > Mike Leach > > Principal Engineer, ARM Ltd. > > Manchester Design Centre. UK >
On 4/2/2025 4:17 PM, Mike Leach wrote: > Hi, > > On Wed, 2 Apr 2025 at 01:50, Jie Gan <quic_jiegan@quicinc.com> wrote: >> >> >> >> On 4/1/2025 5:56 PM, Mike Leach wrote: >>> Hi, >>> >>> On Tue, 1 Apr 2025 at 07:11, Anshuman Khandual >>> <anshuman.khandual@arm.com> wrote: >>>> >>>> On 4/1/25 07:12, Jie Gan wrote: >>>>> The trace_id in coresight_path may contain an error number which means a >>>>> negative integer, but the current type of the trace_id is u8. Change the >>>>> type to int to fix it. >>>>> >>>>> Reported-by: Dan Carpenter <dan.carpenter@linaro.org> >>>>> Fixes: 3c03c49b2fa5 ("Coresight: Introduce a new struct coresight_path") >>>>> Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> >>>> >>>> LGTM >>>> >>>> Reviewed-by: Anshuman Khandual <anshuman.khandual@arm.com> >>>> >>>>> --- >>>>> include/linux/coresight.h | 2 +- >>>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>>>> >>>>> diff --git a/include/linux/coresight.h b/include/linux/coresight.h >>>>> index d79a242b271d..c2bf10c43e7c 100644 >>>>> --- a/include/linux/coresight.h >>>>> +++ b/include/linux/coresight.h >>>>> @@ -337,7 +337,7 @@ static struct coresight_dev_list (var) = { \ >>>>> */ >>>>> struct coresight_path { >>>>> struct list_head path_list; >>>>> - u8 trace_id; >>>>> + int trace_id; >>>>> }; >>>>> >>>>> enum cs_mode { >>> >>> There are many places in the Coresight drivers that assign a u8 >>> traceid from the path trace ID. >>> >>> e.g. >>> In coresight-etm4x-core.c : etm4_enable_perf() >>> >>> drvdata->trcid = path->trace_id; >>> >>> drvdata->trcid is defined as a u8 - the reason being trace IDs are >>> 128 bits wide with some reserved values. >>> >>> Will this not just trigger the same issue if path->trace_id is changed >>> to an int? Even if not it is inconsistent handling of the trace ID >>> values. >>> >>> Trace ID errors should be handled by returning an invalid trace ID >>> value - were the trace ID value will fail the macro >>> IS_VALID_CS_TRACE_ID(), or separate the return of a trace ID from an >>> error return in a function. >>> >> >> Hi Mike, >> >> The path->trace_id is verified after it has been assigned with the logic >> you mentioned: >> >> if (!IS_VALID_CS_TRACE_ID(path->trace_id)) >> goto err_path; >> >> So it should be safe to assign to another u8 parameter, like you mentioned: >> >> In coresight-etm4x-core.c : etm4_enable_perf() >> >> drvdata->trcid = path->trace_id; >> > > It is safe but will it not trigger a warning just like the one you are > trying to fix as the types are mismatched? Hi Mike, It should trigger another type mismatch warning. This patch aims to fix the situation like assign a negative value to u8 will cause a integer toggle(it may happen and I think I should fix it), e.g. -22 for 234. Thanks, Jie > > Mike > >> Thanks, >> Jie >> >> >>> Regards >>> >>> Mike >>> >>> >>> >>> -- >>> Mike Leach >>> Principal Engineer, ARM Ltd. >>> Manchester Design Centre. UK >> > >
diff --git a/include/linux/coresight.h b/include/linux/coresight.h index d79a242b271d..c2bf10c43e7c 100644 --- a/include/linux/coresight.h +++ b/include/linux/coresight.h @@ -337,7 +337,7 @@ static struct coresight_dev_list (var) = { \ */ struct coresight_path { struct list_head path_list; - u8 trace_id; + int trace_id; }; enum cs_mode {
The trace_id in coresight_path may contain an error number which means a negative integer, but the current type of the trace_id is u8. Change the type to int to fix it. Reported-by: Dan Carpenter <dan.carpenter@linaro.org> Fixes: 3c03c49b2fa5 ("Coresight: Introduce a new struct coresight_path") Signed-off-by: Jie Gan <jie.gan@oss.qualcomm.com> --- include/linux/coresight.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)