Re: [v3] Coccinelle: suggest replacing strncpy+truncation by strscpy
> The problem is that I don't know if the script is correct Will the clarification of such a concern evolve into another interesting software development adventure? > - I'm not familiar with these string functions. How are the chances to improve the understanding of affected programming interfaces anyhow? https://elixir.bootlin.com/linux/v4.18-rc5/source/lib/string.c#L154 https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/lib/string.c?id=28c20cc73b9cc4288c86c2a3fc62af4087de4b19#n154 Regards, Markus
[PATCH 4/6] Coccinelle: atomic_as_refcounter: Replace disjunction by a constraint in two SmPL rules
From: Markus Elfring Date: Mon, 2 Jul 2018 18:45:15 +0200 Three function names were specified for a search of function calls by the means of a disjunction in two rules of a script for the semantic patch language. Use a regular expression as a constraint for this source code search pattern instead so that duplication of SmPL code can be avoided. Signed-off-by: Markus Elfring --- .../coccinelle/api/atomic_as_refcounter.cocci | 21 +-- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci index 57af2db9463e..372ae99591fd 100644 --- a/scripts/coccinelle/api/atomic_as_refcounter.cocci +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci @@ -80,17 +80,11 @@ coccilib.report.print_report(p1[0], msg % (p2[0].line)) @r2 exists@ expression a; -identifier x; +identifier F =~ "^atomic(?:64|_long)?_add_unless$", x; position p1; @@ -( -atomic_add_unless(&(a)->x,-1,1)@p1 -| -atomic_long_add_unless(&(a)->x,-1,1)@p1 -| -atomic64_add_unless(&(a)->x,-1,1)@p1 -) + F(&(a)->x, -1, 1)@p1 @script:python depends on report@ p1 << r2.p1; @@ -99,17 +93,12 @@ msg = "atomic_add_unless" coccilib.report.print_report(p1[0], msg) @r3 exists@ -identifier x; +expression E; +identifier F =~ "^atomic(?:64|_long)?_add_return$"; position p1; @@ -( -x = atomic_add_return@p1(-1, ...); -| -x = atomic_long_add_return@p1(-1, ...); -| -x = atomic64_add_return@p1(-1, ...); -) + E = F@p1(-1, ...); @script:python depends on report@ p1 << r3.p1; -- 2.18.0
[PATCH 3/6] Coccinelle: atomic_as_refcounter: Use type “expression” for another metavariable
From: Markus Elfring Date: Mon, 2 Jul 2018 17:55:27 +0200 The metavariable “a” is enclosed by parentheses in three rules of a script for the semantic patch language. Replace its type by “expression” so that the corresponding source code search becomes more powerful. Signed-off-by: Markus Elfring --- scripts/coccinelle/api/atomic_as_refcounter.cocci | 9 ++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/scripts/coccinelle/api/atomic_as_refcounter.cocci b/scripts/coccinelle/api/atomic_as_refcounter.cocci index 5571eea04c7b..57af2db9463e 100644 --- a/scripts/coccinelle/api/atomic_as_refcounter.cocci +++ b/scripts/coccinelle/api/atomic_as_refcounter.cocci @@ -10,7 +10,8 @@ virtual report @r1 exists@ -identifier a, x; +expression a; +identifier x; position p1, p2; identifier fname =~ "free"; identifier fname2 =~ "(?:call_rcu|de(?:l|stroy)|(?:queue|schedule)_work)"; @@ -45,7 +46,8 @@ msg = "atomic_dec_and_test variation before object free at line %s." coccilib.report.print_report(p1[0], msg % (p2[0].line)) @r4 exists@ -identifier a, x, y; +expression a; +identifier x, y; position p1, p2; identifier fname =~ "free"; @@ @@ -77,7 +79,8 @@ msg = "atomic_dec_and_test variation before object free at line %s." coccilib.report.print_report(p1[0], msg % (p2[0].line)) @r2 exists@ -identifier a, x; +expression a; +identifier x; position p1; @@ -- 2.18.0
[PATCH 0/6] Coccinelle: atomic_as_refcounter: Improvements for source code search specifications
From: Markus Elfring Date: Tue, 3 Jul 2018 09:15:26 +0200 This source code search pattern was programmed in the way that some implementation details could be improved further. I suggest to avoid unnecessary code repetition also in this script for the semantic patch language. Markus Elfring (6): Omit placeholder specifications from two SmPL constraints Optimise a disjunction in the first SmPL rule Use type “expression” for another metavariable Replace disjunction by a constraint in two SmPL rules Use nested disjunctions in two SmPL rules Use format strings directly in SmPL rules .../coccinelle/api/atomic_as_refcounter.cocci | 104 +++--- 1 file changed, 39 insertions(+), 65 deletions(-) -- 2.18.0
Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family
>> * The repetition of such a constraint in subsequent SmPL rules could be >> avoided >> if inheritance will be used for this metavariable. > > This is quite incorrect. I suggest to consider additional software design options. > Inheritance is only possible when a match of the previous rule has succeeded. I agree with this information. > If a rule never applies in a given file, the rules that inherit from it > won't apply either. I would like to point the possibility out to specify a source code search which will find interesting function calls at least by an inital SmPL rule. > Furthermore, what is inherited is the value, not the constraint. This technical detail can be fine. > If the original binding of alloc only ever matches kmalloc, > then the inherited references will only match kmalloc too. Can the desired search pattern be extended in significant ways? Regards, Markus
Re: [PATCH v3 12/16] treewide: Use array_size() for kmalloc()-family
> For kmalloc()-family allocations, instead of A * B, use array_size(). > Similarly, instead of A * B *C, use array3_size(). It took a while until my software development attention was caught also by this update suggestion. > Note that: > kmalloc(array_size(a, b), ...); > could be written as: > kmalloc_array(a, b, ...) > (and similarly, kzalloc(array_size(a, b), ...) as kcalloc(a, b, ...)) This is good to know, isn't it? > but this made the Coccinelle script WAY larger. Such a consequence is usual when the corresponding software development challenges grow. Are there further approaches to consider? > There is no desire to replace kmalloc_array() with kmalloc(array_size(...), > ...), > but given the number of changes made treewide, The number of changed source files can be impressive overall. > I opted for Coccinelle simplicity. I suggest to reconsider corresponding consequences. I find that an important aspect can be missing in this commit description then. How would you like to determine if the array size calculation (multiplication) should be performed together with an overflow check (or not)? How do you think about to express such a case distinction also in a bigger script for the semantic patch language? > This also tries to isolate sizeof() and constant factors, in an attempt > to regularize the argument ordering. This desire is reasonable. > Automatically generated using the following Coccinelle script: I have taken another look at implementation details. * My view might not matter for the generated changes from this run of a limited SmPL script. * My suggestions will influence the run time characteristics if such a source code transformation pattern will be executed again. > // 2-factor product with sizeof(variable) > @@ > identifier alloc =~ "kmalloc|kzalloc|kvmalloc|kvzalloc"; * This regular expression could be optimised to the specification “kv?[mz]alloc”. Extensions will be useful for further function names. * The repetition of such a constraint in subsequent SmPL rules could be avoided if inheritance will be used for this metavariable. > expression GFP, THING; > identifier COUNT; > @@ > > - alloc(sizeof(THING) * COUNT, GFP) > + alloc(array_size(COUNT, sizeof(THING)), GFP) More change items are specified here than what would be essentially necessary. * Function name * Second parameter This can be a design option to give the Coccinelle software the opportunity for additional source code formatting (pretty printing). These SmPL rules were designed in the way so far that they are independent from previous rules. This approach contains the risk that a metavariable type like “expression” can match more source code than it was expected. This technical detail matters for the selection of the replacement “array3_size”. The comments in the script indicate a desire for specific case distinctions. I have got the impression that the use of SmPL disjunctions will be more appropriate for the desired condition checks. A priority could be specified then for involved pattern evaluation. Would you like to adjust the transformation pattern any further? Regards, Markus
Re: [v3] [media] Use common error handling code in 19 functions
> @@ -656,18 +656,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
> tsfeed->priv = filter;
>
> ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>
> ret = tsfeed->start_filtering(tsfeed);
> - if (ret < 0) {
> - dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> - return ret;
> - }
> + if (ret < 0)
> + goto release_feed;
>
> return 0;
> +
> +release_feed:
> + dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
> + return ret;
> }
>
> There's *nothing* wrong with the above. It works fine,
I can agree to this view in principle according to the required control flow.
> avoids goto
How does this wording fit to information from the section
“7) Centralized exiting of functions” in the document “coding-style.rst”?
> and probably even produce the same code, as gcc will likely optimize it.
Would you like to clarify the current situation around supported
software optimisations any more?
> It is also easier to review, as the error handling is closer
> to the code.
Do we stumble on different coding style preferences once more?
> On the other hand, there's nothing wrong on taking the approach
> you're proposing.
Thanks for another bit of positive feedback.
> In the end, using goto or not on error handling like the above is
> a matter of personal taste - and taste changes with time
Do Linux guidelines need any adjustments?
> and with developer. I really don't have time to keep reviewing patches
> that are just churning the code just due to someone's personal taste.
I tried to apply another general source code transformation pattern.
> I'm pretty sure if I start accepting things like that,
> someone else would be on some future doing patches just reverting it,
> and I would be likely having to apply them too.
Why?
I hope also that the source code can be kept consistent to some degree.
> So, except if the patch is really fixing something - e.g. a broken
> error handling code, I'll just ignore such patches and mark as
> rejected without further notice/comments from now on.
I would find such a communication style questionable.
Do you distinguish between bug fixes and possible corrections for
other error categories (or software weaknesses)?
Regards,
Markus
Re: [v3] [media] Use common error handling code in 19 functions
> Adjust jump targets so that a bit of exception handling can be better > reused at the end of these functions. Why was this update suggestion rejected once more a moment ago? https://patchwork.linuxtv.org/patch/47827/ lkml.kernel.org/r/<57ef3a56-2578-1d5f-1268-348b49b0c...@users.sourceforge.net> https://lkml.org/lkml/2018/3/9/823 Would you like to integrate such a source code transformation after any further adjustments? Regards, Markus
Re: [0/9] UML vector network driver: Adjustments for three function implementations
> Date: Sun, 11 Mar 2018 16:06:16 +0100 > > Some update suggestions were taken into account > from static source code analysis. … > Delete unnecessary code in user_init_raw_fds() > Less checks in user_init_raw_fds() after error detection > Adjust an error message in user_init_socket_fds() > Delete an unnecessary check before kfree() in user_init_socket_fds() > Delete two unnecessary checks before freeaddrinfo() in > user_init_socket_fds() > Less checks in user_init_socket_fds() after error detection > Adjust an error message in user_init_tap_fds() > Less checks in user_init_tap_fds() after error detection > Delete an unnecessary variable initialisation in user_init_tap_fds() … Would you like to pick any idea up from this patch series? Regards, Markus
Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions
>> The mutex was (and is still only) locked within case branches, isn't it? >> > You are correct, this does however reflect the issue with the resulting > lack of balance here. Do you find changes for the other function implementations easier to integrate? Regards, Markus
Re: [2/4] scsi: hpsa: Less function calls in hpsa_big_passthru_ioctl() after error detection
>> @@ -6501,14 +6501,16 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info
>> *h, void __user *argp)
>> cleanup0:
>> cmd_free(h, c);
>> cleanup1:
>> - if (buff) {
>> + {
>> int i;
>>
>> for (i = 0; i < sg_used; i++)
>> kfree(buff[i]);
>> kfree(buff);
>> }
>
> Thanks for looking at the hpsa driver.
>
> This HUNK ends up with an unnamed block.
Which identifier would you like to use there?
> I would prefer to not have it structured like this.
Would you like to show a source code layout alternative?
Regards,
Markus
Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions
>> The mutex was (and is still only) locked within case branches, isn't it? >> > You are correct, this does however reflect the issue with the resulting > lack of balance here. I suggest to reconsider affected software aspects a bit more. > I saw the mutex was getting unlocked outside the local scope and so assumed > that it was also take outside the local scope. Assumptions and corresponding expectations might need further clarifications. > That isn't true, so we have hurt readability. Does your conclusion need any adjustment? > I read it quickly and got the wrong idea which generally implies it is not > as clear as we would like. > > Hence this change isn't going anywhere I'm afraid. I imagine that more time will be needed then to get used to additional adjustments of implementation details in these functions. Regards, Markus
Re: iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions
Am 17.03.2018 um 20:54 schrieb Jonathan Cameron: > On Wed, 14 Mar 2018 16:15:32 +0100 > SF Markus Elfring wrote: > >> From: Markus Elfring >> Date: Wed, 14 Mar 2018 16:06:49 +0100 >> >> * Add jump targets so that a call of the function "mutex_unlock" is stored >> only once in these function implementations. >> >> * Replace 19 calls by goto statements. >> >> This issue was detected by using the Coccinelle software. >> >> Signed-off-by: Markus Elfring > > Hi Markus, > > Some of these are good and sensible changes Such feedback is nice. > - others break the code. Which concrete places do you find questionable here? >> -return ret; >> + >> +goto set_power_state; >> default: >> return -EINVAL; > We exit with the mutex locked now and it should not be. I wonder about your source code interpretation here. The mutex was (and is still only) locked within case branches, isn't it? > >> } >> >> return -EINVAL; > Mutex is still locked here and the return is wrong. Should this statement get any more software development attention? Regards, Markus
[PATCH] cxgb4/cudbg_lib: Use common error handling code in cudbg_collect_tid()
From: Markus Elfring
Date: Thu, 15 Mar 2018 14:56:10 +0100
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c | 21 +++--
1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
index 9da6f57901a9..d5b5b6221d1f 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cudbg_lib.c
@@ -1660,11 +1660,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[0] = FW_PARAM_PFVF_A(ETHOFLD_START);
para[1] = FW_PARAM_PFVF_A(ETHOFLD_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2, para, val);
- if (rc < 0) {
- cudbg_err->sys_err = rc;
- cudbg_put_buff(pdbg_init, &temp_buff);
- return rc;
- }
+ if (rc < 0)
+ goto put_buffer;
+
tid->uotid_base = val[0];
tid->nuotids = val[1] - val[0] + 1;
@@ -1679,11 +1677,9 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
para[1] = FW_PARAM_PFVF_A(HPFILTER_END);
rc = t4_query_params(padap, padap->mbox, padap->pf, 0, 2,
para, val);
- if (rc < 0) {
- cudbg_err->sys_err = rc;
- cudbg_put_buff(pdbg_init, &temp_buff);
- return rc;
- }
+ if (rc < 0)
+ goto put_buffer;
+
tid->hpftid_base = val[0];
tid->nhpftids = val[1] - val[0] + 1;
}
@@ -1711,6 +1707,11 @@ int cudbg_collect_tid(struct cudbg_init *pdbg_init,
tid->ipv6_users = t4_read_reg(padap, LE_DB_ACT_CNT_IPV6_A);
return cudbg_write_and_release_buff(pdbg_init, &temp_buff, dbg_buff);
+
+put_buffer:
+ cudbg_put_buff(pdbg_init, &temp_buff);
+ cudbg_err->sys_err = rc;
+ return rc;
}
int cudbg_collect_pcie_config(struct cudbg_init *pdbg_init,
--
2.16.2
Re: [media] ov5645: Move an error code assignment in ov5645_probe()
>> Move an assignment for a specific error code so that it is stored only once
>> in this function implementation.
>>
>> This issue was detected by using the Coccinelle software.
>
> How?
Would you like to experiment a bit more with the following approach
for the semantic patch language?
show_same_statements3.cocci:
@duplicated_code@
identifier work;
statement s1, s2;
type T;
@@
T work(...)
{
... when any
*if ((...) < 0)
*{
...
* s1
* s2
*}
... when any
*if ((...) < 0)
*{
...
* s1
* s2
*}
... when any
}
>> @@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
>>
>> power_down:
>> ov5645_s_power(&ov5645->sd, false);
>> +ret = -ENODEV;
>
> I don't think this is where people would expect you to set the error code
> in general.
This can be. - The view depends on some factors.
> It should rather take place before goto, not after it.
I proposed another software design direction.
> That'd mean another variable,
To which detail do you refer here?
> and I'm not convinced the result would improve the driver.
Can you see the relevance of a small code reduction in this function?
Regards,
Markus
[PATCH] [media] ov5645: Move an error code assignment in ov5645_probe()
From: Markus Elfring
Date: Wed, 14 Mar 2018 22:02:52 +0100
Move an assignment for a specific error code so that it is stored only once
in this function implementation.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/media/i2c/ov5645.c | 6 +-
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index d28845f7356f..374576380fd4 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -1284,13 +1284,11 @@ static int ov5645_probe(struct i2c_client *client,
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_HIGH, &chip_id_high);
if (ret < 0 || chip_id_high != OV5645_CHIP_ID_HIGH_BYTE) {
dev_err(dev, "could not read ID high\n");
- ret = -ENODEV;
goto power_down;
}
ret = ov5645_read_reg(ov5645, OV5645_CHIP_ID_LOW, &chip_id_low);
if (ret < 0 || chip_id_low != OV5645_CHIP_ID_LOW_BYTE) {
dev_err(dev, "could not read ID low\n");
- ret = -ENODEV;
goto power_down;
}
@@ -1300,7 +1298,6 @@ static int ov5645_probe(struct i2c_client *client,
&ov5645->aec_pk_manual);
if (ret < 0) {
dev_err(dev, "could not read AEC/AGC mode\n");
- ret = -ENODEV;
goto power_down;
}
@@ -1308,7 +1305,6 @@ static int ov5645_probe(struct i2c_client *client,
&ov5645->timing_tc_reg20);
if (ret < 0) {
dev_err(dev, "could not read vflip value\n");
- ret = -ENODEV;
goto power_down;
}
@@ -1316,7 +1312,6 @@ static int ov5645_probe(struct i2c_client *client,
&ov5645->timing_tc_reg21);
if (ret < 0) {
dev_err(dev, "could not read hflip value\n");
- ret = -ENODEV;
goto power_down;
}
@@ -1334,6 +1329,7 @@ static int ov5645_probe(struct i2c_client *client,
power_down:
ov5645_s_power(&ov5645->sd, false);
+ ret = -ENODEV;
free_entity:
media_entity_cleanup(&ov5645->sd.entity);
free_ctrl:
--
2.16.2
[PATCH] iio/gyro/bmg160_core: Improve unlocking of a mutex in five functions
From: Markus Elfring
Date: Wed, 14 Mar 2018 16:06:49 +0100
* Add jump targets so that a call of the function "mutex_unlock" is stored
only once in these function implementations.
* Replace 19 calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/iio/gyro/bmg160_core.c | 103 ++---
1 file changed, 45 insertions(+), 58 deletions(-)
diff --git a/drivers/iio/gyro/bmg160_core.c b/drivers/iio/gyro/bmg160_core.c
index 63ca31628a93..fa367fd7bc8c 100644
--- a/drivers/iio/gyro/bmg160_core.c
+++ b/drivers/iio/gyro/bmg160_core.c
@@ -499,21 +499,19 @@ static int bmg160_get_temp(struct bmg160_data *data, int
*val)
mutex_lock(&data->mutex);
ret = bmg160_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
ret = regmap_read(data->regmap, BMG160_REG_TEMP, &raw_val);
if (ret < 0) {
dev_err(dev, "Error reading reg_temp\n");
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
*val = sign_extend32(raw_val, 7);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -529,22 +527,20 @@ static int bmg160_get_axis(struct bmg160_data *data, int
axis, int *val)
mutex_lock(&data->mutex);
ret = bmg160_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
ret = regmap_bulk_read(data->regmap, BMG160_AXIS_TO_REG(axis), &raw_val,
sizeof(raw_val));
if (ret < 0) {
dev_err(dev, "Error reading axis %d\n", axis);
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
*val = sign_extend32(le16_to_cpu(raw_val), 15);
ret = bmg160_set_power_state(data, false);
+unlock:
mutex_unlock(&data->mutex);
if (ret < 0)
return ret;
@@ -632,19 +628,16 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
* mode to power on for other writes.
*/
ret = bmg160_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
ret = bmg160_set_bw(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
- ret = bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+
+ goto set_power_state;
case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
if (val2)
return -EINVAL;
@@ -653,18 +646,15 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
ret = bmg160_set_power_state(data, true);
if (ret < 0) {
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
ret = bmg160_set_filter(data, val);
if (ret < 0) {
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
- ret = bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+
+ goto set_power_state;
case IIO_CHAN_INFO_SCALE:
if (val)
return -EINVAL;
@@ -672,24 +662,27 @@ static int bmg160_write_raw(struct iio_dev *indio_dev,
mutex_lock(&data->mutex);
/* Refer to comments above for the suspend mode ops */
ret = bmg160_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
ret = bmg160_set_scale(data, val2);
if (ret < 0) {
bmg160_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+
[PATCH] iio/adc/nau7802: Improve unlocking of a mutex in nau7802_read_raw()
From: Markus Elfring
Date: Tue, 13 Mar 2018 20:52:26 +0100
* Add a jump target so that a call of the function "mutex_unlock" is stored
only once in this function implementation.
* Replace two calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/iio/adc/nau7802.c | 15 +--
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/iio/adc/nau7802.c b/drivers/iio/adc/nau7802.c
index 8997e74a8847..68d06a492760 100644
--- a/drivers/iio/adc/nau7802.c
+++ b/drivers/iio/adc/nau7802.c
@@ -303,10 +303,8 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
* - Channel 2 is value 1 in the CHS register
*/
ret = i2c_smbus_read_byte_data(st->client, NAU7802_REG_CTRL2);
- if (ret < 0) {
- mutex_unlock(&st->lock);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
if (((ret & NAU7802_CTRL2_CHS_BIT) && !chan->channel) ||
(!(ret & NAU7802_CTRL2_CHS_BIT) &&
@@ -316,18 +314,15 @@ static int nau7802_read_raw(struct iio_dev *indio_dev,
NAU7802_REG_CTRL2,
NAU7802_CTRL2_CHS(chan->channel) |
NAU7802_CTRL2_CRS(st->sample_rate));
-
- if (ret < 0) {
- mutex_unlock(&st->lock);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
}
if (st->client->irq)
ret = nau7802_read_irq(indio_dev, chan, val);
else
ret = nau7802_read_poll(indio_dev, chan, val);
-
+unlock:
mutex_unlock(&st->lock);
return ret;
--
2.16.2
[PATCH] iio/adc/ad7291: Improve unlocking of a mutex in ad7291_read_raw()
From: Markus Elfring
Date: Tue, 13 Mar 2018 20:08:40 +0100
* Add a jump target so that a call of the function "mutex_unlock" is stored
only once in this function implementation.
* Replace three calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/iio/adc/ad7291.c | 22 +++---
1 file changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/iio/adc/ad7291.c b/drivers/iio/adc/ad7291.c
index a862b5d8fb4b..b2c3d4c79909 100644
--- a/drivers/iio/adc/ad7291.c
+++ b/drivers/iio/adc/ad7291.c
@@ -335,27 +335,27 @@ static int ad7291_read_raw(struct iio_dev *indio_dev,
mutex_lock(&chip->state_lock);
/* If in autocycle mode drop through */
if (chip->command & AD7291_AUTOCYCLE) {
- mutex_unlock(&chip->state_lock);
- return -EBUSY;
+ ret = -EBUSY;
+ goto unlock;
}
/* Enable this channel alone */
regval = chip->command & (~AD7291_VOLTAGE_MASK);
regval |= BIT(15 - chan->channel);
ret = ad7291_i2c_write(chip, AD7291_COMMAND, regval);
- if (ret < 0) {
- mutex_unlock(&chip->state_lock);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
/* Read voltage */
ret = i2c_smbus_read_word_swapped(chip->client,
AD7291_VOLTAGE);
- if (ret < 0) {
- mutex_unlock(&chip->state_lock);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
*val = ret & AD7291_VALUE_MASK;
+ ret = IIO_VAL_INT;
+unlock:
mutex_unlock(&chip->state_lock);
- return IIO_VAL_INT;
+ return ret;
case IIO_TEMP:
/* Assumes tsense bit of command register always set */
ret = i2c_smbus_read_word_swapped(chip->client,
--
2.16.2
[PATCH] iio/accel/kxcjk-1013: Improve unlocking of a mutex in three functions
From: Markus Elfring
Date: Tue, 13 Mar 2018 13:40:12 +0100
* Add jump targets so that a call of the function "mutex_unlock" is stored
less often in these function implementations.
* Replace eight calls by goto statements.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/iio/accel/kxcjk-1013.c | 46 ++
1 file changed, 20 insertions(+), 26 deletions(-)
diff --git a/drivers/iio/accel/kxcjk-1013.c b/drivers/iio/accel/kxcjk-1013.c
index af53a1084ee5..4adf38b6d939 100644
--- a/drivers/iio/accel/kxcjk-1013.c
+++ b/drivers/iio/accel/kxcjk-1013.c
@@ -765,19 +765,18 @@ static int kxcjk1013_read_raw(struct iio_dev *indio_dev,
ret = -EBUSY;
else {
ret = kxcjk1013_set_power_state(data, true);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
ret = kxcjk1013_get_acc_reg(data, chan->scan_index);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
*val = sign_extend32(ret >> 4, 11);
ret = kxcjk1013_set_power_state(data, false);
}
+unlock:
mutex_unlock(&data->mutex);
if (ret < 0)
@@ -905,8 +904,8 @@ static int kxcjk1013_write_event_config(struct iio_dev
*indio_dev,
if (!state && data->motion_trigger_on) {
data->ev_enable_state = 0;
- mutex_unlock(&data->mutex);
- return 0;
+ ret = 0;
+ goto unlock;
}
/*
@@ -919,23 +918,20 @@ static int kxcjk1013_write_event_config(struct iio_dev
*indio_dev,
* is always on so sequence doesn't matter
*/
ret = kxcjk1013_set_power_state(data, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
ret = kxcjk1013_setup_any_motion_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
data->ev_enable_state = 0;
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
data->ev_enable_state = state;
+unlock:
mutex_unlock(&data->mutex);
-
- return 0;
+ return ret;
}
static int kxcjk1013_buffer_preenable(struct iio_dev *indio_dev)
@@ -1086,32 +1082,30 @@ static int kxcjk1013_data_rdy_trigger_set_state(struct
iio_trigger *trig,
if (!state && data->ev_enable_state && data->motion_trigger_on) {
data->motion_trigger_on = false;
- mutex_unlock(&data->mutex);
- return 0;
+ ret = 0;
+ goto unlock;
}
ret = kxcjk1013_set_power_state(data, state);
- if (ret < 0) {
- mutex_unlock(&data->mutex);
- return ret;
- }
+ if (ret < 0)
+ goto unlock;
+
if (data->motion_trig == trig)
ret = kxcjk1013_setup_any_motion_interrupt(data, state);
else
ret = kxcjk1013_setup_new_data_interrupt(data, state);
if (ret < 0) {
kxcjk1013_set_power_state(data, false);
- mutex_unlock(&data->mutex);
- return ret;
+ goto unlock;
}
if (data->motion_trig == trig)
data->motion_trigger_on = state;
else
data->dready_trigger_on = state;
+unlock:
mutex_unlock(&data->mutex);
-
- return 0;
+ return ret;
}
static const struct iio_trigger_ops kxcjk1013_trigger_ops = {
--
2.16.2
Re: hwmon/sch5627: Use common error handling code in sch5627_probe()
> 1 file changed, 29 insertions(+), 31 deletions(-) > > So you are asking people to review 60 changed lines to save 2, A bit of object code reduction might become useful also in this case. > that alone should be the point where you stop yourself from > *even* sending this patch. I proposed just another collateral evolution. > Next time before you send a patch please carefully think if the > saving is worth the combination of reviewers time + the risk of > regressions (and keep in mind that both the reviewers time and > the risk of regressions cost increase for more complex changes). Source code transformations were integrated in other software areas according to such a change pattern. > As for this specific discussion, there are certain "design-patterns" > in the kernel, goto style error handling is one of them, the pattern > there ALWAYS is: … > Notice the fall-thoughs those are ALWAYS there, never, ever is > there a goto after a cleanup label. It seems that I present an unusual update suggestion as a software design variant. > Your patches black goto magic completely messes this up You can view the proposal in such a way. > and clearly falls under the CS101 rule: never use goto. There might a target conflict with information from the section “7) Centralized exiting of functions” in the document “coding-style.rst”. Regards, Markus
Re: hwmon/sch5627: Use common error handling code in sch5627_probe()
>> Adjust jump targets so that a bit of exception handling can be better >> reused at the end of this function. … > goto-s going to a label calling another goto is completely unreadable. I got an other software development view. > I really do not see any reason for the proposed changes, I suggest to look once more. > they may remove a small amount of code duplication, This was my software design goal in this case. > but at a hugh cost wrt readability. I proposed a different trade-off here. Regards, Markus
Re: [PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()
>> Add a jump target so that the setting of a specific error code is stored
>> only once at the end of this function.
>>
>> Signed-off-by: Markus Elfring
>> ---
>> drivers/bluetooth/btmrvl_sdio.c | 13 +++--
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/bluetooth/btmrvl_sdio.c
>> b/drivers/bluetooth/btmrvl_sdio.c
>> index 05c78fcc13ff..24ed62fe2aeb 100644
>> --- a/drivers/bluetooth/btmrvl_sdio.c
>> +++ b/drivers/bluetooth/btmrvl_sdio.c
>> @@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
>> btmrvl_sdio_card *card)
>> " base0 = 0x%04X(%d)."
>> " Terminating download",
>> base0, base0);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>> }
>> base1 = sdio_readb(card->func,
>> card->reg->sq_read_base_addr_a1, &ret);
>> @@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
>> btmrvl_sdio_card *card)
>> " base1 = 0x%04X(%d)."
>> " Terminating download",
>> base1, base1);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>> }
>>
>> len = (((u16) base1) << 8) | base0;
>> @@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
>> btmrvl_sdio_card *card)
>> if (count > MAX_WRITE_IOMEM_RETRY) {
>> BT_ERR("FW download failure @%d, "
>> "over max retry count", offset);
>> -ret = -EIO;
>> -goto done;
>> +goto e_io;
>> }
>> BT_ERR("FW CRC error indicated by the helper: "
>> "len = 0x%04X, txlen = %d", len, txlen);
>> @@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct
>> btmrvl_sdio_card *card)
>> kfree(tmpfwbuf);
>> release_firmware(fw_firmware);
>> return ret;
>> +
>> +e_io:
>> +ret = -EIO;
>> +goto done;
>> }
>
> I am not applying this one. I see zero benefit in this change.
Would you care for a bit of object code reduction in this function
implementation.
> It is not even saving a single line since it actually is more code.
Should I fiddle with any other source code layout?
Do you find an extra blank line inappropriate before the added jump target
in this use case?
Regards,
Markus
Re: [3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host()
>> @@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct
>> btmrvl_private *priv)
>> break;
>> }
>>
>> -exit:
>> -if (ret) {
>> -hdev->stat.err_rx++;
>> -kfree_skb(skb);
>> -}
>> +return 0;
>> +
>> +free_skb:
>> +kfree_skb(skb);
>> +e_io:
>> +ret = -EIO;
>> +goto increment_counter;
>>
>> +e_inval:
>> +ret = -EINVAL;
>> +increment_counter:
>> +hdev->stat.err_rx++;
>> return ret;
>
> Nope!
>
> This is not easier to read for me. This goto exit jumping and I hate that.
Can the software design direction become feasible to omit the repeated check
for the variable “ret” (and further initialisations)?
Regards,
Markus
Re: [2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()
>> Use three values directly for a condition check without assigning them >> to intermediate variables. > > Hi, > > what is the benefit of this? I proposed a small source code reduction. Other software design directions might become more interesting for this use case. Regards, Markus
Re: [v2 14/17 4/4] mfd: tps65910: Checking patch structures
> How have you managed to insert 4 patches into the x/17 thread? I dared to group the desired patch series into dedicated mail threads. Regards, Markus
Re: [2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection
> +set_error_code: > + err = -errno; > + os_close_file(rxfd); I have taken another look at this change idea. Now I notice that I should have preserved a sanity check there. if (rxfd >= 0) os_close_file(rxfd); Regards, Markus
[PATCH] hwmon/sch5627: Use common error handling code in sch5627_probe()
From: Markus Elfring
Date: Mon, 12 Mar 2018 22:15:59 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/hwmon/sch5627.c | 60 -
1 file changed, 29 insertions(+), 31 deletions(-)
diff --git a/drivers/hwmon/sch5627.c b/drivers/hwmon/sch5627.c
index 91544f2312e6..e7a2a974ab74 100644
--- a/drivers/hwmon/sch5627.c
+++ b/drivers/hwmon/sch5627.c
@@ -480,72 +480,64 @@ static int sch5627_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_HWMON_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_HWMON_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "hwmon",
val, SCH5627_HWMON_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_COMPANY_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_COMPANY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "company",
val, SCH5627_COMPANY_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_PRIMARY_ID);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
if (val != SCH5627_PRIMARY_ID) {
pr_err("invalid %s id: 0x%02X (expected 0x%02X)\n", "primary",
val, SCH5627_PRIMARY_ID);
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
build_code = sch56xx_read_virtual_reg(data->addr,
SCH5627_REG_BUILD_CODE);
if (build_code < 0) {
err = build_code;
- goto error;
+ goto remove_device;
}
build_id = sch56xx_read_virtual_reg16(data->addr,
SCH5627_REG_BUILD_ID);
if (build_id < 0) {
err = build_id;
- goto error;
+ goto remove_device;
}
hwmon_rev = sch56xx_read_virtual_reg(data->addr,
SCH5627_REG_HWMON_REV);
if (hwmon_rev < 0) {
err = hwmon_rev;
- goto error;
+ goto remove_device;
}
val = sch56xx_read_virtual_reg(data->addr, SCH5627_REG_CTRL);
- if (val < 0) {
- err = val;
- goto error;
- }
+ if (val < 0)
+ goto set_error_code;
+
data->control = val;
if (!(data->control & 0x01)) {
pr_err("hardware monitoring not enabled\n");
- err = -ENODEV;
- goto error;
+ goto e_nodev;
}
/* Trigger a Vbat voltage measurement, so that we get a valid reading
the first time we read Vbat */
@@ -559,7 +551,7 @@ static int sch5627_probe(struct platform_device *pdev)
*/
err = sch5627_read_limits(data);
if (err)
- goto error;
+ goto remove_device;
pr_info("found %s chip at %#hx\n", DEVNAME, data->addr);
pr_info("firmware build: code 0x%02X, id 0x%04X, hwmon: rev 0x%02X\n",
@@ -568,13 +560,13 @@ static int sch5627_probe(struct platform_device *pdev)
/* Register sysfs interface files */
err = sysfs_create_group(&pdev->dev.kobj, &sch5627_group);
if (err)
- goto error;
+ goto remove_device;
data->hwmon_dev = hwmon_device_register(&pdev->dev);
if (IS_ERR(data->hwmon_dev)) {
err = PTR_ERR(data->hwmon_dev);
data->hwmon_dev = NULL;
- goto error;
+ goto remove_device;
}
/* Note failing to register the watchdog is not a fatal error */
@@ -584,7 +576,13 @@ static int sch5627_probe(struct platform_device *pdev)
return 0;
-error:
+set_error_code:
+ err = val;
+ goto remove_device;
+
+e_nodev:
+ err = -ENODEV;
+remove_device:
sch5627_remove(pdev);
return err;
}
--
2.16.2
[PATCH] altera_edac: Use common error handling code in altr_sdram_probe()
From: Markus Elfring
Date: Mon, 12 Mar 2018 16:23:53 +0100
Add a jump target so that a specific error code is assigned to the
local variable "res" at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/edac/altera_edac.c | 11 +--
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 11d6419788c2..d5c15b27d520 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -419,8 +419,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
"Unable to request irq %d\n", irq2);
- res = -ENODEV;
- goto err2;
+ goto e_nodev;
}
res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
@@ -435,8 +434,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res < 0) {
edac_mc_printk(mci, KERN_ERR,
"Unable to request irq %d\n", irq);
- res = -ENODEV;
- goto err2;
+ goto e_nodev;
}
/* Infrastructure ready - enable the IRQ */
@@ -444,8 +442,7 @@ static int altr_sdram_probe(struct platform_device *pdev)
priv->ecc_irq_en_mask, priv->ecc_irq_en_mask)) {
edac_mc_printk(mci, KERN_ERR,
"Error enabling SDRAM ECC IRQ\n");
- res = -ENODEV;
- goto err2;
+ goto e_nodev;
}
altr_sdr_mc_create_debugfs_nodes(mci);
@@ -454,6 +451,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
return 0;
+e_nodev:
+ res = -ENODEV;
err2:
edac_mc_del_mc(&pdev->dev);
err:
--
2.16.2
Re: dmaengine: edma: Use common error handling code in three functions
> Date: Sun, 22 Oct 2017 16:46:34 +0200 > > Add a jump target so that a bit of exception handling can be better reused > at the end of these functions. How are the chances to integrate such a change into another Linux repository? https://lkml.org/lkml/2017/10/22/78 https://patchwork.kernel.org/patch/10021725/ https://lkml.kernel.org/r/ Regards, Markus
[PATCH 2/2] crypto: talitos: Delete an error message for a failed memory allocation in talitos_edesc_alloc()
From: Markus Elfring
Date: Mon, 12 Mar 2018 14:18:23 +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/crypto/talitos.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index a2271322db34..4c7318981d28 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1399,7 +1399,6 @@ static struct talitos_edesc *talitos_edesc_alloc(struct
device *dev,
edesc = kmalloc(alloc_len, GFP_DMA | flags);
if (!edesc) {
- dev_err(dev, "could not allocate edescriptor\n");
err = ERR_PTR(-ENOMEM);
goto error_sg;
}
--
2.16.2
[PATCH 1/2] crypto: talitos: Use common error handling code in talitos_edesc_alloc()
From: Markus Elfring
Date: Mon, 12 Mar 2018 14:08:55 +0100
Add jump targets so that an error message and the setting of a specific
error code is stored only once at the end of this function.
Signed-off-by: Markus Elfring
---
drivers/crypto/talitos.c | 24
1 file changed, 12 insertions(+), 12 deletions(-)
diff --git a/drivers/crypto/talitos.c b/drivers/crypto/talitos.c
index 6882fa2f8bad..a2271322db34 100644
--- a/drivers/crypto/talitos.c
+++ b/drivers/crypto/talitos.c
@@ -1352,29 +1352,24 @@ static struct talitos_edesc *talitos_edesc_alloc(struct
device *dev,
if (!dst || dst == src) {
src_len = assoclen + cryptlen + authsize;
src_nents = sg_nents_for_len(src, src_len);
- if (src_nents < 0) {
- dev_err(dev, "Invalid number of src SG.\n");
- err = ERR_PTR(-EINVAL);
- goto error_sg;
- }
+ if (src_nents < 0)
+ goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_nents = dst ? src_nents : 0;
dst_len = 0;
} else { /* dst && dst != src*/
src_len = assoclen + cryptlen + (encrypt ? 0 : authsize);
src_nents = sg_nents_for_len(src, src_len);
- if (src_nents < 0) {
- dev_err(dev, "Invalid number of src SG.\n");
- err = ERR_PTR(-EINVAL);
- goto error_sg;
- }
+ if (src_nents < 0)
+ goto report_failure;
+
src_nents = (src_nents == 1) ? 0 : src_nents;
dst_len = assoclen + cryptlen + (encrypt ? authsize : 0);
dst_nents = sg_nents_for_len(dst, dst_len);
if (dst_nents < 0) {
dev_err(dev, "Invalid number of dst SG.\n");
- err = ERR_PTR(-EINVAL);
- goto error_sg;
+ goto set_error_code;
}
dst_nents = (dst_nents == 1) ? 0 : dst_nents;
}
@@ -1424,6 +1419,11 @@ static struct talitos_edesc *talitos_edesc_alloc(struct
device *dev,
DMA_BIDIRECTIONAL);
}
return edesc;
+
+report_failure:
+ dev_err(dev, "Invalid number of src SG.\n");
+set_error_code:
+ err = ERR_PTR(-EINVAL);
error_sg:
if (iv_dma)
dma_unmap_single(dev, iv_dma, ivsize, DMA_TO_DEVICE);
--
2.16.2
[PATCH 0/2] crypto/talitos: Adjustments for talitos_edesc_alloc()
From: Markus Elfring Date: Mon, 12 Mar 2018 14:24:34 +0100 Two update suggestions were taken into account from static source code analysis. Markus Elfring (2): Use common error handling code Delete an error message for a failed memory allocation drivers/crypto/talitos.c | 25 - 1 file changed, 12 insertions(+), 13 deletions(-) -- 2.16.2
[PATCH 5/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_download_fw_w_helper()
From: Markus Elfring
Date: Mon, 12 Mar 2018 11:30:28 +0100
Add a jump target so that the setting of a specific error code is stored
only once at the end of this function.
Signed-off-by: Markus Elfring
---
drivers/bluetooth/btmrvl_sdio.c | 13 +++--
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 05c78fcc13ff..24ed62fe2aeb 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -601,8 +601,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
btmrvl_sdio_card *card)
" base0 = 0x%04X(%d)."
" Terminating download",
base0, base0);
- ret = -EIO;
- goto done;
+ goto e_io;
}
base1 = sdio_readb(card->func,
card->reg->sq_read_base_addr_a1, &ret);
@@ -611,8 +610,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
btmrvl_sdio_card *card)
" base1 = 0x%04X(%d)."
" Terminating download",
base1, base1);
- ret = -EIO;
- goto done;
+ goto e_io;
}
len = (((u16) base1) << 8) | base0;
@@ -638,8 +636,7 @@ static int btmrvl_sdio_download_fw_w_helper(struct
btmrvl_sdio_card *card)
if (count > MAX_WRITE_IOMEM_RETRY) {
BT_ERR("FW download failure @%d, "
"over max retry count", offset);
- ret = -EIO;
- goto done;
+ goto e_io;
}
BT_ERR("FW CRC error indicated by the helper: "
"len = 0x%04X, txlen = %d", len, txlen);
@@ -681,6 +678,10 @@ static int btmrvl_sdio_download_fw_w_helper(struct
btmrvl_sdio_card *card)
kfree(tmpfwbuf);
release_firmware(fw_firmware);
return ret;
+
+e_io:
+ ret = -EIO;
+ goto done;
}
static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv)
--
2.16.2
[PATCH 4/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host()
From: Markus Elfring Date: Mon, 12 Mar 2018 11:15:59 +0100 The variable "payload" will eventually be set to an appropriate pointer a bit later. Thus omit the explicit initialisation at the beginning. Signed-off-by: Markus Elfring --- drivers/bluetooth/btmrvl_sdio.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c index 9854addc8e96..05c78fcc13ff 100644 --- a/drivers/bluetooth/btmrvl_sdio.c +++ b/drivers/bluetooth/btmrvl_sdio.c @@ -691,5 +691,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private *priv) u32 type; - u8 *payload = NULL; + u8 *payload; struct hci_dev *hdev = priv->btmrvl_dev.hcidev; struct btmrvl_sdio_card *card = priv->btmrvl_dev.card; -- 2.16.2
[PATCH 3/5] Bluetooth: btmrvl: One check less in btmrvl_sdio_card_to_host() after error detection
From: Markus Elfring
Date: Mon, 12 Mar 2018 11:13:00 +0100
One check could be repeated by the btmrvl_sdio_card_to_host() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets so that an extra check can be omitted at the end.
* Reuse a bit of exception handling better.
* Delete an initialisation for the local variable "skb"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
drivers/bluetooth/btmrvl_sdio.c | 35 ++-
1 file changed, 18 insertions(+), 17 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 84c222abf0f7..9854addc8e96 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -687,5 +687,5 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
{
u16 buf_len = 0;
int ret, num_blocks, blksz;
- struct sk_buff *skb = NULL;
+ struct sk_buff *skb;
u32 type;
@@ -695,16 +695,14 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
if (!card || !card->func) {
BT_ERR("card or function is NULL!");
- ret = -EINVAL;
- goto exit;
+ goto e_inval;
}
/* Read the length of data to be transferred */
ret = btmrvl_sdio_read_rx_len(card, &buf_len);
if (ret < 0) {
BT_ERR("read rx_len failed");
- ret = -EIO;
- goto exit;
+ goto e_io;
}
blksz = SDIO_BLOCK_SIZE;
@@ -713,8 +711,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
if (buf_len <= SDIO_HEADER_LEN
|| (num_blocks * blksz) > ALLOC_BUF_SIZE) {
BT_ERR("invalid packet length: %d", buf_len);
- ret = -EINVAL;
- goto exit;
+ goto e_inval;
}
/* Allocate buffer */
@@ -722,7 +719,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
if (!skb) {
BT_ERR("No free skb");
ret = -ENOMEM;
- goto exit;
+ goto increment_counter;
}
if ((unsigned long) skb->data & (BTSDIO_DMA_ALIGN - 1)) {
@@ -738,8 +735,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
num_blocks * blksz);
if (ret < 0) {
BT_ERR("readsb failed: %d", ret);
- ret = -EIO;
- goto exit;
+ goto free_skb;
}
/* This is SDIO specific header length: byte[2][1][0], type: byte[3]
@@ -753,8 +749,7 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
if (buf_len > blksz * num_blocks) {
BT_ERR("Skip incorrect packet: hdrlen %d buffer %d",
buf_len, blksz * num_blocks);
- ret = -EIO;
- goto exit;
+ goto free_skb;
}
type = payload[3];
@@ -797,12 +792,18 @@ static int btmrvl_sdio_card_to_host(struct btmrvl_private
*priv)
break;
}
-exit:
- if (ret) {
- hdev->stat.err_rx++;
- kfree_skb(skb);
- }
+ return 0;
+
+free_skb:
+ kfree_skb(skb);
+e_io:
+ ret = -EIO;
+ goto increment_counter;
+e_inval:
+ ret = -EINVAL;
+increment_counter:
+ hdev->stat.err_rx++;
return ret;
}
--
2.16.2
[PATCH 2/5] Bluetooth: btmrvl: Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev()
From: Markus Elfring
Date: Mon, 12 Mar 2018 10:20:04 +0100
The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/bluetooth/btmrvl_sdio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index df2a04bd8428..84c222abf0f7 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -920,7 +920,7 @@ static int btmrvl_sdio_register_dev(struct btmrvl_sdio_card
*card)
{
struct sdio_func *func;
u8 reg;
- int ret = 0;
+ int ret;
if (!card || !card->func) {
BT_ERR("Error: card or function is NULL!");
--
2.16.2
[PATCH 1/5] Bluetooth: btmrvl: Use common error handling code in btmrvl_sdio_register_dev()
From: Markus Elfring
Date: Mon, 12 Mar 2018 10:15:17 +0100
Adjust a jump target so that a bit of exception handling can be better
reused at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/bluetooth/btmrvl_sdio.c | 50 -
1 file changed, 19 insertions(+), 31 deletions(-)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 0f020254fd39..df2a04bd8428 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -949,31 +949,24 @@ static int btmrvl_sdio_register_dev(struct
btmrvl_sdio_card *card)
ret = sdio_set_block_size(card->func, SDIO_BLOCK_SIZE);
if (ret) {
BT_ERR("cannot set SDIO block size");
- ret = -EIO;
- goto release_irq;
+ goto release_with_eio;
}
reg = sdio_readb(func, card->reg->io_port_0, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
card->ioport = reg;
reg = sdio_readb(func, card->reg->io_port_1, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
card->ioport |= (reg << 8);
reg = sdio_readb(func, card->reg->io_port_2, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
card->ioport |= (reg << 16);
@@ -981,26 +974,20 @@ static int btmrvl_sdio_register_dev(struct
btmrvl_sdio_card *card)
if (card->reg->int_read_to_clear) {
reg = sdio_readb(func, card->reg->host_int_rsr, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
+
sdio_writeb(func, reg | 0x3f, card->reg->host_int_rsr, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
reg = sdio_readb(func, card->reg->card_misc_cfg, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
+
sdio_writeb(func, reg | 0x10, card->reg->card_misc_cfg, &ret);
- if (ret < 0) {
- ret = -EIO;
- goto release_irq;
- }
+ if (ret < 0)
+ goto release_with_eio;
}
sdio_set_drvdata(func, card);
@@ -1009,7 +996,8 @@ static int btmrvl_sdio_register_dev(struct
btmrvl_sdio_card *card)
return 0;
-release_irq:
+release_with_eio:
+ ret = -EIO;
sdio_release_irq(func);
disable_func:
--
2.16.2
[PATCH 0/5] Bluetooth/btmrvl_sdio: Adjustments for three function implementations
From: Markus Elfring Date: Mon, 12 Mar 2018 12:10:24 +0100 A few update suggestions were taken into account from static source code analysis. Markus Elfring (5): Use common error handling code in btmrvl_sdio_register_dev() Delete an unnecessary variable initialisation in btmrvl_sdio_register_dev() One check less in btmrvl_sdio_card_to_host() after error detection Delete an unnecessary variable initialisation in btmrvl_sdio_card_to_host() Use common error handling code in btmrvl_sdio_download_fw_w_helper() drivers/bluetooth/btmrvl_sdio.c | 102 ++-- 1 file changed, 46 insertions(+), 56 deletions(-) -- 2.16.2
[PATCH 9/9] um/drivers/vector_user: Delete an unnecessary variable initialisation in user_init_tap_fds()
From: Markus Elfring
Date: Sun, 11 Mar 2018 15:50:29 +0100
The local variable "fd" will eventually be set to an appropriate value
a bit later. Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index bd625034a0f0..3bd510eded58 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -119,9 +119,8 @@ struct arglist *uml_parse_vector_ifspec(char *arg)
static struct vector_fds *user_init_tap_fds(struct arglist *ifspec)
{
struct ifreq ifr;
- int fd = -1;
struct sockaddr_ll sock;
- int err = -ENOMEM, offload;
+ int err = -ENOMEM, fd, offload;
char *iface;
struct vector_fds *result;
--
2.16.2
[PATCH 8/9] um/drivers/vector_user: Less checks in user_init_tap_fds() after error detection
From: Markus Elfring
Date: Sun, 11 Mar 2018 15:43:31 +0100
Three checks could be repeated by the user_init_tap_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete three sanity checks and an initialisation (for the local
variable "result") which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 31 +++
1 file changed, 15 insertions(+), 16 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 5e78723c34d4..bd625034a0f0 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -123,18 +123,18 @@ static struct vector_fds *user_init_tap_fds(struct
arglist *ifspec)
struct sockaddr_ll sock;
int err = -ENOMEM, offload;
char *iface;
- struct vector_fds *result = NULL;
+ struct vector_fds *result;
iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
if (iface == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to parse interface spec\n");
- goto tap_cleanup;
+ goto report_failure;
}
result = uml_kmalloc(sizeof(struct vector_fds), UM_GFP_KERNEL);
if (result == NULL) {
printk(UM_KERN_ERR "uml_tap: failed to allocate file
descriptors\n");
- goto tap_cleanup;
+ goto report_failure;
}
result->rx_fd = -1;
result->tx_fd = -1;
@@ -146,7 +146,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
fd = open(PATH_NET_TUN, O_RDWR);
if (fd < 0) {
printk(UM_KERN_ERR "uml_tap: failed to open tun device\n");
- goto tap_cleanup;
+ goto free_result;
}
result->tx_fd = fd;
memset(&ifr, 0, sizeof(ifr));
@@ -156,7 +156,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
err = ioctl(fd, TUNSETIFF, (void *) &ifr);
if (err != 0) {
printk(UM_KERN_ERR "uml_tap: failed to select tap interface\n");
- goto tap_cleanup;
+ goto close_tx_fd;
}
offload = TUN_F_CSUM | TUN_F_TSO4 | TUN_F_TSO6;
@@ -168,7 +168,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
if (fd == -1) {
printk(UM_KERN_ERR
"uml_tap: failed to create socket: %i\n", -errno);
- goto tap_cleanup;
+ goto close_tx_fd;
}
result->rx_fd = fd;
memset(&ifr, 0, sizeof(ifr));
@@ -176,7 +176,7 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
if (ioctl(fd, SIOCGIFINDEX, (void *) &ifr) < 0) {
printk(UM_KERN_ERR
"uml_tap: failed to set interface: %i\n", -errno);
- goto tap_cleanup;
+ goto close_rx_fd;
}
sock.sll_family = AF_PACKET;
@@ -188,18 +188,17 @@ static struct vector_fds *user_init_tap_fds(struct
arglist *ifspec)
printk(UM_KERN_ERR
"user_init_tap: failed to bind raw pair, err %d\n",
-errno);
- goto tap_cleanup;
+ goto close_rx_fd;
}
return result;
-tap_cleanup:
- if (result != NULL) {
- if (result->rx_fd >= 0)
- os_close_file(result->rx_fd);
- if (result->tx_fd >= 0)
- os_close_file(result->tx_fd);
- kfree(result);
- }
+close_rx_fd:
+ os_close_file(result->rx_fd);
+close_tx_fd:
+ os_close_file(result->tx_fd);
+free_result:
+ kfree(result);
+report_failure:
printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
[PATCH 7/9] um/drivers/vector_user: Adjust an error message in user_init_tap_fds()
From: Markus Elfring
Date: Sun, 11 Mar 2018 15:10:05 +0100
Adjust an error message at the end of this function so that its name
will be automatically determined as a parameter.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 4c265262a369..5e78723c34d4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -192,7 +192,6 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
}
return result;
tap_cleanup:
- printk(UM_KERN_ERR "user_init_tap: init failed, error %d", err);
if (result != NULL) {
if (result->rx_fd >= 0)
os_close_file(result->rx_fd);
@@ -200,6 +199,8 @@ static struct vector_fds *user_init_tap_fds(struct arglist
*ifspec)
os_close_file(result->tx_fd);
kfree(result);
}
+
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
[PATCH 6/9] um/drivers/vector_user: Less checks in user_init_socket_fds() after error detection
From: Markus Elfring
Date: Sun, 11 Mar 2018 14:56:38 +0100
Two checks could be repeated by the user_init_socket_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets.
* Delete two sanity checks and a call of the function "kfree"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 22 ++
1 file changed, 10 insertions(+), 12 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 2dee1e183387..4c265262a369 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -375,13 +375,13 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
"socket_open : could not open socket, error = %d",
-errno
);
- goto cleanup;
+ goto free_info;
}
if (bind(fd,
(struct sockaddr *) gairesult->ai_addr,
gairesult->ai_addrlen)) {
printk(UM_KERN_ERR L2TPV3_BIND_FAIL, errno);
- goto cleanup;
+ goto close_file;
}
freeaddrinfo(gairesult);
@@ -403,7 +403,8 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
result->remote_addr = uml_kmalloc(
gairesult->ai_addrlen, UM_GFP_KERNEL);
if (result->remote_addr == NULL)
- goto cleanup;
+ goto free_result;
+
result->remote_addr_size = gairesult->ai_addrlen;
memcpy(
result->remote_addr,
@@ -413,16 +414,13 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
}
freeaddrinfo(gairesult);
return result;
-cleanup:
- freeaddrinfo(gairesult);
-
- if (fd >= 0)
- os_close_file(fd);
- if (result != NULL) {
- kfree(result->remote_addr);
- kfree(result);
- }
+free_result:
+ kfree(result);
+close_file:
+ os_close_file(fd);
+free_info:
+ freeaddrinfo(gairesult);
printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
}
--
2.16.2
[PATCH 5/9] um/drivers/vector_user: Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds()
From: Markus Elfring Date: Sun, 11 Mar 2018 14:20:46 +0100 The implementation returns from this function if a null pointer was detected in the local variable "gairesult". Thus the check before two calls of the function "freeaddrinfo" is not needed. Signed-off-by: Markus Elfring --- arch/um/drivers/vector_user.c | 7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c index e831bd85cad4..2dee1e183387 100644 --- a/arch/um/drivers/vector_user.c +++ b/arch/um/drivers/vector_user.c @@ -384,9 +384,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) goto cleanup; } - if (gairesult != NULL) - freeaddrinfo(gairesult); - + freeaddrinfo(gairesult); gairesult = NULL; gairet = getaddrinfo(dst, dstport, &dsthints, &gairesult); @@ -416,8 +414,7 @@ static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id) freeaddrinfo(gairesult); return result; cleanup: - if (gairesult != NULL) - freeaddrinfo(gairesult); + freeaddrinfo(gairesult); if (fd >= 0) os_close_file(fd); -- 2.16.2
[PATCH 4/9] um/drivers/vector_user: Delete an unnecessary check before kfree() in user_init_socket_fds()
From: Markus Elfring
Date: Sun, 11 Mar 2018 14:00:09 +0100
The kfree() function tests whether its argument is NULL and then
returns immediately. Thus the test around the call is not needed.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 9cc4651990e3..e831bd85cad4 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -422,8 +422,7 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
- if (result->remote_addr != NULL)
- kfree(result->remote_addr);
+ kfree(result->remote_addr);
kfree(result);
}
--
2.16.2
[PATCH 3/9] um/drivers/vector_user: Adjust an error message in user_init_socket_fds()
From: Markus Elfring
Date: Sun, 11 Mar 2018 13:53:08 +0100
* Adjust an error message at the end of this function.
* Delete the local variable "err" which became unnecessary
with this refactoring.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index 037cd85ee424..9cc4651990e3 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -305,7 +305,6 @@ bool uml_tap_enable_vnet_headers(int fd)
static struct vector_fds *user_init_socket_fds(struct arglist *ifspec, int id)
{
- int err = -ENOMEM;
int fd = -1, gairet;
struct addrinfo srchints;
struct addrinfo dsthints;
@@ -419,7 +418,7 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
cleanup:
if (gairesult != NULL)
freeaddrinfo(gairesult);
- printk(UM_KERN_ERR "user_init_socket: init failed, error %d", err);
+
if (fd >= 0)
os_close_file(fd);
if (result != NULL) {
@@ -427,6 +426,8 @@ static struct vector_fds *user_init_socket_fds(struct
arglist *ifspec, int id)
kfree(result->remote_addr);
kfree(result);
}
+
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, -ENOMEM);
return NULL;
}
--
2.16.2
[PATCH 2/9] um/drivers/vector_user: Less checks in user_init_raw_fds() after error detection
From: Markus Elfring
Date: Sun, 11 Mar 2018 11:40:14 +0100
Up to two checks could be repeated by the user_init_raw_fds() function
during error handling even if the relevant properties can be determined
for the involved variables before by source code analysis.
* Adjust jump targets so that an extra check can be omitted at the end.
* Delete an initialisation for the local variable "rxfd"
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
arch/um/drivers/vector_user.c | 61 ---
1 file changed, 28 insertions(+), 33 deletions(-)
diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c
index d6a6207d4061..037cd85ee424 100644
--- a/arch/um/drivers/vector_user.c
+++ b/arch/um/drivers/vector_user.c
@@ -207,54 +207,46 @@ static struct vector_fds *user_init_tap_fds(struct
arglist *ifspec)
static struct vector_fds *user_init_raw_fds(struct arglist *ifspec)
{
struct ifreq ifr;
- int rxfd = -1, txfd = -1;
+ int rxfd, txfd = -1;
struct sockaddr_ll sock;
- int err = -ENOMEM;
- char *iface;
struct vector_fds *result;
int optval = 1;
+ int err;
+ char *iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
-
- iface = uml_vector_fetch_arg(ifspec, TOKEN_IFNAME);
- if (iface == NULL)
- goto cleanup;
+ if (!iface) {
+ err = -ENOMEM;
+ goto report_failure;
+ }
rxfd = socket(AF_PACKET, SOCK_RAW, ETH_P_ALL);
- if (rxfd == -1) {
- err = -errno;
- goto cleanup;
- }
+ if (rxfd == -1)
+ goto set_error_code;
+
txfd = socket(AF_PACKET, SOCK_RAW, 0); /* Turn off RX on this fd */
- if (txfd == -1) {
- err = -errno;
- goto cleanup;
- }
+ if (txfd == -1)
+ goto set_error_code;
+
memset(&ifr, 0, sizeof(ifr));
strncpy((char *)&ifr.ifr_name, iface, sizeof(ifr.ifr_name) - 1);
- if (ioctl(rxfd, SIOCGIFINDEX, (void *) &ifr) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (ioctl(rxfd, SIOCGIFINDEX, (void *) &ifr) < 0)
+ goto set_error_code;
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_ALL);
sock.sll_ifindex = ifr.ifr_ifindex;
- if (bind(rxfd,
- (struct sockaddr *) &sock, sizeof(struct sockaddr_ll)) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (bind(rxfd, (struct sockaddr *)&sock, sizeof(struct sockaddr_ll))
+ < 0)
+ goto set_error_code;
sock.sll_family = AF_PACKET;
sock.sll_protocol = htons(ETH_P_IP);
sock.sll_ifindex = ifr.ifr_ifindex;
- if (bind(txfd,
- (struct sockaddr *) &sock, sizeof(struct sockaddr_ll)) < 0) {
- err = -errno;
- goto cleanup;
- }
+ if (bind(txfd, (struct sockaddr *)&sock, sizeof(struct sockaddr_ll))
+ < 0)
+ goto set_error_code;
if (setsockopt(txfd,
SOL_PACKET, PACKET_QDISC_BYPASS,
@@ -270,12 +262,15 @@ static struct vector_fds *user_init_raw_fds(struct
arglist *ifspec)
result->remote_addr_size = 0;
}
return result;
-cleanup:
- printk(UM_KERN_ERR "user_init_raw: init failed, error %d", err);
- if (rxfd >= 0)
- os_close_file(rxfd);
+
+set_error_code:
+ err = -errno;
+ os_close_file(rxfd);
+
if (txfd >= 0)
os_close_file(txfd);
+report_failure:
+ printk(UM_KERN_ERR "%s: init failed: %d", __func__, err);
return NULL;
}
--
2.16.2
[PATCH 1/9] um/drivers/vector_user: Delete unnecessary code in user_init_raw_fds()
From: Markus Elfring Date: Sun, 11 Mar 2018 11:36:18 +0100 * One condition check could never be reached with a non-null pointer at the end of this function. Thus remove the corresponding statement. * Delete an initialisation for the local variable "result" which became unnecessary with this refactoring. Signed-off-by: Markus Elfring --- arch/um/drivers/vector_user.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/arch/um/drivers/vector_user.c b/arch/um/drivers/vector_user.c index 4291f1a5d342..d6a6207d4061 100644 --- a/arch/um/drivers/vector_user.c +++ b/arch/um/drivers/vector_user.c @@ -211,7 +211,7 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec) struct sockaddr_ll sock; int err = -ENOMEM; char *iface; - struct vector_fds *result = NULL; + struct vector_fds *result; int optval = 1; @@ -276,8 +276,6 @@ static struct vector_fds *user_init_raw_fds(struct arglist *ifspec) os_close_file(rxfd); if (txfd >= 0) os_close_file(txfd); - if (result != NULL) - kfree(result); return NULL; } -- 2.16.2
[PATCH 0/9] UML vector network driver: Adjustments for three function implementations
From: Markus Elfring Date: Sun, 11 Mar 2018 16:06:16 +0100 Some update suggestions were taken into account from static source code analysis. Markus Elfring (9): Delete unnecessary code in user_init_raw_fds() Less checks in user_init_raw_fds() after error detection Adjust an error message in user_init_socket_fds() Delete an unnecessary check before kfree() in user_init_socket_fds() Delete two unnecessary checks before freeaddrinfo() in user_init_socket_fds() Less checks in user_init_socket_fds() after error detection Adjust an error message in user_init_tap_fds() Less checks in user_init_tap_fds() after error detection Delete an unnecessary variable initialisation in user_init_tap_fds() arch/um/drivers/vector_user.c | 133 +++--- 1 file changed, 60 insertions(+), 73 deletions(-) -- 2.16.2
[PATCH] powerpc: Use common error handling code in setup_new_fdt()
From: Markus Elfring
Date: Sun, 11 Mar 2018 09:03:42 +0100
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
arch/powerpc/kernel/machine_kexec_file_64.c | 28
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/arch/powerpc/kernel/machine_kexec_file_64.c
b/arch/powerpc/kernel/machine_kexec_file_64.c
index e4395f937d63..90c6004c2eec 100644
--- a/arch/powerpc/kernel/machine_kexec_file_64.c
+++ b/arch/powerpc/kernel/machine_kexec_file_64.c
@@ -302,18 +302,14 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
ret = fdt_setprop_u64(fdt, chosen_node,
"linux,initrd-start",
initrd_load_addr);
- if (ret < 0) {
- pr_err("Error setting up the new device tree.\n");
- return -EINVAL;
- }
+ if (ret < 0)
+ goto report_setup_failure;
/* initrd-end is the first address after the initrd image. */
ret = fdt_setprop_u64(fdt, chosen_node, "linux,initrd-end",
initrd_load_addr + initrd_len);
- if (ret < 0) {
- pr_err("Error setting up the new device tree.\n");
- return -EINVAL;
- }
+ if (ret < 0)
+ goto report_setup_failure;
ret = fdt_add_mem_rsv(fdt, initrd_load_addr, initrd_len);
if (ret) {
@@ -325,10 +321,8 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
if (cmdline != NULL) {
ret = fdt_setprop_string(fdt, chosen_node, "bootargs", cmdline);
- if (ret < 0) {
- pr_err("Error setting up the new device tree.\n");
- return -EINVAL;
- }
+ if (ret < 0)
+ goto report_setup_failure;
} else {
ret = fdt_delprop(fdt, chosen_node, "bootargs");
if (ret && ret != -FDT_ERR_NOTFOUND) {
@@ -344,10 +338,12 @@ int setup_new_fdt(const struct kimage *image, void *fdt,
}
ret = fdt_setprop(fdt, chosen_node, "linux,booted-from-kexec", NULL, 0);
- if (ret) {
- pr_err("Error setting up the new device tree.\n");
- return -EINVAL;
- }
+ if (ret)
+ goto report_setup_failure;
return 0;
+
+report_setup_failure:
+ pr_err("Error setting up the new device tree.\n");
+ return -EINVAL;
}
--
2.16.2
[PATCH 3/3] wlcore: Use common error handling code in wl1271_acx_sta_rate_policies()
From: Markus Elfring
Date: Sat, 10 Mar 2018 22:18:45 +0100
Add a jump target so that a bit of exception handling can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/net/wireless/ti/wlcore/acx.c | 24 +++-
1 file changed, 11 insertions(+), 13 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/acx.c
b/drivers/net/wireless/ti/wlcore/acx.c
index 1cc5bba670e1..7d37a417c756 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -757,10 +757,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
- if (ret < 0) {
- wl1271_warning("Setting of rate policies failed: %d", ret);
- goto out;
- }
+ if (ret < 0)
+ goto report_failure;
/* configure one AP supported rate class */
acx->rate_policy_idx = cpu_to_le32(wlvif->sta.ap_rate_idx);
@@ -773,10 +771,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct
wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
- if (ret < 0) {
- wl1271_warning("Setting of rate policies failed: %d", ret);
- goto out;
- }
+ if (ret < 0)
+ goto report_failure;
/*
* configure one rate class for basic p2p operations.
@@ -791,14 +787,16 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl,
struct wl12xx_vif *wlvif)
acx->rate_policy.aflags = c->aflags;
ret = wl1271_cmd_configure(wl, ACX_RATE_POLICY, acx, sizeof(*acx));
- if (ret < 0) {
- wl1271_warning("Setting of rate policies failed: %d", ret);
- goto out;
- }
+ if (ret < 0)
+ goto report_failure;
-out:
+free_acx:
kfree(acx);
return ret;
+
+report_failure:
+ wl1271_warning("Setting of rate policies failed: %d", ret);
+ goto free_acx;
}
int wl1271_acx_ap_rate_policy(struct wl1271 *wl, struct conf_tx_rate_class *c,
--
2.16.2
[PATCH 2/3] wlcore: Return directly after a failed kzalloc() in wl1271_acx_sta_rate_policies()
From: Markus Elfring
Date: Sat, 10 Mar 2018 22:00:31 +0100
Return directly after a call of the function "kzalloc" failed
at the beginning.
Signed-off-by: Markus Elfring
---
drivers/net/wireless/ti/wlcore/acx.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ti/wlcore/acx.c
b/drivers/net/wireless/ti/wlcore/acx.c
index 6991fee8fe61..1cc5bba670e1 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -743,11 +743,8 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct
wl12xx_vif *wlvif)
wl1271_debug(DEBUG_ACX, "acx rate policies");
acx = kzalloc(sizeof(*acx), GFP_KERNEL);
-
- if (!acx) {
- ret = -ENOMEM;
- goto out;
- }
+ if (!acx)
+ return -ENOMEM;
wl1271_debug(DEBUG_ACX, "basic_rate: 0x%x, full_rate: 0x%x",
wlvif->basic_rate, wlvif->rate_set);
--
2.16.2
[PATCH 1/3] wlcore: Delete an unnecessary variable initialisation in wl1271_acx_sta_rate_policies()
From: Markus Elfring
Date: Sat, 10 Mar 2018 21:51:17 +0100
The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
drivers/net/wireless/ti/wlcore/acx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ti/wlcore/acx.c
b/drivers/net/wireless/ti/wlcore/acx.c
index 3ca9167d6146..6991fee8fe61 100644
--- a/drivers/net/wireless/ti/wlcore/acx.c
+++ b/drivers/net/wireless/ti/wlcore/acx.c
@@ -738,7 +738,7 @@ int wl1271_acx_sta_rate_policies(struct wl1271 *wl, struct
wl12xx_vif *wlvif)
{
struct acx_rate_policy *acx;
struct conf_tx_rate_class *c = &wl->conf.tx.sta_rc_conf;
- int ret = 0;
+ int ret;
wl1271_debug(DEBUG_ACX, "acx rate policies");
--
2.16.2
[PATCH 0/3] wlcore: Adjustments for wl1271_acx_sta_rate_policies()
From: Markus Elfring Date: Sat, 10 Mar 2018 22:25:45 +0100 Three update suggestions were taken into account from static source code analysis. Markus Elfring (3): Delete an unnecessary variable initialisation Return directly after a failed kzalloc() Use common error handling code drivers/net/wireless/ti/wlcore/acx.c | 33 ++--- 1 file changed, 14 insertions(+), 19 deletions(-) -- 2.16.2
[PATCH 2/2] net/usb/ax88179_178a: Delete three unnecessary variables in ax88179_chk_eee()
From: Markus Elfring
Date: Sat, 10 Mar 2018 18:53:28 +0100
Use three values directly for a condition check without assigning them
to intermediate variables.
Signed-off-by: Markus Elfring
---
drivers/net/usb/ax88179_178a.c | 13 ++---
1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e4b0baa98e9a..3e83be232504 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -684,7 +684,7 @@ static int ax88179_chk_eee(struct usbnet *dev)
if (ecmd.duplex & DUPLEX_FULL) {
int eee_lp, eee_cap, eee_adv;
- u32 lp, cap, adv, supported = 0;
+ u32 cap;
eee_cap = ax88179_phy_read_mmd_indirect(dev,
MDIO_PCS_EEE_ABLE,
@@ -708,12 +708,11 @@ static int ax88179_chk_eee(struct usbnet *dev)
if (eee_adv < 0)
goto set_inactive;
- adv = mmd_eee_adv_to_ethtool_adv_t(eee_adv);
- lp = mmd_eee_adv_to_ethtool_adv_t(eee_lp);
- supported = (ecmd.speed == SPEED_1000) ?
-SUPPORTED_1000baseT_Full :
-SUPPORTED_100baseT_Full;
- if (!(lp & adv & supported))
+ if (!(mmd_eee_adv_to_ethtool_adv_t(eee_lp) &
+ mmd_eee_adv_to_ethtool_adv_t(eee_adv) &
+ ((ecmd.speed == SPEED_1000)
+ ? SUPPORTED_1000baseT_Full
+ : SUPPORTED_100baseT_Full)))
goto set_inactive;
priv->eee_active = 1;
--
2.16.2
[PATCH 1/2] net/usb/ax88179_178a: Use common code in ax88179_chk_eee()
From: Markus Elfring
Date: Sat, 10 Mar 2018 18:22:43 +0100
Adjust a jump target so that a bit of common code can be better reused
at the end of this function.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/net/usb/ax88179_178a.c | 34 +++---
1 file changed, 11 insertions(+), 23 deletions(-)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index f32261ecd215..e4b0baa98e9a 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -689,49 +689,37 @@ static int ax88179_chk_eee(struct usbnet *dev)
eee_cap = ax88179_phy_read_mmd_indirect(dev,
MDIO_PCS_EEE_ABLE,
MDIO_MMD_PCS);
- if (eee_cap < 0) {
- priv->eee_active = 0;
- return false;
- }
+ if (eee_cap < 0)
+ goto set_inactive;
cap = mmd_eee_cap_to_ethtool_sup_t(eee_cap);
- if (!cap) {
- priv->eee_active = 0;
- return false;
- }
+ if (!cap)
+ goto set_inactive;
eee_lp = ax88179_phy_read_mmd_indirect(dev,
MDIO_AN_EEE_LPABLE,
MDIO_MMD_AN);
- if (eee_lp < 0) {
- priv->eee_active = 0;
- return false;
- }
+ if (eee_lp < 0)
+ goto set_inactive;
eee_adv = ax88179_phy_read_mmd_indirect(dev,
MDIO_AN_EEE_ADV,
MDIO_MMD_AN);
-
- if (eee_adv < 0) {
- priv->eee_active = 0;
- return false;
- }
+ if (eee_adv < 0)
+ goto set_inactive;
adv = mmd_eee_adv_to_ethtool_adv_t(eee_adv);
lp = mmd_eee_adv_to_ethtool_adv_t(eee_lp);
supported = (ecmd.speed == SPEED_1000) ?
SUPPORTED_1000baseT_Full :
SUPPORTED_100baseT_Full;
-
- if (!(lp & adv & supported)) {
- priv->eee_active = 0;
- return false;
- }
+ if (!(lp & adv & supported))
+ goto set_inactive;
priv->eee_active = 1;
return true;
}
-
+set_inactive:
priv->eee_active = 0;
return false;
}
--
2.16.2
[PATCH 0/2] net/usb/ax88179_178a: Adjustments for ax88179_chk_eee()
From: Markus Elfring Date: Sat, 10 Mar 2018 19:05:45 +0100 Two update suggestions were taken into account from static source code analysis. Markus Elfring (2): Use common code Delete three unnecessary variables drivers/net/usb/ax88179_178a.c | 45 +++--- 1 file changed, 16 insertions(+), 29 deletions(-) -- 2.16.2
[PATCH v3] [media] Use common error handling code in 19 functions
From: Markus Elfring
Date: Fri, 9 Mar 2018 21:00:12 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of these functions.
This issue was partly detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
v3:
Laurent Pinchart and Todor Tomov requested a few adjustments.
Updates were rebased on source files from Linux next-20180308.
v2:
Hans Verkuil insisted on patch squashing. Thus several changes
were recombined based on source files from Linux next-20180216.
The implementation of the function "tda8261_set_params" was improved
after a notification by Christoph Böhmwalder on 2017-09-26.
drivers/media/dvb-core/dmxdev.c| 16
drivers/media/dvb-frontends/tda1004x.c | 20 ++
drivers/media/dvb-frontends/tda8261.c | 19 ++
drivers/media/pci/bt8xx/dst.c | 19 ++
drivers/media/pci/bt8xx/dst_ca.c | 30 +++
drivers/media/pci/cx88/cx88-input.c| 17 +
drivers/media/platform/omap3isp/ispvideo.c | 28 ++
.../media/platform/qcom/camss-8x16/camss-csid.c| 19 +-
drivers/media/tuners/tuner-xc2028.c| 30 +++
drivers/media/usb/cpia2/cpia2_usb.c| 13 ---
drivers/media/usb/gspca/gspca.c| 17 +
drivers/media/usb/gspca/sn9c20x.c | 17 +
drivers/media/usb/pvrusb2/pvrusb2-ioread.c | 10 +++--
drivers/media/usb/tm6000/tm6000-cards.c| 7 ++--
drivers/media/usb/tm6000/tm6000-dvb.c | 11 --
drivers/media/usb/tm6000/tm6000-video.c| 13 ---
drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c | 13 +++
drivers/media/usb/ttusb-dec/ttusb_dec.c| 43 --
18 files changed, 171 insertions(+), 171 deletions(-)
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 61a750fae465..17d05b05fa9d 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -656,18 +656,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
tsfeed->priv = filter;
ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
ret = tsfeed->start_filtering(tsfeed);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
return 0;
+
+release_feed:
+ dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
+ return ret;
}
static int dvb_dmxdev_filter_start(struct dmxdev_filter *filter)
diff --git a/drivers/media/dvb-frontends/tda1004x.c
b/drivers/media/dvb-frontends/tda1004x.c
index 58e3beff5adc..85ca111fc8c4 100644
--- a/drivers/media/dvb-frontends/tda1004x.c
+++ b/drivers/media/dvb-frontends/tda1004x.c
@@ -1299,20 +1299,22 @@ struct dvb_frontend* tda10045_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10045: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x25) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10045_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
static const struct dvb_frontend_ops tda10046_ops = {
@@ -1369,19 +1371,21 @@ struct dvb_frontend* tda10046_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10046: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x46) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10046_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
module_param(debug, int, 0644);
diff --git a/drivers/media/dvb-frontends/tda8261.c
b/d
[PATCH v2 17/17] mfd: viperboard: Delete an error message for a failed memory allocation in vprbrd_probe()
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:56:31 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/viperboard.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/viperboard.c b/drivers/mfd/viperboard.c
index e6b3c70aeb22..e9f61262d583 100644
--- a/drivers/mfd/viperboard.c
+++ b/drivers/mfd/viperboard.c
@@ -59,10 +59,8 @@ static int vprbrd_probe(struct usb_interface *interface,
/* allocate memory for our device state and initialize it */
vb = kzalloc(sizeof(*vb), GFP_KERNEL);
- if (vb == NULL) {
- dev_err(&interface->dev, "Out of memory\n");
+ if (!vb)
return -ENOMEM;
- }
mutex_init(&vb->lock);
--
2.16.2
[PATCH v2 16/17] mfd: twl6030-irq: Delete an error message for a failed memory allocation in twl6030_init_irq()
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:49:32 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/twl6030-irq.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/twl6030-irq.c b/drivers/mfd/twl6030-irq.c
index e3ec8dfa9f1e..e939431ed10c 100644
--- a/drivers/mfd/twl6030-irq.c
+++ b/drivers/mfd/twl6030-irq.c
@@ -392,10 +392,8 @@ int twl6030_init_irq(struct device *dev, int irq_num)
nr_irqs = TWL6030_NR_IRQS;
twl6030_irq = devm_kzalloc(dev, sizeof(*twl6030_irq), GFP_KERNEL);
- if (!twl6030_irq) {
- dev_err(dev, "twl6030_irq: Memory allocation failed\n");
+ if (!twl6030_irq)
return -ENOMEM;
- }
mask[0] = 0xFF;
mask[1] = 0xFF;
--
2.16.2
[PATCH v2 15/17] mfd: tps80031: Delete an error message for a failed memory allocation in tps80031_probe()
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:45:13 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps80031.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/tps80031.c b/drivers/mfd/tps80031.c
index 0812df3b0d47..608c7f77830e 100644
--- a/drivers/mfd/tps80031.c
+++ b/drivers/mfd/tps80031.c
@@ -431,10 +431,8 @@ static int tps80031_probe(struct i2c_client *client,
}
tps80031 = devm_kzalloc(&client->dev, sizeof(*tps80031), GFP_KERNEL);
- if (!tps80031) {
- dev_err(&client->dev, "Malloc failed for tps80031\n");
+ if (!tps80031)
return -ENOMEM;
- }
for (i = 0; i < TPS80031_NUM_SLAVES; i++) {
if (tps80031_slave_address[i] == client->addr)
--
2.16.2
[PATCH v2 14/17 4/4] mfd: tps65910: Move an assignment in tps65910_sleepinit()
From: Markus Elfring Date: Fri, 9 Mar 2018 09:19:42 +0100 Move the assignment for the local variable "dev" so that its setting will be performed after a configuration check by this function. Signed-off-by: Markus Elfring --- v2: Lee Jones requested a resend for this patch. The change was rebased on source files from Linux next-20180308. drivers/mfd/tps65910.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c index 7e7d3d1642c6..bf16cbe6fd88 100644 --- a/drivers/mfd/tps65910.c +++ b/drivers/mfd/tps65910.c @@ -315,11 +315,11 @@ static int tps65910_sleepinit(struct tps65910 *tps65910, struct device *dev; int ret; - dev = tps65910->dev; - if (!pmic_pdata->en_dev_slp) return 0; + dev = tps65910->dev; + /* enabling SLEEP device state */ ret = tps65910_reg_set_bits(tps65910, TPS65910_DEVCTRL, DEVCTRL_DEV_SLP_MASK); -- 2.16.2
[PATCH v2 13/17 3/4] mfd: tps65910: Delete an unnecessary variable initialisation in tps65910_sleepinit()
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:10:09 +0100
The local variable "dev" will be reassigned by a following statement.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps65910.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 80ea1474c654..7e7d3d1642c6 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -312,7 +312,7 @@ static int tps65910_ck32k_init(struct tps65910 *tps65910,
static int tps65910_sleepinit(struct tps65910 *tps65910,
struct tps65910_board *pmic_pdata)
{
- struct device *dev = NULL;
+ struct device *dev;
int ret;
dev = tps65910->dev;
--
2.16.2
[PATCH v2 12/17 2/4] mfd: tps65910: Delete an unnecessary variable initialisation in four functions
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:06:14 +0100
The local variable "ret" will be set to an appropriate value a bit later.
Thus omit the explicit initialisation at the beginning.
Signed-off-by: Markus Elfring
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps65910.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 555bd394efc3..80ea1474c654 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -229,7 +229,7 @@ static struct regmap_irq_chip tps65910_irq_chip = {
static int tps65910_irq_init(struct tps65910 *tps65910, int irq,
struct tps65910_platform_data *pdata)
{
- int ret = 0;
+ int ret;
static struct regmap_irq_chip *tps6591x_irqs_chip;
if (!irq) {
@@ -313,7 +313,7 @@ static int tps65910_sleepinit(struct tps65910 *tps65910,
struct tps65910_board *pmic_pdata)
{
struct device *dev = NULL;
- int ret = 0;
+ int ret;
dev = tps65910->dev;
@@ -383,7 +383,7 @@ static struct tps65910_board *tps65910_parse_dt(struct
i2c_client *client,
struct tps65910_board *board_info;
unsigned int prop;
const struct of_device_id *match;
- int ret = 0;
+ int ret;
match = of_match_device(tps65910_of_match, &client->dev);
if (!match) {
@@ -460,7 +460,7 @@ static int tps65910_i2c_probe(struct i2c_client *i2c,
struct tps65910_board *of_pmic_plat_data = NULL;
struct tps65910_platform_data *init_data;
unsigned long chip_id = id->driver_data;
- int ret = 0;
+ int ret;
pmic_plat_data = dev_get_platdata(&i2c->dev);
--
2.16.2
[PATCH v2 11/17] mfd: tps65910: Delete an error message for a failed memory allocation in tps65910_parse_dt()
From: Markus Elfring
Date: Fri, 9 Mar 2018 09:00:59 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps65910.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/tps65910.c b/drivers/mfd/tps65910.c
index 8263605f6d2f..555bd394efc3 100644
--- a/drivers/mfd/tps65910.c
+++ b/drivers/mfd/tps65910.c
@@ -395,10 +395,8 @@ static struct tps65910_board *tps65910_parse_dt(struct
i2c_client *client,
board_info = devm_kzalloc(&client->dev, sizeof(*board_info),
GFP_KERNEL);
- if (!board_info) {
- dev_err(&client->dev, "Failed to allocate pdata\n");
+ if (!board_info)
return NULL;
- }
ret = of_property_read_u32(np, "ti,vmbch-threshold", &prop);
if (!ret)
--
2.16.2
[PATCH v2 0/4] mfd/tps65910: Adjustments for four function implementations
From: Markus Elfring A few update suggestions were taken into account from static source code analysis. Markus Elfring (4): Delete an error message for a failed memory allocation in tps65910_parse_dt() Delete an unnecessary variable initialisation in four functions Delete an unnecessary variable initialisation in tps65910_sleepinit() Move an assignment in tps65910_sleepinit() --- v2: Lee Jones requested a resend for these patches. The changes were rebased on source files from Linux next-20180308. drivers/mfd/tps65910.c | 18 -- 1 file changed, 8 insertions(+), 10 deletions(-) -- 2.15.1
[PATCH v2 10/17] mfd: tps6586x: Delete an error message for a failed memory allocation in tps6586x_parse_dt()
From: Markus Elfring
Date: Thu, 8 Mar 2018 15:30:54 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps6586x.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/tps6586x.c b/drivers/mfd/tps6586x.c
index 5628a6b5b19b..b89379782741 100644
--- a/drivers/mfd/tps6586x.c
+++ b/drivers/mfd/tps6586x.c
@@ -423,10 +423,8 @@ static struct tps6586x_platform_data
*tps6586x_parse_dt(struct i2c_client *clien
struct tps6586x_platform_data *pdata;
pdata = devm_kzalloc(&client->dev, sizeof(*pdata), GFP_KERNEL);
- if (!pdata) {
- dev_err(&client->dev, "Memory allocation failed\n");
+ if (!pdata)
return NULL;
- }
pdata->num_subdevs = 0;
pdata->subdevs = NULL;
--
2.16.2
[PATCH v2 09/17] mfd: tps65090: Delete an error message for a failed memory allocation in tps65090_i2c_probe()
From: Markus Elfring
Date: Thu, 8 Mar 2018 15:25:58 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/tps65090.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/mfd/tps65090.c b/drivers/mfd/tps65090.c
index d7ec318c40c3..f13e4cd06e89 100644
--- a/drivers/mfd/tps65090.c
+++ b/drivers/mfd/tps65090.c
@@ -192,10 +192,8 @@ static int tps65090_i2c_probe(struct i2c_client *client,
irq_base = pdata->irq_base;
tps65090 = devm_kzalloc(&client->dev, sizeof(*tps65090), GFP_KERNEL);
- if (!tps65090) {
- dev_err(&client->dev, "mem alloc for tps65090 failed\n");
+ if (!tps65090)
return -ENOMEM;
- }
tps65090->dev = &client->dev;
i2c_set_clientdata(client, tps65090);
--
2.16.2
[PATCH v2 08/17] mfd: ti_am335x_tscadc: Delete an error message for a failed memory allocation in ti_tscadc_probe()
From: Markus Elfring
Date: Thu, 8 Mar 2018 15:15:51 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/ti_am335x_tscadc.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/ti_am335x_tscadc.c b/drivers/mfd/ti_am335x_tscadc.c
index 3cd958a31f36..47012c0899cd 100644
--- a/drivers/mfd/ti_am335x_tscadc.c
+++ b/drivers/mfd/ti_am335x_tscadc.c
@@ -169,10 +169,9 @@ static int ti_tscadc_probe(struct platform_device
*pdev)
/* Allocate memory for device */
tscadc = devm_kzalloc(&pdev->dev, sizeof(*tscadc), GFP_KERNEL);
- if (!tscadc) {
- dev_err(&pdev->dev, "failed to allocate memory.\n");
+ if (!tscadc)
return -ENOMEM;
- }
+
tscadc->dev = &pdev->dev;
err = platform_get_irq(pdev, 0);
--
2.16.2
[PATCH v2 07/17] mfd: smsc-ece1099: Improve a size determination in smsc_i2c_probe()
From: Markus Elfring Date: Thu, 8 Mar 2018 15:05:16 +0100 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. This issue was detected by using the Coccinelle software. Signed-off-by: Markus Elfring --- v2: Lee Jones requested a different source code layout for this transformation. The change was rebased on source files from Linux next-20180308. drivers/mfd/smsc-ece1099.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/mfd/smsc-ece1099.c b/drivers/mfd/smsc-ece1099.c index b9d96651cc0d..57b792eb58fd 100644 --- a/drivers/mfd/smsc-ece1099.c +++ b/drivers/mfd/smsc-ece1099.c @@ -37,8 +37,7 @@ static int smsc_i2c_probe(struct i2c_client *i2c, int devid, rev, venid_l, venid_h; int ret; - smsc = devm_kzalloc(&i2c->dev, sizeof(struct smsc), - GFP_KERNEL); + smsc = devm_kzalloc(&i2c->dev, sizeof(*smsc), GFP_KERNEL); if (!smsc) return -ENOMEM; -- 2.16.2
[PATCH v2 06/17 2/2] mfd: sm501: Adjust 12 checks for null pointers
From: Markus Elfring
Date: Thu, 8 Mar 2018 14:20:06 +0100
The script “checkpatch.pl” pointed information out like the following.
Comparison to NULL could be written …
Thus fix the affected source code places.
Signed-off-by: Markus Elfring
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/sm501.c | 26 +++---
1 file changed, 11 insertions(+), 15 deletions(-)
diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index 4f4957ea8fa3..55d19fd0994e 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -1050,13 +1050,13 @@ static int sm501_register_gpio(struct sm501_devdata *sm)
spin_lock_init(&gpio->lock);
gpio->regs_res = request_mem_region(iobase, 0x20, "sm501-gpio");
- if (gpio->regs_res == NULL) {
+ if (!gpio->regs_res) {
dev_err(sm->dev, "gpio: failed to request region\n");
return -ENXIO;
}
gpio->regs = ioremap(iobase, 0x20);
- if (gpio->regs == NULL) {
+ if (!gpio->regs) {
dev_err(sm->dev, "gpio: failed to remap registers\n");
ret = -ENXIO;
goto err_claimed;
@@ -1358,7 +1358,7 @@ static int sm501_init_dev(struct sm501_devdata *sm)
sm501_register_gpio(sm);
}
- if (pdata && pdata->gpio_i2c != NULL && pdata->gpio_i2c_nr > 0) {
+ if (pdata && pdata->gpio_i2c && pdata->gpio_i2c_nr > 0) {
if (!sm501_gpio_isregistered(sm))
dev_err(sm->dev, "no gpio available for i2c gpio.\n");
else
@@ -1384,7 +1384,7 @@ static int sm501_plat_probe(struct platform_device *dev)
int ret;
sm = kzalloc(sizeof(*sm), GFP_KERNEL);
- if (sm == NULL) {
+ if (!sm) {
ret = -ENOMEM;
goto err1;
}
@@ -1402,8 +1402,7 @@ static int sm501_plat_probe(struct platform_device *dev)
sm->io_res = platform_get_resource(dev, IORESOURCE_MEM, 1);
sm->mem_res = platform_get_resource(dev, IORESOURCE_MEM, 0);
-
- if (sm->io_res == NULL || sm->mem_res == NULL) {
+ if (!sm->io_res || !sm->mem_res) {
dev_err(&dev->dev, "failed to get IO resource\n");
ret = -ENOENT;
goto err_res;
@@ -1411,8 +1410,7 @@ static int sm501_plat_probe(struct platform_device *dev)
sm->regs_claim = request_mem_region(sm->io_res->start,
0x100, "sm501");
-
- if (sm->regs_claim == NULL) {
+ if (!sm->regs_claim) {
dev_err(&dev->dev, "cannot claim registers\n");
ret = -EBUSY;
goto err_res;
@@ -1421,8 +1419,7 @@ static int sm501_plat_probe(struct platform_device *dev)
platform_set_drvdata(dev, sm);
sm->regs = ioremap(sm->io_res->start, resource_size(sm->io_res));
-
- if (sm->regs == NULL) {
+ if (!sm->regs) {
dev_err(&dev->dev, "cannot remap registers\n");
ret = -EIO;
goto err_claim;
@@ -1448,7 +1445,7 @@ static void sm501_set_power(struct sm501_devdata *sm, int
on)
{
struct sm501_platdata *pd = sm->platdata;
- if (pd == NULL)
+ if (!pd)
return;
if (pd->get_power) {
@@ -1573,7 +1570,7 @@ static int sm501_pci_probe(struct pci_dev *dev,
int err;
sm = kzalloc(sizeof(*sm), GFP_KERNEL);
- if (sm == NULL) {
+ if (!sm) {
err = -ENOMEM;
goto err1;
}
@@ -1624,15 +1621,14 @@ static int sm501_pci_probe(struct pci_dev *dev,
sm->regs_claim = request_mem_region(sm->io_res->start,
0x100, "sm501");
- if (sm->regs_claim == NULL) {
+ if (!sm->regs_claim) {
dev_err(&dev->dev, "cannot claim registers\n");
err= -EBUSY;
goto err3;
}
sm->regs = pci_ioremap_bar(dev, 1);
-
- if (sm->regs == NULL) {
+ if (!sm->regs) {
dev_err(&dev->dev, "cannot remap registers\n");
err = -EIO;
goto err4;
--
2.16.2
[PATCH v2 05/17 1/2] mfd: sm501: Improve a size determination in two functions
From: Markus Elfring
Date: Thu, 8 Mar 2018 14:05:41 +0100
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 issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
v2:
Lee Jones requested a different source code layout for this transformation.
The change was rebased on source files from Linux next-20180308.
drivers/mfd/sm501.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/mfd/sm501.c b/drivers/mfd/sm501.c
index 7298d6b571a1..4f4957ea8fa3 100644
--- a/drivers/mfd/sm501.c
+++ b/drivers/mfd/sm501.c
@@ -1383,7 +1383,7 @@ static int sm501_plat_probe(struct platform_device *dev)
struct sm501_devdata *sm;
int ret;
- sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
+ sm = kzalloc(sizeof(*sm), GFP_KERNEL);
if (sm == NULL) {
ret = -ENOMEM;
goto err1;
@@ -1572,7 +1572,7 @@ static int sm501_pci_probe(struct pci_dev *dev,
struct sm501_devdata *sm;
int err;
- sm = kzalloc(sizeof(struct sm501_devdata), GFP_KERNEL);
+ sm = kzalloc(sizeof(*sm), GFP_KERNEL);
if (sm == NULL) {
err = -ENOMEM;
goto err1;
--
2.16.2
[PATCH v2 0/2] mfd/sm501: Adjustments for five function implementations
From: Markus Elfring A few update suggestions were taken into account from static source code analysis. Markus Elfring (2): Improve a size determination in two functions Adjust 12 checks for null pointers --- v2: Lee Jones requested a resend for these patches. The changes were rebased on source files from Linux next-20180308. drivers/mfd/sm501.c | 26 +++--- 1 file changed, 11 insertions(+), 15 deletions(-) -- 2.15.1
[PATCH v2 04/17] mfd: si476x-i2c: Delete an error message for a failed memory allocation in si476x_core_probe()
From: Markus Elfring
Date: Thu, 8 Mar 2018 13:45:31 +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
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/si476x-i2c.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/si476x-i2c.c b/drivers/mfd/si476x-i2c.c
index e6a3d999a376..2c5ec9c3 100644
--- a/drivers/mfd/si476x-i2c.c
+++ b/drivers/mfd/si476x-i2c.c
@@ -697,11 +697,9 @@ static int si476x_core_probe(struct i2c_client *client,
int cell_num;
core = devm_kzalloc(&client->dev, sizeof(*core), GFP_KERNEL);
- if (!core) {
- dev_err(&client->dev,
- "failed to allocate 'struct si476x_core'\n");
+ if (!core)
return -ENOMEM;
- }
+
core->client = client;
core->regmap = devm_regmap_init_si476x(core);
--
2.16.2
[PATCH v2 03/17 3/3] mfd: abx500-core: Adjust 14 checks for null pointers
From: Markus Elfring Date: Thu, 8 Mar 2018 12:50:12 +0100 The script “checkpatch.pl” pointed information out like the following. Comparison to NULL could be written … Thus fix the affected source code places. Signed-off-by: Markus Elfring Reviewed-by: Linus Walleij --- v2: Lee Jones requested a resend for this patch. The change was rebased on source files from Linux next-20180308. drivers/mfd/abx500-core.c | 14 +++--- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c index 17176e91cbd0..f282d39a5917 100644 --- a/drivers/mfd/abx500-core.c +++ b/drivers/mfd/abx500-core.c @@ -65,7 +65,7 @@ int abx500_set_register_interruptible(struct device *dev, u8 bank, u8 reg, struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->set_register != NULL)) + if (ops && ops->set_register) return ops->set_register(dev, bank, reg, value); else return -ENOTSUPP; @@ -78,7 +78,7 @@ int abx500_get_register_interruptible(struct device *dev, u8 bank, u8 reg, struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->get_register != NULL)) + if (ops && ops->get_register) return ops->get_register(dev, bank, reg, value); else return -ENOTSUPP; @@ -91,7 +91,7 @@ int abx500_get_register_page_interruptible(struct device *dev, u8 bank, struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->get_register_page != NULL)) + if (ops && ops->get_register_page) return ops->get_register_page(dev, bank, first_reg, regvals, numregs); else @@ -105,7 +105,7 @@ int abx500_mask_and_set_register_interruptible(struct device *dev, u8 bank, struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->mask_and_set_register != NULL)) + if (ops && ops->mask_and_set_register) return ops->mask_and_set_register(dev, bank, reg, bitmask, bitvalues); else @@ -118,7 +118,7 @@ int abx500_get_chip_id(struct device *dev) struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->get_chip_id != NULL)) + if (ops && ops->get_chip_id) return ops->get_chip_id(dev); else return -ENOTSUPP; @@ -130,7 +130,7 @@ int abx500_event_registers_startup_state_get(struct device *dev, u8 *event) struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->event_registers_startup_state_get != NULL)) + if (ops && ops->event_registers_startup_state_get) return ops->event_registers_startup_state_get(dev, event); else return -ENOTSUPP; @@ -142,7 +142,7 @@ int abx500_startup_irq_enabled(struct device *dev, unsigned int irq) struct abx500_ops *ops; lookup_ops(dev->parent, &ops); - if ((ops != NULL) && (ops->startup_irq_enabled != NULL)) + if (ops && ops->startup_irq_enabled) return ops->startup_irq_enabled(dev, irq); else return -ENOTSUPP; -- 2.16.2
[PATCH v2 02/17 2/3] mfd: abx500-core: Improve two size determinations in abx500_register_ops()
From: Markus Elfring
Date: Thu, 8 Mar 2018 12:46:47 +0100
Replace the specification of two 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 issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
Reviewed-by: Linus Walleij
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/abx500-core.c | 6 ++
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c
index c8c9d41abcaa..17176e91cbd0 100644
--- a/drivers/mfd/abx500-core.c
+++ b/drivers/mfd/abx500-core.c
@@ -37,14 +37,12 @@ int abx500_register_ops(struct device *dev, struct
abx500_ops *ops)
{
struct abx500_device_entry *dev_entry;
- dev_entry = devm_kzalloc(dev,
-sizeof(struct abx500_device_entry),
-GFP_KERNEL);
+ dev_entry = devm_kzalloc(dev, sizeof(*dev_entry), GFP_KERNEL);
if (!dev_entry)
return -ENOMEM;
dev_entry->dev = dev;
- memcpy(&dev_entry->ops, ops, sizeof(struct abx500_ops));
+ memcpy(&dev_entry->ops, ops, sizeof(*ops));
list_add_tail(&dev_entry->list, &abx500_list);
return 0;
--
2.16.2
[PATCH v2 01/17 1/3] mfd: abx500-core: Delete an error message for a failed memory allocation in abx500_register_ops()
From: Markus Elfring
Date: Thu, 8 Mar 2018 11:44:33 +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
Reviewed-by: Linus Walleij
---
v2:
Lee Jones requested a resend for this patch. The change was rebased
on source files from Linux next-20180308.
drivers/mfd/abx500-core.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/mfd/abx500-core.c b/drivers/mfd/abx500-core.c
index 0d3846a4767c..c8c9d41abcaa 100644
--- a/drivers/mfd/abx500-core.c
+++ b/drivers/mfd/abx500-core.c
@@ -40,10 +40,9 @@ int abx500_register_ops(struct device *dev, struct
abx500_ops *ops)
dev_entry = devm_kzalloc(dev,
sizeof(struct abx500_device_entry),
GFP_KERNEL);
- if (!dev_entry) {
- dev_err(dev, "register_ops kzalloc failed");
+ if (!dev_entry)
return -ENOMEM;
- }
+
dev_entry->dev = dev;
memcpy(&dev_entry->ops, ops, sizeof(struct abx500_ops));
--
2.16.2
[PATCH v2 0/3] mfd/abx500-core: Adjustments for eight function implementations
From: Markus Elfring 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 abx500_register_ops() Improve two size determinations in abx500_register_ops() Adjust 14 checks for null pointers Reviewed-by: Linus Walleij --- v2: Lee Jones requested a resend for these patches. The changes were rebased on source files from Linux next-20180308. drivers/mfd/abx500-core.c | 25 +++-- 1 file changed, 11 insertions(+), 14 deletions(-) -- 2.15.1
[PATCH v2 00/17] MFD: Adjustments for several function implementations
From: Markus Elfring Date: Fri, 9 Mar 2018 10:24:42 +0100 Several update suggestions were taken into account from static source code analysis. Markus Elfring (17): abx500-core: Adjustments for eight function implementations si476x-i2c: Delete an error message for a failed memory allocation in si476x_core_probe() sm501: Adjustments for five function implementations smsc-ece1099: Improve a size determination in smsc_i2c_probe() ti_am335x_tscadc: Delete an error message for a failed memory allocation in ti_tscadc_probe() tps65090: Delete an error message for a failed memory allocation in tps65090_i2c_probe() tps6586x: Delete an error message for a failed memory allocation in tps6586x_parse_dt() tps65910: Adjustments for four function implementations tps80031: Delete an error message for a failed memory allocation in tps80031_probe() twl6030-irq: Delete an error message for a failed memory allocation in twl6030_init_irq() viperboard: Delete an error message for a failed memory allocation in vprbrd_probe() --- v2: Lee Jones requested a resend for some of these patches. Thus several changes were rebased on source files from Linux next-20180308. drivers/mfd/abx500-core.c | 25 +++-- drivers/mfd/si476x-i2c.c | 6 ++ drivers/mfd/sm501.c| 30 +- drivers/mfd/smsc-ece1099.c | 3 +-- drivers/mfd/ti_am335x_tscadc.c | 5 ++--- drivers/mfd/tps65090.c | 4 +--- drivers/mfd/tps6586x.c | 4 +--- drivers/mfd/tps65910.c | 18 -- drivers/mfd/tps80031.c | 4 +--- drivers/mfd/twl6030-irq.c | 4 +--- drivers/mfd/viperboard.c | 4 +--- 11 files changed, 42 insertions(+), 65 deletions(-) -- 2.16.2
[PATCH 2/2] VMCI: Improve a size determination in two functions
From: Markus Elfring
Date: Mon, 5 Mar 2018 17:00:19 +0100
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 issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
drivers/misc/vmw_vmci/vmci_context.c | 2 +-
drivers/misc/vmw_vmci/vmci_host.c| 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/misc/vmw_vmci/vmci_context.c
b/drivers/misc/vmw_vmci/vmci_context.c
index 21d0fa592145..18404318dd71 100644
--- a/drivers/misc/vmw_vmci/vmci_context.c
+++ b/drivers/misc/vmw_vmci/vmci_context.c
@@ -620,7 +620,7 @@ int vmci_ctx_add_notification(u32 context_id, u32
remote_cid)
goto out;
}
- notifier = kmalloc(sizeof(struct vmci_handle_list), GFP_KERNEL);
+ notifier = kmalloc(sizeof(*notifier), GFP_KERNEL);
if (!notifier) {
result = VMCI_ERROR_NO_MEM;
goto out;
diff --git a/drivers/misc/vmw_vmci/vmci_host.c
b/drivers/misc/vmw_vmci/vmci_host.c
index fb3d92a21afa..12419f97681d 100644
--- a/drivers/misc/vmw_vmci/vmci_host.c
+++ b/drivers/misc/vmw_vmci/vmci_host.c
@@ -124,7 +124,7 @@ static int vmci_host_open(struct inode *inode, struct file
*filp)
{
struct vmci_host_dev *vmci_host_dev;
- vmci_host_dev = kzalloc(sizeof(struct vmci_host_dev), GFP_KERNEL);
+ vmci_host_dev = kzalloc(sizeof(*vmci_host_dev), GFP_KERNEL);
if (vmci_host_dev == NULL)
return -ENOMEM;
--
2.16.2
[PATCH 1/2] VMCI: Use memdup_user() rather than duplicating its implementation
From: Markus Elfring
Date: Mon, 5 Mar 2018 16:40:29 +0100
* Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
* Return directly after this function call failed at the beginning.
* Delete the label "out" which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
drivers/misc/vmw_vmci/vmci_host.c | 19 ---
1 file changed, 4 insertions(+), 15 deletions(-)
diff --git a/drivers/misc/vmw_vmci/vmci_host.c
b/drivers/misc/vmw_vmci/vmci_host.c
index 83e0c95d20a4..fb3d92a21afa 100644
--- a/drivers/misc/vmw_vmci/vmci_host.c
+++ b/drivers/misc/vmw_vmci/vmci_host.c
@@ -754,27 +754,16 @@ static int vmci_host_do_ctx_set_cpt_state(struct
vmci_host_dev *vmci_host_dev,
if (copy_from_user(&set_info, uptr, sizeof(set_info)))
return -EFAULT;
- cpt_buf = kmalloc(set_info.buf_size, GFP_KERNEL);
- if (!cpt_buf) {
- vmci_ioctl_err(
- "cannot allocate memory to set cpt state (type=%d)\n",
- set_info.cpt_type);
- return -ENOMEM;
- }
-
- if (copy_from_user(cpt_buf, (void __user *)(uintptr_t)set_info.cpt_buf,
- set_info.buf_size)) {
- retval = -EFAULT;
- goto out;
- }
+ cpt_buf = memdup_user((void __user *)(uintptr_t)set_info.cpt_buf,
+ set_info.buf_size);
+ if (IS_ERR(cpt_buf))
+ return PTR_ERR(cpt_buf);
cid = vmci_ctx_get_id(vmci_host_dev->context);
set_info.result = vmci_ctx_set_chkpt_state(cid, set_info.cpt_type,
set_info.buf_size, cpt_buf);
retval = copy_to_user(uptr, &set_info, sizeof(set_info)) ? -EFAULT : 0;
-
-out:
kfree(cpt_buf);
return retval;
}
--
2.16.2
[PATCH 0/2] VMCI: Adjustments for three function implementations
From: Markus Elfring Date: Mon, 5 Mar 2018 17:12:34 +0100 Two update suggestions were taken into account from static source code analysis. Markus Elfring (2): Use memdup_user() rather than duplicating its implementation Improve a size determination in two functions drivers/misc/vmw_vmci/vmci_context.c | 2 +- drivers/misc/vmw_vmci/vmci_host.c| 21 + 2 files changed, 6 insertions(+), 17 deletions(-) -- 2.16.2
[PATCH] crypto: ccp: Use memdup_user() rather than duplicating its implementation
From: Markus Elfring
Date: Mon, 5 Mar 2018 13:50:13 +0100
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/crypto/ccp/psp-dev.c | 15 +--
1 file changed, 1 insertion(+), 14 deletions(-)
diff --git a/drivers/crypto/ccp/psp-dev.c b/drivers/crypto/ccp/psp-dev.c
index fcfa5b1eae61..8255258cd040 100644
--- a/drivers/crypto/ccp/psp-dev.c
+++ b/drivers/crypto/ccp/psp-dev.c
@@ -367,8 +367,6 @@ static int sev_ioctl_do_pek_csr(struct sev_issue_cmd *argp)
void *psp_copy_user_blob(u64 __user uaddr, u32 len)
{
- void *data;
-
if (!uaddr || !len)
return ERR_PTR(-EINVAL);
@@ -376,18 +374,7 @@ void *psp_copy_user_blob(u64 __user uaddr, u32 len)
if (len > SEV_FW_BLOB_MAX_SIZE)
return ERR_PTR(-EINVAL);
- data = kmalloc(len, GFP_KERNEL);
- if (!data)
- return ERR_PTR(-ENOMEM);
-
- if (copy_from_user(data, (void __user *)(uintptr_t)uaddr, len))
- goto e_free;
-
- return data;
-
-e_free:
- kfree(data);
- return ERR_PTR(-EFAULT);
+ return memdup_user((void __user *)(uintptr_t)uaddr, len);
}
EXPORT_SYMBOL_GPL(psp_copy_user_blob);
--
2.16.2
[PATCH] mmc/core/block: Use memdup_user() rather than duplicating its implementation
From: Markus Elfring
Date: Mon, 5 Mar 2018 11:33:21 +0100
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/mmc/core/block.c | 15 ---
1 file changed, 4 insertions(+), 11 deletions(-)
diff --git a/drivers/mmc/core/block.c b/drivers/mmc/core/block.c
index 20135a5de748..4b09c7380e70 100644
--- a/drivers/mmc/core/block.c
+++ b/drivers/mmc/core/block.c
@@ -375,22 +375,15 @@ static struct mmc_blk_ioc_data
*mmc_blk_ioctl_copy_from_user(
return idata;
}
- idata->buf = kmalloc(idata->buf_bytes, GFP_KERNEL);
- if (!idata->buf) {
- err = -ENOMEM;
+ idata->buf = memdup_user((void __user *)(unsigned long)
+idata->ic.data_ptr, idata->buf_bytes);
+ if (IS_ERR(idata->buf)) {
+ err = PTR_ERR(idata->buf);
goto idata_err;
}
- if (copy_from_user(idata->buf, (void __user *)(unsigned long)
- idata->ic.data_ptr, idata->buf_bytes)) {
- err = -EFAULT;
- goto copy_err;
- }
-
return idata;
-copy_err:
- kfree(idata->buf);
idata_err:
kfree(idata);
out:
--
2.16.2
[PATCH 4/4] scsi: hpsa: Move a variable assignment in hpsa_big_passthru_ioctl()
From: Markus Elfring
Date: Sun, 4 Mar 2018 22:16:05 +0100
Move an assignment for the local variable "sg_used" so that its setting
will only be performed after corresponding memory allocations succeeded
by this function.
Signed-off-by: Markus Elfring
---
drivers/scsi/hpsa.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 86d371ab39e7..bb6df194ac31 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6380,7 +6380,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
unsigned char **buff;
int *buff_size;
u64 temp64;
- BYTE sg_used = 0;
+ BYTE sg_used;
int status;
u32 left;
u32 sz;
@@ -6420,6 +6420,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
}
left = ioc->buf_size;
data_ptr = ioc->buf;
+ sg_used = 0;
while (left) {
sz = (left > ioc->malloc_size) ? ioc->malloc_size : left;
buff_size[sg_used] = sz;
--
2.16.2
[PATCH 3/4] scsi: hpsa: Delete an unnecessary initialisation in hpsa_big_passthru_ioctl()
From: Markus Elfring Date: Sun, 4 Mar 2018 22:02:10 +0100 The variable "status" will be set to an appropriate value a bit later. Thus omit the explicit initialisation at the beginning. Signed-off-by: Markus Elfring --- drivers/scsi/hpsa.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c index 45177ead811f..86d371ab39e7 100644 --- a/drivers/scsi/hpsa.c +++ b/drivers/scsi/hpsa.c @@ -6381,7 +6381,7 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h, void __user *argp) int *buff_size; u64 temp64; BYTE sg_used = 0; - int status = 0; + int status; u32 left; u32 sz; BYTE __user *data_ptr; -- 2.16.2
[PATCH 2/4] scsi: hpsa: Less function calls in hpsa_big_passthru_ioctl() after error detection
From: Markus Elfring
Date: Sun, 4 Mar 2018 22:00:19 +0100
The function "kfree" was called in a few cases by
the hpsa_big_passthru_ioctl() function during error handling
even if the passed variable contained a null pointer.
* Adjust jump targets.
* Delete two initialisations and a check (for the local variable "buff")
which became unnecessary with this refactoring.
Signed-off-by: Markus Elfring
---
drivers/scsi/hpsa.c | 26 ++
1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index b35248becef9..45177ead811f 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6377,8 +6377,8 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
{
BIG_IOCTL_Command_struct *ioc;
struct CommandList *c;
- unsigned char **buff = NULL;
- int *buff_size = NULL;
+ unsigned char **buff;
+ int *buff_size;
u64 temp64;
BYTE sg_used = 0;
int status = 0;
@@ -6397,26 +6397,26 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
if ((ioc->buf_size < 1) &&
(ioc->Request.Type.Direction != XFER_NONE)) {
status = -EINVAL;
- goto cleanup1;
+ goto free_ioc;
}
/* Check kmalloc limits using all SGs */
if (ioc->malloc_size > MAX_KMALLOC_SIZE) {
status = -EINVAL;
- goto cleanup1;
+ goto free_ioc;
}
if (ioc->buf_size > ioc->malloc_size * SG_ENTRIES_IN_CMD) {
status = -EINVAL;
- goto cleanup1;
- }
- buff = kzalloc(SG_ENTRIES_IN_CMD * sizeof(char *), GFP_KERNEL);
- if (!buff) {
- status = -ENOMEM;
- goto cleanup1;
+ goto free_ioc;
}
buff_size = kmalloc(SG_ENTRIES_IN_CMD * sizeof(int), GFP_KERNEL);
if (!buff_size) {
status = -ENOMEM;
- goto cleanup1;
+ goto free_ioc;
+ }
+ buff = kzalloc(SG_ENTRIES_IN_CMD * sizeof(char *), GFP_KERNEL);
+ if (!buff) {
+ status = -ENOMEM;
+ goto free_buff_size;
}
left = ioc->buf_size;
data_ptr = ioc->buf;
@@ -6501,14 +6501,16 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
cleanup0:
cmd_free(h, c);
cleanup1:
- if (buff) {
+ {
int i;
for (i = 0; i < sg_used; i++)
kfree(buff[i]);
kfree(buff);
}
+free_buff_size:
kfree(buff_size);
+free_ioc:
kfree(ioc);
return status;
}
--
2.16.2
[PATCH 1/4] scsi: hpsa: Use memdup_user() rather than duplicating its implementation
From: Markus Elfring
Date: Sun, 4 Mar 2018 21:19:52 +0100
* Reuse existing functionality from memdup_user() instead of keeping
duplicate source code.
This issue was detected by using the Coccinelle software.
* Return directly after this function call failed at the beginning.
Signed-off-by: Markus Elfring
---
drivers/scsi/hpsa.c | 13 -
1 file changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 5293e6827ce5..b35248becef9 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -6390,15 +6390,10 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
void __user *argp)
return -EINVAL;
if (!capable(CAP_SYS_RAWIO))
return -EPERM;
- ioc = kmalloc(sizeof(*ioc), GFP_KERNEL);
- if (!ioc) {
- status = -ENOMEM;
- goto cleanup1;
- }
- if (copy_from_user(ioc, argp, sizeof(*ioc))) {
- status = -EFAULT;
- goto cleanup1;
- }
+ ioc = memdup_user(argp, sizeof(*ioc));
+ if (IS_ERR(ioc))
+ return PTR_ERR(ioc);
+
if ((ioc->buf_size < 1) &&
(ioc->Request.Type.Direction != XFER_NONE)) {
status = -EINVAL;
--
2.16.2
[PATCH 0/4] SCSI-HPSA: Adjustments for hpsa_big_passthru_ioctl()
From: Markus Elfring Date: Mon, 5 Mar 2018 09:14:32 +0100 A few update suggestions were taken into account from static source code analysis. Markus Elfring (4): Use memdup_user() rather than duplicating its implementation Less function calls in hpsa_big_passthru_ioctl() after error detection Delete an unnecessary initialisation Move a variable assignment drivers/scsi/hpsa.c | 44 +--- 1 file changed, 21 insertions(+), 23 deletions(-) -- 2.16.2
Re: [v2] [media] Use common error handling code in 20 functions
>> +put_isp:
>> +omap3isp_put(video->isp);
>> +delete_fh:
>> +v4l2_fh_del(&handle->vfh);
>> +v4l2_fh_exit(&handle->vfh);
>> +kfree(handle);
>
> Please prefix the error labels with error_.
How often do you really need such an extra prefix?
>> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
>> @@ -994,10 +994,8 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, struct v4l2_queryctrl qc = { .id = ctrl->id };
>>
>> ret = uvc_query_v4l2_ctrl(chain, &qc);
>> -if (ret < 0) {
>> -ctrls->error_idx = i;
>> -return ret;
>> -}
>> +if (ret < 0)
>> +goto set_index;
>>
>> ctrl->value = qc.default_value;
>> }
>> @@ -1013,14 +1011,17 @@ static int uvc_ioctl_g_ext_ctrls(struct file *file,
>> void *fh, ret = uvc_ctrl_get(chain, ctrl);
>> if (ret < 0) {
>> uvc_ctrl_rollback(handle);
>> -ctrls->error_idx = i;
>> -return ret;
>> +goto set_index;
>> }
>> }
>>
>> ctrls->error_idx = 0;
>>
>> return uvc_ctrl_rollback(handle);
>> +
>> +set_index:
>> +ctrls->error_idx = i;
>> +return ret;
>> }
>
> For uvcvideo I find this to hinder readability
I got an other development view.
> without adding much added value.
There can be a small effect for such a function implementation.
> Please drop the uvcvideo change from this patch.
Would it be nice if this source code adjustment could be integrated also?
Regards,
Markus
Re: [0/8] target-iSCSI: Adjustments for several function implementations
>> Can a passed null pointer really work in this function?
>>
>> https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751
>>
>> static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
>> {
>> return &tfm->base;
>> }
>
> Yes. It's not a dereference,
Do any processors treat the zero address still special there?
> it's just doing pointer math to get the address.
Can eventually happen anything unexpected?
Can it be nicer to avoid such a software behaviour concern generally
just by adjusting a few jump labels (as I proposed it)?
Regards,
Markus
Re: [0/8] target-iSCSI: Adjustments for several function implementations
> Calling crypto_free_shash(NULL) is actually fine.
Really?
> It doesn't dereference the parameter, it just does pointer math on it in
> crypto_shash_tfm() and returns if it's NULL in crypto_destroy_tfm().
Can a passed null pointer really work in this function?
https://elixir.bootlin.com/linux/v4.16-rc2/source/include/crypto/hash.h#L684
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/crypto/hash.h?id=0f9da844d87796ac31b04e81ee95e155e9043132#n751
static inline struct crypto_tfm *crypto_shash_tfm(struct crypto_shash *tfm)
{
return &tfm->base;
}
Regards,
Markus
Re: [2/2] crypto: bcm: One function call less in do_shash() after error detection
> This patch is pointless as kfree on NULL is a no-op. I prefer to avoid unnecessary function calls generally. Regards, Markus
Re: [0/8] target-iSCSI: Adjustments for several function implementations
> You're 1/8 patch had an actual bug fix hidden amongst the style churn. It showed the general possibility to adjust the source code structure for the function “chap_server_compute_md5” also because of the usage of the single jump label “out” before. > I don't see any such fixes in the other patches. This view is appropriate. Further update steps show different transformation possibilities. > My opinion from https://www.spinics.net/lists/target-devel/msg16342.html > hasn't changed. FWIW, I'd prefer to see LIO adopt a policy similar to: > https://btrfs.wiki.kernel.org/index.php/Developer%27s_FAQ#How_not_to_start It seems that you express a few aspects for general change resistance. Will the circumstances evolve for similar software improvements? Regards, Markus
Re: [PATCH 2/2] crypto: omap: Improve a size determination in three functions
>> @@ -1032,14 +1032,13 @@ static int omap_aes_get_res_pdev(struct omap_aes_dev
>> *dd,
>> static int omap_aes_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> -struct omap_aes_dev *dd;
>> struct crypto_alg *algp;
>> struct aead_alg *aalg;
>> struct resource res;
>> int err = -ENOMEM, i, j, irq = -1;
>> u32 reg;
>> +struct omap_aes_dev *dd = devm_kzalloc(dev, sizeof(*dd), GFP_KERNEL);
>>
>> -dd = devm_kzalloc(dev, sizeof(struct omap_aes_dev), GFP_KERNEL);
>
> I'm fine with sizeof(*dd)
Thanks for your feedback.
> but please don't combine the allocation with the declaration.
Why do you not like such an implementation detail?
Regards,
Markus
[PATCH v2] [media] Delete unnecessary variable initialisations in seven functions
From: Markus Elfring
Date: Thu, 22 Feb 2018 21:45:47 +0100
Some local variables will be set to an appropriate value before usage.
Thus omit explicit initialisations at the beginning of these functions.
Signed-off-by: Markus Elfring
---
v2:
Hans Verkuil insisted on patch squashing. Thus some changes
were recombined based on source files from Linux next-20180216.
drivers/media/radio/radio-mr800.c | 2 +-
drivers/media/radio/radio-wl1273.c| 2 +-
drivers/media/radio/si470x/radio-si470x-usb.c | 2 +-
drivers/media/usb/cx231xx/cx231xx-cards.c | 2 +-
drivers/media/usb/cx231xx/cx231xx-dvb.c | 2 +-
drivers/media/usb/go7007/snd-go7007.c | 2 +-
drivers/media/usb/tm6000/tm6000-cards.c | 2 +-
7 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/media/radio/radio-mr800.c
b/drivers/media/radio/radio-mr800.c
index dc6c4f985911..0f292c6ba338 100644
--- a/drivers/media/radio/radio-mr800.c
+++ b/drivers/media/radio/radio-mr800.c
@@ -511,5 +511,5 @@ static int usb_amradio_probe(struct usb_interface *intf,
const struct usb_device_id *id)
{
struct amradio_device *radio;
- int retval = 0;
+ int retval;
diff --git a/drivers/media/radio/radio-wl1273.c
b/drivers/media/radio/radio-wl1273.c
index 58e944591602..8f9f8dfc3497 100644
--- a/drivers/media/radio/radio-wl1273.c
+++ b/drivers/media/radio/radio-wl1273.c
@@ -671,6 +671,6 @@ static int wl1273_fm_start(struct wl1273_device *radio, int
new_mode)
static int wl1273_fm_suspend(struct wl1273_device *radio)
{
struct wl1273_core *core = radio->core;
- int r = 0;
+ int r;
/* Cannot go from OFF to SUSPENDED */
diff --git a/drivers/media/radio/si470x/radio-si470x-usb.c
b/drivers/media/radio/si470x/radio-si470x-usb.c
index c311f9951d80..2277e850bb5e 100644
--- a/drivers/media/radio/si470x/radio-si470x-usb.c
+++ b/drivers/media/radio/si470x/radio-si470x-usb.c
@@ -578,6 +578,6 @@ static int si470x_usb_driver_probe(struct usb_interface
*intf,
struct si470x_device *radio;
struct usb_host_interface *iface_desc;
struct usb_endpoint_descriptor *endpoint;
- int i, int_end_size, retval = 0;
+ int i, int_end_size, retval;
unsigned char version_warning = 0;
diff --git a/drivers/media/usb/cx231xx/cx231xx-cards.c
b/drivers/media/usb/cx231xx/cx231xx-cards.c
index f9ec7fedcd5b..14e3814f55d9 100644
--- a/drivers/media/usb/cx231xx/cx231xx-cards.c
+++ b/drivers/media/usb/cx231xx/cx231xx-cards.c
@@ -1135,6 +1135,6 @@ static void cx231xx_config_tuner(struct cx231xx *dev)
static int read_eeprom(struct cx231xx *dev, struct i2c_client *client,
u8 *eedata, int len)
{
- int ret = 0;
+ int ret;
u8 start_offset = 0;
int len_todo = len;
diff --git a/drivers/media/usb/cx231xx/cx231xx-dvb.c
b/drivers/media/usb/cx231xx/cx231xx-dvb.c
index fb5654062b1a..24ca011c49bb 100644
--- a/drivers/media/usb/cx231xx/cx231xx-dvb.c
+++ b/drivers/media/usb/cx231xx/cx231xx-dvb.c
@@ -604,6 +604,6 @@ static void unregister_dvb(struct cx231xx_dvb *dvb)
static int dvb_init(struct cx231xx *dev)
{
- int result = 0;
+ int result;
struct cx231xx_dvb *dvb;
struct i2c_adapter *tuner_i2c;
diff --git a/drivers/media/usb/go7007/snd-go7007.c
b/drivers/media/usb/go7007/snd-go7007.c
index c618764480c6..f84a2130f033 100644
--- a/drivers/media/usb/go7007/snd-go7007.c
+++ b/drivers/media/usb/go7007/snd-go7007.c
@@ -227,7 +227,7 @@ int go7007_snd_init(struct go7007 *go)
{
static int dev;
struct go7007_snd *gosnd;
- int ret = 0;
+ int ret;
if (dev >= SNDRV_CARDS)
return -ENODEV;
diff --git a/drivers/media/usb/tm6000/tm6000-cards.c
b/drivers/media/usb/tm6000/tm6000-cards.c
index 4d5f4cc4887e..70939e96b856 100644
--- a/drivers/media/usb/tm6000/tm6000-cards.c
+++ b/drivers/media/usb/tm6000/tm6000-cards.c
@@ -1174,7 +1174,7 @@ static int tm6000_usb_probe(struct usb_interface
*interface,
{
struct usb_device *usbdev;
struct tm6000_core *dev;
- int i, rc = 0;
+ int i, rc;
int nr = 0;
char *speed;
--
2.16.2
Re: [PATCH 0/8] target-iSCSI: Adjustments for several function implementations
> Date: Tue, 12 Dec 2017 22:22:11 +0100 > > Some update suggestions were taken into account > from static source code analysis. > > Markus Elfring (8): > Less function calls in chap_server_compute_md5() after error detection > Move resetting of seven variables in chap_server_compute_md5() > Delete 36 error messages for a failed memory allocation > Delete an unnecessary variable initialisation in iscsit_allocate_ooo_cmdsn() > Delete an unnecessary variable initialisation in iscsi_copy_param_list() > Delete an unnecessary variable initialisation in > iscsi_create_default_params() > Delete an unnecessary variable initialisation in iscsi_set_default_param() > Improve 16 size determinations > > drivers/target/iscsi/iscsi_target.c | 2 - > drivers/target/iscsi/iscsi_target_auth.c | 110 > +++--- > drivers/target/iscsi/iscsi_target_datain_values.c | 6 +- > drivers/target/iscsi/iscsi_target_erl1.c | 14 +-- > drivers/target/iscsi/iscsi_target_erl2.c | 8 +- > drivers/target/iscsi/iscsi_target_login.c | 29 ++ > drivers/target/iscsi/iscsi_target_nego.c | 4 +- > drivers/target/iscsi/iscsi_target_parameters.c| 58 +--- > drivers/target/iscsi/iscsi_target_seq_pdu_list.c | 34 +++ > drivers/target/iscsi/iscsi_target_tpg.c | 13 +-- > drivers/target/iscsi/iscsi_target_util.c | 11 +-- > 11 files changed, 118 insertions(+), 171 deletions(-) One of these update suggestions resulted in the commit “target: avoid NULL dereference in CHAP auth error path” which is considered for integration into Linux stable versions now. https://patchwork.kernel.org/patch/10110459/ https://lkml.kernel.org/r/<20171213172230.12767-1-dd...@suse.de> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/target/iscsi/iscsi_target_auth.c?id=ce512d79d0466a604793addb6b769d12ee326822 Now I am curious if more remaining change possibilities can be picked up from this patch series. * Would you like to improve any more implementation details? * Do you need additional explanations for further benefits? Regards, Markus
[PATCH v2] [media] Use common error handling code in 20 functions
From: Markus Elfring
Date: Mon, 19 Feb 2018 18:50:40 +0100
Adjust jump targets so that a bit of exception handling can be better
reused at the end of these functions.
This issue was partly detected by using the Coccinelle software.
Signed-off-by: Markus Elfring
---
v2:
Hans Verkuil insisted on patch squashing. Thus several changes
were recombined based on source files from Linux next-20180216.
The implementation of the function "tda8261_set_params" was improved
after a notification by Christoph Böhmwalder on 2017-09-26.
drivers/media/dvb-core/dmxdev.c| 16
drivers/media/dvb-frontends/tda1004x.c | 20 ++
drivers/media/dvb-frontends/tda8261.c | 19 ++
drivers/media/pci/bt8xx/dst.c | 19 ++
drivers/media/pci/bt8xx/dst_ca.c | 30 +++
drivers/media/pci/cx88/cx88-input.c| 17 +
drivers/media/platform/omap3isp/ispvideo.c | 29 +++
.../media/platform/qcom/camss-8x16/camss-csid.c| 20 +-
drivers/media/tuners/tuner-xc2028.c| 30 +++
drivers/media/usb/cpia2/cpia2_usb.c| 13 ---
drivers/media/usb/gspca/gspca.c| 17 +
drivers/media/usb/gspca/sn9c20x.c | 17 +
drivers/media/usb/pvrusb2/pvrusb2-ioread.c | 10 +++--
drivers/media/usb/tm6000/tm6000-cards.c| 7 ++--
drivers/media/usb/tm6000/tm6000-dvb.c | 11 --
drivers/media/usb/tm6000/tm6000-video.c| 13 ---
drivers/media/usb/ttusb-budget/dvb-ttusb-budget.c | 13 +++
drivers/media/usb/ttusb-dec/ttusb_dec.c| 43 --
drivers/media/usb/uvc/uvc_v4l2.c | 13 ---
19 files changed, 180 insertions(+), 177 deletions(-)
diff --git a/drivers/media/dvb-core/dmxdev.c b/drivers/media/dvb-core/dmxdev.c
index 6d53af00190e..6a0411c91195 100644
--- a/drivers/media/dvb-core/dmxdev.c
+++ b/drivers/media/dvb-core/dmxdev.c
@@ -645,18 +645,18 @@ static int dvb_dmxdev_start_feed(struct dmxdev *dmxdev,
tsfeed->priv = filter;
ret = tsfeed->set(tsfeed, feed->pid, ts_type, ts_pes, timeout);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
ret = tsfeed->start_filtering(tsfeed);
- if (ret < 0) {
- dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
- return ret;
- }
+ if (ret < 0)
+ goto release_feed;
return 0;
+
+release_feed:
+ dmxdev->demux->release_ts_feed(dmxdev->demux, tsfeed);
+ return ret;
}
static int dvb_dmxdev_filter_start(struct dmxdev_filter *filter)
diff --git a/drivers/media/dvb-frontends/tda1004x.c
b/drivers/media/dvb-frontends/tda1004x.c
index 58e3beff5adc..85ca111fc8c4 100644
--- a/drivers/media/dvb-frontends/tda1004x.c
+++ b/drivers/media/dvb-frontends/tda1004x.c
@@ -1299,20 +1299,22 @@ struct dvb_frontend* tda10045_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10045: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x25) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10045_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
static const struct dvb_frontend_ops tda10046_ops = {
@@ -1369,19 +1371,21 @@ struct dvb_frontend* tda10046_attach(const struct
tda1004x_config* config,
id = tda1004x_read_byte(state, TDA1004X_CHIPID);
if (id < 0) {
printk(KERN_ERR "tda10046: chip is not answering. Giving
up.\n");
- kfree(state);
- return NULL;
+ goto free_state;
}
if (id != 0x46) {
printk(KERN_ERR "Invalid tda1004x ID = 0x%02x. Can't
proceed\n", id);
- kfree(state);
- return NULL;
+ goto free_state;
}
/* create dvb_frontend */
memcpy(&state->frontend.ops, &tda10046_ops, sizeof(struct
dvb_frontend_ops));
state->frontend.demodulator_priv = state;
return &state->frontend;
+
+free_state:
+ kfree(state);
+ return NULL;
}
module_param(debug, int, 0644);
diff --git a/drivers/media/dvb-frontends/tda8261.c
b/drivers/media/dvb-frontends/tda8261.c
index f72a54e7eb23..7b9a29
[PATCH] libata-scsi: Delete an unnecessary variable in ata_scsi_slave_config()
From: Markus Elfring
Date: Fri, 16 Feb 2018 18:19:13 +0100
* Return a result code without storing it in an intermediate variable.
* Reduce the needed source code.
Signed-off-by: Markus Elfring
---
drivers/ata/libata-scsi.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 66be961c93a4..93456cff051b 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1345,14 +1345,9 @@ int ata_scsi_slave_config(struct scsi_device *sdev)
{
struct ata_port *ap = ata_shost_to_port(sdev->host);
struct ata_device *dev = __ata_scsi_find_dev(ap, sdev);
- int rc = 0;
ata_scsi_sdev_config(sdev);
-
- if (dev)
- rc = ata_scsi_dev_config(sdev, dev);
-
- return rc;
+ return dev ? ata_scsi_dev_config(sdev, dev) : 0;
}
/**
--
2.16.1
[PATCH 3/3] pata_arasan_cf: Move two variable assignments in arasan_cf_probe()
From: Markus Elfring
Date: Fri, 16 Feb 2018 16:42:26 +0100
Move assignments for the local variables "irq_handler" and "pdata"
so that their setting will only be performed after a call
of the function "devm_kzalloc" succeeded by this function.
Thus adjust a corresponding if statement.
Signed-off-by: Markus Elfring
---
drivers/ata/pata_arasan_cf.c | 11 +++
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/ata/pata_arasan_cf.c b/drivers/ata/pata_arasan_cf.c
index ebecab8c3f36..27f241b1b1b1 100644
--- a/drivers/ata/pata_arasan_cf.c
+++ b/drivers/ata/pata_arasan_cf.c
@@ -790,12 +790,12 @@ static struct ata_port_operations arasan_cf_ops = {
static int arasan_cf_probe(struct platform_device *pdev)
{
struct arasan_cf_dev *acdev;
- struct arasan_cf_pdata *pdata = dev_get_platdata(&pdev->dev);
+ struct arasan_cf_pdata *pdata;
struct ata_host *host;
struct ata_port *ap;
struct resource *res;
u32 quirk;
- irq_handler_t irq_handler = NULL;
+ irq_handler_t irq_handler;
int ret;
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -812,6 +812,7 @@ static int arasan_cf_probe(struct platform_device *pdev)
if (!acdev)
return -ENOMEM;
+ pdata = dev_get_platdata(&pdev->dev);
if (pdata)
quirk = pdata->quirk;
else
@@ -819,10 +820,12 @@ static int arasan_cf_probe(struct platform_device *pdev)
/* if irq is 0, support only PIO */
acdev->irq = platform_get_irq(pdev, 0);
- if (acdev->irq)
+ if (acdev->irq) {
irq_handler = arasan_cf_interrupt;
- else
+ } else {
quirk |= CF_BROKEN_MWDMA | CF_BROKEN_UDMA;
+ irq_handler = NULL;
+ }
acdev->pbase = res->start;
acdev->vbase = devm_ioremap_nocache(&pdev->dev, res->start,
--
2.16.1
