* [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
@ 2016-03-21 12:10 Nicolai Stange
2016-03-21 16:09 ` Alex Deucher
0 siblings, 1 reply; 5+ messages in thread
From: Nicolai Stange @ 2016-03-21 12:10 UTC (permalink / raw)
To: David Airlie, Alex Deucher, Christian König
Cc: dri-devel, linux-kernel, Nicolai Stange
The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id
enum transform 1:1 into bit positions within the 'enabled' bitset as
assembled by evergreen_hpd_init():
enabled |= 1 << radeon_connector->hpd.hpd;
However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN
reports
UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
shift exponent 255 is too large for 32-bit type 'int'
[...]
Call Trace:
[<ffffffff818c4d35>] dump_stack+0xbc/0x117
[<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
[<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
[<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
[<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
[<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
[<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
[<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
[<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
[<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
[<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
[<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
[<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
[<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
[<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
[<ffffffff814b83f0>] ? kfree+0x220/0x370
[<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
[...]
=====================================================================
radeon 0000:01:00.0: No connectors reported connected with modes
At least on x86, there should be no user-visible impact as there
1 << 0xff == 1 << (0xff & 31) == 1 << 31
holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.
All of the above applies analogously to evergreen_hpd_fini().
Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it
into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset
in evergreen_hpd_fini() respectively.
Signed-off-by: Nicolai Stange <nicstange@gmail.com>
---
Applicable to linux-next-20160321.
Changes to v1:
- Turn commit message's impact part into a non-impact part.
drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index 76c4bdf..6360717 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
break;
}
radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
- enabled |= 1 << radeon_connector->hpd.hpd;
+ if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+ enabled |= 1 << radeon_connector->hpd.hpd;
}
radeon_irq_kms_enable_hpd(rdev, enabled);
}
@@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
default:
break;
}
- disabled |= 1 << radeon_connector->hpd.hpd;
+ if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
+ disabled |= 1 << radeon_connector->hpd.hpd;
}
radeon_irq_kms_disable_hpd(rdev, disabled);
}
--
2.7.3
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
2016-03-21 12:10 [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset Nicolai Stange
@ 2016-03-21 16:09 ` Alex Deucher
2016-03-22 12:29 ` Nicolai Stange
0 siblings, 1 reply; 5+ messages in thread
From: Alex Deucher @ 2016-03-21 16:09 UTC (permalink / raw)
To: Nicolai Stange
Cc: David Airlie, Alex Deucher, Christian König, LKML,
Maling list - DRI developers
On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote:
> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id
> enum transform 1:1 into bit positions within the 'enabled' bitset as
> assembled by evergreen_hpd_init():
>
> enabled |= 1 << radeon_connector->hpd.hpd;
>
> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN
> reports
>
> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
> shift exponent 255 is too large for 32-bit type 'int'
> [...]
> Call Trace:
> [<ffffffff818c4d35>] dump_stack+0xbc/0x117
> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
> [<ffffffff814b83f0>] ? kfree+0x220/0x370
> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
> [...]
> =====================================================================
> radeon 0000:01:00.0: No connectors reported connected with modes
>
> At least on x86, there should be no user-visible impact as there
>
> 1 << 0xff == 1 << (0xff & 31) == 1 << 31
>
> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.
>
> All of the above applies analogously to evergreen_hpd_fini().
>
> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it
> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset
> in evergreen_hpd_fini() respectively.
>
> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
Can you please fix all instances of this? This same pattern is
repeated in various asic families.
Alex
> ---
> Applicable to linux-next-20160321.
>
> Changes to v1:
> - Turn commit message's impact part into a non-impact part.
>
> drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
> index 76c4bdf..6360717 100644
> --- a/drivers/gpu/drm/radeon/evergreen.c
> +++ b/drivers/gpu/drm/radeon/evergreen.c
> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
> break;
> }
> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
> - enabled |= 1 << radeon_connector->hpd.hpd;
> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
> + enabled |= 1 << radeon_connector->hpd.hpd;
> }
> radeon_irq_kms_enable_hpd(rdev, enabled);
> }
> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
> default:
> break;
> }
> - disabled |= 1 << radeon_connector->hpd.hpd;
> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
> + disabled |= 1 << radeon_connector->hpd.hpd;
> }
> radeon_irq_kms_disable_hpd(rdev, disabled);
> }
> --
> 2.7.3
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
2016-03-21 16:09 ` Alex Deucher
@ 2016-03-22 12:29 ` Nicolai Stange
2016-03-22 14:32 ` Alex Deucher
0 siblings, 1 reply; 5+ messages in thread
From: Nicolai Stange @ 2016-03-22 12:29 UTC (permalink / raw)
To: Alex Deucher
Cc: Nicolai Stange, David Airlie, Alex Deucher, Christian König,
LKML, Maling list - DRI developers
Alex Deucher <alexdeucher@gmail.com> writes:
> On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote:
>> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id
>> enum transform 1:1 into bit positions within the 'enabled' bitset as
>> assembled by evergreen_hpd_init():
>>
>> enabled |= 1 << radeon_connector->hpd.hpd;
>>
>> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN
>> reports
>>
>> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
>> shift exponent 255 is too large for 32-bit type 'int'
>> [...]
>> Call Trace:
>> [<ffffffff818c4d35>] dump_stack+0xbc/0x117
>> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
>> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
>> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
>> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
>> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
>> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
>> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
>> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
>> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
>> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
>> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
>> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
>> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
>> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
>> [<ffffffff814b83f0>] ? kfree+0x220/0x370
>> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
>> [...]
>> =====================================================================
>> radeon 0000:01:00.0: No connectors reported connected with modes
>>
>> At least on x86, there should be no user-visible impact as there
>>
>> 1 << 0xff == 1 << (0xff & 31) == 1 << 31
>>
>> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.
>>
>> All of the above applies analogously to evergreen_hpd_fini().
>>
>> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it
>> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset
>> in evergreen_hpd_fini() respectively.
>>
>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>
> Can you please fix all instances of this? This same pattern is
> repeated in various asic families.
Sure.
The places to fix are:
$ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/
drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd;
drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd;
May I throw all of those into one single patch?
Thanks,
Nicolai
>
>> ---
>> Applicable to linux-next-20160321.
>>
>> Changes to v1:
>> - Turn commit message's impact part into a non-impact part.
>>
>> drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>> index 76c4bdf..6360717 100644
>> --- a/drivers/gpu/drm/radeon/evergreen.c
>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
>> break;
>> }
>> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
>> - enabled |= 1 << radeon_connector->hpd.hpd;
>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>> + enabled |= 1 << radeon_connector->hpd.hpd;
>> }
>> radeon_irq_kms_enable_hpd(rdev, enabled);
>> }
>> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
>> default:
>> break;
>> }
>> - disabled |= 1 << radeon_connector->hpd.hpd;
>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>> + disabled |= 1 << radeon_connector->hpd.hpd;
>> }
>> radeon_irq_kms_disable_hpd(rdev, disabled);
>> }
>> --
>> 2.7.3
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
2016-03-22 12:29 ` Nicolai Stange
@ 2016-03-22 14:32 ` Alex Deucher
0 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2016-03-22 14:32 UTC (permalink / raw)
To: Nicolai Stange
Cc: David Airlie, Alex Deucher, Christian König, LKML,
Maling list - DRI developers
On Tue, Mar 22, 2016 at 8:29 AM, Nicolai Stange <nicstange@gmail.com> wrote:
> Alex Deucher <alexdeucher@gmail.com> writes:
>
>> On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote:
>>> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id
>>> enum transform 1:1 into bit positions within the 'enabled' bitset as
>>> assembled by evergreen_hpd_init():
>>>
>>> enabled |= 1 << radeon_connector->hpd.hpd;
>>>
>>> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN
>>> reports
>>>
>>> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
>>> shift exponent 255 is too large for 32-bit type 'int'
>>> [...]
>>> Call Trace:
>>> [<ffffffff818c4d35>] dump_stack+0xbc/0x117
>>> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
>>> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
>>> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
>>> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
>>> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
>>> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
>>> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
>>> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
>>> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
>>> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
>>> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
>>> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
>>> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
>>> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
>>> [<ffffffff814b83f0>] ? kfree+0x220/0x370
>>> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
>>> [...]
>>> =====================================================================
>>> radeon 0000:01:00.0: No connectors reported connected with modes
>>>
>>> At least on x86, there should be no user-visible impact as there
>>>
>>> 1 << 0xff == 1 << (0xff & 31) == 1 << 31
>>>
>>> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.
>>>
>>> All of the above applies analogously to evergreen_hpd_fini().
>>>
>>> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it
>>> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset
>>> in evergreen_hpd_fini() respectively.
>>>
>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>
>> Can you please fix all instances of this? This same pattern is
>> repeated in various asic families.
>
> Sure.
>
> The places to fix are:
> $ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/
> drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd;
>
> May I throw all of those into one single patch?
Yes, that would be fine.
Alex
>
>
> Thanks,
>
> Nicolai
>
>
>>
>>> ---
>>> Applicable to linux-next-20160321.
>>>
>>> Changes to v1:
>>> - Turn commit message's impact part into a non-impact part.
>>>
>>> drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>>> index 76c4bdf..6360717 100644
>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
>>> break;
>>> }
>>> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
>>> - enabled |= 1 << radeon_connector->hpd.hpd;
>>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>>> + enabled |= 1 << radeon_connector->hpd.hpd;
>>> }
>>> radeon_irq_kms_enable_hpd(rdev, enabled);
>>> }
>>> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
>>> default:
>>> break;
>>> }
>>> - disabled |= 1 << radeon_connector->hpd.hpd;
>>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>>> + disabled |= 1 << radeon_connector->hpd.hpd;
>>> }
>>> radeon_irq_kms_disable_hpd(rdev, disabled);
>>> }
>>> --
>>> 2.7.3
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset
@ 2016-03-22 14:32 ` Alex Deucher
0 siblings, 0 replies; 5+ messages in thread
From: Alex Deucher @ 2016-03-22 14:32 UTC (permalink / raw)
To: Nicolai Stange
Cc: Alex Deucher, Christian König, Maling list - DRI developers, LKML
On Tue, Mar 22, 2016 at 8:29 AM, Nicolai Stange <nicstange@gmail.com> wrote:
> Alex Deucher <alexdeucher@gmail.com> writes:
>
>> On Mon, Mar 21, 2016 at 8:10 AM, Nicolai Stange <nicstange@gmail.com> wrote:
>>> The values of all but the RADEON_HPD_NONE members of the radeon_hpd_id
>>> enum transform 1:1 into bit positions within the 'enabled' bitset as
>>> assembled by evergreen_hpd_init():
>>>
>>> enabled |= 1 << radeon_connector->hpd.hpd;
>>>
>>> However, if ->hpd.hpd happens to equal RADEON_HPD_NONE == 0xff, UBSAN
>>> reports
>>>
>>> UBSAN: Undefined behaviour in drivers/gpu/drm/radeon/evergreen.c:1867:16
>>> shift exponent 255 is too large for 32-bit type 'int'
>>> [...]
>>> Call Trace:
>>> [<ffffffff818c4d35>] dump_stack+0xbc/0x117
>>> [<ffffffff818c4c79>] ? _atomic_dec_and_lock+0x169/0x169
>>> [<ffffffff819411bb>] ubsan_epilogue+0xd/0x4e
>>> [<ffffffff81941cbc>] __ubsan_handle_shift_out_of_bounds+0x1fb/0x254
>>> [<ffffffffa0ba7f2e>] ? atom_execute_table+0x3e/0x50 [radeon]
>>> [<ffffffff81941ac1>] ? __ubsan_handle_load_invalid_value+0x158/0x158
>>> [<ffffffffa0b87700>] ? radeon_get_pll_use_mask+0x130/0x130 [radeon]
>>> [<ffffffff81219930>] ? wake_up_klogd_work_func+0x60/0x60
>>> [<ffffffff8121a35e>] ? vprintk_default+0x3e/0x60
>>> [<ffffffffa0c603c4>] evergreen_hpd_init+0x274/0x2d0 [radeon]
>>> [<ffffffffa0c603c4>] ? evergreen_hpd_init+0x274/0x2d0 [radeon]
>>> [<ffffffffa0bd196e>] radeon_modeset_init+0x8ce/0x18d0 [radeon]
>>> [<ffffffffa0b71d86>] radeon_driver_load_kms+0x186/0x350 [radeon]
>>> [<ffffffffa03b6b16>] drm_dev_register+0xc6/0x100 [drm]
>>> [<ffffffffa03bc8c4>] drm_get_pci_dev+0xe4/0x490 [drm]
>>> [<ffffffff814b83f0>] ? kfree+0x220/0x370
>>> [<ffffffffa0b687c2>] radeon_pci_probe+0x112/0x140 [radeon]
>>> [...]
>>> =====================================================================
>>> radeon 0000:01:00.0: No connectors reported connected with modes
>>>
>>> At least on x86, there should be no user-visible impact as there
>>>
>>> 1 << 0xff == 1 << (0xff & 31) == 1 << 31
>>>
>>> holds and 31 > RADEON_MAX_HPD_PINS. Thus, this patch is a cosmetic one.
>>>
>>> All of the above applies analogously to evergreen_hpd_fini().
>>>
>>> Silence UBSAN by checking ->hpd.hpd for RADEON_HPD_NONE before oring it
>>> into the 'enabled' bitset in evergreen_hpd_init() or the 'disabled' bitset
>>> in evergreen_hpd_fini() respectively.
>>>
>>> Signed-off-by: Nicolai Stange <nicstange@gmail.com>
>>
>> Can you please fix all instances of this? This same pattern is
>> repeated in various asic families.
>
> Sure.
>
> The places to fix are:
> $ git grep -n '1 *<<.*hpd' -- drivers/gpu/drm/radeon/
> drivers/gpu/drm/radeon/evergreen.c:1868: enabled |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/evergreen.c:1912: disabled |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r100.c:595: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r100.c:617: disable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r600.c:1005: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/r600.c:1058: disable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/rs600.c:416: enable |= 1 << radeon_connector->hpd.hpd;
> drivers/gpu/drm/radeon/rs600.c:442: disable |= 1 << radeon_connector->hpd.hpd;
>
> May I throw all of those into one single patch?
Yes, that would be fine.
Alex
>
>
> Thanks,
>
> Nicolai
>
>
>>
>>> ---
>>> Applicable to linux-next-20160321.
>>>
>>> Changes to v1:
>>> - Turn commit message's impact part into a non-impact part.
>>>
>>> drivers/gpu/drm/radeon/evergreen.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
>>> index 76c4bdf..6360717 100644
>>> --- a/drivers/gpu/drm/radeon/evergreen.c
>>> +++ b/drivers/gpu/drm/radeon/evergreen.c
>>> @@ -1864,7 +1864,8 @@ void evergreen_hpd_init(struct radeon_device *rdev)
>>> break;
>>> }
>>> radeon_hpd_set_polarity(rdev, radeon_connector->hpd.hpd);
>>> - enabled |= 1 << radeon_connector->hpd.hpd;
>>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>>> + enabled |= 1 << radeon_connector->hpd.hpd;
>>> }
>>> radeon_irq_kms_enable_hpd(rdev, enabled);
>>> }
>>> @@ -1907,7 +1908,8 @@ void evergreen_hpd_fini(struct radeon_device *rdev)
>>> default:
>>> break;
>>> }
>>> - disabled |= 1 << radeon_connector->hpd.hpd;
>>> + if (radeon_connector->hpd.hpd != RADEON_HPD_NONE)
>>> + disabled |= 1 << radeon_connector->hpd.hpd;
>>> }
>>> radeon_irq_kms_disable_hpd(rdev, disabled);
>>> }
>>> --
>>> 2.7.3
>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-03-22 14:32 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-03-21 12:10 [PATCH v2] drm/radeon: evergreen_hpd_init()/_fini(): fix HPD IRQ bitset Nicolai Stange
2016-03-21 16:09 ` Alex Deucher
2016-03-22 12:29 ` Nicolai Stange
2016-03-22 14:32 ` Alex Deucher
2016-03-22 14:32 ` Alex Deucher
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.