From: Markus Elfring
Date: Thu, 31 Dec 2015 21:06:27 +0100
This issue was detected by using the Coccinelle software.
* Return directly if a memory allocation failed.
* Drop the explicit initialisation for the variable "rc"
at the beginning then.
Signed-off-by:
From: Markus Elfring
Date: Thu, 31 Dec 2015 20:34:51 +0100
Replace an explicit initialisation for the variable "rc" at the beginning
by assignments that will only be performed if a memory allocation failed.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Fri, 1 Jan 2016 13:15:34 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
Less function calls after error detection
Delete unnecessary variable initialisations
Extend an
From: Markus Elfring
Date: Fri, 1 Jan 2016 16:54:13 +0100
Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Fri, 1 Jan 2016 11:16:04 +0100
The kfree() function was called in one case by the
gfar_ethflow_to_filer_table() function during error handling
even if a passed variable contained a null pointer.
* Return directly if a memory allocation
From: Markus Elfring
Date: Fri, 1 Jan 2016 12:56:23 +0100
Omit explicit initialisation at the beginning for four local variables
which are redefined before their first use.
Signed-off-by: Markus Elfring
---
>> +++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
>> @@ -778,11 +778,13 @@ static int gfar_ethflow_to_filer_table(struct
>> gfar_private *priv, u64 ethflow,
>>
>> local_rqfpr = kmalloc_array(MAX_FILER_IDX + 1, sizeof(unsigned int),
>> GFP_KERNEL);
From: Markus Elfring
Date: Fri, 1 Jan 2016 15:11:09 +0100
Replace explicit initialisations for four local variables at the beginning
by assignments that will only be performed if the corresponding code
will really be executed.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Fri, 1 Jan 2016 13:00:06 +0100
Move the assignment for the variable "j" from the beginning
into an initialisation clause of a for loop.
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Fri, 1 Jan 2016 13:56:09 +0100
The kfree() function was called in one case by the
gfar_ethflow_to_filer_table() function during error handling
even if a passed variable contained a null pointer.
* Return directly if a memory allocation
From: Markus Elfring
Date: Fri, 1 Jan 2016 20:20:15 +0100
Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.
Signed-off-by: Markus Elfring
---
>> I prefer that assignments for variables like "card" and "slic_regs"
>> will only be performed immediately before the corresponding content will be
>> read again (after a few condition checks were executed).
>>
>> Another description could be this view:
>> I suggest to move the variable
>> I am a bit surprised that you do not like such source code fine-tuning.
>
> It's moving stuff around for no real reason, why would I like it?
Can such fine-tuning result in positive effects for the run-time behaviour?
> Reading and reviewing and applying this type of stuff takes away from
>
>> Pass the address of the data structure element "time" directly in calls
>> of the function "rtc_update_irq" instead of an extra initialisation
>> for one local variable at the beginning.
>
> Why is it better?
I suggest to refer to the data item "rtc_data->rtc" directly because
the variable
>> Pass the address of the data structure element "time" directly in calls
>> of the function "rtc_update_irq" instead of an extra initialisation
>> for one local variable at the beginning.
>
> Also, I don't see anything related to time in this patch.
I should have referred to the data structure
>> Replace explicit initialisation for two local variables at the beginning
>> by assignments.
>
> Why?
I prefer that assignments for variables like "card" and "slic_regs"
will only be performed immediately before the corresponding content will be
read again (after a few condition checks were
>> ret = regmap_update_bits(data->regmap, ABB5ZES3_REG_TIM_CLK,
>> - mask, ABB5ZES3_REG_TIM_CLK_TAC1);
>> + ABB5ZES3_REG_TIM_CLK_TAC0
>> + | ABB5ZES3_REG_TIM_CLK_TAC1,
>> +
From: Markus Elfring
Date: Fri, 1 Jan 2016 17:32:07 +0100
Reduce the scope for the local variable "desc" to one branch
of an if statement.
Signed-off-by: Markus Elfring
---
drivers/net/usb/qmi_wwan.c | 3 ++-
1 file changed, 2
From: Markus Elfring
Date: Fri, 1 Jan 2016 19:16:05 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Delete an unnecessary variable initialisation in ath9k_hif_usb_rx_stream()
Replace a variable
From: Markus Elfring
Date: Fri, 1 Jan 2016 17:47:46 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Refactoring for qmi_wwan_bind()
Delete an unnecessary variable initialisation in
From: Markus Elfring
Date: Fri, 1 Jan 2016 17:35:03 +0100
Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.
Signed-off-by: Markus Elfring
---
drivers/net/usb/qmi_wwan.c | 2
From: Markus Elfring
Date: Fri, 1 Jan 2016 19:09:32 +0100
Replace an explicit initialisation for one local variable at the beginning
by a conditional assignment.
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Fri, 1 Jan 2016 19:00:53 +0100
Omit explicit initialisation at the beginning for one local variable
that is redefined before its first use.
Signed-off-by: Markus Elfring
---
>> Can such fine-tuning result in positive effects for the run-time behaviour?
>
> If you can not benchmark and show the proof, don't even start to claim
> such a thing.
Which measurement results would you accept for further discussion?
>> My suggestions can result in measurable differences.
>
From: Markus Elfring
Date: Mon, 21 Dec 2015 19:30:42 +0100
The script "checkpatch.pl" pointed out that labels should not be indented.
Thus delete a horizontal tab before the jump label "out"
in the function "osc_get_info".
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Mon, 21 Dec 2015 18:58:51 +0100
A few checks would be performed by the mgc_process_recover_log() function
even though it was determined that the passed variable "pages" contained
a null pointer or a call of the alloc_page() function
From: Markus Elfring
Date: Mon, 21 Dec 2015 18:24:45 +0100
The variable "mne_swab" will eventually be set to an appropriate value
from a call of the ptlrpc_rep_need_swab() function.
Thus let us omit the explicit initialisation at the beginning.
Signed-off-by:
From: Markus Elfring
Date: Mon, 21 Dec 2015 22:24:18 +0100
Another update suggestion was taken into account after a patch was applied
from static source code analysis.
Markus Elfring (2):
Change return type of cpu_set_cclk() to that of clk_set_rate()
Mark
From: Markus Elfring
Date: Mon, 21 Dec 2015 20:00:02 +0100
Several update suggestions were taken into account
from static source code analysis.
Markus Elfring (4):
Delete unnecessary goto statements in six functions
Delete an unnecessary variable
From: Markus Elfring
Date: Mon, 21 Dec 2015 21:56:27 +0100
The return type "unsigned long" was used by the cpu_set_cclk() function
while the type "int" is provided by the clk_set_rate() function.
Let us make this usage consistent.
This issue was detected by using
From: Markus Elfring
Date: Mon, 21 Dec 2015 22:12:26 +0100
The cpu_set_cclk() function was only used in a single source file so far.
Indicate this setting also by the corresponding linkage specifier.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Mon, 21 Dec 2015 18:15:45 +0100
Six goto statements referred to a source code position
directly behind them.
Thus omit such unnecessary jumps.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Tue, 22 Dec 2015 22:32:07 +0100
This issue was detected by using the Coccinelle software.
Let us return directly if a memory allocation failed.
Signed-off-by: Markus Elfring
---
block/partitions/ldm.c |
From: Markus Elfring
Date: Tue, 22 Dec 2015 16:43:12 +0100
Another update suggestion was taken into account after a patch was applied
from static source code analysis.
Markus Elfring (2):
Use a signed return type for fhci_create_ep()
Use a signed return type
Hello,
I have looked at the source file for the ACPI fan driver once more.
I would appreciate if a specific implementation detail can be clarified there.
Static source code analysis can point out that functions like the following
share an approach for error detection and corresponding exception
From: Markus Elfring
Date: Wed, 23 Dec 2015 17:32:12 +0100
Several update suggestions were taken into account
from static source code analysis.
Markus Elfring (5):
One function call less in ldm_validate_tocblocks() after error detection
Delete extra log
From: Markus Elfring
Date: Wed, 23 Dec 2015 13:32:51 +0100
Let us return directly if a memory allocation failed.
Signed-off-by: Markus Elfring
---
block/partitions/ldm.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff
From: Markus Elfring
Date: Wed, 23 Dec 2015 14:31:01 +0100
The kfree() function was called by the ldm_validate_privheads() function
during error handling even if the passed variable "ph" contained
a null pointer.
* Corresponding implementation details could be
From: Markus Elfring
Date: Wed, 23 Dec 2015 13:21:01 +0100
A failed call of the kmalloc() function can generate appropriate
information about insufficient memory already.
Thus remove unnecessary log messages for such failures.
Suggested-by: Julia Lawall
From: Markus Elfring
Date: Wed, 23 Dec 2015 17:23:25 +0100
1. Remove some space characters and add a few according to the Linux coding
style convention.
2. Reformat a bit of source code also for one switch statement.
3. Apply a few more recommendations from
From: Markus Elfring
Date: Tue, 22 Dec 2015 22:32:07 +0100
This issue was detected by using the Coccinelle software.
Let us return directly if a memory allocation failed.
Signed-off-by: Markus Elfring
---
block/partitions/ldm.c |
From: Markus Elfring
Date: Fri, 25 Dec 2015 19:36:20 +0100
The variable "err" will eventually be set to an appropriate value
from a call of the gpiochip_add() function.
Thus let us omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
Hello,
I have looked at the source file for the Marvell SATA support driver once more.
I would appreciate if a specific implementation detail can be clarified there.
Static source code analysis can point out that functions like the following
are called by the mv_platform_probe() function.
From: Markus Elfring
Date: Sat, 26 Dec 2015 07:30:35 +0100
The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.
Implementation details could be
From: Markus Elfring
Date: Sat, 26 Dec 2015 08:00:52 +0100
The kfree() function was called in one case by the
acpi_i2c_space_handler() function during error handling
even if the passed variable "client" contained a null pointer.
Implementation details could be
From: Markus Elfring
Date: Sat, 26 Dec 2015 18:40:43 +0100
Return the value from a call of the ocrdma_mbx_modify_qp() function
without using an extra assignment for the local variable "status".
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Sat, 26 Dec 2015 18:54:47 +0100
Return constant integer values without storing them in the local
variable "status".
Signed-off-by: Markus Elfring
---
drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 9
From: Markus Elfring
Date: Sat, 26 Dec 2015 19:09:23 +0100
1. Return zero in one case directly.
2. Return the value from a call of the _ocrdma_dealloc_pd() function
without using an extra assignment for the local variable.
3. Remove the variable "status" in
From: Markus Elfring
Date: Sat, 26 Dec 2015 18:18:18 +0100
The variable "status" will be set to an appropriate value a bit later.
Thus let us omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Sat, 26 Dec 2015 17:16:00 +0100
This issue was detected by using the Coccinelle software.
* Let us return directly if a call of the _ocrdma_alloc_pd()
function failed.
* Delete the jump label "err" and the local variable "status"
From: Markus Elfring
Date: Sat, 26 Dec 2015 19:30:54 +0100
Several update suggestions were taken into account
from static source code analysis.
Markus Elfring (6):
One variable and jump label less in ocrdma_alloc_ucontext_pd()
Delete unnecessary variable
From: Markus Elfring
Date: Sat, 26 Dec 2015 18:28:35 +0100
Return zero at the end without using the local variable "status".
Signed-off-by: Markus Elfring
---
drivers/infiniband/hw/ocrdma/ocrdma_hw.c | 3 +--
1 file changed, 1
>> I would appreciate if an unnecessary function call can be avoided here
>> so that the affected exception handling can become also a bit more efficient.
>
> Simpler code is easier to maintain.
There are different opinions available around the desired simplicity.
> See your patch, you didn't
From: Markus Elfring
Date: Sat, 26 Dec 2015 10:01:36 +0100
The kfree() function was called in two cases by the
ide_get_dev_handle() function during error handling
even if the passed variable "dinfo" contained a null pointer.
* Let us return directly if a call of
From: Markus Elfring
Date: Sat, 26 Dec 2015 10:33:48 +0100
The variable "dinfo" will contain an appropropriate pointer after a call
of the acpi_get_object_info() function succeeded.
Thus remove two safety checks.
Signed-off-by: Markus Elfring
>> The kfree() function was called in one case by the
>> acpi_i2c_space_handler() function during error handling
>> even if the passed variable "client" contained a null pointer.
>
> This is OK. kfree() is known to be NULL-tolerant and we rely on it in
> various places to keep the code simpler.
From: Markus Elfring
Date: Sat, 26 Dec 2015 10:50:33 +0100
Move the assignment for the variable "addr" to the statement
where its value is used after previous function calls succeeded.
Signed-off-by: Markus Elfring
---
From: Markus Elfring
Date: Sat, 26 Dec 2015 13:53:15 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" does not need to be checked once more
after it was
From: Markus Elfring
Date: Sat, 26 Dec 2015 11:04:56 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (3):
One function call less in ide_get_dev_handle() after error detection
Delete unnecessary null
From: Markus Elfring
Date: Sat, 26 Dec 2015 22:18:38 +0100
This issue was detected by using the Coccinelle software.
* Let us return directly if a call of the _ocrdma_alloc_pd()
function failed.
* Reduce the scope for the local variable "status" to one case
>> A single call of the fprintf() function is sufficient for the desired
>> display of the usage information.
>
> This seems like churn that doesn't actually fix anything.
Will it matter occasionally to reduce the number of function calls
by combining several text fragments into the passing of
From: Markus Elfring
Date: Sat, 19 Dec 2015 19:32:27 +0100
The return type "size_t" was used by the functions "lpfc_wwn_set"
and "lpfc_oas_lun_state_set" despite of the aspect that they will
eventually return a negative error code.
Improve this implementation
From: Markus Elfring
Date: Sat, 19 Dec 2015 21:10:20 +0100
The return type "u32" was used by the fhci_create_ep() function even though
it will eventually return a negative error code.
Improve this implementation detail by using the type "s32" instead.
This issue
>>> This introduces a compile warning.
>>
>> How do you think about to show the exact message you get?
>
> I can't actually compile it myself.
It seems that I can understand your feedback also a bit better
since I received the information from a background process
like "kbuild test robot".
Will
From: Markus Elfring
Date: Sat, 19 Dec 2015 17:55:39 +0100
The return type "unsigned int" was used by the adi_gpio_irq_startup()
function despite of the aspect that it will eventually return a negative
error code.
Improve this implementation detail by deletion of
> Just make it an int.
Thanks for your suggestion.
Will any more software developers prefer this data type
at some source code places?
> The caller also casts it to u32...
Do you want to get rid of similar casts in affected functions?
Regards,
Markus
--
To unsubscribe from this list: send
From: Markus Elfring
Date: Sat, 19 Dec 2015 17:15:34 +0100
The return type "unsigned long" was used by the st_ll_sleep_state()
function despite of the aspect that it will eventually return a negative
error code.
Improve this implementation detail by deletion of the
From: Markus Elfring
Date: Sat, 19 Dec 2015 20:30:39 +0100
The return type "u32" was used by the slic_card_locate() function
despite of the aspect that it will eventually return a negative
error code.
Improve this implementation detail by using the type "s32"
> This introduces a compile warning.
How do you think about to show the exact message you get?
> These functions are supposed to return 1 if there is an IRQ pending.
> Change the -EINVAL to 0.
Is there any more source code clean-up needed around the comment "FIXME"
in the affected function?
From: Markus Elfring
Date: Fri, 18 Dec 2015 19:43:27 +0100
The return type "unsigned long" was used by the cpu_set_cclk() function
while the type "int" is provided by the clk_set_rate() function.
Let us make this usage consistent.
This issue was detected by using
Reuse the type from this poll call instead.
>>>
>>> Why use uint when the function return type it unsigned int?
>>
>> Do you prefer to express the type modifier once more there?
>
> I don't know what the sentence means,
Can it be a matter of taste if the key word "unsigned" should be
From: Markus Elfring
Date: Sun, 20 Dec 2015 07:56:36 +0100
The return type "unsigned long" was used by the suffix_kstrtoint()
function even though it will eventually return a negative error code.
Improve this implementation detail by using the type "int" instead.
From: Markus Elfring
Date: Sun, 20 Dec 2015 09:09:34 +0100
The data type "int" was used by the variable "result" in the
function "posix_clock_poll" even though the type "uint" will usually
be needed for the return value from a call of the function which was
>> Reuse the type from this poll call instead.
>
> Why use uint when the function return type it unsigned int?
Do you prefer to express the type modifier once more there?
> On the other hand, why is the function return type unsigned int
> when there is a return of a negative constant?
This
From: Markus Elfring
Date: Sun, 20 Dec 2015 10:34:25 +0100
The return type "unsigned int" was used by the ssm2518_lookup_mcs()
function even though it will eventually return a negative error code.
Improve this implementation detail by deletion of the type modifier
>> https://cwe.mitre.org/data/definitions/252.html
>
> The value is not unchecked.
Would you like to express any stronger relationship between
the function call example and the occurrence of an if statement
by the discussed SmPL script?
> I made a specific rule because the specific problem is
From: Markus Elfring
Date: Sun, 27 Dec 2015 22:02:21 +0100
Return constant integer values without storing them in the local
variable "err" or "rc".
Signed-off-by: Markus Elfring
---
drivers/media/pci/bt8xx/bttv-driver.c | 25
From: Markus Elfring
Date: Sun, 27 Dec 2015 18:23:57 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a call of the si2165_readreg8()
function failed.
2. Reduce the scope for the local variables "ret" and "tmp" to
From: Markus Elfring
Date: Mon, 28 Dec 2015 22:52:48 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a buffer allocation failed.
2. Delete the jump label "err" then.
3. Drop the explicit initialisation for the
>> The close() function could be called by the main() function even if
>> the passed variable "cfd" was assigned a negative value.
…
> This seems more easily fixed by simply making the condition > 0.
How do you think about the reuse of the error predicate "cfd != -1"
for the return value from a
> The error return value of platform_get_irq seems to often get dropped.
How do you think about any more fine-tuning here?
Commit message:
* … of the platform_get_irq() function seems to get dropped too often.
* Why do you concentrate on a single function name?
Do you plan to extend this
>> Move the jump label directly before the desired log statement
>> so that the variable "ret" will not be checked once more
>> after it was determined that a function call failed.
>
> Why not avoid both unnecessary ifs
I would find such a fine-tuning also nice in principle at more source code
From: Markus Elfring
Date: Mon, 28 Dec 2015 10:10:34 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after it was determined that
From: Markus Elfring
Date: Mon, 28 Dec 2015 15:10:30 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after a function call.
Use
From: Markus Elfring
Date: Mon, 28 Dec 2015 15:20:45 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a call of the regmap_write() function failed.
2. Delete the jump label "err" then.
3. Return zero as a constant at
From: Markus Elfring
Date: Mon, 28 Dec 2015 15:32:20 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Better exception handling in five functions
Refactoring for m88rs6000t_sleep()
>> Move the jump label directly before the desired log statement
>> so that the variable "ret" will not be checked once more
>> after a function call.
>
> This commit message fits with the previous change.
Do you prefer an other wording?
> It could be nice to put a blank line before the error
From: Markus Elfring
Date: Mon, 28 Dec 2015 16:36:44 +0100
The variable "rc" will be set to an appropriate value from a call of
the r820t_set_tv_standard() function.
Thus let us omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Mon, 28 Dec 2015 17:13:02 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "rc" will not be checked once more
after a function call.
Use
From: Markus Elfring
Date: Mon, 28 Dec 2015 17:18:34 +0100
A few update suggestions were taken into account
from static source code analysis.
Markus Elfring (2):
Delete an unnecessary variable initialisation
Better exception handling
From: Markus Elfring
Date: Sun, 27 Dec 2015 13:12:10 +0100
Subject: [PATCH 0/2] InfiniBand-iSER: Refactoring for two function
implementations
I suggest to return directly instead of using the jump label "err"
in two functions (which are working without clean-up
From: Markus Elfring
Date: Sun, 27 Dec 2015 11:41:42 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a call of the iser_set_sig_attrs()
function failed.
2. Delete the jump label "err" then.
3. Return zero as a
From: Markus Elfring
Date: Sun, 27 Dec 2015 12:54:52 +0100
This issue was detected by using the Coccinelle software.
1. Let us return directly if a call of the function "isert_set_sig_attrs"
or "ib_post_send" failed.
2. Delete the jump label "err" then.
3.
From: Markus Elfring
Date: Mon, 28 Dec 2015 22:10:28 +0100
This issue was detected by using the Coccinelle software.
Move the jump label directly before the desired log statement
so that the variable "ret" will not be checked once more
after a function call.
Use
From: Markus Elfring
Date: Mon, 28 Dec 2015 20:10:30 +0100
This issue was detected by using the Coccinelle software.
Split the previous if statement at the end so that each final log statement
will eventually be performed by a direct jump to these labels.
*
> I think the first patch is already Acked now and can be applied
> as it is. Just send the next patch based on the previous one.
Does it matter to provide the proposed changes as a tiny patch series
with two update steps?
Regards,
Markus
--
To unsubscribe from this list: send the line
> This should be static, yeah.
Would you like to integrate a corresponding small source code change yourself?
(Do you need a separate patch from me?)
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
> In this case, there is indeed a problem. However, just changing the
> type won't fix it.
Would you like to explain your view on the handling of potential
data type mismatches a bit more?
Regards,
Markus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of
> Since you reported the issue, it will be good if you can send a patch
> for this as well.
I'm sorry if I become a bit picky for this implementation detail.
In which order would you prefer that the properties of the
function "cpu_set_cclk" will be improved?
* Should the linkage specifier be
>> 6. Apply a recommendation from the script "checkpatch.pl".
>
> That's 6 different things, shouldn't this be 6 different patches?
>
> please redo.
Dan Carpenter requested to squash the previous update steps 5 and 6
into a single patch for better source code review.
Now I see further software
801 - 900 of 10642 matches
Mail list logo