* [PATCH 0/1] coresight: Fix TRCCONFIGR.QE sysfs interface @ 2022-01-14 17:32 James Clark 2022-01-14 17:32 ` [PATCH 1/1] " James Clark 0 siblings, 1 reply; 5+ messages in thread From: James Clark @ 2022-01-14 17:32 UTC (permalink / raw) To: mathieu.poirier, coresight Cc: suzuki.poulose, James Clark, Mike Leach, Leo Yan, linux-arm-kernel, linux-kernel I was working on refactoring some of the magic numbers for the register accesses and I saw this issue with programming one of the values. I don't have any evidence that someone encountered it not working, probably there is no hardware where QSUPP==0b10, or it just didn't get used yet. But the issue is that this silently writes a reserved value even if the user provided one was correct so it might ruin someones day if it ever comes up. It's a small change so I didn't see the harm in fixing it. Applies to coresight/next f9809d565135 James Clark (1): coresight: Fix TRCCONFIGR.QE sysfs interface drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.17.1 ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] coresight: Fix TRCCONFIGR.QE sysfs interface 2022-01-14 17:32 [PATCH 0/1] coresight: Fix TRCCONFIGR.QE sysfs interface James Clark @ 2022-01-14 17:32 ` James Clark 2022-01-19 15:22 ` Mike Leach 0 siblings, 1 reply; 5+ messages in thread From: James Clark @ 2022-01-14 17:32 UTC (permalink / raw) To: mathieu.poirier, coresight Cc: suzuki.poulose, James Clark, Mike Leach, Leo Yan, linux-arm-kernel, linux-kernel It's impossible to program a valid value for TRCCONFIGR.QE when TRCIDR0.QSUPP==0b10. In that case the following is true: Q element support is implemented, and only supports Q elements without instruction counts. TRCCONFIGR.QE can only take the values 0b00 or 0b11. Currently the low bit of QSUPP is checked to see if the low bit of QE can be written to, but as you can see when QSUPP==0b10 the low bit is cleared making it impossible to ever write the only valid value of 0b11 to QE. 0b10 would be written instead, which is a reserved QE value even for all values of QSUPP. The fix is to allow writing the low bit of QE for any non zero value of QSUPP. This change doesn't go any further to validate if the user supplied value is valid, because none of the other parts this function do, but it does fix the case where it was impossible to ever set a valid value. Signed-off-by: James Clark <james.clark@arm.com> --- drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c index a0640fa5c55b..a99bb537ea23 100644 --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c @@ -368,7 +368,7 @@ static ssize_t mode_store(struct device *dev, /* start by clearing QE bits */ config->cfg &= ~(BIT(13) | BIT(14)); /* if supported, Q elements with instruction counts are enabled */ - if ((mode & BIT(0)) && (drvdata->q_support & BIT(0))) + if ((mode & BIT(0)) && drvdata->q_support) config->cfg |= BIT(13); /* * if supported, Q elements with and without instruction -- 2.17.1 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] coresight: Fix TRCCONFIGR.QE sysfs interface 2022-01-14 17:32 ` [PATCH 1/1] " James Clark @ 2022-01-19 15:22 ` Mike Leach 2022-01-20 11:37 ` James Clark 0 siblings, 1 reply; 5+ messages in thread From: Mike Leach @ 2022-01-19 15:22 UTC (permalink / raw) To: James Clark Cc: mathieu.poirier, coresight, suzuki.poulose, Leo Yan, linux-arm-kernel, linux-kernel Hi James On Fri, 14 Jan 2022 at 17:33, James Clark <james.clark@arm.com> wrote: > > It's impossible to program a valid value for TRCCONFIGR.QE > when TRCIDR0.QSUPP==0b10. In that case the following is true: > > Q element support is implemented, and only supports Q elements without > instruction counts. TRCCONFIGR.QE can only take the values 0b00 or 0b11. > > Currently the low bit of QSUPP is checked to see if the low bit of QE can > be written to, but as you can see when QSUPP==0b10 the low bit is cleared > making it impossible to ever write the only valid value of 0b11 to QE. > 0b10 would be written instead, which is a reserved QE value even for all > values of QSUPP. > > The fix is to allow writing the low bit of QE for any non zero value of > QSUPP. > > This change doesn't go any further to validate if the user supplied value > is valid, because none of the other parts this function do, but it does fix > the case where it was impossible to ever set a valid value. > I concur that the input is not checked as valid, However all the other fields are single bit - with no invalid values - other than the cond field, which controls tracing of non-branch conditionals - architecturally unsupported for A profile in ETMv4/. > Signed-off-by: James Clark <james.clark@arm.com> > --- > drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > index a0640fa5c55b..a99bb537ea23 100644 > --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > @@ -368,7 +368,7 @@ static ssize_t mode_store(struct device *dev, > /* start by clearing QE bits */ > config->cfg &= ~(BIT(13) | BIT(14)); > /* if supported, Q elements with instruction counts are enabled */ > - if ((mode & BIT(0)) && (drvdata->q_support & BIT(0))) > + if ((mode & BIT(0)) && drvdata->q_support) > config->cfg |= BIT(13); This can be trivially changed to if ((mode) && drvdata->q_support) config->cfg |= BIT(13); to ensure that any input mode 2'b01, 2'b10, 2'b11 results in output settings of 2'b01, 2'b11, 2'b11 respectively With that Reviewed-by: Mike Leach <mike.leach@linaro.org> > /* > * if supported, Q elements with and without instruction > -- > 2.17.1 > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] coresight: Fix TRCCONFIGR.QE sysfs interface 2022-01-19 15:22 ` Mike Leach @ 2022-01-20 11:37 ` James Clark 2022-01-21 12:40 ` Mike Leach 0 siblings, 1 reply; 5+ messages in thread From: James Clark @ 2022-01-20 11:37 UTC (permalink / raw) To: Mike Leach Cc: mathieu.poirier, coresight, suzuki.poulose, Leo Yan, linux-arm-kernel, linux-kernel On 19/01/2022 15:22, Mike Leach wrote: > Hi James > > On Fri, 14 Jan 2022 at 17:33, James Clark <james.clark@arm.com> wrote: >> >> It's impossible to program a valid value for TRCCONFIGR.QE >> when TRCIDR0.QSUPP==0b10. In that case the following is true: >> >> Q element support is implemented, and only supports Q elements without >> instruction counts. TRCCONFIGR.QE can only take the values 0b00 or 0b11. >> >> Currently the low bit of QSUPP is checked to see if the low bit of QE can >> be written to, but as you can see when QSUPP==0b10 the low bit is cleared >> making it impossible to ever write the only valid value of 0b11 to QE. >> 0b10 would be written instead, which is a reserved QE value even for all >> values of QSUPP. >> >> The fix is to allow writing the low bit of QE for any non zero value of >> QSUPP. >> >> This change doesn't go any further to validate if the user supplied value >> is valid, because none of the other parts this function do, but it does fix >> the case where it was impossible to ever set a valid value. >> > > I concur that the input is not checked as valid, However all the other > fields are single bit - with no invalid values - other than the cond > field, which controls tracing of non-branch conditionals - > architecturally unsupported for A profile in ETMv4/. > > >> Signed-off-by: James Clark <james.clark@arm.com> >> --- >> drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> index a0640fa5c55b..a99bb537ea23 100644 >> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c >> @@ -368,7 +368,7 @@ static ssize_t mode_store(struct device *dev, >> /* start by clearing QE bits */ >> config->cfg &= ~(BIT(13) | BIT(14)); >> /* if supported, Q elements with instruction counts are enabled */ >> - if ((mode & BIT(0)) && (drvdata->q_support & BIT(0))) >> + if ((mode & BIT(0)) && drvdata->q_support) >> config->cfg |= BIT(13); > > This can be trivially changed to > > if ((mode) && drvdata->q_support) > config->cfg |= BIT(13); > > to ensure that any input mode 2'b01, 2'b10, 2'b11 results in output > settings of 2'b01, 2'b11, 2'b11 respectively > > With that > > Reviewed-by: Mike Leach <mike.leach@linaro.org> > Thanks for the review. Can I also get your opinion about this one? At the very least I think the comment is not detailed enough, but I also think that case 0 should set BIT(0) rather than BIT(1). Or is there some edge case and it's correct? /* start by clearing all instruction event enable bits */ config->eventctrl1 &= ~(BIT(0) | BIT(1) | BIT(2) | BIT(3)); switch (drvdata->nr_event) { case 0x0: /* generate Event element for event 1 */ config->eventctrl1 |= val & BIT(1); break; case 0x1: /* generate Event element for event 1 and 2 */ config->eventctrl1 |= val & (BIT(0) | BIT(1)); break; case 0x2: /* generate Event element for event 1, 2 and 3 */ config->eventctrl1 |= val & (BIT(0) | BIT(1) | BIT(2)); break; case 0x3: /* generate Event element for all 4 events */ config->eventctrl1 |= val & 0xF; break; default: break; } >> /* >> * if supported, Q elements with and without instruction >> -- >> 2.17.1 >> > > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] coresight: Fix TRCCONFIGR.QE sysfs interface 2022-01-20 11:37 ` James Clark @ 2022-01-21 12:40 ` Mike Leach 0 siblings, 0 replies; 5+ messages in thread From: Mike Leach @ 2022-01-21 12:40 UTC (permalink / raw) To: James Clark Cc: mathieu.poirier, coresight, suzuki.poulose, Leo Yan, linux-arm-kernel, linux-kernel Hi James, On Thu, 20 Jan 2022 at 11:37, James Clark <james.clark@arm.com> wrote: > > > > On 19/01/2022 15:22, Mike Leach wrote: > > Hi James > > > > On Fri, 14 Jan 2022 at 17:33, James Clark <james.clark@arm.com> wrote: > >> > >> It's impossible to program a valid value for TRCCONFIGR.QE > >> when TRCIDR0.QSUPP==0b10. In that case the following is true: > >> > >> Q element support is implemented, and only supports Q elements without > >> instruction counts. TRCCONFIGR.QE can only take the values 0b00 or 0b11. > >> > >> Currently the low bit of QSUPP is checked to see if the low bit of QE can > >> be written to, but as you can see when QSUPP==0b10 the low bit is cleared > >> making it impossible to ever write the only valid value of 0b11 to QE. > >> 0b10 would be written instead, which is a reserved QE value even for all > >> values of QSUPP. > >> > >> The fix is to allow writing the low bit of QE for any non zero value of > >> QSUPP. > >> > >> This change doesn't go any further to validate if the user supplied value > >> is valid, because none of the other parts this function do, but it does fix > >> the case where it was impossible to ever set a valid value. > >> > > > > I concur that the input is not checked as valid, However all the other > > fields are single bit - with no invalid values - other than the cond > > field, which controls tracing of non-branch conditionals - > > architecturally unsupported for A profile in ETMv4/. > > > > > >> Signed-off-by: James Clark <james.clark@arm.com> > >> --- > >> drivers/hwtracing/coresight/coresight-etm4x-sysfs.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> index a0640fa5c55b..a99bb537ea23 100644 > >> --- a/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> +++ b/drivers/hwtracing/coresight/coresight-etm4x-sysfs.c > >> @@ -368,7 +368,7 @@ static ssize_t mode_store(struct device *dev, > >> /* start by clearing QE bits */ > >> config->cfg &= ~(BIT(13) | BIT(14)); > >> /* if supported, Q elements with instruction counts are enabled */ > >> - if ((mode & BIT(0)) && (drvdata->q_support & BIT(0))) > >> + if ((mode & BIT(0)) && drvdata->q_support) > >> config->cfg |= BIT(13); > > > > This can be trivially changed to > > > > if ((mode) && drvdata->q_support) > > config->cfg |= BIT(13); > > > > to ensure that any input mode 2'b01, 2'b10, 2'b11 results in output > > settings of 2'b01, 2'b11, 2'b11 respectively > > > > With that > > > > Reviewed-by: Mike Leach <mike.leach@linaro.org> > > > > Thanks for the review. Can I also get your opinion about this one? At the very least > I think the comment is not detailed enough, but I also think that case 0 should set > BIT(0) rather than BIT(1). Or is there some edge case and it's correct? > Agreed - case 0 should be BIT(0) However, now you mention it there is a corner case too... On an ETM4.3 or later, if TRCIDR4.NUMRSPAIR == 0, then drvdata->nr_event == 0 means zero events, otherwise drvdata->nr_event == 0 means 1 event as implemented in the code below. So a check on drvdata->nr_resource (which is the processed NUMRSPAIR value wrt etm arch ) to skip setting eventctrl1 if this is 0 could be added. Regards Mike Mike > > /* start by clearing all instruction event enable bits */ > config->eventctrl1 &= ~(BIT(0) | BIT(1) | BIT(2) | BIT(3)); > switch (drvdata->nr_event) { > case 0x0: > /* generate Event element for event 1 */ > config->eventctrl1 |= val & BIT(1); > break; > case 0x1: > /* generate Event element for event 1 and 2 */ > config->eventctrl1 |= val & (BIT(0) | BIT(1)); > break; > case 0x2: > /* generate Event element for event 1, 2 and 3 */ > config->eventctrl1 |= val & (BIT(0) | BIT(1) | BIT(2)); > break; > case 0x3: > /* generate Event element for all 4 events */ > config->eventctrl1 |= val & 0xF; > break; > default: > break; > } > > > >> /* > >> * if supported, Q elements with and without instruction > >> -- > >> 2.17.1 > >> > > > > -- Mike Leach Principal Engineer, ARM Ltd. Manchester Design Centre. UK ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-01-21 12:41 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-01-14 17:32 [PATCH 0/1] coresight: Fix TRCCONFIGR.QE sysfs interface James Clark 2022-01-14 17:32 ` [PATCH 1/1] " James Clark 2022-01-19 15:22 ` Mike Leach 2022-01-20 11:37 ` James Clark 2022-01-21 12:40 ` Mike Leach
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).