From: Markus Elfring
Date: Thu, 10 Dec 2020 17:00:13 +0100
A local variable was used only within an if branch.
Thus move the definition for the variable “page” into the corresponding
code block.
This issue was detected by using the Coccinelle software.
Signed-off-by: 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
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
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
> @@ -720,13 +720,10 @@ static bool meson_hdmi_connector_is_available(struct
> device *dev)
>
> /* If the endpoint node exists, consider it enabled */
> remote = of_graph_get_remote_port(ep);
> - if (remote) {
> - of_node_put(ep);
> - return true;
> - }
> @@ -337,17 +338,20 @@ int pl111_versatile_init(struct device *dev, struct
> pl111_drm_dev_private *priv)
> pdev = of_find_device_by_node(np);
> if (!pdev) {
> dev_err(dev, "can't find the sysreg device,
> deferring\n");
> + o
> @@ -208,8 +208,10 @@ static int __init omapdss_boot_init(void)
>
> dss = of_find_matching_node(NULL, omapdss_of_match);
>
> - if (dss == NULL || !of_device_is_available(dss))
> + if (dss == NULL || !of_device_is_available(dss)) {
> + of_node_put(dss);
> re
> v2->v1: convert a if statement into a ternary statement.
Would you like to omit the arrow in such version information?
> @@ -720,15 +720,10 @@ static bool meson_hdmi_connector_is_available(struct
> device *dev)
>
> /* If the endpoint node exists, consider it enabled */
> remote =
> v2->v1: turn the return into a goto done.
* The version identification can be shorter, can't it?
* The expection handling should be completed for the implementation
of the function “rcar_du_of_lvds_patch” in a different way.
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.g
> @@ -221,8 +221,10 @@ int tegra_dc_rgb_probe(struct tegra_dc *
> int err;
>
> np = of_get_child_by_name(dc->dev->of_node, "rgb");
> - if (!np || !of_device_is_available(np))
> + if (!np || !of_device_is_available(np)) {
> + of_node_put(np);
> return -
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
> 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
…
> +++ b/drivers/gpu/drm/tiny/hx8357d.c
> @@ -232,44 +232,49 @@ static int hx8357d_probe(struct spi_device *spi)
…
> + goto free_dbidev;
>
> spi_set_drvdata(spi, drm);
I got another development concern here.
Can it make sense to pass the variable “dbidev” instead of “drm”?
…
>
Please avoid also another typo in the (previous) patch subject.
Regards,
Markus
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
> …
>> +free_dbidev:
>> +kfree(dbidev);
> …
>
> I became curious if there is a need for such a memory release at another
> place.
I have taken another look also at a related implementation detail.
Now I have realised that the desired clean-up should usually be achieved by
the callback functio
Hello,
I have taken another look also at the implementation of the function
“mcde_probe”.
Now I wonder why the statement “drm->dev_private = mcde;” was specified twice
there.
https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/gpu/drm/mcde/mcde_drv.c#L339
https://git.kernel.org/pub/scm/linu
From: Markus Elfring
Date: Thu, 7 Nov 2019 18:05:08 +0100
A coccicheck run provided information like the following.
drivers/gpu/drm/qxl/qxl_kms.c:295:1-7: ERROR: missing iounmap;
ioremap on line 178 and execution via conditional on line 185
Generated by: scripts/coccinelle/free/iounmap.cocci
> v3d_submit_cl_ioctl call kfree() with variable 'bin' twice.
I would prefer a wording like “kfree() was called for the same variable twice
within an if branch.”.
> Fix it by removing the latter one.
I find the wording “Delete a duplicate function call.” more appropriate.
Please add the tag “F
>> Please add the tag “Fixes” to your change description.
>
> I got the results from "git blame":
> git blame -L 570,575 drivers/gpu/drm/v3d/v3d_gem.c
…
> 0d352a3a8a1f2 (Iago Toral Quiroga 2019-09-16 09:11:25 +0200 571)
> kfree(bin);
> a783a09ee76d6 (Eric Anholt 2019-04-16 15
> In imx_pd_bind, the duplicated memory for imxpd->edid via kmemdup should
> be released in drm_of_find_panel_or_bridge or imx_pd_register fail.
Please improve this change description.
> +++ b/drivers/gpu/drm/imx/parallel-display.c
> @@ -227,14 +227,18 @@ static int imx_pd_bind(struct device *de
I have taken another look also at the implementation of the function
“imx_pd_bind”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/gpu/drm/imx/parallel-display.c?id=43b815c6a8e7dbccb5b8bd9c4b099c24bc22d135#n197
https://elixir.bootlin.com/linux/v5.4-rc1/source/drive
> I agree with you, kmemdup may fail so a null check seems necessary there.
Would this place (and similar ones) be pointed out for further considerations
also by the source code analysis tool which your software research group
seems to be developing?
https://github.com/umnsec/cheq/
Regards,
Marku
From: Markus Elfring
Date: Sat, 12 Oct 2019 10:45:45 +0200
Two update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Fix error handling for a kmemdup() call in imx_pd_bind()
Fix error handling for a kmemdup() call in imx_ldb_panel_ddc()
drivers
From: Markus Elfring
Date: Sat, 12 Oct 2019 10:30:21 +0200
The return value from a call of the function “kmemdup” was not checked
in this function implementation. Thus add the corresponding error handling.
Fixes: 19022aaae677dfa171a719e9d1ff04823ce65a65 ("staging: drm/imx: Add
parallel di
From: Markus Elfring
Date: Sat, 12 Oct 2019 10:33:47 +0200
The return value from a call of the function “kmemdup” was not checked
in this function implementation. Thus add the corresponding error handling.
This issue was detected by using the Coccinelle software.
Fixes
Hello,
I tried another script for the semantic patch language out.
This source code analysis approach points out that the implementation
of the function “nouveau_connector_of_detect” contains still
an unchecked call of the function “kmemdup”.
https://git.kernel.org/pub/scm/linux/kernel/git/torvald
> +free_edid:
> + kfree(imxpd->edid);
> + return ret;
I have taken another look at this change idea.
Can the function call “devm_kfree(dev, imxpd)” become relevant
also at this place?
Would you like to combine it with the update suggestion
“Fix error handling for a kmemdup() call in imx_p
Hello,
I tried another script for the semantic patch language out.
This source code analysis approach points a call of the member
function “get_config_mode” out for further considerations
according to the implementation of the function “mdfld_dsi_output_init”.
https://git.kernel.org/pub/scm/linux/
> In the impelementation of v3d_submit_cl_ioctl() there are two memory leaks.
Please avoid another typo also in this change description.
> … If kcalloc fails to allocate memory for bin then
> render->base should be put. Also, if v3d_job_init() fails to initialize
> bin->base then allocated memor
> +++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
> @@ -276,8 +276,10 @@ nouveau_bo_alloc(struct nouveau_cli *cli, u64 *size, int
> *align, u32 flags,
> break;
> }
>
> - if (WARN_ON(pi < 0))
> + if (WARN_ON(pi < 0)) {
> + kfree(nvbo);
> retu
> …
>> +++ b/drivers/gpu/drm/v3d/v3d_gem.c
>> @@ -557,13 +557,16 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
> …
>> if (ret) {
>> v3d_job_put(&render->base);
>> +kfree(bin);
> …
>
> Can it be helpful to move the added function cal
> kfree() was called for the same variable twice within an if branch.
I wonder still how this software situation happened.
Now I imagine that it could be more logical to delete the second of
this function call if you would like to look at the history of previous
two patches once more.
How do y
From: Markus Elfring
Date: Thu, 19 Sep 2019 16:26:56 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/ocfb.c | 9 +
1 file changed, 1
From: Markus Elfring
Date: Thu, 19 Sep 2019 16:51:38 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/pxafb.c | 10 +-
1 file changed, 1
From: Markus Elfring
Date: Sat, 21 Sep 2019 19:43:51 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/arm/display/komeda/komeda_dev.c | 9 +
1
From: Markus Elfring
Date: Sat, 21 Sep 2019 20:04:08 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 7 +--
1 file
From: Markus Elfring
Date: Sat, 21 Sep 2019 20:32:25 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/rockchip/rk3066_hdmi.c | 8 +---
1 file changed
From: Markus Elfring
Date: Sat, 21 Sep 2019 20:51:52 +0200
Simplify this function implementation by using a known wrapper function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/tegra/vic.c | 9 +
1 file changed, 1
> 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
> +++ 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
> ---
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;
+# nested likely/unlikely calls
+ if ($line =~
/\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
+ WARN("LIKELY_MISUSE",
How do you think about to use the specification
“(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
in this regular expression?
>>> +# nested likely/unlikely calls
>>> + if ($line =~
>>> /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
>>> + WARN("LIKELY_MISUSE",
>>
>> How do you think about to use the specification
>> “(?:IS_ERR(?:_(?:OR_NULL|VALUE))?|WARN)”
>> in this regular expr
> +# nested likely/unlikely calls
> + if ($line =~
> /\b(?:(?:un)?likely)\s*\(\s*!?\s*(IS_ERR(?:_OR_NULL|_VALUE)?|WARN)/) {
> + WARN("LIKELY_MISUSE",
…
>> \b(?:un)?likely\s*
>
> This pair of brackets is required to match "unlikely"
> and it's optional in order to
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
> 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
>> 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
> 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?
From: Markus Elfring
Date: Fri, 6 Sep 2019 18:40:48 +0200
Simplify these function implementations by using a known function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/omapdrm/dss/hdmi4.c | 6 +-
drivers/gpu/drm
the null pointer check.
This issue was detected by using the Coccinelle software.
Fixes: 5fad79fd66ff90b8c0a95319dad0b099008f8347 ("drm/mm: cleanup and improve
next_hole_*_addr()")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/drm_mm.c | 8 ++--
1 file changed, 6 insert
> This patch seems to be a part of a series without being marked as such,
The mentioned patch affects only a single function implementation.
> this causes issues when importing this patch with maintainer tools
> like b4 which automatically pull in the entire series and not
> just the specific pa
> Fix the email Sign-off email != Sender email issue, resubmit and I'll
> be able to apply this.
You can pick the email from my tag “Signed-off-by” up also directly
as an ordinary patch author email, can't you?
Regards,
Markus
From: Markus Elfring
Date: Sun, 16 Apr 2023 17:30:46 +0200
The address of a data structure member was determined before
a corresponding null pointer check in the implementation of
the function “receive_timing_debugfs_show”.
Thus avoid the risk for undefined behaviour by moving the assignment
because of a failed call of the function “u_memcpya”.
Thus use an additional label.
This issue was detected by using the Coccinelle software.
Fixes: 2be65641642ef423f82162c3a5f28c754d1637d2 ("drm/nouveau: fix relocations
applying logic and a double-free")
Signed-off-by: Markus Elfring
--
” 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
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
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
: 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
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
initialisations for the variable “c” (also because it was already
reassigned with the same value behind this pointer check).
This issue was detected by using the Coccinelle software.
Fixes: 25fdd5933e4c0f5fe2ea5cd59994f8ac5fbe90ef ("drm/msm: Add SDM845 DPU
support")
Signed-off-by: Mark
null pointer check.
This issue was detected by using the Coccinelle software.
Fixes: 3b495f2bb749b828499135743b9ddec46e34fda8 ("Au1100 FB driver uplift for
2.6.")
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/au1100fb.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletio
Date: Sun, 16 Apr 2023 11:22:23 +0200
Several update suggestions were taken into account
from static source code analysis.
Markus Elfring (9):
debugfs: Move an expression into a function call parameter
in nouveau_debugfs_pstate_set()
debugfs: Move a variable assignment behind a null
parameter for a function call at the end.
This issue was detected by using the Coccinelle software.
Fixes: 6e9fc177399f08446293fec7607913fdbc95e191 ("drm/nouveau/debugfs: add copy
of sysfs pstate interface ported to debugfs")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nouveau
” behind the null pointer check.
This issue was detected by using the Coccinelle software.
Fixes: 6e9fc177399f08446293fec7607913fdbc95e191 ("drm/nouveau/debugfs: add copy
of sysfs pstate interface ported to debugfs")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nouveau_deb
Date: Sat, 15 Apr 2023 21:48:47 +0200
A single character (line break) 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/nouveau/nouveau_debugfs.c | 2 +-
1
Date: Sat, 15 Apr 2023 22:02:31 +0200
Five strings which did not contain a data format specification 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
---
drivers/gpu/drm
parameter for a macro call in one if branch.
This issue was detected by using the Coccinelle software.
Fixes: e5f8eabc0077ea3f77b3362e28d3969ae62e70f0
("drm/nouveau/bios/power_budget: Add basic power budget parsing")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/s
null pointer check.
This issue was detected by using the Coccinelle software.
Fixes: 7c856522069755ab9d163a24ac332cd3cb35fe30 ("drm/nouveau/clk: implement
power state and engine clock control in core")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/subdev/clk/base.c
some condition checks.
This issue was detected by using the Coccinelle software.
Fixes: bcc19d9bf5cd8d49428c487adced1aa101271b18 ("drm/nouveau/pci: implement
generic code for pcie speed change")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/subdev/pci/pcie.c | 3 +
Date: Sun, 16 Apr 2023 08:45:31 +0200
The variable “pbus” was read only once in the implementation of
the function “nvkm_pcie_set_link”.
Thus move the usage of an expression into a parameter for a function call.
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/subdev/pci/pcie.c
data structure member “fan” behind two null pointer checks.
This issue was detected by using the Coccinelle software.
Fixes: da06b46b720687117178d3ee85a601762f1c36b5 ("drm/nouveau/therm: cosmetic
changes")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/nouveau/nvkm/subdev/therm/fa
” behind the null pointer check.
This issue was detected by using the Coccinelle software.
Fixes: b5c84a9edcd418cd055becad6a22439e7c5e3bf8 ("drm/bridge: add it6505
driver")
Signed-off-by: Markus Elfring
---
drivers/gpu/drm/bridge/ite-it6505.c | 3 ++-
1 file changed, 2 insertions(+),
“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
>> 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
>>
>> 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.
>
>
From: Markus Elfring
Date: Tue, 23 May 2023 19:07:19 +0200
Some update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Fix error handling in init_imstt()
Fix exception handling in imsttfb_probe()
Move a variable assignment for an error code in
From: Markus Elfring
Date: Tue, 23 May 2023 14:32:39 +0200
The return value was overlooked from a call of
the function “fb_alloc_cmap”.
* Thus use a corresponding error check.
* Add two jump targets so that a bit of exception handling
can be better reused at the end of this function
From: Markus Elfring
Date: Tue, 23 May 2023 15:40:43 +0200
The exception handling was incomplete.
The label “error” was used to jump to another pointer check despite of
the detail in the implementation of the function “imsttfb_probe”
that it was determined already that the corresponding
From: Markus Elfring
Date: Tue, 23 May 2023 18:30:26 +0200
The value “-ENOMEM” was assigned to the variable “ret”
at the beginning.
Move this statement directly before the first ioremap() call.
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/imsttfb.c | 2 +-
1 file changed, 1 insertion
From: Markus Elfring
Date: Tue, 23 May 2023 18:50:53 +0200
Replace the specification of a data structure by a pointer dereference
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
Sig
From: Markus Elfring
Date: Tue, 23 May 2023 22:04:33 +0200
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Move two variable assignments
Convert a variable initialisation into a later assignment
drivers/video/fbdev/core/fbcmap.c | 22
From: Markus Elfring
Date: Tue, 23 May 2023 21:30:29 +0200
Move the assignment for the local variables “size” and “flags”
because the computed values were only used in a single if branch.
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/core/fbcmap.c | 7 ---
1 file changed, 4
From: Markus Elfring
Date: Tue, 23 May 2023 21:56:55 +0200
* Assign the value “-ENOMEM” to the local variable “ret” only for
the exception handling.
* Use an additional label.
Signed-off-by: Markus Elfring
---
drivers/video/fbdev/core/fbcmap.c | 15 ++-
1 file changed, 10
>> Move the assignment for the local variables “size” and “flags”
>> because the computed values were only used in a single if branch.
>
> Please do not move such variables without real need.
Is there a need to explain desirable effects better?
> It makes backporting (of this and maybe follow-up
>> The value “-ENOMEM” was assigned to the variable “ret”
>> at the beginning.
>> Move this statement directly before the first ioremap() call.
>
> Please do not move such variables without real need.
Is there a need to explain desirable effects better?
> It makes backporting (of this and maybe
>> The return value was overlooked from a call of
>> the function “fb_alloc_cmap”.
>>
>> * Thus use a corresponding error check.
>>
>> * Add two jump targets so that a bit of exception handling
>> can be better reused at the end of this function.
…
>> +++ b/drivers/video/fbdev/imsttfb.c
…
>> @@
>> Can it be helpful to distinguish involved error codes better?
>
> No.
I find such a feedback surprising.
May the error code be preserved from a failed call of the function
“fb_alloc_cmap”?
Regards,
Markus
> Slicing configuration data from user is stored in a temporary buffer
> which should be freed unconditionally.
Are imperative change descriptions still preferred?
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.5
> I think the return value isn't checked at all, so you could
> simply return below "-ENODEV" for all cases (instead of "return ret").
> Then you don't need the e_nodev label and can simplify the flow.
I noticed that the software evolution was continued with an other change
variant.
fbdev: imstt
> The exception handling was incomplete.
>
> The label “error” was used to jump to another pointer check despite of
> the detail in the implementation of the function “imsttfb_probe”
> that it was determined already that the corresponding variable
> contained a null pointer.
>
> * Thus use more app
> Modern Linux distrobutions have adopted DRM drivers for graphics
> output and use fbdev only for the kernel's framebuffer console.
Would you like to avoid a typo in subsequent cover letters?
Regards,
Markus
> Ha! It says 'distrobutions'.
Do you tend to prefer any distributions?
Regards,
Markus
> 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
Would a corresponding imperative description be helpful also for such a small
change?
See also:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.4#n94
Regards,
Markus
> virtio_gpu_get_vbuf always be successful,
> so remove the error judgment.
How do you think about to improve this change description any more?
Regards,
Markus
> Make IS_ERR() judge the mipi_dsi_device_register_full() function return
Would the term “return value” be relevant here?
Would you like to improve this change description also according to
review comments from other patches?
How do you think about to add the tag “Fixes” because of a desirable
> Make IS_ERR() judge the mipi_dsi_device_register_full() function return
Would the term “return value” be relevant here?
Would you like to improve this change description also according to
review comments from other patches?
How do you think about to add the tag “Fixes” because of a desirable
1 - 100 of 742 matches
Mail list logo
