[bug report] drm/amdgpu/gfx7: set USER_SHADER_ARRAY_CONFIG based on disable_cu parameter
Am 03.08.2016 um 11:16 schrieb Nicolai Hähnle:
> On 03.08.2016 11:09, Dan Carpenter wrote:
>> Hello Nicolai Hähnle,
>>
>> The patch 324c614a819a: "drm/amdgpu/gfx7: set
>> USER_SHADER_ARRAY_CONFIG based on disable_cu parameter" from Jun 17,
>> 2016, leads to the following static checker warning:
>>
>> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c:5057 gfx_v7_0_get_cu_info()
>> error: buffer overflow 'cu_info->bitmap' 4 <= 4
>>
>> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>> 5035 static void gfx_v7_0_get_cu_info(struct amdgpu_device *adev)
>> 5036 {
>> 5037 int i, j, k, counter, active_cu_number = 0;
>> 5038 u32 mask, bitmap, ao_bitmap, ao_cu_mask = 0;
>> 5039 struct amdgpu_cu_info *cu_info = >gfx.cu_info;
>> 5040 unsigned disable_masks[4 * 2];
>> 5041
>> 5042 memset(cu_info, 0, sizeof(*cu_info));
>> 5043
>> 5044 amdgpu_gfx_parse_disable_cu(disable_masks, 4, 2);
>> 5045
>> 5046 mutex_lock(>grbm_idx_mutex);
>> 5047 for (i = 0; i < adev->gfx.config.max_shader_engines;
>> i++) {
>> 5048 for (j = 0; j <
>> adev->gfx.config.max_sh_per_se; j++) {
>> 5049 mask = 1;
>> 5050 ao_bitmap = 0;
>> 5051 counter = 0;
>> 5052 gfx_v7_0_select_se_sh(adev, i, j,
>> 0x);
>> 5053 if (i < 4 && j < 2)
>> ^
>> Is it really possible for i to be >= 4?
>
> No, because for that to happen we would have to add support for
> hardware with > 4 shader engines. What's the idiomatic way to express
> this kind of assumption in the kernel?
> BUG_ON(adev->gfx.config.max_shader_engines > 4)? Some other form of
> assert?
Either WARN_ON() or BUG_ON().
BUG_ON() stops any current processing because we run into such a fatal
issue that continuing would cause more damage than just stopping the
current process and waiting forever.
So for this case WARN_ON() sounds more appropriate.
Regards,
Christian.
>
> Nicolai
>
>>
>> 5054 gfx_v7_0_set_user_cu_inactive_bitmap(
>> 5055 adev, disable_masks[i
>> * 2 + j]);
>> 5056 bitmap =
>> gfx_v7_0_get_cu_active_bitmap(adev);
>> 5057 cu_info->bitmap[i][j] = bitmap;
>> ^
>> Because if so, then we are screwed here.
>>
>> 5058
>> 5059 for (k = 0; k < 16; k ++) {
>> 5060 if (bitmap & mask) {
>> 5061 if (counter < 2)
>> 5062 ao_bitmap |= mask;
>> 5063 counter ++;
>> 5064 }
>> 5065 mask <<= 1;
>> 5066 }
>> 5067 active_cu_number += counter;
>> 5068 ao_cu_mask |= (ao_bitmap << (i * 16 +
>> j * 8));
>> 5069 }
>> 5070 }
>> 5071 gfx_v7_0_select_se_sh(adev, 0x, 0x,
>> 0x);
>> 5072 mutex_unlock(>grbm_idx_mutex);
>> 5073
>> 5074 cu_info->number = active_cu_number;
>> 5075 cu_info->ao_cu_mask = ao_cu_mask;
>> 5076 }
>>
>> regards,
>> dan carpenter
>>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
[bug report] drm/amdgpu/gfx7: set USER_SHADER_ARRAY_CONFIG based on disable_cu parameter
Hello Nicolai Hähnle,
The patch 324c614a819a: "drm/amdgpu/gfx7: set
USER_SHADER_ARRAY_CONFIG based on disable_cu parameter" from Jun 17,
2016, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c:5057 gfx_v7_0_get_cu_info()
error: buffer overflow 'cu_info->bitmap' 4 <= 4
drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
5035 static void gfx_v7_0_get_cu_info(struct amdgpu_device *adev)
5036 {
5037 int i, j, k, counter, active_cu_number = 0;
5038 u32 mask, bitmap, ao_bitmap, ao_cu_mask = 0;
5039 struct amdgpu_cu_info *cu_info = >gfx.cu_info;
5040 unsigned disable_masks[4 * 2];
5041
5042 memset(cu_info, 0, sizeof(*cu_info));
5043
5044 amdgpu_gfx_parse_disable_cu(disable_masks, 4, 2);
5045
5046 mutex_lock(>grbm_idx_mutex);
5047 for (i = 0; i < adev->gfx.config.max_shader_engines; i++) {
5048 for (j = 0; j < adev->gfx.config.max_sh_per_se; j++) {
5049 mask = 1;
5050 ao_bitmap = 0;
5051 counter = 0;
5052 gfx_v7_0_select_se_sh(adev, i, j, 0x);
5053 if (i < 4 && j < 2)
^
Is it really possible for i to be >= 4?
5054 gfx_v7_0_set_user_cu_inactive_bitmap(
5055 adev, disable_masks[i * 2 + j]);
5056 bitmap = gfx_v7_0_get_cu_active_bitmap(adev);
5057 cu_info->bitmap[i][j] = bitmap;
^
Because if so, then we are screwed here.
5058
5059 for (k = 0; k < 16; k ++) {
5060 if (bitmap & mask) {
5061 if (counter < 2)
5062 ao_bitmap |= mask;
5063 counter ++;
5064 }
5065 mask <<= 1;
5066 }
5067 active_cu_number += counter;
5068 ao_cu_mask |= (ao_bitmap << (i * 16 + j * 8));
5069 }
5070 }
5071 gfx_v7_0_select_se_sh(adev, 0x, 0x, 0x);
5072 mutex_unlock(>grbm_idx_mutex);
5073
5074 cu_info->number = active_cu_number;
5075 cu_info->ao_cu_mask = ao_cu_mask;
5076 }
regards,
dan carpenter
[bug report] drm/amdgpu/gfx7: set USER_SHADER_ARRAY_CONFIG based on disable_cu parameter
On 03.08.2016 11:09, Dan Carpenter wrote:
> Hello Nicolai Hähnle,
>
> The patch 324c614a819a: "drm/amdgpu/gfx7: set
> USER_SHADER_ARRAY_CONFIG based on disable_cu parameter" from Jun 17,
> 2016, leads to the following static checker warning:
>
> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c:5057 gfx_v7_0_get_cu_info()
> error: buffer overflow 'cu_info->bitmap' 4 <= 4
>
> drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> 5035 static void gfx_v7_0_get_cu_info(struct amdgpu_device *adev)
> 5036 {
> 5037 int i, j, k, counter, active_cu_number = 0;
> 5038 u32 mask, bitmap, ao_bitmap, ao_cu_mask = 0;
> 5039 struct amdgpu_cu_info *cu_info = >gfx.cu_info;
> 5040 unsigned disable_masks[4 * 2];
> 5041
> 5042 memset(cu_info, 0, sizeof(*cu_info));
> 5043
> 5044 amdgpu_gfx_parse_disable_cu(disable_masks, 4, 2);
> 5045
> 5046 mutex_lock(>grbm_idx_mutex);
> 5047 for (i = 0; i < adev->gfx.config.max_shader_engines; i++) {
> 5048 for (j = 0; j < adev->gfx.config.max_sh_per_se; j++) {
> 5049 mask = 1;
> 5050 ao_bitmap = 0;
> 5051 counter = 0;
> 5052 gfx_v7_0_select_se_sh(adev, i, j, 0x);
> 5053 if (i < 4 && j < 2)
> ^
> Is it really possible for i to be >= 4?
No, because for that to happen we would have to add support for hardware
with > 4 shader engines. What's the idiomatic way to express this kind
of assumption in the kernel? BUG_ON(adev->gfx.config.max_shader_engines
> 4)? Some other form of assert?
Nicolai
>
> 5054 gfx_v7_0_set_user_cu_inactive_bitmap(
> 5055 adev, disable_masks[i * 2 +
> j]);
> 5056 bitmap = gfx_v7_0_get_cu_active_bitmap(adev);
> 5057 cu_info->bitmap[i][j] = bitmap;
> ^
> Because if so, then we are screwed here.
>
> 5058
> 5059 for (k = 0; k < 16; k ++) {
> 5060 if (bitmap & mask) {
> 5061 if (counter < 2)
> 5062 ao_bitmap |= mask;
> 5063 counter ++;
> 5064 }
> 5065 mask <<= 1;
> 5066 }
> 5067 active_cu_number += counter;
> 5068 ao_cu_mask |= (ao_bitmap << (i * 16 + j * 8));
> 5069 }
> 5070 }
> 5071 gfx_v7_0_select_se_sh(adev, 0x, 0x,
> 0x);
> 5072 mutex_unlock(>grbm_idx_mutex);
> 5073
> 5074 cu_info->number = active_cu_number;
> 5075 cu_info->ao_cu_mask = ao_cu_mask;
> 5076 }
>
> regards,
> dan carpenter
>
