…
> Although this case may be unrealistic for the current code, it is
> still better to protect against possible bugs.
>
> Bail out also when status is AE_NOT_FOUND.
…
How do you think about to add any tags (like “Fixes” and “Cc”) accordingly?
https://git.kernel.org/pub/scm/linux/kernel/git/torval
…
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_discovery.c
> @@ -1757,11 +1757,13 @@ int amdgpu_discovery_get_nps_info(struct
> amdgpu_device *adev,
>
> switch (le16_to_cpu(nps_info->v1.header.version_major)) {
> case 1:
> + mem_ranges = kvcalloc(nps_info->v1.count,
> +
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 Elfri
> 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.
>
> Fixes: d38ceaf99ed0 ("drm/amdgpu: add core driver (v4)")
> Signed-off-by: Ma Ke
Are you g
> 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
> > 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/
> 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 point
>> 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
> 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 f
> 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
> 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
…
> +++ 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_
> 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()
> d
> 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 patc
>> 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.
>
>
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
>> +++ 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, &info->head);
>> - struct ras_ih_data *data
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
” 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
: 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
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 +++
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
>> 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
>>
“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/a
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
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
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
> 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 pat
> 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 co
…
> 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 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 change
> 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 change
>> 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 th
> 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 j
> 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 change
> 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
_
> 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
>> 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 g
>>> 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,
> 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/proces
> 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
_
> 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 u
> Maybe we could reduce the mutex_lock(&mem->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
_
> 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_han
> 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
> 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 hand
> 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.freedes
> ---
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;
> +++ 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
> 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->ac
> 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?
>> 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 attribu
> 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 c
> 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: remo
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
> 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/p
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
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
From: Markus Elfring
Date: Thu, 8 Feb 2018 22:23:57 +0100
Omit an extra message for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/amd/amdkfd/kfd_topology.c | 1 -
1 file changed, 1
From: Markus Elfring
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: Mark
From: Markus Elfring
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
From: Markus Elfring
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
---
drivers/gpu/drm/amd/powerplay/hwmgr/cz_hwmgr.c | 4 +---
drivers/gpu
From: Markus Elfring
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 th
From: Markus Elfring
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
---
drivers/gpu/drm/
From: Markus Elfring
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: Mark
From: Markus Elfring
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
---
drivers/gpu/drm/radeon/radeo
From: Markus Elfring
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
From: Markus Elfring
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: Mark
68 matches
Mail list logo
