[Mesa-dev] radeonsi: bugfix in performance counters

Submitted by Mauro Rossi on May 12, 2017, 10:07 p.m.

Details

Message ID 20170512220716.1553-1-issor.oruam@gmail.com
State New
Headers show
Series "radeonsi: bugfix in performance counters" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Mauro Rossi May 12, 2017, 10:07 p.m.
'if (regs->counters)' expression at line 623 is always true,
spotted because of an error when building android-x86.

'if (regs->counters[idx])' is used instead.

Fixes the following building error in Android:

external/mesa/src/gallium/drivers/radeonsi/si_perfcounter.c:617:14:
error: address of array 'regs->counters' will always evaluate to 'true' [-Werror,-Wpointer-bool-conversion]
                        if (regs->counters)
                        ~~  ~~~~~~^~~~~~~~

Fixes: ad22006 "radeonsi: implement AMD_performance_monitor for CIK+"
---
 src/gallium/drivers/radeonsi/si_perfcounter.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/radeonsi/si_perfcounter.c b/src/gallium/drivers/radeonsi/si_perfcounter.c
index 41dd52edb1..85b6e5c55a 100644
--- a/src/gallium/drivers/radeonsi/si_perfcounter.c
+++ b/src/gallium/drivers/radeonsi/si_perfcounter.c
@@ -620,7 +620,7 @@  static void si_pc_emit_read(struct r600_common_context *ctx,
 			reg_delta = -reg_delta;
 
 		for (idx = 0; idx < count; ++idx) {
-			if (regs->counters)
+			if (regs->counters[idx])
 				reg = regs->counters[idx];
 
 			radeon_emit(cs, PKT3(PKT3_COPY_DATA, 4, 0));

Comments

Reviewed-by: Marek Olšák <marek.olsak@amd.com>

Note that this is not meant for master, because master doesn't need this
fix anymore.

Marek

On Sat, May 13, 2017 at 12:07 AM, Mauro Rossi <issor.oruam@gmail.com> wrote:

> 'if (regs->counters)' expression at line 623 is always true,
> spotted because of an error when building android-x86.
>
> 'if (regs->counters[idx])' is used instead.
>
> Fixes the following building error in Android:
>
> external/mesa/src/gallium/drivers/radeonsi/si_perfcounter.c:617:14:
> error: address of array 'regs->counters' will always evaluate to 'true'
> [-Werror,-Wpointer-bool-conversion]
>                         if (regs->counters)
>                         ~~  ~~~~~~^~~~~~~~
>
> Fixes: ad22006 "radeonsi: implement AMD_performance_monitor for CIK+"
> ---
>  src/gallium/drivers/radeonsi/si_perfcounter.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_perfcounter.c
> b/src/gallium/drivers/radeonsi/si_perfcounter.c
> index 41dd52edb1..85b6e5c55a 100644
> --- a/src/gallium/drivers/radeonsi/si_perfcounter.c
> +++ b/src/gallium/drivers/radeonsi/si_perfcounter.c
> @@ -620,7 +620,7 @@ static void si_pc_emit_read(struct r600_common_context
> *ctx,
>                         reg_delta = -reg_delta;
>
>                 for (idx = 0; idx < count; ++idx) {
> -                       if (regs->counters)
> +                       if (regs->counters[idx])
>                                 reg = regs->counters[idx];
>
>                         radeon_emit(cs, PKT3(PKT3_COPY_DATA, 4, 0));
> --
> 2.11.0
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On 13 May 2017 at 20:32, Marek Olšák <maraeo@gmail.com> wrote:
> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>
> Note that this is not meant for master, because master doesn't need this fix
> anymore.
>
A tad silly question: is that really the case?
AFAICT there's some related changes with commit 156e81f305b although
the patch seem applicable to master.

Thanks
Emil
On Mon, May 15, 2017 at 1:11 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 13 May 2017 at 20:32, Marek Olšák <maraeo@gmail.com> wrote:
>> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>>
>> Note that this is not meant for master, because master doesn't need this fix
>> anymore.
>>
> A tad silly question: is that really the case?
> AFAICT there's some related changes with commit 156e81f305b although
> the patch seem applicable to master.

The patch may apply cleanly, but I think it's not correct with the
current code in master.

Marek
On 15 May 2017 at 13:25, Marek Olšák <maraeo@gmail.com> wrote:
> On Mon, May 15, 2017 at 1:11 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>> On 13 May 2017 at 20:32, Marek Olšák <maraeo@gmail.com> wrote:
>>> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>>>
>>> Note that this is not meant for master, because master doesn't need this fix
>>> anymore.
>>>
>> A tad silly question: is that really the case?
>> AFAICT there's some related changes with commit 156e81f305b although
>> the patch seem applicable to master.
>
> The patch may apply cleanly, but I think it's not correct with the
> current code in master.
>
Right, thanks for the correction. Forwarding to mesa-stable.

Is it me or as-is the patch can cause null ptr deref? In all but one
case the counters ptr is NULL, yet most of the time one would read
more than one counter.
Perhaps one should use - if (regs->counters && regs->counters[idx])

-Emil
On Mon, May 15, 2017 at 4:31 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
> On 15 May 2017 at 13:25, Marek Olšák <maraeo@gmail.com> wrote:
>> On Mon, May 15, 2017 at 1:11 PM, Emil Velikov <emil.l.velikov@gmail.com> wrote:
>>> On 13 May 2017 at 20:32, Marek Olšák <maraeo@gmail.com> wrote:
>>>> Reviewed-by: Marek Olšák <marek.olsak@amd.com>
>>>>
>>>> Note that this is not meant for master, because master doesn't need this fix
>>>> anymore.
>>>>
>>> A tad silly question: is that really the case?
>>> AFAICT there's some related changes with commit 156e81f305b although
>>> the patch seem applicable to master.
>>
>> The patch may apply cleanly, but I think it's not correct with the
>> current code in master.
>>
> Right, thanks for the correction. Forwarding to mesa-stable.
>
> Is it me or as-is the patch can cause null ptr deref? In all but one
> case the counters ptr is NULL, yet most of the time one would read
> more than one counter.
> Perhaps one should use - if (regs->counters && regs->counters[idx])

Please don't apply the patch to stable either. The patch is for a bug
that was on master for only 3 days:

Added here:
https://cgit.freedesktop.org/mesa/mesa/commit/?id=7088b655e8828bb960f528dd33132de27c505b8f
Reverted here:
https://cgit.freedesktop.org/mesa/mesa/commit/?id=314657dc11db651268ce5a0c9713337ddc68fe31

Marek