| Message ID | 20250401014210.2576993-1-jie.gan@oss.qualcomm.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | coresight: fix the wrong type of the trace_id in coresight_path | expand |
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 {
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. Regards Mike -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK
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; Thanks, Jie > Regards > > Mike > > > > -- > Mike Leach > Principal Engineer, ARM Ltd. > Manchester Design Centre. UK
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(-)