> I think the original code was fine.
I suggest to reconsider involved implementation details once more.
> x = blah(); if (x) ... is a perfectly familiar kernel coding pattern.
I can agree to such a general information.
> There is no benefit in terms of performance
It might be possible that
> if (atomic_read(&priv->sleepstatus.status) == 0) {
> rw_data = GCR_B_DOZE;
> - retval =
> - ks7010_sdio_write(priv, GCR_B, &rw_data, sizeof(rw_data));
> - if (retval) {
> + if (ks7010_sdio_write(priv,
> +
>> @@ -323,14 +323,14 @@ static void tx_device_task(void *dev)
>> {
>> struct ks_wlan_private *priv = (struct ks_wlan_private *)dev;
>> struct tx_device_buffer *sp;
>> -int rc = 0;
>>
>> DPRINTK(4, "\n");
>> if (cnt_txqbody(priv) > 0
>> && atomic_read(&priv->psstat
>> * Do you occasionally care for a refactoring like "Reduce scope of variable"?
>>
>> http://refactoring.com/catalog/reduceScopeOfVariable.html
>
> Probably not. Certainly not in this case.
In which use cases would the suggested change pattern be more interesting
for you?
Regards,
Markus
___
> That being said... checkpatch does not complain about leading space
> before labels. Not even with --strict. So why are you mentioning it here?
I remembered a warning like "INDENTED_LABEL" instead.
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d2
>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/scripts/checkpatch.pl?id=92d21ac74a9e3c09b0b01c764e530657e4c85c49#n4326
>
> "#goto labels aren't indented, allow a single space however"
>
> Can't be clearer :-)
Should such information from a comment in this script be also
>> Can you follow expectations around the proposed refactoring of any
>> function implementations?
>
> I don't understand both questions. Maybe you need to give examples?
I suggest to try the following script (semantic patch for working with
the Coccinelle software) out on the discussed source fi
>> I guess that further clarification might be needed for affected
>> implementation details.
>
> That's OK, too.
>
> Acked-by: Wolfram Sang
Does this acknowledgement include also the acceptance for
the suggested change around calls of the functions "sdio_claim_host"
and "sdio_release_host" wit
From: Markus Elfring
Further update suggestions were taken into account
after a patch was applied from static source code analysis.
Markus Elfring (3):
Delete an unnecessary check before the function call "release_firmware"
One function call less in mac_ioctl() after error
From: Markus Elfring
Date: Sun, 24 Jul 2016 21:00:20 +0200
The release_firmware() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Sun, 24 Jul 2016 21:15:23 +0200
The kfree() function was called in two cases by the mac_ioctl() function
during error handling even if the passed variable did not contain a pointer
for a valid data item.
Improve this implementation detail by the introduction of
From: Markus Elfring
Date: Sun, 24 Jul 2016 21:45:37 +0200
Three local variables were used only within a single case branch.
* Thus move the data type definition for "rssi" and "size" into the
corresponding code block.
* The variable "length" was not modified af
>> Would you like to support the renaming of a label like "error_out1"
>> (in the function "ks7010_upload_firmware" for example)?
>
> They should be renamed too. Anything using numbers instead of explicit
Interesting …
> Anything using numbers instead of explicit labels should be updated.
Woul
From: Markus Elfring
Date: Mon, 25 Jul 2016 22:20:24 +0200
The vfree() function performs also input parameter validation.
Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/staging/rtl8188eu/core
>> -if (strncasecmp(buff, "RSSI", length) == 0) {
>> +if (strncasecmp(buff, "RSSI", 0) == 0) {
>> +s8 rssi;
>> +
>
> Um, please think a second about if it makes any sense at all to compare
> zero chars of two strings.
Under whic
> I have a lot of other things to work on, of much greater interest (to me.)
This is fine.
Thanks for your feedback.
>>> Personally I see no value in such statistics.
>>
>> Do they indicate any code smells eventually?
>
> I have no idea what you mean, sorry.
How do you think about to take ano
From: Markus Elfring
Further update suggestions were taken into account
after a patch was applied from static source code analysis.
Markus Elfring (12):
ldlm: Delete unnecessary checks before the function call "kset_unregister"
Delete unnecessary checks before the function call &q
From: Markus Elfring
Date: Tue, 26 Jul 2016 11:33:43 +0200
The kset_unregister() function tests whether its argument is NULL
and then returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Tue, 26 Jul 2016 13:00:32 +0200
The kobject_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
From: Markus Elfring
Date: Tue, 26 Jul 2016 13:40:47 +0200
The kobject_put() function was called in a few cases by the
class_register_type() function during error handling even if the passed
data structure element did not contain a pointer for a valid data item.
Adjust jump targets according to
From: Markus Elfring
Date: Tue, 26 Jul 2016 14:10:55 +0200
The kfree() function was called in up to three cases by the
class_register_type() function during error handling even if
the passed data structure element contained a null pointer.
* Split a condition check for memory allocation
From: Markus Elfring
Date: Tue, 26 Jul 2016 16:32:31 +0200
Return directly after a memory allocation failed at the beginning.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers
From: Markus Elfring
Date: Tue, 26 Jul 2016 17:12:51 +0200
Release memory directly after a page allocation failed at the beginning.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff
From: Markus Elfring
Date: Tue, 26 Jul 2016 17:54:24 +0200
Release memory directly after a call of the function
"ptlrpc_request_alloc" failed.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
di
From: Markus Elfring
Date: Tue, 26 Jul 2016 17:56:49 +0200
Delete a check for the local variable "req" which became unnecessary with
the previous update step.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 3 +--
1 file changed, 1 insertion(+), 2
From: Markus Elfring
Date: Tue, 26 Jul 2016 19:29:11 +0200
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 19 +--
1 file changed, 9 insertions(+), 10 deletions(-)
diff
From: Markus Elfring
Date: Tue, 26 Jul 2016 19:40:28 +0200
Move the assignment for the local variable "eof" behind the source code
for memory allocations by this function.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/mgc/mgc_request.c | 3 ++-
1 file changed, 2
From: Markus Elfring
Date: Tue, 26 Jul 2016 14:23:23 +0200
Return a constant error code without storing it in the local variable "rc"
after a failed memory allocation at the beginning of this function.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/obdclass/ge
From: Markus Elfring
Date: Tue, 26 Jul 2016 19:50:40 +0200
The variable "req" will eventually be set to an appropriate pointer
from a call of the ptlrpc_request_alloc() function.
Thus omit the explicit initialisation which became unnecessary with
a previous update step.
Signed-off-
> But kobject_put() already checks for NULL, right?
Yes. - Such an input parameter validation is performed by the
function implementation.
> you just submitted another batch about that in other area.
I sent update suggestions because of this function property for two
Linux software modules in t
> NAK.
> when you do this, the next statement below breaks:
I wonder about this conclusion.
>> type = kzalloc(sizeof(*type), GFP_NOFS);
>> if (!type)
>> -return rc;
>> +return -ENOMEM;
>>
>> type->typ_dt_ops = kzalloc(sizeof(*type->typ_dt_ops), GFP_NOFS);
>
> This function is called several times during lustre module insert.
> Namely it's called 5 times for 5 types:
> osc, mdc, lov, lmv, mgc.
Will any extra memory accesses matter for the successful execution
in this use case?
> It's not called any more than that, so it's not exactly a super hot-pat
> Which circumstances do "not any sense at all" imply?
Should the expression 'strlen("RSSI")' be passed for the parameter 'length'
instead?
> I suggest to fix this since it is indeed a bug,
We can agree that this function implementation was broken for a while there.
> instead of doing "micr
> In typical deployments outside of testing environment, this function is
> called 5 times every system boot and never again.
Does this information mean that a bit more fine-tuning is insignificant
at such a source code place?
>> Did the assignment for the local variable "rc" with a well-known e
>> Please and and use pr_fmt
>
> Can't we use dev_* on the SDIO device?
How should a connection be constructed from the data structure "sdio_device_id"
to the corresponding device information for such an use case?
Regards,
Markus
___
devel mailing list
From: Markus Elfring
Further update suggestions were taken into account
after a patch was applied from static source code analysis.
Markus Elfring (10):
Delete unnecessary checks before the function call "kfree"
Delete unnecessary assignments for buffer variables
Return direct
From: Markus Elfring
Date: Wed, 10 Aug 2016 17:15:15 +0200
The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the calls is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers
From: Markus Elfring
Date: Wed, 10 Aug 2016 17:26:01 +0200
A few variables were assigned a null pointer despite of the detail
that they were immediately reassigned by the following statement.
Thus remove such unnecessary assignments.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source
From: Markus Elfring
Date: Wed, 10 Aug 2016 17:34:12 +0200
Return directly after a memory allocation failed at the beginning.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
drivers/staging/ks7010/ks7010_sdio.c | 19 +++
1 file
From: Markus Elfring
Date: Wed, 10 Aug 2016 17:57:50 +0200
Adjust jump targets according to the Linux coding style convention.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
Touched four source code places less.
drivers/staging/ks7010
From: Markus Elfring
Date: Wed, 10 Aug 2016 18:56:31 +0200
A few return values can also be directly used for condition checks.
Thus remove a local variable for intermediate assignments.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
Touched
From: Markus Elfring
Date: Wed, 10 Aug 2016 19:57:01 +0200
Do not use curly brackets at some source code places
where a single statement should be sufficient.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
Touched less source code places
From: Markus Elfring
Date: Wed, 10 Aug 2016 20:45:11 +0200
Add a definition for the macro "pr_fmt" so that its information can be used
for consistent message output.
Suggested-by: Joe Perches
Signed-off-by: Markus Elfring
---
v2: Addition from source code review
drivers/stag
From: Markus Elfring
Date: Wed, 10 Aug 2016 21:56:15 +0200
Prefer usage of the macro "pr_err" over the interface "printk".
Fix a typo in an error message.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
drivers/staging/ks70
From: Markus Elfring
Date: Wed, 10 Aug 2016 22:10:20 +0200
The local variable "rc" was assigned a zero at one place.
But it was not read within this function. Thus delete it.
Signed-off-by: Markus Elfring
Reviewed-by: Wolfram Sang
---
v2: Rebased on the source files from Linux nex
From: Markus Elfring
Date: Wed, 10 Aug 2016 22:33:52 +0200
Three variables will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
v2: Rebased on the source files from Linux next-20160809.
It was omitted to
>>> Can't we use dev_* on the SDIO device?
>>
>> How should a connection be constructed from the data structure
>> "sdio_device_id"
>> to the corresponding device information for such an use case?
>
> What did you try so far?
Nothing what I would find noteworthy for increasing the usage of a fun
>> Prefer usage of the macro "pr_err" over the interface "printk".
> Not correct
A checkpatch warning like "PREFER_PR_LEVEL" can point additional possibilities
out
for this use case.
Would you like to introduce any of the higher level logging functions instead?
>> diff --git a/drivers/staging/k
> I think pr_ is OK if reworking the code
> to support dev_ is not easy.
Thanks for this explanation. - It sounds more constructive than the previous
short
feedback "Not correct".
>> Would you accept that another update will be appended to the discussed patch
>> series?
>
> No. Patches shoul
> You might have noticed I also wrote in the same reply:
>
> "All of these pr_fmt uses are redundant as pr_err already does pr_fmt"
I admit that I made another software development mistake there. - It might not
matter much
when a final fix could be to get rid of the three affected logging calls
> I added some Acked- and Reviewed-by tags last time.
I noticed this of course.
> Did the patches change
Yes. - The amount of source code which I touched in this software module
is different for the second series.
> or why didn't you add them?
I imagined that your acknowledgements to the pre
> Really now, this is basic fixes and cleanups,
I agree to this view to some degree.
> you have been asked many times in the past to move on beyond these,
It is more useful when more severe bugs or bigger software improvements can be
found.
Which source code clean-ups are picked better up by o
From: Markus Elfring
Date: Sun, 21 Aug 2016 11:30:57 +0200
Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/staging/lustre/lustre/llite/dir.c | 12
From: Markus Elfring
Date: Fri, 25 Aug 2017 13:15:43 +0200
Several update suggestions were taken into account
from static source code analysis.
Markus Elfring (14):
Delete 11 error messages for a failed memory allocation
Improve 11 size determinations
Move an assignment in
From: Markus Elfring
Date: Thu, 24 Aug 2017 21:38:20 +0200
Omit extra messages for a memory allocation failure in these functions.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/vme/vme.c | 51
From: Markus Elfring
Date: Thu, 24 Aug 2017 22:04:45 +0200
Assign a pointer to a data structure member without using an intermediate
local variable.
Signed-off-by: Markus Elfring
---
drivers/vme/vme.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/vme/vme.c
From: Markus Elfring
Date: Thu, 24 Aug 2017 21:52:00 +0200
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This
From: Markus Elfring
Date: Thu, 24 Aug 2017 22:24:38 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code
From: Markus Elfring
Date: Thu, 24 Aug 2017 22:32:14 +0200
Return directly without using an intermediate local variable
in these functions.
Signed-off-by: Markus Elfring
---
drivers/vme/vme.c | 10 --
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/vme/vme.c b
From: Markus Elfring
Date: Fri, 25 Aug 2017 09:31:46 +0200
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/vme/bridges/vme_fake.c | 1 -
1 file changed, 1 deletion
From: Markus Elfring
Date: Fri, 25 Aug 2017 09:46:13 +0200
Replace the specification of data structures by pointer dereferences
as the parameter for the operator "sizeof" to make the corresponding size
determination a bit safer according to the Linux coding style convention.
This
From: Markus Elfring
Date: Fri, 25 Aug 2017 10:01:16 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code
From: Markus Elfring
Date: Fri, 25 Aug 2017 10:20:03 +0200
Omit extra messages for a memory allocation failure in these functions.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/vme/bridges/vme_ca91cx42.c | 16
1 file
From: Markus Elfring
Date: Fri, 25 Aug 2017 10:56:41 +0200
Replace the specification of data structures by variable references
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: Fri, 25 Aug 2017 11:01:29 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code
From: Markus Elfring
Date: Fri, 25 Aug 2017 11:10:07 +0200
Omit extra messages for a memory allocation failure in these functions.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/vme/bridges/vme_tsi148.c | 18 --
1 file
From: Markus Elfring
Date: Fri, 25 Aug 2017 11:55:03 +0200
Replace the specification of data structures by variable references
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: Fri, 25 Aug 2017 12:00:17 +0200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code
>> @@ -2363,5 +2364,5 @@ static int tsi148_probe(struct pci_dev *pdev, const
>> struct pci_device_id *id)
>> master_num--;
>>
>> tsi148_device->flush_image =
>> -kmalloc(sizeof(struct vme_master_resource), GFP_KERNEL);
>> +kmalloc
From: Markus Elfring
Date: Sat, 26 Aug 2017 18:23:52 +0200
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/staging/fsl-mc/bus/fsl-mc-allocator.c | 2 --
1 file changed
From: Markus Elfring
Date: Sat, 26 Aug 2017 18:44:12 +0200
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/staging/netlogic/xlr_net.c | 4 +---
1 file changed, 1
@@ -2363,5 +2364,5 @@ static int tsi148_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
master_num--;
tsi148_device->flush_image =
-kmalloc(sizeof(struct vme_master_resource),
GFP_KERNEL);
+
From: Markus Elfring
Date: Thu, 5 Nov 2015 08:41:00 +0100
The functions "sc_return_credits" and "vfree" perform also input
parameter validation. Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-of
From: Markus Elfring
Date: Thu, 5 Nov 2015 10:18:45 +0100
The functions kobject_put() and kset_unregister() test whether their
argument is NULL and then return immediately.
Thus the tests around their calls are not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by
From: Markus Elfring
Date: Thu, 5 Nov 2015 10:55:16 +0100
The variable "rc" will be eventually set to an error return code in the
class_register_type() function.
Thus let us omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/staging/lus
From: Markus Elfring
Date: Thu, 5 Nov 2015 12:48:58 +0100
The functions "kfree" and "kobject_put" were called in a few cases by the
function "class_register_type" during error handling even if the passed
variable contained a null pointer.
This implementation d
From: Markus Elfring
Date: Thu, 5 Nov 2015 13:03:33 +0100
Further update suggestions were taken into account after a patch
was applied from static source code analysis.
Markus Elfring (3):
Delete unnecessary checks before two function calls
Delete an unnecessary variable initialisation in
From: Markus Elfring
Date: Thu, 5 Nov 2015 14:34:43 +0100
The module_put() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
> Please stop sending these patches for drivers/staging.
Will further contributors take another look at similar update suggestions?
> You are welcome to send them for other subsystems
Thanks for this suggestion.
> which I don't care about.
I am curious if other software developers will give
>From 45970693cad6c12da2d5ac7da3d2bd7a566170d7 Mon Sep 17 00:00:00 2001
From: Markus Elfring
Date: Thu, 23 Oct 2014 20:55:13 +0200
Subject: [PATCH] staging - rtl8188eu: Deletion of unnecessary checks before
three function calls
The functions kfree(), rtw_free_netdev() and vfree() test whet
The functions kfree(), rtw_free_netdev() and vfree() test whether their
argument is NULL and then return immediately. Thus the test around the call
is not needed.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/staging/rtl8188eu/core
>> The functions kfree(), rtw_free_netdev() and vfree() test whether their
>> argument is NULL and then return immediately. Thus the test around the call
>> is not needed.
>>
>> This issue was detected by using the Coccinelle software.
>>
>> Signed-
> Recent commits to kernel/git/torvalds/linux.git have made the following
> functions able to tolerate NULL arguments:
>
> kmem_cache_destroy (commit 3942d29918522)
> mempool_destroy (commit 4e3ca3e033d1)
> dma_pool_destroy (commit 44d7175da6ea)
How do you think about to extend an other SmPL scrip
From: Markus Elfring
Date: Sun, 7 Jan 2018 21:03:26 +0100
Omit extra messages for a memory allocation failure in this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/net/hyperv/netvsc.c | 5 -
1 file changed, 5 deletions
> These messages are not displayed anywhere else:
> "unable to allocate receive buffer of size %u\n"
> "unable to allocate send buffer of size %u\n",
>
> After set ret = -ENOMEM; and cleanup, we won't know which buffer allocation
> failed without the error message.
Do you notice a Linux allocati
> These messages are not displayed anywhere else:
> "unable to allocate receive buffer of size %u\n"
> "unable to allocate send buffer of size %u\n",
>
> After set ret = -ENOMEM; and cleanup, we won't know which buffer allocation
> failed without the error message.
How do you think about to achi
201 - 286 of 286 matches
Mail list logo
