Re: [PATCH] drm/amd/display: Add null check before access structs

2024-06-27 Thread Markus Elfring 
> In enable_phantom_plane, we should better check null pointer before
> accessing various structs.

1. Can a wording approach (like the following) be a better change description?
   
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n45

   A null pointer is stored in the local variable “phantom_plane” after a call
   of the function “create_phantom_plane” (as a data structure menber) failed.
   This pointer was used in subsequent statements where an undesirable
   dereference will be performed then.
   Thus add a corresponding return value check.


2. How do you think about to use a summary phrase like
   “Prevent null pointer dereference in enable_phantom_plane()”?

Regards,
Markus

Re: [PATCH] drm/amd/display: Add null check before access structs

2024-06-27 Thread Markus Elfring 
> > In enable_phantom_plane, we should better check null pointer before
> > accessing various structs.
>
> Thanks for the fix, I'll apply this.

Do you care for better commit messages and summary phrases?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.10-rc5#n45

Regards,
Markus

Re: [PATCH] drm/radeon: fix null pointer dereference in radeon_add_common_modes

2024-06-26 Thread Markus Elfring 
> In radeon_add_common_modes(), the return value of drm_cvt_mode() is
> assigned to mode, which will lead to a possible NULL pointer dereference
> on failure of drm_cvt_mode(). Add a check to avoid npd.

1. Can a wording approach (like the following) be a better change description?

   A null pointer is stored in the local variable “mode” after a call
   of the function “drm_cvt_mode” failed. This pointer was passed to
   a subsequent call of the function “drm_mode_probed_add” where an undesirable
   dereference will be performed then.
   Thus add a corresponding return value check.


2. Would you like to add any tags (like “Fixes” and “Cc”) accordingly?


3. How do you think about to append parentheses to the function name
   in the summary phrase?


Regards,
Markus

Re: [PATCH] drm/amd/display: Check pipe_ctx before it is used

2024-06-25 Thread Markus Elfring 
>> resource_get_otg_master_for_stream() could return NULL, we
>> should check the return value of 'otg_master' before it is
>> used in resource_log_pipe_for_stream().
>
> A similar fix was integrated already according to a contribution
> by Natanel Roizenman.
> From which Linux version did you take source files for your static code 
> analyses?
>
> Please take another look at the corresponding software update.
> [PATCH 16/37] drm/amd/display: Add null check in 
> resource_log_pipe_topology_update
> https://lore.kernel.org/amd-gfx/20240422152817.2765349-17-aurabindo.pil...@amd.com/

How “interesting” is it that a similar source code correction needed
to be repeated by Hersen Wu?

drm/amd/display: Add otg_master NULL check within 
resource_log_pipe_topology_update
https://lore.kernel.org/amd-gfx/20240501071651.3541919-31-chiahsuan.ch...@amd.com/

Regards,
Markus

Re: [PATCH] drm/amd/display: Check pipe_ctx before it is used

2024-06-25 Thread Markus Elfring 
> resource_get_otg_master_for_stream() could return NULL, we
> should check the return value of 'otg_master' before it is
> used in resource_log_pipe_for_stream().

A similar fix was integrated already according to a contribution
by Natanel Roizenman.
>From which Linux version did you take source files for your static code 
>analyses?

Please take another look at the corresponding software update.
[PATCH 16/37] drm/amd/display: Add null check in 
resource_log_pipe_topology_update
https://lore.kernel.org/amd-gfx/20240422152817.2765349-17-aurabindo.pil...@amd.com/

Regards,
Markus

Re: [PATCH] drm/amdgpu: fix a possible null pointer dereference

2024-06-22 Thread Markus Elfring 
> In amdgpu_connector_add_common_modes(), the return value of drm_cvt_mode()
> is assigned to mode, which will lead to a NULL pointer dereference on
> failure of drm_cvt_mode(). Add a check to avoid npd.

Can a wording approach (like the following) be a better change description?

   A null pointer is stored in the local variable “mode” after a call
   of the function “drm_cvt_mode” failed. This pointer was passed to
   a subsequent call of the function “drm_mode_probed_add” where an undesirable
   dereference will be performed then.
   Thus add a corresponding return value check.


Would you like to add any tags (like “Fixes”) accordingly?


How do you think about to use a summary phrase like “Avoid null pointer 
dereference
in amdgpu_connector_add_common_modes()”?

Regards,
Markus

Re: [PATCH] drm/amd/pm: Fix error code in vega10_hwmgr_backend_init()

2024-05-06 Thread Markus Elfring 
> Return -EINVAL on error instead of success.  Also on the success path,
> return a literal zero instead of "return result;"

How do you think about to omit the initialisation for the variable “result”
in another update step?

Regards,
Markus

Re: [PATCH] drm/amd/display: Fix division by zero in setup_dsc_config

2024-04-23 Thread Markus Elfring 
…
> +++ b/drivers/gpu/drm/amd/display/dc/dsc/dc_dsc.c
> @@ -1055,7 +1055,12 @@ static bool setup_dsc_config(
>   if (!is_dsc_possible)
>   goto done;
>
> - dsc_cfg->num_slices_v = pic_height/slice_height;
> + if (slice_height > 0)
> + dsc_cfg->num_slices_v = pic_height/slice_height;
> + else {
> + is_dsc_possible = false;
> + goto done;
> + }
>
>   if (target_bandwidth_kbps > 0) {
>   is_dsc_possible = decide_dsc_target_bpp_x16(

I suggest to take another coding style concern into account.
Please use curly brackets for both if branches.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v6.9-rc5#n213

Regards,
Markus

Re: [PATCH 0/5] drm/amd: Adjustments for three function implementations

2024-01-05 Thread Markus Elfring 
> Date: Tue, 11 Apr 2023 14:36:36 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5)
>   amdgpu: Move a variable assignment behind a null pointer check in 
> amdgpu_ras_interrupt_dispatch()
>   display: Move three variable assignments behind condition checks in 
> trigger_hotplug()
>   display: Delete three unnecessary variable initialisations in 
> trigger_hotplug()
>   display: Delete a redundant statement in trigger_hotplug()
>   display: Move an expression into a return statement in 
> dcn201_link_encoder_create()
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c   |  3 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 19 ++-
>  .../amd/display/dc/dcn201/dcn201_resource.c   |  4 +---
>  3 files changed, 13 insertions(+), 13 deletions(-)

Is this patch series still in review queues?

See also:
https://lore.kernel.org/cocci/2258ce64-2a14-6778-8319-b342b06a1...@web.de/
https://sympa.inria.fr/sympa/arc/cocci/2023-04/msg00034.html

Regards,
Markus

Re: [PATCH] gpu: drm/amd: Fix the bug in list_for_each_entry() loops

2023-06-12 Thread Markus Elfring 
> pqn bound in list_for_each_entry loop will not be null, so there is
> no need to check whether pqn is NULL or not.

Would it be more appropriate to use the wording “Delete an unnecessary check
within a list_for_each_entry() loop” instead of
“Fix the bug in list_for_each_entry() loops” in the patch subject?


> We could remove this check.

I suggest to use the sentence “Thus remove a redundant null pointer check.”.

See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4-rc6#n94

Regards,
Markus

Re: [PATCH 2/5] drm/amd/display: Move three variable assignments behind condition checks in trigger_hotplug()

2023-05-16 Thread Markus Elfring 
>> The address of a data structure member was determined before
>> a corresponding null pointer check in the implementation of
>> the function “trigger_hotplug”.
>>
>> Thus avoid the risk for undefined behaviour by moving the assignment
>> for three local variables behind some condition checks.
>
> It might be that the NULL check doesn't make sense in the first place, but 
> since I'm not an expert for this code I can't fully judge.

Will the source code and patch review evolve any more?


> On the other hand the patches clearly look like nice cleanups to me, so feel 
> free to add an Acked-by: Christian König  to the 
> series.

Will such a positive feedback trigger any further collateral evolution?

Regards,
Markus

[PATCH 5/5] drm/amd/display: Move an expression into a return statement in dcn201_link_encoder_create()

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 14:04:57 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “dcn201_link_encoder_create”.

Thus avoid the risk for undefined behaviour by moving the usage
of an expression into a return statement.

This issue was detected by using the Coccinelle software.

Fixes: 3f68c01be9a2227de1e190317fe34a6fb835a094 ("drm/amd/display: add 
cyan_skillfish display support")
Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/dc/dcn201/dcn201_resource.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dcn201/dcn201_resource.c 
b/drivers/gpu/drm/amd/display/dc/dcn201/dcn201_resource.c
index 6ea70da28aaa..a1b44c7bd34b 100644
--- a/drivers/gpu/drm/amd/display/dc/dcn201/dcn201_resource.c
+++ b/drivers/gpu/drm/amd/display/dc/dcn201/dcn201_resource.c
@@ -791,7 +791,6 @@ static struct link_encoder *dcn201_link_encoder_create(
 {
struct dcn20_link_encoder *enc20 =
kzalloc(sizeof(struct dcn20_link_encoder), GFP_ATOMIC);
-   struct dcn10_link_encoder *enc10 = >enc10;

if (!enc20)
return NULL;
@@ -804,8 +803,7 @@ static struct link_encoder *dcn201_link_encoder_create(
_enc_hpd_regs[enc_init_data->hpd_source],
_shift,
_mask);
-
-   return >base;
+   return >enc10.base;
 }

 static struct clock_source *dcn201_clock_source_create(
--
2.40.0

Re: [PATCH 1/5] drm/amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()

2023-04-11 Thread Markus Elfring 
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
>> @@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct 
>> amdgpu_device *adev,
>>   struct ras_dispatch_if *info)
>>   {
>>   struct ras_manager *obj = amdgpu_ras_find_obj(adev, >head);
>> -    struct ras_ih_data *data = >ih_data;
>> +    struct ras_ih_data *data;
> I'm curious, this only takes the address of obj->ih_data.

Even if a null pointer would accidentally be returned by a call of
the function “amdgpu_ras_find_obj”?
https://elixir.bootlin.com/linux/v6.3-rc6/source/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c#L618


> It doesn't dereference the pointer until after the !obj check below.

Does the used arrow operator indicate a pointer dereference?


> How is this undefined behaviour?

I guess that another information source can be helpful for such an issue.
https://wiki.sei.cmu.edu/confluence/display/c/EXP34-C.+Do+not+dereference+null+pointers?focusedCommentId=405504153#comment-405504153

Regards,
Markus

[PATCH 0/5] drm/amd: Adjustments for three function implementations

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 14:36:36 +0200

Some update suggestions were taken into account
from static source code analysis.

Markus Elfring (5)
  amdgpu: Move a variable assignment behind a null pointer check in 
amdgpu_ras_interrupt_dispatch()
  display: Move three variable assignments behind condition checks in 
trigger_hotplug()
  display: Delete three unnecessary variable initialisations in 
trigger_hotplug()
  display: Delete a redundant statement in trigger_hotplug()
  display: Move an expression into a return statement in 
dcn201_link_encoder_create()

 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c   |  3 ++-
 .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 19 ++-
 .../amd/display/dc/dcn201/dcn201_resource.c   |  4 +---
 3 files changed, 13 insertions(+), 13 deletions(-)

--
2.40.0

[PATCH 1/5] drm/amdgpu: Move a variable assignment behind a null pointer check in amdgpu_ras_interrupt_dispatch()

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 10:52:48 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “amdgpu_ras_interrupt_dispatch”.

Thus avoid the risk for undefined behaviour by moving the assignment
for the variable “data” behind the null pointer check.

This issue was detected by using the Coccinelle software.

Fixes: c030f2e4166c3f5597c7e7a70bcd9ab383695de4 ("drm/amdgpu: add amdgpu_ras.c 
to support ras (v2)")
Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
index 4069bce9479f..a920c7888d07 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c
@@ -1730,11 +1730,12 @@ int amdgpu_ras_interrupt_dispatch(struct amdgpu_device 
*adev,
struct ras_dispatch_if *info)
 {
struct ras_manager *obj = amdgpu_ras_find_obj(adev, >head);
-   struct ras_ih_data *data = >ih_data;
+   struct ras_ih_data *data;

if (!obj)
return -EINVAL;

+   data = >ih_data;
if (data->inuse == 0)
return 0;

--
2.40.0

[PATCH 4/5] drm/amd/display: Delete a redundant statement in trigger_hotplug()

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 13:26:35 +0200

An immediate return is performed by this function after a null pointer
was detected for the member “dc_link” in the data
structure “amdgpu_dm_connector”.
This check was repeated within one if branch.

Thus omit such a redundant statement.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index a37d23a13d7b..4805a482dc49 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -1278,9 +1278,6 @@ static ssize_t trigger_hotplug(struct file *f, const char 
__user *buf,

drm_kms_helper_connector_hotplug_event(connector);
} else if (param[0] == 0) {
-   if (!aconnector->dc_link)
-   goto unlock;
-
link = aconnector->dc_link;

if (link->local_sink) {
--
2.40.0



Am 11.04.23 um 15:36 schrieb Markus Elfring:
> Date: Tue, 11 Apr 2023 14:36:36 +0200
>
> Some update suggestions were taken into account
> from static source code analysis.
>
> Markus Elfring (5)
>   amdgpu: Move a variable assignment behind a null pointer check in 
> amdgpu_ras_interrupt_dispatch()
>   display: Move three variable assignments behind condition checks in 
> trigger_hotplug()
>   display: Delete three unnecessary variable initialisations in 
> trigger_hotplug()
>   display: Delete a redundant statement in trigger_hotplug()
>   display: Move an expression into a return statement in 
> dcn201_link_encoder_create()
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu_ras.c   |  3 ++-
>  .../amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 19 ++-
>  .../amd/display/dc/dcn201/dcn201_resource.c   |  4 +---
>  3 files changed, 13 insertions(+), 13 deletions(-)
>

[PATCH 2/5] drm/amd/display: Move three variable assignments behind condition checks in trigger_hotplug()

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 11:39:02 +0200

The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “trigger_hotplug”.

Thus avoid the risk for undefined behaviour by moving the assignment
for three local variables behind some condition checks.

This issue was detected by using the Coccinelle software.

Fixes: 6f77b2ac628073f647041a92b36c824ae3aef16e ("drm/amd/display: Add 
connector HPD trigger debugfs entry")
Signed-off-by: Markus Elfring 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c  | 10 +++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index 827fcb4fb3b3..b3cfd7dfbb28 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -1205,10 +1205,10 @@ static ssize_t trigger_hotplug(struct file *f, const 
char __user *buf,
size_t size, loff_t 
*pos)
 {
struct amdgpu_dm_connector *aconnector = file_inode(f)->i_private;
-   struct drm_connector *connector = >base;
+   struct drm_connector *connector;
struct dc_link *link = NULL;
-   struct drm_device *dev = connector->dev;
-   struct amdgpu_device *adev = drm_to_adev(dev);
+   struct drm_device *dev;
+   struct amdgpu_device *adev;
enum dc_connection_type new_connection_type = dc_connection_none;
char *wr_buf = NULL;
uint32_t wr_buf_size = 42;
@@ -1253,12 +1253,16 @@ static ssize_t trigger_hotplug(struct file *f, const 
char __user *buf,
return -EINVAL;
}

+   connector = >base;
+   dev = connector->dev;
+
if (param[0] == 1) {

if (!dc_link_detect_connection_type(aconnector->dc_link, 
_connection_type) &&
new_connection_type != dc_connection_none)
goto unlock;

+   adev = drm_to_adev(dev);
mutex_lock(>dm.dc_lock);
ret = dc_link_detect(aconnector->dc_link, DETECT_REASON_HPD);
mutex_unlock(>dm.dc_lock);
--
2.40.0

[PATCH 3/5] drm/amd/display: Delete three unnecessary variable initialisations in trigger_hotplug()

2023-04-11 Thread Markus Elfring 
Date: Tue, 11 Apr 2023 12:34:42 +0200

The variables “link”, “wr_buf” and “ret” will eventually be set
to appropriate values a bit later.
Thus omit the explicit initialisation at the beginning.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
index b3cfd7dfbb28..a37d23a13d7b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_debugfs.c
@@ -1206,16 +1206,16 @@ static ssize_t trigger_hotplug(struct file *f, const 
char __user *buf,
 {
struct amdgpu_dm_connector *aconnector = file_inode(f)->i_private;
struct drm_connector *connector;
-   struct dc_link *link = NULL;
+   struct dc_link *link;
struct drm_device *dev;
struct amdgpu_device *adev;
enum dc_connection_type new_connection_type = dc_connection_none;
-   char *wr_buf = NULL;
+   char *wr_buf;
uint32_t wr_buf_size = 42;
int max_param_num = 1;
long param[1] = {0};
uint8_t param_nums = 0;
-   bool ret = false;
+   bool ret;

if (!aconnector || !aconnector->dc_link)
return -EINVAL;
--
2.40.0

Re: [PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

2023-03-24 Thread Markus Elfring 
>> The label “cleanup” was used to jump to another pointer check despite of
>> the detail in the implementation of the function 
>> “dm_validate_stream_and_context”
>> that it was determined already that corresponding variables contained
>> still null pointers.
>>
>> 1. Thus return directly if
>>    * a null pointer was passed for the function parameter “stream”
>>  or
>>    * a call of the function “dc_create_plane_state” failed.
>>
>> 2. Use a more appropriate label instead.
>>
>> 3. Delete two questionable checks.
>>
>> 4. Omit extra initialisations (for the variables “dc_state” and 
>> “dc_plane_state”)
>>    which became unnecessary with this refactoring.
>>
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter 
>> Invalid 420 Modes for HDMI TMDS")
>
> Please truncate the hash to 12 characters.

May longer identifiers (or even the complete SHA-1 ID) occasionally also
be tolerated for the tag “Fixes”?

How do you think about the proposed change possibilities?

Regards,
Markus

[PATCH resent] drm/amd/display: Fix exception handling in dm_validate_stream_and_context()

2023-03-24 Thread Markus Elfring 
Date: Sat, 18 Mar 2023 16:21:32 +0100

The label “cleanup” was used to jump to another pointer check despite of
the detail in the implementation of the function 
“dm_validate_stream_and_context”
that it was determined already that corresponding variables contained
still null pointers.

1. Thus return directly if
   * a null pointer was passed for the function parameter “stream”
 or
   * a call of the function “dc_create_plane_state” failed.

2. Use a more appropriate label instead.

3. Delete two questionable checks.

4. Omit extra initialisations (for the variables “dc_state” and 
“dc_plane_state”)
   which became unnecessary with this refactoring.


This issue was detected by using the Coccinelle software.

Fixes: 5468c36d628524effbb89a9503eb1a2318804759 ("drm/amd/display: Filter 
Invalid 420 Modes for HDMI TMDS")
Signed-off-by: Markus Elfring 
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ---
 1 file changed, 8 insertions(+), 12 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 eeaeca8b51f4..3086613f5f5d 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6426,19 +6426,19 @@ static enum dc_status 
dm_validate_stream_and_context(struct dc *dc,
struct dc_stream_state *stream)
 {
enum dc_status dc_result = DC_ERROR_UNEXPECTED;
-   struct dc_plane_state *dc_plane_state = NULL;
-   struct dc_state *dc_state = NULL;
+   struct dc_plane_state *dc_plane_state;
+   struct dc_state *dc_state;

if (!stream)
-   goto cleanup;
+   return dc_result;

dc_plane_state = dc_create_plane_state(dc);
if (!dc_plane_state)
-   goto cleanup;
+   return dc_result;

dc_state = dc_create_state(dc);
if (!dc_state)
-   goto cleanup;
+   goto release_plane_state;

/* populate stream to plane */
dc_plane_state->src_rect.height  = stream->src.height;
@@ -6475,13 +6475,9 @@ static enum dc_status 
dm_validate_stream_and_context(struct dc *dc,
if (dc_result == DC_OK)
dc_result = dc_validate_global_state(dc, dc_state, true);

-cleanup:
-   if (dc_state)
-   dc_release_state(dc_state);
-
-   if (dc_plane_state)
-   dc_plane_state_release(dc_plane_state);
-
+   dc_release_state(dc_state);
+release_plane_state:
+   dc_plane_state_release(dc_plane_state);
return dc_result;
 }

--
2.40.0

[PATCH 1/2] drm/amd/display: Return directly after a failed kzalloc() in dc_create()

2020-12-20 Thread Markus Elfring 
From: Markus Elfring 
Date: Sat, 19 Dec 2020 18:04:33 +0100

* Return directly after a call of the function “kzalloc” failed
  at the beginning.

* Delete a label which became unnecessary with this refactoring.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index 7339d9855ec8..e35fbfcb4d0e 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -964,8 +964,8 @@ struct dc *dc_create(const struct dc_init_data *init_params)
struct dc *dc = kzalloc(sizeof(*dc), GFP_KERNEL);
unsigned int full_pipe_count;

-   if (NULL == dc)
-   goto alloc_fail;
+   if (!dc)
+   return NULL;

if (init_params->dce_environment == DCE_ENV_VIRTUAL_HW) {
if (false == dc_construct_ctx(dc, init_params)) {
@@ -1009,8 +1009,6 @@ struct dc *dc_create(const struct dc_init_data 
*init_params)

 construct_fail:
kfree(dc);
-
-alloc_fail:
return NULL;
 }

--
2.29.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amd/display: Use common error handling code in dc_create()

2020-12-20 Thread Markus Elfring 
From: Markus Elfring 
Date: Sat, 19 Dec 2020 18:18:59 +0100

Adjust a jump target so that a bit of exception handling can be better
reused at the end of this function.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/dc/core/dc.c | 15 ++-
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc.c 
b/drivers/gpu/drm/amd/display/dc/core/dc.c
index e35fbfcb4d0e..64344c054c93 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc.c
@@ -968,15 +968,11 @@ struct dc *dc_create(const struct dc_init_data 
*init_params)
return NULL;

if (init_params->dce_environment == DCE_ENV_VIRTUAL_HW) {
-   if (false == dc_construct_ctx(dc, init_params)) {
-   dc_destruct(dc);
-   goto construct_fail;
-   }
+   if (!dc_construct_ctx(dc, init_params))
+   goto destruct_dc;
} else {
-   if (false == dc_construct(dc, init_params)) {
-   dc_destruct(dc);
-   goto construct_fail;
-   }
+   if (!dc_construct(dc, init_params))
+   goto destruct_dc;

full_pipe_count = dc->res_pool->pipe_count;
if (dc->res_pool->underlay_pipe_index != NO_UNDERLAY_PIPE)
@@ -1007,7 +1003,8 @@ struct dc *dc_create(const struct dc_init_data 
*init_params)

return dc;

-construct_fail:
+destruct_dc:
+   dc_destruct(dc);
kfree(dc);
return NULL;
 }
--
2.29.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 0/2] drm/amd/display: Adjustments for dc_create()

2020-12-20 Thread Markus Elfring 
From: Markus Elfring 
Date: Sat, 19 Dec 2020 18:30:56 +0100

Two update suggestions were taken into account
from static source code analysis.

Markus Elfring (2):
  Return directly after a failed kzalloc()
  Use common error handling code

 drivers/gpu/drm/amd/display/dc/core/dc.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

--
2.29.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Avoid null pointer dereference in soc15_reg_base_init()

2020-10-01 Thread Markus Elfring 
> that change, the NULL pointer dereference does not occur:

* Please provide a proper tag “Signed-off-by”.

* How do you think about to add the tag “Fixes” to the commit message?

* Would another imperative wording become helpful for the change description?

* Would you like to choose an other patch subject?


…
> +++ b/drivers/gpu/drm/amd/amdgpu/soc15.c
> @@ -699,7 +699,8 @@  static void soc15_reg_base_init(struct amdgpu_device 
> *adev)
>  "fallback to legacy init method\n");
> vega10_reg_base_init(adev);
> }
> -   }
> +   } else
> +   vega10_reg_base_init(adev);
> break;
…

Will curly brackets be relevant also for this else branch?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Fix memory leak in amdgpu_dm_mode_config_init()

2020-08-27 Thread Markus Elfring 
> When amdgpu_display_modeset_create_props() fails, state and
> state->context should be freed to prevent memleak. It's the
> same when amdgpu_dm_audio_init() fails.

* Can another imperative wording become helpful for the change description?

* Would you like to consider the tag “Fixes” for the commit message?

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c?id=08572451b4b1783fdff787b0188c4d50fdf96b81


…
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -2834,12 +2834,18 @@  static int amdgpu_dm_mode_config_init(struct 
> amdgpu_device *adev)
>   _atomic_state_funcs);
>
>   r = amdgpu_display_modeset_create_props(adev);
> - if (r)
> + if (r) {
> + dc_release_state(state->context);
> + kfree(state);
>   return r;
> + }
>
>   r = amdgpu_dm_audio_init(adev);
> - if (r)
> + if (r) {
> + dc_release_state(state->context);
> + kfree(state);
>   return r;
> + }
>
>   return 0;
>  }

I imagine that the exception handling code can be improved another bit
for this function implementation.
How do you think about to avoid such duplicate source code?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=15bc20c6af4ceee97a1f90b43c0e386643c071b4#n475

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: Put ACPI table after using it

2020-07-23 Thread Markus Elfring 
…
> and in kfd_create_vcrat_image_cpu(), the acpi_table is only used to
> get the OEM info, so those table mappings need to be release after
…

1. Please avoid a typo for this change description.

2. An imperative wording can be preferred here, can't it?

3. Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v3] drm/amd: Fix memory leak according to error branch

2020-06-22 Thread Markus Elfring 
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.
…
> Changes since V2:
> *remove duplicate kobject_put in kfd_procfs_init.

Under which circumstances are going to improve this change description 
accordingly?

Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches

2020-06-22 Thread Markus Elfring 
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.

I suggest to improve this change description.

* Can an other wording variant be nicer?

* Will the tag “Fixes” become helpful for the commit message?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdkfd: Fix memory leaks according to error branches

2020-06-22 Thread Markus Elfring 
>> I suggest to improve this change description.
>>
>> * Can an other wording variant be nicer?
>
> Markus's suggestion is as usual extremely imprecise.

I pointed a general possibility out. I did not propose an exact wording
alternative as it happened for other patches.


> However, I also find the message quite unclear.

I find this response interesting.


> It would be good to always use English words.

I am curious how this review will evolve further with such information
also after the third patch version.
https://lore.kernel.org/lkml/20200620091152.11206-1-bern...@vivo.com/
https://lore.kernel.org/patchwork/patch/1260303/

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [v2] drm/amdkfd: Fix memory leaks according to error branches

2020-06-22 Thread Markus Elfring 
> memleak is also not an English word.  Memory leak is only a few more
> characters, and doesn't require the reader to make the small extra effort
> to figure out what you mean.

Would you like to achieve similar adjustments at any more places?

How do you think about effects from a corresponding jargon?
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/gpu/drm/msm/msm_submitqueue.c?id=177d3819633cd520e3f95df541a04644aab4c657

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdkfd: Fix memory leaks according to error branches

2020-06-19 Thread Markus Elfring 
> The function kobject_init_and_add alloc memory like:
> kobject_init_and_add->kobject_add_varg->kobject_set_name_vargs
> ->kvasprintf_const->kstrdup_const->kstrdup->kmalloc_track_caller
> ->kmalloc_slab, in err branch this memory not free. If use
> kmemleak, this path maybe catched.
> These changes are to add kobject_put in kobject_init_and_add
> failed branch, fix potential memleak.

I suggest to improve this change description.

* Can an other wording variant be nicer?

* Will the tag “Fixes” become helpful?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu/debugfs: fix memory leak when pm_runtime_get_sync failed

2020-06-16 Thread Markus Elfring 
> Fix memory leak in amdgpu_debugfs_gpr_read not freeing data when
> pm_runtime_get_sync failed.

* Would you like to improve the exception handling any more for this software 
module?

* How do you think about calling the function “pm_runtime_put_noidle”?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: fix ref count leak in amdgpu_display_crtc_set_config

2020-06-15 Thread Markus Elfring 
> in amdgpu_display_crtc_set_config, …

* Can the term “reference count” become relevant also for this commit message
  besides other possible adjustments?

* Can it be nicer to combine proposed updates for this software module
  as a patch series (with a cover letter)?

* Would you like to add the tag “Fixes”?


…
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
…
> @@ -306,6 +306,7 @@  int amdgpu_display_crtc_set_config(struct drm_mode_set 
> *set,
>   adev->have_disp_power_ref = false;
>   }
>
> +out:
>   /* drop the power reference we got coming in here */
>   pm_runtime_put_autosuspend(dev->dev);
>   return ret;

Perhaps use the label “put_runtime” instead?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [V3] amdgpu: remove unnecessary condition check

2020-04-21 Thread Markus Elfring 
>> But i have to say there are so many code not follow the kernel code-style in 
>> amdgpu module.
>> And also the ./scripts/checkpatch.pl did not throw any warning or error.
>
> That is unfortunately true, yes. But we try to push new code through the 
> usual code review and improve things as we go.
>
> On the other hand patches just to fix the coding style are usually seen as an 
> unnecessary interruption and just a waste of time.

Would you become interested in adjusting deviations from known programming 
guidelines
in an automatic way with the help of corresponding advanced development tools?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [V3] amdgpu: remove unnecessary condition check

2020-04-21 Thread Markus Elfring 
>>> There is no need to if check again, maybe we could merge
>>> into the above else branch.

I find also this commit message still improvable (besides the mentioned
implementation details around coding style concerns).
How will corresponding review comments be taken better into account?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: Return more error codes in amdgpu_connector_set_property()

2020-04-21 Thread Markus Elfring 
> The "if(!encoder)" branch return the same value 0 of the success
> branch, maybe return -EINVAL is more better.

I suggest to improve the commit message.

* Are you still unsure about the next changes?
  
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=ae83d0b416db002fe95601e7f97f64b59514d936#n151

* Would you like to adjust the patch subject another bit?

* How do you think about to add the tag “Fixes”
  because of adjustments for the exception handling?


It can be nicer if all patch reviewers (including me) will be explicitly 
specified
as recipients for such messages, can't it?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [V3] amdgpu: remove unnecessary condition check

2020-04-21 Thread Markus Elfring 
> But i have to say there are so many code not follow the kernel code-style in 
> amdgpu module.
> And also the ./scripts/checkpatch.pl did not throw any warning or error.

Will such information become more interesting for further evolution
in the affected software areas?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH V2] drm/amdgpu: Remove an unnecessary condition check in reserve_bo_and_cond_vms()

2020-04-21 Thread Markus Elfring 
> There is no need to if check again,

Thanks for this information.

* Should the function name be mentioned in this commit message?

* Would you like to adjust the patch subject another bit?


> maybe we could merge into the above else branch.

I suggest to reconsider this wording.
Are you still unsure about the next changes?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=ae83d0b416db002fe95601e7f97f64b59514d936#n151

How do you think about to add the tag “Fixes”?


It can be nicer if all patch reviewers (including me) will be explicitly 
specified
as recipients for such messages, can't it?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Reduce a lock scope in amdgpu_amdkfd_gpuvm_free_memory_of_gpu()

2020-04-19 Thread Markus Elfring 
> Maybe we could reduce the mutex_lock(>lock)`s protected code area,
> and noneed to protect pr_debug.

I suggest to improve the commit message.
Would you like to adjust the patch subject?

Do you imagine that data synchronisation should evolve in other ways?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Remove an unnecessary null pointer check in amdgpu_cs_bo_handles_chunk()

2020-04-19 Thread Markus Elfring 
> kvfree ensure that the null pointer will do nothing.

I suggest to improve the commit message.
Would you like to adjust the patch subject?

Are you looking for further questionable condition checks?


…
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -98,8 +98,7 @@ static int amdgpu_cs_bo_handles_chunk(struct 
> amdgpu_cs_parser *p,
…
> + kvfree(info);
>
>   return r;
>  }

How do you think about to omit a blank line behind the function call
at this source code place?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Remove an unnecessary condition check in reserve_bo_and_cond_vms()

2020-04-19 Thread Markus Elfring 
> There is no need to if check again,

Thanks for this information.

* Should the function name be mentioned in this change description?

* Would you like to adjust the patch subject?


> maybe we could merge into the above else branch.

I suggest to reconsider this wording.


…
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -735,10 +735,8 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem,
…

I propose to take further coding style aspects into account.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=90280eaa88ac1a9140dc759941123530d5545bb6#n191

Possible refactoring:
if (ret) {
pr_err(…);
…
} else {
ctx->reserved = true;
}


How do you think about to add the tag “Fixes”?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: Return more error codes in amdgpu_connector_set_property()

2020-04-19 Thread Markus Elfring 
> The "if(!encoder)" branch return the same value 0 of the success
> branch, maybe return -EINVAL is more better.

I suggest to improve the commit message.

* Would you like to adjust the patch subject?

* How do you think about to add the tag “Fixes”
  because of adjustments for the exception handling?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amd/display: Fix a compilation warning

2020-04-03 Thread Markus Elfring 
> When I compile the code in X86,there is a warning about
> "'PixelPTEReqHeightPTES' may be used uninitialized in this function".

Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v4] drm/amdgpu: fix multiple memory leaks in acp_hw_init

2019-10-01 Thread Markus Elfring 
> ---

Why did you omit the patch change log at this place?


>  drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 34 -


> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> @@ -189,7 +189,7 @@ static int acp_hw_init(void *handle)
…
> + struct i2s_platform_data *i2s_pdata = NULL;
…

I propose to reconsider this update suggestion once more.


> @@ -393,6 +396,13 @@ static int acp_hw_init(void *handle)
>   val &= ~ACP_SOFT_RESET__SoftResetAud_MASK;
>   cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val);
>   return 0;
> +
> +failure:
> + kfree(i2s_pdata);
> + kfree(adev->acp.acp_res);
> + kfree(adev->acp.acp_cell);
> + kfree(adev->acp.acp_genpd);
> + return r;
>  }
>
>  /**

Are you going to follow a known programming guideline?
https://wiki.sei.cmu.edu/confluence/display/c/MEM12-C.+Consider+using+a+goto+chain+when+leaving+a+function+on+error+when+using+and+releasing+resources#MEM12-C.Considerusingagotochainwhenleavingafunctiononerrorwhenusingandreleasingresources-CompliantSolution%28copy_process%28%29fromLinuxkernel%29

Regards,
Markus

Re: [PATCH v3] drm/amdgpu: fix multiple memory leaks in acp_hw_init

2019-10-01 Thread Markus Elfring 
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c
> @@ -184,12 +184,12 @@ static struct device *get_mfd_cell_dev(const char 
> *device_name, int r)
…
> + struct i2s_platform_data *i2s_pdata = NULL;
…

I propose to reconsider this update suggestion.


> @@ -231,20 +231,21 @@ static int acp_hw_init(void *handle)
>   adev->acp.acp_cell = kcalloc(ACP_DEVS, sizeof(struct mfd_cell),
>   GFP_KERNEL);
>
> - if (adev->acp.acp_cell == NULL)
> - return -ENOMEM;
…

I suggest to keep this source code place unchanged (at the moment).
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c#n456


> @@ -393,6 +399,13 @@ static int acp_hw_init(void *handle)
>   val &= ~ACP_SOFT_RESET__SoftResetAud_MASK;
>   cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val);
>   return 0;
> +
> +failure:
> + kfree(i2s_pdata);
> + kfree(adev->acp.acp_res);
> + kfree(adev->acp.acp_cell);
> + kfree(adev->acp.acp_genpd);
> + return ret;
>  }
>
>  /**

I would prefer separate jump targets for efficient exception handling.
Please choose more appropriate labels for this function implementation.


> ---

I suggest to replace this second delimiter by a blank line.

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: fix multiple memory leaks

2019-09-27 Thread Markus Elfring 
> v2: moved the released into goto error handlings

A better version comment should be moved below the triple dashes.


Will the tag “Fixes” be added?


> @@ -393,6 +395,16 @@ static int acp_hw_init(void *handle)
>   val &= ~ACP_SOFT_RESET__SoftResetAud_MASK;
>   cgs_write_register(adev->acp.cgs_device, mmACP_SOFT_RESET, val);
>   return 0;
> +
> +out4:
> + kfree(i2s_pdata);
> +out3:
> + kfree(adev->acp.acp_res);
> +out2:
> + kfree(adev->acp.acp_cell);
> +out1:
> + kfree(adev->acp.acp_genpd);
> + return ret;
>  }
>
>  /**

I suggest to reconsider the label selection according to the Linux coding style.

Regards,
Markus

Re: [v2] drm/amdgpu: Remove two redundant null pointer checks

2019-09-06 Thread Markus Elfring 
> The functions "debugfs_remove" and "kfree" tolerate the passing
> of null pointers. Hence it is unnecessary to check such arguments
> around the calls. Thus remove the extra condition check at two places.

Will a tag like “Generated-by: scripts/coccinelle/free/ifnullfree.cocci” be 
relevant here?

How do you think about to compare this change approach with another patch 
variant?

drm/amdgpu: Delete an unnecessary check before two function calls
https://lkml.org/lkml/2019/9/4/401
https://lore.kernel.org/patchwork/patch/1123689/
https://lore.kernel.org/r/a3739125-5fa8-cadb-d2b8-8a9f12e9b...@web.de/

Regards,
Markus
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: drm/amdgpu: remove the redundant null check

2019-09-05 Thread Markus Elfring 
>> Were any source code analysis tools involved for finding
>> these update candidates?
> With the help of Coccinelle. You can find out some example in 
> scripts/coccinelle/.

Thanks for such background information.
Was the script “ifnullfree.cocci” applied here?

Will it be helpful to add attribution for such tools
to any more descriptions in your patches?

Regards,
Markus

Re: drm/amdgpu: remove the redundant null check

2019-09-04 Thread Markus Elfring 
> debugfs_remove and kfree has taken the null check in account.
> hence it is unnecessary to check it. Just remove the condition.

How do you think about a wording like the following?

  The functions “debugfs_remove” and “kfree” tolerate the passing
  of null pointers. Hence it is unnecessary to check such arguments
  around the calls. Thus remove the extra condition check at two places.


> No functional change.

I find this information questionable while it is partly reasonable
according to the shown software refactoring.

Can a subject like “[PATCH] drm/amdgpu: Remove two redundant
null pointer checks” be nicer here?


Were any source code analysis tools involved for finding
these update candidates?

Regards,
Markus

Re: drm/amdgpu: Delete an unnecessary check before two function calls

2019-09-04 Thread Markus Elfring 
> The functions “debugfs_remove” and “kfree” test whether their argument
> is NULL and then return immediately.
> Thus the tests around the shown calls are not needed.
>
> This issue was detected by using the Coccinelle software.

I suggest to take another look at a similar patch.

drm/amdgpu: remove the redundant null check
https://lkml.org/lkml/2019/9/3/59
https://lore.kernel.org/patchwork/patch/1123118/
https://lore.kernel.org/r/1567491305-18320-1-git-send-email-zhongji...@huawei.com/

Regards,
Markus

[PATCH] drm/amdgpu: Delete an unnecessary check before two function calls

2019-09-04 Thread Markus Elfring 
From: Markus Elfring 
Date: Wed, 4 Sep 2019 12:30:23 +0200

The functions “debugfs_remove” and “kfree” test whether their argument
is NULL and then return immediately.
Thus the tests around the shown calls are not needed.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 5652cc72ed3a..d321c72d63d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -1076,10 +1076,7 @@ static int amdgpu_debugfs_ib_preempt(void *data, u64 val)
kthread_unpark(ring->sched.thread);

ttm_bo_unlock_delayed_workqueue(>mman.bdev, resched);
-
-   if (fences)
-   kfree(fences);
-
+   kfree(fences);
return 0;
 }

@@ -1103,8 +1100,7 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)

 void amdgpu_debugfs_preempt_cleanup(struct amdgpu_device *adev)
 {
-   if (adev->debugfs_preempt)
-   debugfs_remove(adev->debugfs_preempt);
+   debugfs_remove(adev->debugfs_preempt);
 }

 #else
--
2.23.0

Re: drm/amd/powerplay: remove redundant memset

2019-07-15 Thread Markus Elfring 
> kzalloc has already zeroed the memory.
> So the memset is unneeded.

See also a previous patch:
drm/amd/powerplay: Delete a redundant memory setting in 
vega20_set_default_od8_setttings()
https://lore.kernel.org/lkml/de3f6a5e-8ac4-bc8e-0d0c-3a4a5db28...@web.de/
https://lore.kernel.org/patchwork/patch/1089691/
https://lkml.org/lkml/2019/6/17/460

Regards,
Markus

[PATCH] drm/amd/powerplay: Delete a redundant memory setting in vega20_set_default_od8_setttings()

2019-06-17 Thread Markus Elfring 
From: Markus Elfring 
Date: Mon, 17 Jun 2019 14:24:14 +0200

The memory was set to zero already by a call of the function “kzalloc”.
Thus remove an extra call of the function “memset” for this purpose.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/powerplay/vega20_ppt.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c 
b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
index 4aa8f5a69c4c..62497ad66a39 100644
--- a/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
+++ b/drivers/gpu/drm/amd/powerplay/vega20_ppt.c
@@ -1295,7 +1295,6 @@ static int vega20_set_default_od8_setttings(struct 
smu_context *smu)
if (!table_context->od8_settings)
return -ENOMEM;

-   memset(table_context->od8_settings, 0, sizeof(struct 
vega20_od8_settings));
od8_settings = (struct vega20_od8_settings 
*)table_context->od8_settings;

if (smu_feature_is_enabled(smu, FEATURE_DPM_SOCCLK_BIT)) {
--
2.22.0

[PATCH] drm/amd/display: Delete a redundant memory setting in amdgpu_dm_irq_register_interrupt()

2019-06-17 Thread Markus Elfring 
From: Markus Elfring 
Date: Mon, 17 Jun 2019 13:56:39 +0200

The memory was set to zero already by a call of the function “kzalloc”.
Thus remove an extra call of the function “memset” for this purpose.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring 
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c 
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
index 1b59d3d42f7b..fa5d503d379c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_irq.c
@@ -277,8 +277,6 @@ void *amdgpu_dm_irq_register_interrupt(struct amdgpu_device 
*adev,
return DAL_INVALID_IRQ_HANDLER_IDX;
}

-   memset(handler_data, 0, sizeof(*handler_data));
-
init_handler_common_data(handler_data, ih, handler_args, >dm);

irq_source = int_params->irq_source;
--
2.22.0

[PATCH 3/3] drm/amd/powerplay/hwmgr: Delete an unnecessary return statement in three functions

2018-02-08 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 8 Feb 2018 21:10:58 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: void function return statements are not generally useful

Thus remove such a statement in the affected functions.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c| 3 ---
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c | 1 -
 2 files changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
index 2681c9317d25..e07b32491092 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
@@ -653,8 +653,6 @@ void phm_trim_voltage_table_to_fit_state_table(uint32_t 
max_vol_steps,
vol_table->entries[i] = vol_table->entries[i + diff];
 
vol_table->count = max_vol_steps;
-
-   return;
 }
 
 int phm_reset_single_dpm_table(void *table,
@@ -906,7 +904,6 @@ void hwmgr_init_default_caps(struct pp_hwmgr *hwmgr)
 
phm_cap_set(hwmgr->platform_descriptor.platformCaps,

PHM_PlatformCaps_FanSpeedInTableIsRPM);
-   return;
 }
 
 int hwmgr_set_user_specify_caps(struct pp_hwmgr *hwmgr)
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
index 2d55dabc77d4..fcdb3563d860 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c
@@ -3618,7 +3618,6 @@ static uint32_t vega10_find_highest_dpm_level(
 static void vega10_apply_dal_minimum_voltage_request(
struct pp_hwmgr *hwmgr)
 {
-   return;
 }
 
 static int vega10_get_soc_index_for_max_uclk(struct pp_hwmgr *hwmgr)
-- 
2.16.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 0/3] drm/amd/powerplay/hwmgr: Adjustments for eight function implementations

2018-02-08 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 8 Feb 2018 21:37:42 +0100

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Delete an error message for a failed memory allocation in three functions
  Adjust layout for source code from five if statements
  Delete an unnecessary return statement in three functions

 drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 37 ++
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c| 35 +---
 drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c   |  5 +--
 drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c |  4 +--
 drivers/gpu/drm/amd/powerplay/hwmgr/vega10_hwmgr.c |  1 -
 5 files changed, 35 insertions(+), 47 deletions(-)

-- 
2.16.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/amd/powerplay/hwmgr: Adjust layout for source code from five if statements

2018-02-08 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 8 Feb 2018 21:01:24 +0100

The script "checkpatch.pl" pointed information out like the following.

WARNING: Comparisons should place the constant on the right side
of the test
WARNING: else is not generally useful after a break or return

Thus fix the affected source code places.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c   | 33 +++-
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c  | 31 +++---
 drivers/gpu/drm/amd/powerplay/hwmgr/ppatomctrl.c |  5 ++--
 3 files changed, 33 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
index c0699b884894..870c517f2057 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
@@ -1772,37 +1772,34 @@ static int cz_read_sensor(struct pp_hwmgr *hwmgr, int 
idx,
return 0;
case AMDGPU_PP_SENSOR_UVD_VCLK:
if (!cz_hwmgr->uvd_power_gated) {
-   if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
+   if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS)
return -EINVAL;
-   } else {
-   vclk = uvd_table->entries[uvd_index].vclk;
-   *((uint32_t *)value) = vclk;
-   return 0;
-   }
+
+   vclk = uvd_table->entries[uvd_index].vclk;
+   *((uint32_t *)value) = vclk;
+   return 0;
}
*((uint32_t *)value) = 0;
return 0;
case AMDGPU_PP_SENSOR_UVD_DCLK:
if (!cz_hwmgr->uvd_power_gated) {
-   if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
+   if (uvd_index >= CZ_MAX_HARDWARE_POWERLEVELS)
return -EINVAL;
-   } else {
-   dclk = uvd_table->entries[uvd_index].dclk;
-   *((uint32_t *)value) = dclk;
-   return 0;
-   }
+
+   dclk = uvd_table->entries[uvd_index].dclk;
+   *((uint32_t *)value) = dclk;
+   return 0;
}
*((uint32_t *)value) = 0;
return 0;
case AMDGPU_PP_SENSOR_VCE_ECCLK:
if (!cz_hwmgr->vce_power_gated) {
-   if (vce_index >= CZ_MAX_HARDWARE_POWERLEVELS) {
+   if (vce_index >= CZ_MAX_HARDWARE_POWERLEVELS)
return -EINVAL;
-   } else {
-   ecclk = vce_table->entries[vce_index].ecclk;
-   *((uint32_t *)value) = ecclk;
-   return 0;
-   }
+
+   ecclk = vce_table->entries[vce_index].ecclk;
+   *((uint32_t *)value) = ecclk;
+   return 0;
}
*((uint32_t *)value) = 0;
return 0;
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
index ded33ed03f11..2681c9317d25 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
@@ -813,24 +813,23 @@ int 
phm_initializa_dynamic_state_adjustment_rule_settings(struct pp_hwmgr *hwmgr
/* initialize vddc_dep_on_dal_pwrl table */
table_size = sizeof(uint32_t) + 4 * sizeof(struct 
phm_clock_voltage_dependency_record);
table_clk_vlt = kzalloc(table_size, GFP_KERNEL);
-
-   if (NULL == table_clk_vlt) {
+   if (!table_clk_vlt)
return -ENOMEM;
-   } else {
-   table_clk_vlt->count = 4;
-   table_clk_vlt->entries[0].clk = PP_DAL_POWERLEVEL_ULTRALOW;
-   table_clk_vlt->entries[0].v = 0;
-   table_clk_vlt->entries[1].clk = PP_DAL_POWERLEVEL_LOW;
-   table_clk_vlt->entries[1].v = 720;
-   table_clk_vlt->entries[2].clk = PP_DAL_POWERLEVEL_NOMINAL;
-   table_clk_vlt->entries[2].v = 810;
-   table_clk_vlt->entries[3].clk = PP_DAL_POWERLEVEL_PERFORMANCE;
-   table_clk_vlt->entries[3].v = 900;
-   if (pptable_info != NULL)
-   pptable_info->vddc_dep_on_dal_pwrl = table_clk_vlt;
-   hwmgr->dyn_state.vddc_dep_on_dal_pwrl = table_clk_vlt;
-   }
 
+   table_clk_vlt->count = 4;
+   table_clk_vlt->entries[0].clk = PP_DAL_POWERLEVEL_ULTRALOW;
+   table_clk_vlt->entries[0].v

[PATCH 1/3] drm/amd/powerplay/hwmgr: Delete an error message for a failed memory allocation in three functions

2018-02-08 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Thu, 8 Feb 2018 20:32:39 +0100

Omit an extra message for a memory allocation failure in these functions.

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 4 +---
 drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c| 1 -
 drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c | 4 +---
 3 files changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
index b314d09d41af..c0699b884894 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c
@@ -286,10 +286,8 @@ static int cz_init_dynamic_state_adjustment_rule_settings(
struct phm_clock_voltage_dependency_table *table_clk_vlt =
kzalloc(table_size, GFP_KERNEL);
 
-   if (NULL == table_clk_vlt) {
-   pr_err("Can not allocate memory!\n");
+   if (!table_clk_vlt)
return -ENOMEM;
-   }
 
table_clk_vlt->count = 8;
table_clk_vlt->entries[0].clk = PP_DAL_POWERLEVEL_0;
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
index 0229f774f7a9..ded33ed03f11 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/hwmgr.c
@@ -815,7 +815,6 @@ int 
phm_initializa_dynamic_state_adjustment_rule_settings(struct pp_hwmgr *hwmgr
table_clk_vlt = kzalloc(table_size, GFP_KERNEL);
 
if (NULL == table_clk_vlt) {
-   pr_err("Can not allocate space for vddc_dep_on_dal_pwrl! \n");
return -ENOMEM;
} else {
table_clk_vlt->count = 4;
diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c 
b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
index 569073e3a5a1..967b93e56113 100644
--- a/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
+++ b/drivers/gpu/drm/amd/powerplay/hwmgr/rv_hwmgr.c
@@ -107,10 +107,8 @@ static int rv_init_dynamic_state_adjustment_rule_settings(
struct phm_clock_voltage_dependency_table *table_clk_vlt =
kzalloc(table_size, GFP_KERNEL);
 
-   if (NULL == table_clk_vlt) {
-   pr_err("Can not allocate memory!\n");
+   if (!table_clk_vlt)
return -ENOMEM;
-   }
 
table_clk_vlt->count = 8;
table_clk_vlt->entries[0].clk = PP_DAL_POWERLEVEL_0;
-- 
2.16.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH] drm/amdgpu: Use seq_putc() in two functions

2018-01-12 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Fri, 12 Jan 2018 22:08:50 +0100

A few single characters should be put into a sequence.
Thus use the corresponding function "seq_putc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c  | 9 +++--
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 10805edcf964..7ac233f5abe2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -806,8 +806,8 @@ static int amdgpu_debugfs_gem_bo_info(int id, void *ptr, 
void *data)
pin_count = READ_ONCE(bo->pin_count);
if (pin_count)
seq_printf(m, " pin count %d", pin_count);
-   seq_printf(m, "\n");
 
+   seq_putc(m, '\n');
return 0;
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
index 3144400435b7..57ce42d960ae 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sa.c
@@ -419,11 +419,8 @@ void amdgpu_sa_bo_dump_debug_info(struct amdgpu_sa_manager 
*sa_manager,
list_for_each_entry(i, _manager->olist, olist) {
uint64_t soffset = i->soffset + sa_manager->gpu_addr;
uint64_t eoffset = i->eoffset + sa_manager->gpu_addr;
-   if (>olist == sa_manager->hole) {
-   seq_printf(m, ">");
-   } else {
-   seq_printf(m, " ");
-   }
+
+   seq_putc(m, (>olist == sa_manager->hole) ? '>' : ' ');
seq_printf(m, "[0x%010llx 0x%010llx] size %8lld",
   soffset, eoffset, eoffset - soffset);
 
@@ -431,7 +428,7 @@ void amdgpu_sa_bo_dump_debug_info(struct amdgpu_sa_manager 
*sa_manager,
seq_printf(m, " protected by 0x%08x on context %llu",
   i->fence->seqno, i->fence->context);
 
-   seq_printf(m, "\n");
+   seq_putc(m, '\n');
}
spin_unlock(_manager->wq.lock);
 }
-- 
2.15.1

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] GPU-DRM-Radeon: Use seq_puts() in radeon_debugfs_pm_info()

2017-05-02 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Tue, 2 May 2017 21:50:14 +0200

Two strings which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/radeon/radeon_pm.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_pm.c 
b/drivers/gpu/drm/radeon/radeon_pm.c
index 326ad068c15a..f84ddcc426c1 100644
--- a/drivers/gpu/drm/radeon/radeon_pm.c
+++ b/drivers/gpu/drm/radeon/radeon_pm.c
@@ -1869,13 +1869,14 @@ static int radeon_debugfs_pm_info(struct seq_file *m, 
void *data)
 
if  ((rdev->flags & RADEON_IS_PX) &&
 (ddev->switch_power_state != DRM_SWITCH_POWER_ON)) {
-   seq_printf(m, "PX asic powered off\n");
+   seq_puts(m, "PX asic powered off\n");
} else if (rdev->pm.dpm_enabled) {
mutex_lock(>pm.mutex);
if (rdev->asic->dpm.debugfs_print_current_performance_level)

radeon_dpm_debugfs_print_current_performance_level(rdev, m);
else
-   seq_printf(m, "Debugfs support not implemented for this 
asic\n");
+   seq_puts(m,
+"Debugfs support not implemented for this 
asic\n");
mutex_unlock(>pm.mutex);
} else {
seq_printf(m, "default engine clock: %u0 kHz\n", 
rdev->pm.default_sclk);
-- 
2.12.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/3] GPU-DRM-Radeon: Use seq_putc() in radeon_sa_bo_dump_debug_info()

2017-05-02 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Tue, 2 May 2017 21:35:48 +0200

A few single characters should be put into a sequence.
Thus use the corresponding function "seq_putc".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/radeon/radeon_sa.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon_sa.c 
b/drivers/gpu/drm/radeon/radeon_sa.c
index 197b157b73d0..67bc3618798d 100644
--- a/drivers/gpu/drm/radeon/radeon_sa.c
+++ b/drivers/gpu/drm/radeon/radeon_sa.c
@@ -406,18 +406,15 @@ void radeon_sa_bo_dump_debug_info(struct 
radeon_sa_manager *sa_manager,
list_for_each_entry(i, _manager->olist, olist) {
uint64_t soffset = i->soffset + sa_manager->gpu_addr;
uint64_t eoffset = i->eoffset + sa_manager->gpu_addr;
-   if (>olist == sa_manager->hole) {
-   seq_printf(m, ">");
-   } else {
-   seq_printf(m, " ");
-   }
+
+   seq_putc(m, (>olist == sa_manager->hole) ? '>' : ' ');
seq_printf(m, "[0x%010llx 0x%010llx] size %8lld",
   soffset, eoffset, eoffset - soffset);
if (i->fence) {
seq_printf(m, " protected by 0x%016llx on ring %d",
   i->fence->seq, i->fence->ring);
}
-   seq_printf(m, "\n");
+   seq_putc(m, '\n');
}
spin_unlock(_manager->wq.lock);
 }
-- 
2.12.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 0/3] GPU-DRM-Radeon: Fine-tuning for three function implementations

2017-05-02 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Tue, 2 May 2017 22:00:02 +0200

Three update suggestions were taken into account
from static source code analysis.

Markus Elfring (3):
  Use seq_putc() in radeon_sa_bo_dump_debug_info()
  Use seq_puts() in radeon_debugfs_pm_info()
  Use seq_puts() in r100_debugfs_cp_csq_fifo()

 drivers/gpu/drm/radeon/r100.c  | 6 +++---
 drivers/gpu/drm/radeon/radeon_pm.c | 5 +++--
 drivers/gpu/drm/radeon/radeon_sa.c | 9 +++--
 3 files changed, 9 insertions(+), 11 deletions(-)

-- 
2.12.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/3] GPU-DRM-Radeon: Use seq_puts() in r100_debugfs_cp_csq_fifo()

2017-05-02 Thread SF Markus Elfring 
From: Markus Elfring <elfr...@users.sourceforge.net>
Date: Tue, 2 May 2017 21:54:49 +0200

Strings which did not contain data format specifications should be put
into a sequence. Thus use the corresponding function "seq_puts".

This issue was detected by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfr...@users.sourceforge.net>
---
 drivers/gpu/drm/radeon/r100.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c
index c31e660e35db..09b88738aa07 100644
--- a/drivers/gpu/drm/radeon/r100.c
+++ b/drivers/gpu/drm/radeon/r100.c
@@ -2992,19 +2992,19 @@ static int r100_debugfs_cp_csq_fifo(struct seq_file *m, 
void *data)
seq_printf(m, "Indirect2 wptr %u\n", ib2_wptr);
/* FIXME: 0, 128, 640 depends on fifo setup see cp_init_kms
 * 128 = indirect1_start * 8 & 640 = indirect2_start * 8 */
-   seq_printf(m, "Ring fifo:\n");
+   seq_puts(m, "Ring fifo:\n");
for (i = 0; i < 256; i++) {
WREG32(RADEON_CP_CSQ_ADDR, i << 2);
tmp = RREG32(RADEON_CP_CSQ_DATA);
seq_printf(m, "rfifo[%04d]=0x%08X\n", i, tmp);
}
-   seq_printf(m, "Indirect1 fifo:\n");
+   seq_puts(m, "Indirect1 fifo:\n");
for (i = 256; i <= 512; i++) {
WREG32(RADEON_CP_CSQ_ADDR, i << 2);
tmp = RREG32(RADEON_CP_CSQ_DATA);
seq_printf(m, "ib1fifo[%04d]=0x%08X\n", i, tmp);
}
-   seq_printf(m, "Indirect2 fifo:\n");
+   seq_puts(m, "Indirect2 fifo:\n");
for (i = 640; i < ib1_wptr; i++) {
WREG32(RADEON_CP_CSQ_ADDR, i << 2);
tmp = RREG32(RADEON_CP_CSQ_DATA);
-- 
2.12.2

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx