AW: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

[Walter Harms]

thx for info

Von: Christian König 
Gesendet: Montag, 8. Februar 2021 13:14:49
An: Walter Harms; Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang 
Rui; Junwei Zhang; amd-...@lists.freedesktop.org; 
dri-devel@lists.freedesktop.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a 
int

For start and end?  The hardware has 48 bit address space and that won't
fit into 32bits.

Only the fragment handling can't do more than 2GB at the same time.

Christian.

Am 08.02.21 um 12:05 schrieb Walter Harms:
> i am curious:
> what is the win to have a unsigned 64 bit integer in the first
> place ?
>
> re,
>   wh
> 
> Von: Christian König 
> Gesendet: Montag, 8. Februar 2021 10:17:42
> An: Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei 
> Zhang; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
> Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a 
> int
>
> Am 08.02.21 um 00:07 schrieb Colin King:
>> From: Colin Ian King 
>>
>> The left shift of int 32 bit integer constant 1 is evaluated using 32
>> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
>> case where *frag is 32 or more this can lead to an oveflow.  Avoid this
>> by shifting 1ULL.
> Well that can't happen. Take a look at the code in that function:
>
>>  max_frag = 31;
> ...
>>  if (*frag >= max_frag) {
>>  *frag = max_frag;
>>  *frag_end = end & ~((1ULL << max_frag) - 1);
>>  } else {
>>  *frag_end = start + (1 << *frag);
>>  }
> But I'm fine with applying the patch if it silences your warning.
>
> Regards,
> Christian.
>
>> Addresses-Coverity: ("Unintentional integer overflow")
>> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page 
>> handling")
>> Signed-off-by: Colin Ian King 
>> ---
>>drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>>1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9d19078246c8..53a925600510 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct 
>> amdgpu_vm_update_params *params,
>>*frag = max_frag;
>>*frag_end = end & ~((1ULL << max_frag) - 1);
>>} else {
>> - *frag_end = start + (1 << *frag);
>> + *frag_end = start + (1ULL << *frag);
>>}
>>}
>>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

AW: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a int

[Walter Harms]

i am curious:
what is the win to have a unsigned 64 bit integer in the first
place ?

re,
 wh

Von: Christian König 
Gesendet: Montag, 8. Februar 2021 10:17:42
An: Colin King; Alex Deucher; David Airlie; Daniel Vetter; Huang Rui; Junwei 
Zhang; amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: Re: [PATCH] drm/amdgpu: fix potential integer overflow on shift of a 
int

Am 08.02.21 um 00:07 schrieb Colin King:
> From: Colin Ian King 
>
> The left shift of int 32 bit integer constant 1 is evaluated using 32
> bit arithmetic and then assigned to an unsigned 64 bit integer. In the
> case where *frag is 32 or more this can lead to an oveflow.  Avoid this
> by shifting 1ULL.

Well that can't happen. Take a look at the code in that function:

> max_frag = 31;
...
> if (*frag >= max_frag) {
> *frag = max_frag;
> *frag_end = end & ~((1ULL << max_frag) - 1);
> } else {
> *frag_end = start + (1 << *frag);
> }

But I'm fine with applying the patch if it silences your warning.

Regards,
Christian.

>
> Addresses-Coverity: ("Unintentional integer overflow")
> Fixes: dfcd99f6273e ("drm/amdgpu: meld together VM fragment and huge page 
> handling")
> Signed-off-by: Colin Ian King 
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9d19078246c8..53a925600510 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1412,7 +1412,7 @@ static void amdgpu_vm_fragment(struct 
> amdgpu_vm_update_params *params,
>   *frag = max_frag;
>   *frag_end = end & ~((1ULL << max_frag) - 1);
>   } else {
> - *frag_end = start + (1 << *frag);
> + *frag_end = start + (1ULL << *frag);
>   }
>   }
>

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

AW: [PATCH] drm/amdgpu/display: Fix an error handling path in 'dm_update_crtc_state()'

[Walter Harms]

Von: kernel-janitors-ow...@vger.kernel.org 
 im Auftrag von Christophe JAILLET 

Gesendet: Sonntag, 8. März 2020 10:26
An: harry.wentl...@amd.com; sunpeng...@amd.com; alexander.deuc...@amd.com; 
christian.koe...@amd.com; david1.z...@amd.com; airl...@linux.ie; 
dan...@ffwll.ch; nicholas.kazlaus...@amd.com; bhawanpreet.la...@amd.com; 
mario.kleiner...@gmail.com; david.fran...@amd.com
Cc: amd-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; 
linux-ker...@vger.kernel.org; kernel-janit...@vger.kernel.org; Christophe 
JAILLET
Betreff: [PATCH] drm/amdgpu/display: Fix an error handling path in 
'dm_update_crtc_state()'

'dc_stream_release()' may be called twice. Once here, and once below in the
error handling path if we branch to the 'fail' label.

Set 'new_stream' to NULL, once released to avoid the duplicated release
function call.

Signed-off-by: Christophe JAILLET 
---
Maybe the 'goto fail' at line 7745 should be turned into a 'return ret'
instead. Could be clearer.

No Fixes tag provided because I've not been able to dig deep enough in the
git history.
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 97c1b01c0fc1..9d7773a77c4f 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7704,8 +7704,10 @@ static int dm_update_crtc_state(struct 
amdgpu_display_manager *dm,

 skip_modeset:
/* Release extra reference */
-   if (new_stream)
-dc_stream_release(new_stream);
+   if (new_stream) {
+   dc_stream_release(new_stream);
+   new_stream = NULL;
+   }


dc_stream_release() is NULL-checked, so the if can be dropped.

re,
 wh

/*
 * We want to do dc stream updates that do not require a
--
2.20.1

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

AW: [PATCH][V2] backlight: sky81452: insure while loop does not allow negative array indexing

[Walter Harms]

hi all,
i would suggest converting this in to a more common for() loop.
Programmers are bad in counting backwards. that kind of bug is 
common.

re,
 wh

Von: kernel-janitors-ow...@vger.kernel.org 
 im Auftrag von Colin King 

Gesendet: Mittwoch, 26. Februar 2020 20:58
An: Lee Jones; Daniel Thompson; Jingoo Han; Bartlomiej Zolnierkiewicz; Gyungoh 
Yoo; Bryan Wu; dri-devel@lists.freedesktop.org; linux-fb...@vger.kernel.org
Cc: kernel-janit...@vger.kernel.org; linux-ker...@vger.kernel.org
Betreff: [PATCH][V2] backlight: sky81452: insure while loop does not allow 
negative array indexing

From: Colin Ian King 

In the unlikely event that num_entry is zero, the while loop
pre-decrements num_entry to cause negative array indexing into the
array sources. Fix this by iterating only if num_entry >= 0.

Addresses-Coverity: ("Out-of-bounds read")
Fixes: f705806c9f35 ("backlight: Add support Skyworks SKY81452 backlight 
driver")
Signed-off-by: Colin Ian King 
---

V2: fix typo in commit subject line

---
 drivers/video/backlight/sky81452-backlight.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/backlight/sky81452-backlight.c 
b/drivers/video/backlight/sky81452-backlight.c
index 2355f00f5773..f456930ce78e 100644
--- a/drivers/video/backlight/sky81452-backlight.c
+++ b/drivers/video/backlight/sky81452-backlight.c
@@ -200,7 +200,7 @@ static struct sky81452_bl_platform_data 
*sky81452_bl_parse_dt(
}

pdata->enable = 0;
-   while (--num_entry)
+   while (--num_entry >= 0)
pdata->enable |= (1 << sources[num_entry]);
 
  int i;
for(i=0;ienable |= (1 << sources[i]);

}

--
2.25.0

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH][next] drm/amd/display: fix for-loop with incorrectly sized loop counter

[walter harms]

Am 17.01.2020 14:33, schrieb Colin King:
> From: Colin Ian King 
> 
> A for-loop is iterating from 0 up to 1000 however the loop variable count
> is a u8 and hence not large enough.  Fix this by making count an int.
> Also remove the redundant initialization of count since this is never used
> and add { } on the loop statement make the loop block clearer.
> 
> Addresses-Coverity: ("Operands don't affect result")
> Fixes: ed581a0ace44 ("drm/amd/display: wait for update when setting dpg test 
> pattern")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c 
> b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> index 6ab298c65247..cbed738a4246 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link_dp.c
> @@ -3680,7 +3680,7 @@ static void set_crtc_test_pattern(struct dc_link *link,
>   struct pipe_ctx *odm_pipe;
>   enum controller_dp_color_space controller_color_space;
>   int opp_cnt = 1;
> - uint8_t count = 0;
> + int count;
>  
>   switch (test_pattern_color_space) {
>   case DP_TEST_PATTERN_COLOR_SPACE_RGB:
> @@ -3725,11 +3725,12 @@ static void set_crtc_test_pattern(struct dc_link 
> *link,
>   width,
>   height);
>   /* wait for dpg to blank pixel data with test pattern */
> - for (count = 0; count < 1000; count++)
> + for (count = 0; count < 1000; count++) {
>   if (opp->funcs->dpg_is_blanked(opp))
>   break;
>   else
>   udelay(100);
> + }
>   }
>   }
>   break;

Nitpick:
the else is useless you can remove it.

re,
 wh

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH][drm-next] drm/amd/powerplay: fix a few spelling mistakes

[walter harms]

Am 01.08.2019 10:39, schrieb Colin King:
> From: Colin Ian King 
> 
> There are a few spelling mistakes "unknow" -> "unknown" and
> "enabeld" -> "enabled". Fix these.
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/amd/powerplay/amdgpu_smu.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c 
> b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> index 13b2c8a60232..d029a99e600e 100644
> --- a/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> +++ b/drivers/gpu/drm/amd/powerplay/amdgpu_smu.c
> @@ -39,7 +39,7 @@ static const char* __smu_message_names[] = {
>  const char *smu_get_message_name(struct smu_context *smu, enum 
> smu_message_type type)
>  {
>   if (type < 0 || type > SMU_MSG_MAX_COUNT)
> - return "unknow smu message";
> + return "unknown smu message";
>   return __smu_message_names[type];
>  }
>  
> @@ -52,7 +52,7 @@ static const char* __smu_feature_names[] = {
>  const char *smu_get_feature_name(struct smu_context *smu, enum 
> smu_feature_mask feature)
>  {
>   if (feature < 0 || feature > SMU_FEATURE_COUNT)
> - return "unknow smu feature";
> + return "unknown smu feature";
>   return __smu_feature_names[feature];
>  }
>  
> @@ -79,7 +79,7 @@ size_t smu_sys_get_pp_feature_mask(struct smu_context *smu, 
> char *buf)
>  count++,
>  smu_get_feature_name(smu, i),
>  feature_index,
> -!!smu_feature_is_enabled(smu, i) ? "enabeld" : 
> "disabled");
> +!!smu_feature_is_enabled(smu, i) ? "enabled" : 
> "disabled");

i am wondering,
is that !! really needed in front of smu_feature_is_enabled ?

re,
 wh

>   }
>  
>  failed:
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH][next] drm/amdgpu/psp: fix incorrect logic when checking asic_type

[walter harms]

Am 04.07.2019 16:23, schrieb Colin King:
> From: Colin Ian King 
> 
> Currently the check of the asic_type is always returning true because
> of the use of ||.  Fix this by using && instead.  Also break overly
> wide line.
> 
> Addresses-Coverity: ("Constant expression result")
> Fixes: dab70ff24db6 ("drm/amdgpu/psp: add psp support for navi14")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/amd/amdgpu/psp_v11_0.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c 
> b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> index 527dc371598d..e4afd34e3034 100644
> --- a/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/psp_v11_0.c
> @@ -540,7 +540,8 @@ psp_v11_0_sram_map(struct amdgpu_device *adev,
>  
>   case AMDGPU_UCODE_ID_RLC_G:
>   *sram_offset = 0x2000;
> - if (adev->asic_type != CHIP_NAVI10 || adev->asic_type != 
> CHIP_NAVI14) {
> + if (adev->asic_type != CHIP_NAVI10 &&
> + adev->asic_type != CHIP_NAVI14) {
>   *sram_addr_reg_offset = SOC15_REG_OFFSET(GC, 0, 
> mmRLC_GPM_UCODE_ADDR);
>   *sram_data_reg_offset = SOC15_REG_OFFSET(GC, 0, 
> mmRLC_GPM_UCODE_DATA);
>   } else {
> @@ -551,7 +552,8 @@ psp_v11_0_sram_map(struct amdgpu_device *adev,
>  
>   case AMDGPU_UCODE_ID_SDMA0:
>   *sram_offset = 0x0;
> - if (adev->asic_type != CHIP_NAVI10 || adev->asic_type != 
> CHIP_NAVI14) {
> + if (adev->asic_type != CHIP_NAVI10 &&
> + adev->asic_type != CHIP_NAVI14) {
>   *sram_addr_reg_offset = SOC15_REG_OFFSET(SDMA0, 0, 
> mmSDMA0_UCODE_ADDR);
>   *sram_data_reg_offset = SOC15_REG_OFFSET(SDMA0, 0, 
> mmSDMA0_UCODE_DATA);
>   } else {


maybe it is better to use
if (adev->asic_type == CHIP_NAVI10 ||
adev->asic_type == CHIP_NAVI14) {

i guess tha was intended here and it is more easy to read.
ppl are bad in non-non reading.

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/bridge: sii902x: re-order conditions to prevent out of bounds read

[walter harms]

Am 07.06.2019 09:27, schrieb Dan Carpenter:
> This should check that "i" is within bounds before checking reading from
> the array.
> 
> Fixes: ff5781634c41 ("drm/bridge: sii902x: Implement HDMI audio support")
> Signed-off-by: Dan Carpenter 
> ---
>  drivers/gpu/drm/bridge/sii902x.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/sii902x.c 
> b/drivers/gpu/drm/bridge/sii902x.c
> index d6f98d388ac2..6b03616d6bc3 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -589,8 +589,8 @@ static int sii902x_audio_hw_params(struct device *dev, 
> void *data,
>   if (ret)
>   goto out;
>  
> - for (i = 0; sii902x->audio.i2s_fifo_sequence[i] &&
> -  i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++)
> + for (i = 0; i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence) &&
> + sii902x->audio.i2s_fifo_sequence[i]; i++)
>   regmap_write(sii902x->regmap,
>SII902X_TPI_I2S_ENABLE_MAPPING_REG,
>sii902x->audio.i2s_fifo_sequence[i]);


mmmh, i am a big fan of KISS and in this case i would check 
sii902x->audio.i2s_fifo_sequence[i]
outside for(). like:

 for (i = 0; i < ARRAY_SIZE(sii902x->audio.i2s_fifo_sequence); i++) {
if (!sii902x->audio.i2s_fifo_sequence[i])
 break;

 regmap_write(sii902x->regmap,
 SII902X_TPI_I2S_ENABLE_MAPPING_REG,
 sii902x->audio.i2s_fifo_sequence[i]);
}

just my 2 cents,

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/mgag200: make array m_div_val static, shrinks object size

[Walter Harms]

Am 30.11.2018 00:40, schrieb Colin King:
> From: Colin Ian King 
> 
> Don't populate the const array m_div_val on the stack but instead
> make it static. Makes the object code smaller by 60 bytes:
> 
> Before:
>text  data bss dec hex filename
>   32339  1728   0   340678513 mgag200/mgag200_mode.o
> 
> After:
>text  data bss dec hex filename
>   32215  1792   0   3400784d7 mgag200/mgag200_mode.o
> 
> (gcc version 8.2.0 x86_64)
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/mgag200/mgag200_mode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c
> b/drivers/gpu/drm/mgag200/mgag200_mode.c
> index acf7bfe68454..9939f0174bf7 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_mode.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
> @@ -629,7 +629,7 @@ static int mga_g200er_set_plls(struct mga_device *mdev,
> long clock)
>   unsigned int p, m, n;
>   unsigned int computed, vco;
>   int tmp;
> - const unsigned int m_div_val[] = { 1, 2, 4, 8 };
> + static const unsigned int m_div_val[] = { 1, 2, 4, 8 };

hi,
is that array needed at all ? obvious it is 2^n
i found only one use:
 computed = vco / (m_div_val[testm] * (testo + 1));

just my 2 cents,

re,
 wh


>  
>   m = n = p = 0;
>   vcomax = 1488000;
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/display: indent an if statement

[walter harms]

Am 04.11.2017 07:12, schrieb Dan Carpenter:
> The if statement wasn't indented so it makes static analysis tools and
> probably very recent GCC versions complain.
> 
> Signed-off-by: Dan Carpenter 
> ---
> I went over 80 characters because other lines do already and it seemed
> like the cleanest thing here.
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c 
> b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
> index d911590d08bc..c83ac4d9ca3a 100644
> --- a/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
> +++ b/drivers/gpu/drm/amd/display/dc/dcn10/dcn10_resource.c
> @@ -727,7 +727,7 @@ static void destruct(struct dcn10_resource_pool *pool)
>  
>   for (i = 0; i < pool->base.stream_enc_count; i++) {
>   if (pool->base.stream_enc[i] != NULL)
> - kfree(DCE110STRENC_FROM_STRENC(pool->base.stream_enc[i]));
> + 
> kfree(DCE110STRENC_FROM_STRENC(pool->base.stream_enc[i]));
>   }
>  

Is that "if (pool->base.stream_enc[i] != NULL)" needed at all ?
kfree() should happily handle NULL.

re,
 wh



>   for (i = 0; i < pool->base.audio_count; i++) {
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] video: fbdev: au1200fb: Fix error handling path

[walter harms]

Am 16.10.2017 21:04, schrieb Christophe JAILLET:
> Rewrite the exit path based on 'au1200fb_drv_remove()'.
> We can safely iterate for all already handled planes. Even if not
> completely initialized, the functions that are called will silently accept
> the 'fb_info' structure that is passed.
> 
> As soon as we find a NULL in the '_au1200fb_infos' array, we know that we
> have released all what we needed to release. So we can 'break'.
> 
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/video/fbdev/au1200fb.c | 12 ++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
> index 0d8ed0ef9183..e531543bc707 100644
> --- a/drivers/video/fbdev/au1200fb.c
> +++ b/drivers/video/fbdev/au1200fb.c
> @@ -1760,11 +1760,19 @@ static int au1200fb_drv_probe(struct platform_device 
> *dev)
>   return 0;
>  
>  failed:
> - /* NOTE: This only does the current plane/window that failed; others 
> are still active */
> - if (fbi) {
> + for (plane = 0; plane < device_count; ++plane) {
> + fbi = _au1200fb_infos[plane];
> + if (!fbi)
> + break;
> +
> + /* Clean up all probe data */
> + unregister_framebuffer(fbi);
>   if (fbi->cmap.len != 0)
>   fb_dealloc_cmap(>cmap);

While you are here ... fb_dealloc_cmap() is a collection of free's()
the check for fbi->cmap.len can be omitted (or moved to fb_dealloc_cmap).

re,
 wh

>   kfree(fbi->pseudo_palette);
> +
> + framebuffer_release(fbi);
> + _au1200fb_infos[plane] = NULL;
>   }
>   return ret;
>  }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amdgpu: potential shift wrapping bug

[walter harms]

Am 10.08.2017 15:02, schrieb Christian König:
> Am 10.08.2017 um 14:53 schrieb Dan Carpenter:
>> On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote:
>>> Am 10.08.2017 um 14:16 schrieb Dan Carpenter:
 "frag_align" is a u64, so presumably we want to use the high bits as
 well instead of shift wrapping.

 Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for
 Vega10 v2")
 Signed-off-by: Dan Carpenter 
>>> The fragment field has only 5bits in hardware and can never be more
>>> than 31,
>>> so the correct fix would actually be using uint32_t here instead.
>>>
>> Changing it to uint32_t introduces a new static checker warning:
>>
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1465 amdgpu_vm_frag_ptes()
>>  warn: was expecting a 64 bit value instead of '~(frag_align - 1)'
>>
>> Unfortunately, I get so many thousands of those I can't normally even
>> review that sort of bug...
>>
>> Let me resend the original patch but with a modified changelog to say
>> that the bug is a false positive.
> 
> Ah, yes of course that's why I made it a 64bit value in the first place.
> 
> Mhm, could we use something like (u32)(1 << pages_per_frag) instead to
> silence the static checker warning?
> 
> It doesn't make much sense to use a 64bit shift here.
> 
> Christian.
> 



Why not keeping Dan 1. patch and add a comment that pages_per_frag is always 
>31 ?

Using 32bit in a 64bit is not forbidden, and changing it causes more problems 
than
it solves. But doing so should be done in a clean way.

just my 2 cents,
re,
 wh

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/amd/powerplay: ensure loop does not wraparound on decrement

[walter harms]

Am 17.05.2017 20:13, schrieb Colin King:
> From: Colin Ian King 
> 
> The current for loop decrements i when it is zero and this causes
> a wrap-around back to ~0 because i is unsigned. In the unlikely event
> that mask is 0, the loop will run forever. Fix this so we can't loop
> forever.
> 
> Detected by CoverityScan, CID#1435469 ("Unsigned compared against 0")
> 
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> index ad30f5d3a10d..d92c9b9b15be 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
> @@ -4199,7 +4199,7 @@ static int vega10_force_clock_level(struct pp_hwmgr 
> *hwmgr,
>   }
>   data->smc_state_table.gfx_boot_level = i;
>  
> - for (i = 31; i >= 0; i--) {
> + for (i = 32; --i; ) {
>   if (mask & (1 << i))
>   break;
>   }

nitpicking:
we notices at several points that programmers are bad at counting backwards.
Is there a reason not to start with i=0 ?

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/gma500: fix memory leak on edid

[walter harms]

Am 20.03.2017 18:56, schrieb Colin King:
> From: Colin Ian King 
> 
> edid is allocated on the call to psb_intel_sdvo_get_edid but not
> kfree'd at all, causing a memory leak.  Fix this by kfree'ing
> the edid.  (This may be null, but kfree can handle null frees).
> 
> Detected by CoverityScan, CID#1090730 ("Resource Leak")
> 
> Fixes: 5736995b473b ("gma500: Replace SDVO code with slightly modified 
> version from i915")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/gpu/drm/gma500/psb_intel_sdvo.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpu/drm/gma500/psb_intel_sdvo.c 
> b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> index e787d376ba67..f38e6ad1ab9b 100644
> --- a/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> +++ b/drivers/gpu/drm/gma500/psb_intel_sdvo.c
> @@ -1650,6 +1650,7 @@ static bool psb_intel_sdvo_detect_hdmi_audio(struct 
> drm_connector *connector)
>   edid = psb_intel_sdvo_get_edid(connector);
>   if (edid != NULL && edid->input & DRM_EDID_INPUT_DIGITAL)
is the check here needed at all ?
drm_detect_monitor_audio--> drm_find_cea_extension 
-->drm_find_edid_extension (will check for NULL)

I missed DRM_EDID_INPUT_DIGITAL some where ? NTL i would expected 
drm_detect_monitor_audio to hanle it savely.

re,
 wh

>   has_audio = drm_detect_monitor_audio(edid);
> + kfree(edid);
>  
>   return has_audio;
>  }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2] drm/msm/dsi: Fix the releasing of resources in error path in 'dsi_bus_clk_enable()'

[walter harms]

looks good to me.

Reviewed-by: wha...@bfs.de

Am 26.02.2017 13:10, schrieb Christophe JAILLET:
> If a 'clk_prepare_enable()' fails, then we need to disable_unprepare the
> clk already handled.
> 
> With the current implemenatation, we try to do that on the clk that has
> triggered the error, which is a no-op and leave msm_host->bus_clks[0]
> untouched.
> 
> Count forward in order to fix it and be more future proof.
> 
> Signed-off-by: Christophe JAILLET 
> ---
> v2: change the for loop from a backward to a forward one, to ease reading.
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1fc07ce24686..e6f56cd8ce08 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -422,7 +422,7 @@ static int dsi_clk_init(struct msm_dsi_host *msm_host)
>  static int dsi_bus_clk_enable(struct msm_dsi_host *msm_host)
>  {
>   const struct msm_dsi_config *cfg = msm_host->cfg_hnd->cfg;
> - int i, ret;
> + int i, j, ret;
>  
>   DBG("id=%d", msm_host->id);
>  
> @@ -437,8 +437,8 @@ static int dsi_bus_clk_enable(struct msm_dsi_host 
> *msm_host)
>  
>   return 0;
>  err:
> - for (; i > 0; i--)
> - clk_disable_unprepare(msm_host->bus_clks[i]);
> + for (j = 0; j < i; j++)
> + clk_disable_unprepare(msm_host->bus_clks[j]);
>  
>   return ret;
>  }
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/msm/dsi: Fix the releasing of resources in error path in 'dsi_bus_clk_enable()'

[walter harms]

Am 26.02.2017 08:52, schrieb Christophe JAILLET:
> If a 'clk_prepare_enable()' fails, then we need to disable_unprepare the
> clk already handled.
> 
> With the current implemenatation, we try to do that on the clk that has
> triggered the error, which is a no-op, and leave 'msm_host->bus_clks[0]'
> untouched.
> 
> Shift by one the index array to free resources correctly.
> 
> Signed-off-by: Christophe JAILLET 
> ---
>  drivers/gpu/drm/msm/dsi/dsi_host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
> b/drivers/gpu/drm/msm/dsi/dsi_host.c
> index 1fc07ce24686..eac4987f3946 100644
> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
> @@ -438,7 +438,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host 
> *msm_host)
>   return 0;
>  err:
>   for (; i > 0; i--)
> - clk_disable_unprepare(msm_host->bus_clks[i]);
> + clk_disable_unprepare(msm_host->bus_clks[i-1]);
>  
>   return ret;
>  }

i guess it is technical correct but programmes are
very bad at counting backwards so its more future proof to
do something like:

for (j=0;jbus_clks[j]);

re,
 wh




___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [patch] drm/msm/dsi: free first element on error

[walter harms]

Am 16.02.2017 12:53, schrieb Dan Carpenter:
> On Thu, Feb 16, 2017 at 01:27:47PM +0200, Jani Nikula wrote:
>> On Thu, 16 Feb 2017, Dan Carpenter  wrote:
>>> We want to free msm_host->bus_clks[0] so the > should be >=.
>>>
>>> Fixes: 6e0eb52eba9e ("drm/msm/dsi: Parse bus clocks from a list")
>>> Signed-off-by: Dan Carpenter 
>>>
>>> diff --git a/drivers/gpu/drm/msm/dsi/dsi_host.c 
>>> b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> index 1fc07ce24686..239e79b39a45 100644
>>> --- a/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> +++ b/drivers/gpu/drm/msm/dsi/dsi_host.c
>>> @@ -437,7 +437,7 @@ static int dsi_bus_clk_enable(struct msm_dsi_host 
>>> *msm_host)
>>>  
>>> return 0;
>>>  err:
>>> -   for (; i > 0; i--)
>>> +   for (; i >= 0; i--)
>>> clk_disable_unprepare(msm_host->bus_clks[i]);
>>
>> By the looks of it this is also wrong. I didn't look at the functions,
>> but you probably don't want to unprepare something where prepare failed,
>> i.e. you want to -1 both the start and end offsets. Perhaps the right
>> fix is
>>
>>  while (i--)
>>  clk_disable_unprepare(msm_host->bus_clks[i]);
>>
>> which also seems to be widely used on error paths.
>>
> 

We already know that programmers are bad in counting backwards ...

any chance to make that into a forward loop ?

re,
 wh


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v3 2/5] xf86drm: Add USB support

[walter harms]

Am 19.01.2017 11:20, schrieb Thierry Reding:
> Adding back dri-devel@lists.freedesktop.org
> 
> On Wed, Jan 18, 2017 at 12:21:23PM +0100, walter harms wrote:
>>
>>
>> Am 18.01.2017 10:02, schrieb Thierry Reding:
>>> Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
>>> infrastructure.
>>>
>>> v3:
>>> - guard Linux-specific sysfs parsing code with #ifdef __linux__
>>>
>>> v2:
>>> - make sysfs_uevent_get() more flexible using a format string
>>>
>>> Signed-off-by: Thierry Reding <thierry.red...@gmail.com>
>>> ---
>>>  xf86drm.c | 175 
>>> ++
>>>  xf86drm.h |  13 +
>>>  2 files changed, 188 insertions(+)
>>>
>>> diff --git a/xf86drm.c b/xf86drm.c
>>> index 7766bfe937db..d83674e638c4 100644
>>> --- a/xf86drm.c
>>> +++ b/xf86drm.c
>>> @@ -2886,6 +2886,50 @@ char *drmGetRenderDeviceNameFromFd(int fd)
>>>  return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
>>>  }
>>>  
>>> +#ifdef __linux__
>>> +static char * DRM_PRINTFLIKE(2, 3)
>>> +sysfs_uevent_get(const char *path, const char *fmt, ...)
>>> +{
>>> +char filename[PATH_MAX + 1], *key, *line = NULL, *value = NULL;
>>char *filename=NULL, *key, *line = NULL, *value = NULL;
>>> +size_t size = 0, len;
>>> +ssize_t num;
>>> +va_list ap;
>>> +FILE *fp;
>>> +
>>> +va_start(ap, fmt);
>>> +num = vasprintf(, fmt, ap);
>>> +va_end(ap);
>>> +len = num;
>>> +
>>> +snprintf(filename, sizeof(filename), "%s/uevent", path);
>>
>>  since asprintf() is available you could use:
>>
>> asprintf(,"%s/uevent", path);
>>
>>  same could be done for path below.
> 
> I had thought about that, but a stack-allocated string seemed
> advantageous for three reasons:
> 
>   - asprintf() is a GNU extension. That's not much of an issue
> because this is already protected by #ifdef __linux__ and
> pretty much all C libraries I know support asprintf() and
> friends.
> 
>   - PATH_MAX is the maximum length of a filename, so there's no
> need to dynamically allocate, since it should nicely fit into
> the stack pretty much everywhere (I think the largest value I
> have ever seen for PATH_MAX is 4096 on recent Linux systems).
> 
>   - Most of the other code in xf86drm.c already uses PATH_MAX, so
> the code remains consistent.
> 
> Given the added complexity of asprintf() and the need to free the memory
> the advantages of the stack-allocated string seemed to outweigh.
> 
> Thierry

Granted,

I like to use asprintf every time %s is used to stop malicious users in the 
tracks
(do not aks me how this could be done with our function). Freeing the allocated 
buf
is no practical problem. NTL i have no problem if you are happy with the patch, 
making
an informed decision is ok with me.

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[patch] drm/i915: fix a read size argument

[walter harms]

Am 13.10.2016 10:55, schrieb Dan Carpenter:
> We want to read 3 bytes here, but because the parenthesis are in the
> wrong place we instead read:
> 
>   sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd)
> 
> which is one byte.
> 
> Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only 
> once on eDP")
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 14a3cf0..ee8aa95 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
>   /* Read the eDP Display control capabilities registers */
>   if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & 
> DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
>   drm_dp_dpcd_read(_dp->aux, DP_EDP_DPCD_REV,
> -  intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) ==
> -  sizeof(intel_dp->edp_dpcd)))
> +  intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) ==
> +  sizeof(intel_dp->edp_dpcd))



perhaps we can morph that into something more readable ? I would suggest:


if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE)
  {
size_t size=sizeof(intel_dp->edp_dpcd); /* == EDP_DISPLAY_CTL_CAP_SIZE */
int ret;

ret=drm_dp_dpcd_read(_dp->aux, 
DP_EDP_DPCD_REV,intel_dp->edp_dpcd,size);  

if (ret != size )
..

 }

this way it improves readability and reduces line length.


>   DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) 
> sizeof(intel_dp->edp_dpcd),
> intel_dp->edp_dpcd);
>  
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[PATCH 2/4] GPU-DRM-Etnaviv: Delete unnecessary if statement in __etnaviv_gem_new()

[walter harms]

Am 22.07.2016 17:48, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Fri, 22 Jul 2016 16:45:22 +0200
> 
> Move a return statement into a block for successful function execution.
> Omit a duplicate check for the local variable "ret" then at the end.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/gpu/drm/etnaviv/etnaviv_gem.c | 6 +-
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c 
> b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> index 8eee742..851a8ba 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c
> @@ -661,13 +661,9 @@ static struct drm_gem_object *__etnaviv_gem_new(struct 
> drm_device *dev,
>*/
>   mapping = obj->filp->f_mapping;
>   mapping_set_gfp_mask(mapping, GFP_HIGHUSER);
> + return obj;
>   }
>  
> - if (ret)
> - goto fail;
> -
> - return obj;
> -
>  fail:
>   drm_gem_object_unreference_unlocked(obj);
>   return ERR_PTR(ret);


>From the program flow an readability it would be more nice the branch on error

 ret = drm_gem_object_init(dev, obj, size);
 if (ret)
goto fail;


just m 2 cents

re,
 wh

[PATCH 7/8] drm/amd/powerplay: Change assignment for a buffer variable in phm_dispatch_table()

[walter harms]

Am 16.07.2016 17:08, schrieb SF Markus Elfring:
> From: Markus Elfring 
> Date: Sat, 16 Jul 2016 15:36:36 +0200
> 
> The variable "temp_storage" was eventually reassigned with a pointer.
> Thus omit the explicit initialisation at the beginning.
> 
> Signed-off-by: Markus Elfring 
> ---
>  drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c 
> b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c
> index 024e22e..735aeb0 100644
> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c
> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/functiontables.c
> @@ -60,7 +60,7 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr,
>  void *input, void *output)
>  {
>   int result = 0;
> - void *temp_storage = NULL;
> + void *temp_storage;
>  
>   if (hwmgr == NULL || rt_table == NULL) {
>   printk(KERN_ERR "[ powerplay ] Invalid Parameter!\n");
> @@ -73,7 +73,8 @@ int phm_dispatch_table(struct pp_hwmgr *hwmgr,
>   printk(KERN_ERR "[ powerplay ] Could not allocate table 
> temporary storage\n");
>   return -ENOMEM;
>   }
> - }
> + } else
> + temp_storage = NULL;
>  
>   result = phm_run_table(hwmgr, rt_table, input, output, temp_storage);
>   kfree(temp_storage);


the handling of rt_table->storage_size == 0 is so better visible.

IMHO in this case the function could return directly either with -EINVAL;
or with 0;  -> more direct more obvious.

if (rt_table->storage_size == 0 )
return 0;

re,
 wh

[patch] drm/amdgpu: missing bounds check in amdgpu_set_pp_force_state()

[walter harms]

Am 16.06.2016 08:41, schrieb Dan Carpenter:
> There is no limit on high "idx" can go.  It should be less than
> ARRAY_SIZE(data.states) which is 16.
> 
> The "data" variable wasn't declared in that scope so I shifted the code
> around a bit to make it work.
> 
> Fixes: f3898ea12fc1 ('drm/amd/powerplay: add some sysfs interfaces for 
> powerplay.')
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> index 589b36e..ce9e97f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c
> @@ -275,25 +275,23 @@ static ssize_t amdgpu_set_pp_force_state(struct device 
> *dev,
>  
>   if (strlen(buf) == 1)
>   adev->pp_force_state_enabled = false;
> - else {
> - ret = kstrtol(buf, 0, );
> + else if (adev->pp_enabled) {
> + struct pp_states_info data;
>  
> - if (ret) {
> + ret = kstrtol(buf, 0, );
> + if (ret || idx >= ARRAY_SIZE(data.states)) {
>   count = -EINVAL;
>   goto fail;
>   }


i would also expect a check idx < 0, does it mean this can not happen ?
otherwise maybe kstrtoul is a solution ?

re,
 wh

>  
> - if (adev->pp_enabled) {
> - struct pp_states_info data;
> - amdgpu_dpm_get_pp_num_states(adev, );
> - state = data.states[idx];
> - /* only set user selected power states */
> - if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
> - state != POWER_STATE_TYPE_DEFAULT) {
> - amdgpu_dpm_dispatch_task(adev,
> - AMD_PP_EVENT_ENABLE_USER_STATE, 
> , NULL);
> - adev->pp_force_state_enabled = true;
> - }
> + amdgpu_dpm_get_pp_num_states(adev, );
> + state = data.states[idx];
> + /* only set user selected power states */
> + if (state != POWER_STATE_TYPE_INTERNAL_BOOT &&
> + state != POWER_STATE_TYPE_DEFAULT) {
> + amdgpu_dpm_dispatch_task(adev,
> + AMD_PP_EVENT_ENABLE_USER_STATE, , 
> NULL);
> + adev->pp_force_state_enabled = true;
>   }
>   }
>  fail:
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[patch] drm/amd: cleanup get_mfd_cell_dev()

[walter harms]

Am 27.02.2016 11:40, schrieb Dan Carpenter:
> On Sat, Feb 27, 2016 at 10:50:40AM +0100, walter harms wrote:
>>
>>
>> Am 25.02.2016 08:47, schrieb Dan Carpenter:
>>> It's simpler to just use snprintf() to print this to one buffer instead
>>> of using strcpy() and strcat().  Also using snprintf() is slightly safer
>>> than using sprintf().
>>>
>>> Signed-off-by: Dan Carpenter 
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> index 9f8cfaa..d6b0bff 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
>>> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain 
>>> *genpd)
>>>  static struct device *get_mfd_cell_dev(const char *device_name, int r)
>>>  {
>>> char auto_dev_name[25];
>>> -   char buf[8];
>>> struct device *dev;
>>>  
>>> -   sprintf(buf, ".%d.auto", r);
>>> -   strcpy(auto_dev_name, device_name);
>>> -   strcat(auto_dev_name, buf);
>>> +   snprintf(auto_dev_name, sizeof(auto_dev_name),
>>> +"%s.%d.auto", device_name, r);
>>> dev = bus_find_device_by_name(_bus_type, NULL, auto_dev_name);
>>> dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
>>>  
>>
>> hi,
>> i tried to understand what is the base for char auto_dev_name[25]. It is not 
>> clear
>> from these snipped if that is large or small.
>>
>> (To be aware i assume that
>> get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will 
>> never happen
>> but i could find no reason)
>>
>> A small comment that explains the magic 25 would be nice.
> 
> I have no idea, either of course.  For example,
> mc13xxx_add_subdevice_pdata() assumes device_name by itself can be 30
> characters.  Hence the change to snprintf.
> 

Hi Dan,
i also think that this limit is artificial, one of those "it works" things.
The problem is that i have seen changes in the naming als ready done, like the
shift from /dev/sda to things like 
/dev/disk/by-id/scsi-SATA_WDC_WD5000AAKS-_WD-WCASY5869245-part1
and you are out of the game here. snprintf will cut the tail and the %d.auto 
stuff is dead in the water.
To make it clear, I do not thing that is security related issue but it could 
result in annoying
failures. (the solution is obviously to use asprintf() and free the mem later 
:) ).

re,
 wh

[patch] drm/amd: cleanup get_mfd_cell_dev()

[walter harms]

Am 25.02.2016 08:47, schrieb Dan Carpenter:
> It's simpler to just use snprintf() to print this to one buffer instead
> of using strcpy() and strcat().  Also using snprintf() is slightly safer
> than using sprintf().
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> index 9f8cfaa..d6b0bff 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> @@ -240,12 +240,10 @@ static int acp_poweron(struct generic_pm_domain *genpd)
>  static struct device *get_mfd_cell_dev(const char *device_name, int r)
>  {
>   char auto_dev_name[25];
> - char buf[8];
>   struct device *dev;
>  
> - sprintf(buf, ".%d.auto", r);
> - strcpy(auto_dev_name, device_name);
> - strcat(auto_dev_name, buf);
> + snprintf(auto_dev_name, sizeof(auto_dev_name),
> +  "%s.%d.auto", device_name, r);
>   dev = bus_find_device_by_name(_bus_type, NULL, auto_dev_name);
>   dev_info(dev, "device %s added to pm domain\n", auto_dev_name);
>  

hi,
i tried to understand what is the base for char auto_dev_name[25]. It is not 
clear
from these snipped if that is large or small.

(To be aware i assume that
get_mfd_cell_dev("terrible_long_and_Stupid_name",1234567899346712) will never 
happen
but i could find no reason)

A small comment that explains the magic 25 would be nice.

re,
 wh

[patch] drm/amdgpu: potential NULL dereference on error

[walter harms]

Am 11.06.2015 14:20, schrieb Dan Carpenter:
> On Thu, Jun 11, 2015 at 02:03:18PM +0200, walter harms wrote:
>>
>>
>> Am 11.06.2015 10:49, schrieb Dan Carpenter:
>>> debugfs_create_file() can return an error pointer if debugfs is disabled
>>> or it can return NULL on error.
>>>
>>> Signed-off-by: Dan Carpenter 
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 36be03c..adba2a1 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -1980,6 +1980,8 @@ static int amdgpu_debugfs_regs_init(struct 
>>> amdgpu_device *adev)
>>>   adev, _debugfs_regs_fops);
>>> if (IS_ERR(ent))
>>> return PTR_ERR(ent);
>>> +   if (!ent)
>>> +   return -ENOMEM;
>>> i_size_write(ent->d_inode, adev->rmmio_size);
>>> adev->debugfs_regs = ent;
>>
>>
>>
>> would  PTR_ERR_OR_ZERO() by an option ?
>>
>> on the other hand,
>> why does debugfs_create_file() does not return -ENOMEN instead of NULL ?
>>
> 
> Actually if debugfs is disabled then we should probably carry on.  Let
> me change it to:
> 
>   if (IS_ERR(ent))
>   return 0;
> 
>   if (!ent)
>   return -ENOMEM;
> 

You still have to check 2 types of error return here.
I simply do not understand why ebugfs_create_file() does not return -ENOMEM
(or returns NULL on any error).

re,
 wh

[patch] drm/amdgpu: potential NULL dereference on error

[walter harms]

Am 11.06.2015 10:49, schrieb Dan Carpenter:
> debugfs_create_file() can return an error pointer if debugfs is disabled
> or it can return NULL on error.
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 36be03c..adba2a1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1980,6 +1980,8 @@ static int amdgpu_debugfs_regs_init(struct 
> amdgpu_device *adev)
> adev, _debugfs_regs_fops);
>   if (IS_ERR(ent))
>   return PTR_ERR(ent);
> + if (!ent)
> + return -ENOMEM;
>   i_size_write(ent->d_inode, adev->rmmio_size);
>   adev->debugfs_regs = ent;



would  PTR_ERR_OR_ZERO() by an option ?

on the other hand,
why does debugfs_create_file() does not return -ENOMEN instead of NULL ?


re,
 wh


> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[patch] drm/exynos: potential use after free in exynos_drm_open()

[walter harms]

i have just noticed: The function already exits

194 static void exynos_drm_postclose(struct drm_device *dev, struct drm_file 
*file)
195 {
196 if (!file->driver_priv)
197 return;
198
199 kfree(file->driver_priv);
200 file->driver_priv = NULL;
201 }


Am 21.01.2014 13:37, schrieb walter harms:
> 
> 
> Am 21.01.2014 07:57, schrieb Dan Carpenter:
>> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
>>
>> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
>> Signed-off-by: Dan Carpenter 
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
>> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> index 9d096a0c5f8d..3c845292845a 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
>> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, 
>> struct drm_file *file)
>>  if (ret) {
>>  kfree(file_priv);
>>  file->driver_priv = NULL;
>> +return ret;
>>  }
> 
> using
>   kfree(  file->driver_priv );
>   file->driver_priv = NULL;
> 
> would be less confusing to read, and give checkers a better chance to spot 
> mistakes.
> (btw: file_priv could be removed from this function completely).
> 
> just my 2 cents,
> re,
>  wh
> 
>>  
>>  anon_filp = anon_inode_getfile("exynos_gem", _drm_gem_fops,
>> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, 
>> struct drm_file *file)
>>  anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>>  file_priv->anon_filp = anon_filp;
>>  
>> -return ret;
>> +return 0;
>>  }
>>  
>>  static void exynos_drm_preclose(struct drm_device *dev,
>> --
>> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
>> the body of a message to majordomo at vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[patch] drm/exynos: potential use after free in exynos_drm_open()

[walter harms]

Am 21.01.2014 07:57, schrieb Dan Carpenter:
> If exynos_drm_subdrv_open() fails then we re-use "file_priv".
> 
> Fixes: 96f5421523df ('drm/exynos: use a new anon file for exynos gem mmaper')
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c 
> b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> index 9d096a0c5f8d..3c845292845a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c
> @@ -174,6 +174,7 @@ static int exynos_drm_open(struct drm_device *dev, struct 
> drm_file *file)
>   if (ret) {
>   kfree(file_priv);
>   file->driver_priv = NULL;
> + return ret;
>   }

using
kfree(  file->driver_priv );
file->driver_priv = NULL;

would be less confusing to read, and give checkers a better chance to spot 
mistakes.
(btw: file_priv could be removed from this function completely).

just my 2 cents,
re,
 wh

>  
>   anon_filp = anon_inode_getfile("exynos_gem", _drm_gem_fops,
> @@ -186,7 +187,7 @@ static int exynos_drm_open(struct drm_device *dev, struct 
> drm_file *file)
>   anon_filp->f_mode = FMODE_READ | FMODE_WRITE;
>   file_priv->anon_filp = anon_filp;
>  
> - return ret;
> + return 0;
>  }
>  
>  static void exynos_drm_preclose(struct drm_device *dev,
> --
> To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
> the body of a message to majordomo at vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

[patch] drm/nouveau/disp: add a comment on confusing loop

[walter harms]

Am 13.11.2013 08:45, schrieb Dan Carpenter:
> The "ret = -EIO" is deliberate.  It's a very uncommon thing to do and it
> upsets static checkers because they normally would expect "ret == -EIO".
> 
> Signed-off-by: Dan Carpenter 
> 
> diff --git a/drivers/gpu/drm/nouveau/core/engine/disp/dport.c 
> b/drivers/gpu/drm/nouveau/core/engine/disp/dport.c
> index 1bd4c63..2bc45ae 100644
> --- a/drivers/gpu/drm/nouveau/core/engine/disp/dport.c
> +++ b/drivers/gpu/drm/nouveau/core/engine/disp/dport.c
> @@ -322,6 +322,7 @@ nouveau_dp_train(struct nouveau_disp *disp, const struct 
> nouveau_dp_func *func,
>   while (*link_bw > (dp->dpcd[1] * 27000))
>   link_bw++;
>  
> + /* set ret to -EIO on the last loop iteration */
>   while ((ret = -EIO) && link_bw[0]) {
>   /* find minimum required lane count at this link rate */
>   dp->link_nr = dp->dpcd[2] & DPCD_RC02_MAX_LANE_COUNT;


It is sensible to do so now,
but in the long runs it pays to rewrite that as it confuses not only
static checkers but also the brains of people trying to understand
the code.

just my 2 cents,
re,
 wh

Re: [patch] drm: checking the wrong variable in savage_do_init_bci()

[walter harms]

Am 17.05.2012 09:09, schrieb Dan Carpenter:
 drm_core_ioremap() initializes -handle.  We already know
 dev-agp_buffer_map is a valid pointer.
 
 Signed-off-by: Dan Carpenter dan.carpen...@oracle.com
 
 diff --git a/drivers/gpu/drm/savage/savage_bci.c 
 b/drivers/gpu/drm/savage/savage_bci.c
 index cb1ee4e..6eb507a 100644
 --- a/drivers/gpu/drm/savage/savage_bci.c
 +++ b/drivers/gpu/drm/savage/savage_bci.c
 @@ -735,7 +735,7 @@ static int savage_do_init_bci(struct drm_device * dev, 
 drm_savage_init_t * init)
   return -EINVAL;
   }
   drm_core_ioremap(dev-agp_buffer_map, dev);
 - if (!dev-agp_buffer_map) {
 + if (!dev-agp_buffer_map-handle) {
   DRM_ERROR(failed to ioremap DMA buffer region!\n);
   savage_do_cleanup_bci(dev);
   return -ENOMEM;


ok, the interface asks for that type of error :)

re,
 wh
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel