* re: pinctrl: core: Handling pinmux and pinconf separately
@ 2021-03-11 10:57 Colin Ian King
2021-03-11 11:16 ` Michal Simek
0 siblings, 1 reply; 8+ messages in thread
From: Colin Ian King @ 2021-03-11 10:57 UTC (permalink / raw)
To: Michal Simek; +Cc: Linus Walleij, linux-gpio, linux-kernel
Hi,
Static analysis on linux-next with Coverity has found a potential issue
in drivers/pinctrl/core.c with the following commit:
commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
Author: Michal Simek <michal.simek@xilinx.com>
Date: Wed Mar 10 09:16:54 2021 +0100
pinctrl: core: Handling pinmux and pinconf separately
The analysis is as follows:
1234 /**
1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
to HW
1236 * @p: the pinctrl handle for the device that requests configuration
1237 * @state: the state handle to select/activate/program
1238 */
1239 static int pinctrl_commit_state(struct pinctrl *p, struct
pinctrl_state *state)
1240 {
1241 struct pinctrl_setting *setting, *setting2;
1242 struct pinctrl_state *old_state = p->state;
1. var_decl: Declaring variable ret without initializer.
1243 int ret;
1244
2. Condition p->state, taking true branch.
1245 if (p->state) {
1246 /*
1247 * For each pinmux setting in the old state, forget
SW's record
1248 * of mux owner for that pingroup. Any pingroups
which are
1249 * still owned by the new state will be re-acquired
by the call
1250 * to pinmux_enable_setting() in the loop below.
1251 */
3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
4. Condition !(&setting->node == &p->state->settings), taking true
branch.
7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
8. Condition !(&setting->node == &p->state->settings), taking true
branch.
11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
12. Condition !(&setting->node == &p->state->settings), taking false
branch.
1252 list_for_each_entry(setting, &p->state->settings,
node) {
5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
branch.
9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
branch.
1253 if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
6. Continuing loop.
10. Continuing loop.
1254 continue;
1255 pinmux_disable_setting(setting);
1256 }
1257 }
1258
1259 p->state = NULL;
1260
1261 /* Apply all the settings for the new state - pinmux first */
13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
14. Condition !(&setting->node == &state->settings), taking true branch.
1262 list_for_each_entry(setting, &state->settings, node) {
15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
1263 switch (setting->type) {
1264 case PIN_MAP_TYPE_MUX_GROUP:
1265 ret = pinmux_enable_setting(setting);
1266 break;
1267 case PIN_MAP_TYPE_CONFIGS_PIN:
1268 case PIN_MAP_TYPE_CONFIGS_GROUP:
16. Breaking from switch.
1269 break;
1270 default:
1271 ret = -EINVAL;
1272 break;
1273 }
1274
Uninitialized scalar variable (UNINIT)
17. uninit_use: Using uninitialized value ret.
1275 if (ret < 0)
1276 goto unapply_new_state;
For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
setting->type cases the loop can break out with ret not being set. Since
ret has not been initialized it the ret < 0 check is checking against an
uninitialized value.
I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
the value of ret should be set to (is it an error condition or not?). Or
should ret be initialized to 0 or a default error value at the start of
the function.
Hence I'm reporting this issue.
Colin
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 10:57 pinctrl: core: Handling pinmux and pinconf separately Colin Ian King
@ 2021-03-11 11:16 ` Michal Simek
2021-03-11 11:24 ` Colin Ian King
0 siblings, 1 reply; 8+ messages in thread
From: Michal Simek @ 2021-03-11 11:16 UTC (permalink / raw)
To: Colin Ian King, Michal Simek; +Cc: Linus Walleij, linux-gpio, linux-kernel
On 3/11/21 11:57 AM, Colin Ian King wrote:
> Hi,
>
> Static analysis on linux-next with Coverity has found a potential issue
> in drivers/pinctrl/core.c with the following commit:
>
> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
> Author: Michal Simek <michal.simek@xilinx.com>
> Date: Wed Mar 10 09:16:54 2021 +0100
>
> pinctrl: core: Handling pinmux and pinconf separately
>
> The analysis is as follows:
>
> 1234 /**
> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
> to HW
> 1236 * @p: the pinctrl handle for the device that requests configuration
> 1237 * @state: the state handle to select/activate/program
> 1238 */
> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
> pinctrl_state *state)
> 1240 {
> 1241 struct pinctrl_setting *setting, *setting2;
> 1242 struct pinctrl_state *old_state = p->state;
>
> 1. var_decl: Declaring variable ret without initializer.
>
> 1243 int ret;
> 1244
>
> 2. Condition p->state, taking true branch.
>
> 1245 if (p->state) {
> 1246 /*
> 1247 * For each pinmux setting in the old state, forget
> SW's record
> 1248 * of mux owner for that pingroup. Any pingroups
> which are
> 1249 * still owned by the new state will be re-acquired
> by the call
> 1250 * to pinmux_enable_setting() in the loop below.
> 1251 */
>
> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 4. Condition !(&setting->node == &p->state->settings), taking true
> branch.
> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 8. Condition !(&setting->node == &p->state->settings), taking true
> branch.
> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 12. Condition !(&setting->node == &p->state->settings), taking false
> branch.
>
> 1252 list_for_each_entry(setting, &p->state->settings,
> node) {
>
> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
> branch.
> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
> branch.
> 1253 if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> 6. Continuing loop.
> 10. Continuing loop.
>
> 1254 continue;
> 1255 pinmux_disable_setting(setting);
> 1256 }
> 1257 }
> 1258
> 1259 p->state = NULL;
> 1260
> 1261 /* Apply all the settings for the new state - pinmux first */
>
> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 14. Condition !(&setting->node == &state->settings), taking true branch.
> 1262 list_for_each_entry(setting, &state->settings, node) {
> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>
> 1263 switch (setting->type) {
> 1264 case PIN_MAP_TYPE_MUX_GROUP:
> 1265 ret = pinmux_enable_setting(setting);
> 1266 break;
> 1267 case PIN_MAP_TYPE_CONFIGS_PIN:
> 1268 case PIN_MAP_TYPE_CONFIGS_GROUP:
>
> 16. Breaking from switch.
>
> 1269 break;
> 1270 default:
> 1271 ret = -EINVAL;
> 1272 break;
> 1273 }
> 1274
>
> Uninitialized scalar variable (UNINIT)
> 17. uninit_use: Using uninitialized value ret.
>
> 1275 if (ret < 0)
> 1276 goto unapply_new_state;
>
> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
> setting->type cases the loop can break out with ret not being set. Since
> ret has not been initialized it the ret < 0 check is checking against an
> uninitialized value.
>
> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
> the value of ret should be set to (is it an error condition or not?). Or
> should ret be initialized to 0 or a default error value at the start of
> the function.
>
> Hence I'm reporting this issue.
What about this? Is this passing static analysis?
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index f5c32d2a3c91..136c323d855e 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1266,6 +1266,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
struct pinctrl_state *state)
break;
case PIN_MAP_TYPE_CONFIGS_PIN:
case PIN_MAP_TYPE_CONFIGS_GROUP:
+ ret = 0;
break;
default:
ret = -EINVAL;
@@ -1284,6 +1285,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
struct pinctrl_state *state)
list_for_each_entry(setting, &state->settings, node) {
switch (setting->type) {
case PIN_MAP_TYPE_MUX_GROUP:
+ ret = 0;
break;
case PIN_MAP_TYPE_CONFIGS_PIN:
case PIN_MAP_TYPE_CONFIGS_GROUP:
Thanks,
Michal
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 11:16 ` Michal Simek
@ 2021-03-11 11:24 ` Colin Ian King
2021-03-11 11:28 ` Michal Simek
2021-03-12 12:45 ` Andy Shevchenko
0 siblings, 2 replies; 8+ messages in thread
From: Colin Ian King @ 2021-03-11 11:24 UTC (permalink / raw)
To: Michal Simek; +Cc: Linus Walleij, linux-gpio, linux-kernel
On 11/03/2021 11:16, Michal Simek wrote:
>
>
> On 3/11/21 11:57 AM, Colin Ian King wrote:
>> Hi,
>>
>> Static analysis on linux-next with Coverity has found a potential issue
>> in drivers/pinctrl/core.c with the following commit:
>>
>> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
>> Author: Michal Simek <michal.simek@xilinx.com>
>> Date: Wed Mar 10 09:16:54 2021 +0100
>>
>> pinctrl: core: Handling pinmux and pinconf separately
>>
>> The analysis is as follows:
>>
>> 1234 /**
>> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
>> to HW
>> 1236 * @p: the pinctrl handle for the device that requests configuration
>> 1237 * @state: the state handle to select/activate/program
>> 1238 */
>> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
>> pinctrl_state *state)
>> 1240 {
>> 1241 struct pinctrl_setting *setting, *setting2;
>> 1242 struct pinctrl_state *old_state = p->state;
>>
>> 1. var_decl: Declaring variable ret without initializer.
>>
>> 1243 int ret;
>> 1244
>>
>> 2. Condition p->state, taking true branch.
>>
>> 1245 if (p->state) {
>> 1246 /*
>> 1247 * For each pinmux setting in the old state, forget
>> SW's record
>> 1248 * of mux owner for that pingroup. Any pingroups
>> which are
>> 1249 * still owned by the new state will be re-acquired
>> by the call
>> 1250 * to pinmux_enable_setting() in the loop below.
>> 1251 */
>>
>> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 4. Condition !(&setting->node == &p->state->settings), taking true
>> branch.
>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 8. Condition !(&setting->node == &p->state->settings), taking true
>> branch.
>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 12. Condition !(&setting->node == &p->state->settings), taking false
>> branch.
>>
>> 1252 list_for_each_entry(setting, &p->state->settings,
>> node) {
>>
>> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>> branch.
>> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>> branch.
>> 1253 if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>> 6. Continuing loop.
>> 10. Continuing loop.
>>
>> 1254 continue;
>> 1255 pinmux_disable_setting(setting);
>> 1256 }
>> 1257 }
>> 1258
>> 1259 p->state = NULL;
>> 1260
>> 1261 /* Apply all the settings for the new state - pinmux first */
>>
>> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 14. Condition !(&setting->node == &state->settings), taking true branch.
>> 1262 list_for_each_entry(setting, &state->settings, node) {
>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>
>> 1263 switch (setting->type) {
>> 1264 case PIN_MAP_TYPE_MUX_GROUP:
>> 1265 ret = pinmux_enable_setting(setting);
>> 1266 break;
>> 1267 case PIN_MAP_TYPE_CONFIGS_PIN:
>> 1268 case PIN_MAP_TYPE_CONFIGS_GROUP:
>>
>> 16. Breaking from switch.
>>
>> 1269 break;
>> 1270 default:
>> 1271 ret = -EINVAL;
>> 1272 break;
>> 1273 }
>> 1274
>>
>> Uninitialized scalar variable (UNINIT)
>> 17. uninit_use: Using uninitialized value ret.
>>
>> 1275 if (ret < 0)
>> 1276 goto unapply_new_state;
>>
>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
>> setting->type cases the loop can break out with ret not being set. Since
>> ret has not been initialized it the ret < 0 check is checking against an
>> uninitialized value.
>>
>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
>> the value of ret should be set to (is it an error condition or not?). Or
>> should ret be initialized to 0 or a default error value at the start of
>> the function.
>>
>> Hence I'm reporting this issue.
>
> What about this? Is this passing static analysis?
It will take me 2 hours to re-run the analysis, but from eyeballing the
code I think the assignments will fix this.
Colin
>
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index f5c32d2a3c91..136c323d855e 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1266,6 +1266,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
> struct pinctrl_state *state)
> break;
> case PIN_MAP_TYPE_CONFIGS_PIN:
> case PIN_MAP_TYPE_CONFIGS_GROUP:
> + ret = 0;
> break;
> default:
> ret = -EINVAL;
> @@ -1284,6 +1285,7 @@ static int pinctrl_commit_state(struct pinctrl *p,
> struct pinctrl_state *state)
> list_for_each_entry(setting, &state->settings, node) {
> switch (setting->type) {
> case PIN_MAP_TYPE_MUX_GROUP:
> + ret = 0;
> break;
> case PIN_MAP_TYPE_CONFIGS_PIN:
> case PIN_MAP_TYPE_CONFIGS_GROUP:
>
> Thanks,
> Michal
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 11:24 ` Colin Ian King
@ 2021-03-11 11:28 ` Michal Simek
2021-03-11 14:03 ` Colin Ian King
2021-03-11 14:39 ` Linus Walleij
2021-03-12 12:45 ` Andy Shevchenko
1 sibling, 2 replies; 8+ messages in thread
From: Michal Simek @ 2021-03-11 11:28 UTC (permalink / raw)
To: Colin Ian King, Michal Simek; +Cc: Linus Walleij, linux-gpio, linux-kernel
On 3/11/21 12:24 PM, Colin Ian King wrote:
> On 11/03/2021 11:16, Michal Simek wrote:
>>
>>
>> On 3/11/21 11:57 AM, Colin Ian King wrote:
>>> Hi,
>>>
>>> Static analysis on linux-next with Coverity has found a potential issue
>>> in drivers/pinctrl/core.c with the following commit:
>>>
>>> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
>>> Author: Michal Simek <michal.simek@xilinx.com>
>>> Date: Wed Mar 10 09:16:54 2021 +0100
>>>
>>> pinctrl: core: Handling pinmux and pinconf separately
>>>
>>> The analysis is as follows:
>>>
>>> 1234 /**
>>> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
>>> to HW
>>> 1236 * @p: the pinctrl handle for the device that requests configuration
>>> 1237 * @state: the state handle to select/activate/program
>>> 1238 */
>>> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
>>> pinctrl_state *state)
>>> 1240 {
>>> 1241 struct pinctrl_setting *setting, *setting2;
>>> 1242 struct pinctrl_state *old_state = p->state;
>>>
>>> 1. var_decl: Declaring variable ret without initializer.
>>>
>>> 1243 int ret;
>>> 1244
>>>
>>> 2. Condition p->state, taking true branch.
>>>
>>> 1245 if (p->state) {
>>> 1246 /*
>>> 1247 * For each pinmux setting in the old state, forget
>>> SW's record
>>> 1248 * of mux owner for that pingroup. Any pingroups
>>> which are
>>> 1249 * still owned by the new state will be re-acquired
>>> by the call
>>> 1250 * to pinmux_enable_setting() in the loop below.
>>> 1251 */
>>>
>>> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 4. Condition !(&setting->node == &p->state->settings), taking true
>>> branch.
>>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 8. Condition !(&setting->node == &p->state->settings), taking true
>>> branch.
>>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 12. Condition !(&setting->node == &p->state->settings), taking false
>>> branch.
>>>
>>> 1252 list_for_each_entry(setting, &p->state->settings,
>>> node) {
>>>
>>> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>> branch.
>>> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>> branch.
>>> 1253 if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>> 6. Continuing loop.
>>> 10. Continuing loop.
>>>
>>> 1254 continue;
>>> 1255 pinmux_disable_setting(setting);
>>> 1256 }
>>> 1257 }
>>> 1258
>>> 1259 p->state = NULL;
>>> 1260
>>> 1261 /* Apply all the settings for the new state - pinmux first */
>>>
>>> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 14. Condition !(&setting->node == &state->settings), taking true branch.
>>> 1262 list_for_each_entry(setting, &state->settings, node) {
>>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>>
>>> 1263 switch (setting->type) {
>>> 1264 case PIN_MAP_TYPE_MUX_GROUP:
>>> 1265 ret = pinmux_enable_setting(setting);
>>> 1266 break;
>>> 1267 case PIN_MAP_TYPE_CONFIGS_PIN:
>>> 1268 case PIN_MAP_TYPE_CONFIGS_GROUP:
>>>
>>> 16. Breaking from switch.
>>>
>>> 1269 break;
>>> 1270 default:
>>> 1271 ret = -EINVAL;
>>> 1272 break;
>>> 1273 }
>>> 1274
>>>
>>> Uninitialized scalar variable (UNINIT)
>>> 17. uninit_use: Using uninitialized value ret.
>>>
>>> 1275 if (ret < 0)
>>> 1276 goto unapply_new_state;
>>>
>>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
>>> setting->type cases the loop can break out with ret not being set. Since
>>> ret has not been initialized it the ret < 0 check is checking against an
>>> uninitialized value.
>>>
>>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
>>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
>>> the value of ret should be set to (is it an error condition or not?). Or
>>> should ret be initialized to 0 or a default error value at the start of
>>> the function.
>>>
>>> Hence I'm reporting this issue.
>>
>> What about this? Is this passing static analysis?
>
> It will take me 2 hours to re-run the analysis, but from eyeballing the
> code I think the assignments will fix this.
would be good if you can rerun it and get back to us on this.
I will wait if something else will pop up and then will send v2 with
this that Linus can apply this one instead.
Thanks,
Michal
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 11:28 ` Michal Simek
@ 2021-03-11 14:03 ` Colin Ian King
2021-03-11 14:39 ` Linus Walleij
1 sibling, 0 replies; 8+ messages in thread
From: Colin Ian King @ 2021-03-11 14:03 UTC (permalink / raw)
To: Michal Simek; +Cc: Linus Walleij, linux-gpio, linux-kernel
On 11/03/2021 11:28, Michal Simek wrote:
>
>
> On 3/11/21 12:24 PM, Colin Ian King wrote:
>> On 11/03/2021 11:16, Michal Simek wrote:
>>>
>>>
>>> On 3/11/21 11:57 AM, Colin Ian King wrote:
>>>> Hi,
>>>>
>>>> Static analysis on linux-next with Coverity has found a potential issue
>>>> in drivers/pinctrl/core.c with the following commit:
>>>>
>>>> commit 0952b7ec1614abf232e921aac0cc2bca8e60e162
>>>> Author: Michal Simek <michal.simek@xilinx.com>
>>>> Date: Wed Mar 10 09:16:54 2021 +0100
>>>>
>>>> pinctrl: core: Handling pinmux and pinconf separately
>>>>
>>>> The analysis is as follows:
>>>>
>>>> 1234 /**
>>>> 1235 * pinctrl_commit_state() - select/activate/program a pinctrl state
>>>> to HW
>>>> 1236 * @p: the pinctrl handle for the device that requests configuration
>>>> 1237 * @state: the state handle to select/activate/program
>>>> 1238 */
>>>> 1239 static int pinctrl_commit_state(struct pinctrl *p, struct
>>>> pinctrl_state *state)
>>>> 1240 {
>>>> 1241 struct pinctrl_setting *setting, *setting2;
>>>> 1242 struct pinctrl_state *old_state = p->state;
>>>>
>>>> 1. var_decl: Declaring variable ret without initializer.
>>>>
>>>> 1243 int ret;
>>>> 1244
>>>>
>>>> 2. Condition p->state, taking true branch.
>>>>
>>>> 1245 if (p->state) {
>>>> 1246 /*
>>>> 1247 * For each pinmux setting in the old state, forget
>>>> SW's record
>>>> 1248 * of mux owner for that pingroup. Any pingroups
>>>> which are
>>>> 1249 * still owned by the new state will be re-acquired
>>>> by the call
>>>> 1250 * to pinmux_enable_setting() in the loop below.
>>>> 1251 */
>>>>
>>>> 3. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 4. Condition !(&setting->node == &p->state->settings), taking true
>>>> branch.
>>>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 8. Condition !(&setting->node == &p->state->settings), taking true
>>>> branch.
>>>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 12. Condition !(&setting->node == &p->state->settings), taking false
>>>> branch.
>>>>
>>>> 1252 list_for_each_entry(setting, &p->state->settings,
>>>> node) {
>>>>
>>>> 5. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>>> branch.
>>>> 9. Condition setting->type != PIN_MAP_TYPE_MUX_GROUP, taking true
>>>> branch.
>>>> 1253 if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>>> 6. Continuing loop.
>>>> 10. Continuing loop.
>>>>
>>>> 1254 continue;
>>>> 1255 pinmux_disable_setting(setting);
>>>> 1256 }
>>>> 1257 }
>>>> 1258
>>>> 1259 p->state = NULL;
>>>> 1260
>>>> 1261 /* Apply all the settings for the new state - pinmux first */
>>>>
>>>> 13. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 14. Condition !(&setting->node == &state->settings), taking true branch.
>>>> 1262 list_for_each_entry(setting, &state->settings, node) {
>>>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>>>
>>>> 1263 switch (setting->type) {
>>>> 1264 case PIN_MAP_TYPE_MUX_GROUP:
>>>> 1265 ret = pinmux_enable_setting(setting);
>>>> 1266 break;
>>>> 1267 case PIN_MAP_TYPE_CONFIGS_PIN:
>>>> 1268 case PIN_MAP_TYPE_CONFIGS_GROUP:
>>>>
>>>> 16. Breaking from switch.
>>>>
>>>> 1269 break;
>>>> 1270 default:
>>>> 1271 ret = -EINVAL;
>>>> 1272 break;
>>>> 1273 }
>>>> 1274
>>>>
>>>> Uninitialized scalar variable (UNINIT)
>>>> 17. uninit_use: Using uninitialized value ret.
>>>>
>>>> 1275 if (ret < 0)
>>>> 1276 goto unapply_new_state;
>>>>
>>>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
>>>> setting->type cases the loop can break out with ret not being set. Since
>>>> ret has not been initialized it the ret < 0 check is checking against an
>>>> uninitialized value.
>>>>
>>>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
>>>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
>>>> the value of ret should be set to (is it an error condition or not?). Or
>>>> should ret be initialized to 0 or a default error value at the start of
>>>> the function.
>>>>
>>>> Hence I'm reporting this issue.
>>>
>>> What about this? Is this passing static analysis?
>>
>> It will take me 2 hours to re-run the analysis, but from eyeballing the
>> code I think the assignments will fix this.
>
> would be good if you can rerun it and get back to us on this.
> I will wait if something else will pop up and then will send v2 with
> this that Linus can apply this one instead.
Yep, passed, fixes the issue found by Coverity.
>
> Thanks,
> Michal
>
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 11:28 ` Michal Simek
2021-03-11 14:03 ` Colin Ian King
@ 2021-03-11 14:39 ` Linus Walleij
1 sibling, 0 replies; 8+ messages in thread
From: Linus Walleij @ 2021-03-11 14:39 UTC (permalink / raw)
To: Michal Simek; +Cc: Colin Ian King, linux-gpio, linux-kernel
On Thu, Mar 11, 2021 at 12:28 PM Michal Simek <michal.simek@xilinx.com> wrote:
> > It will take me 2 hours to re-run the analysis, but from eyeballing the
> > code I think the assignments will fix this.
>
> would be good if you can rerun it and get back to us on this.
> I will wait if something else will pop up and then will send v2 with
> this that Linus can apply this one instead.
Just send an incremental patch, that reflects the issues found
in a nice way in the development history.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-11 11:24 ` Colin Ian King
2021-03-11 11:28 ` Michal Simek
@ 2021-03-12 12:45 ` Andy Shevchenko
2021-03-12 12:49 ` Colin Ian King
1 sibling, 1 reply; 8+ messages in thread
From: Andy Shevchenko @ 2021-03-12 12:45 UTC (permalink / raw)
To: Colin Ian King; +Cc: Michal Simek, Linus Walleij, linux-gpio, linux-kernel
On Thu, Mar 11, 2021 at 1:26 PM Colin Ian King <colin.king@canonical.com> wrote:
> On 11/03/2021 11:16, Michal Simek wrote:
> > On 3/11/21 11:57 AM, Colin Ian King wrote:
> >> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
> >> setting->type cases the loop can break out with ret not being set. Since
> >> ret has not been initialized it the ret < 0 check is checking against an
> >> uninitialized value.
> >>
> >> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
> >> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
> >> the value of ret should be set to (is it an error condition or not?). Or
> >> should ret be initialized to 0 or a default error value at the start of
> >> the function.
> >>
> >> Hence I'm reporting this issue.
> >
> > What about this? Is this passing static analysis?
>
> It will take me 2 hours to re-run the analysis, but from eyeballing the
> code I think the assignments will fix this.
It surprises me that tools in the 21st century can't run on a subset
of the data.
Had you filed a bug to the Coverity team that they will provide a way
to rerun analysis on a subset of the data?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: pinctrl: core: Handling pinmux and pinconf separately
2021-03-12 12:45 ` Andy Shevchenko
@ 2021-03-12 12:49 ` Colin Ian King
0 siblings, 0 replies; 8+ messages in thread
From: Colin Ian King @ 2021-03-12 12:49 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Michal Simek, Linus Walleij, linux-gpio, linux-kernel
On 12/03/2021 12:45, Andy Shevchenko wrote:
> On Thu, Mar 11, 2021 at 1:26 PM Colin Ian King <colin.king@canonical.com> wrote:
>> On 11/03/2021 11:16, Michal Simek wrote:
>>> On 3/11/21 11:57 AM, Colin Ian King wrote:
>
>>>> For the PIN_MAP_TYPE_CONFIGS_PIN and PIN_MAP_TYPE_CONFIGS_GROUP
>>>> setting->type cases the loop can break out with ret not being set. Since
>>>> ret has not been initialized it the ret < 0 check is checking against an
>>>> uninitialized value.
>>>>
>>>> I was not sure if the PIN_MAP_TYPE_CONFIGS_PIN and
>>>> PIN_MAP_TYPE_CONFIGS_GROUP cases should be setting ret and if so, what
>>>> the value of ret should be set to (is it an error condition or not?). Or
>>>> should ret be initialized to 0 or a default error value at the start of
>>>> the function.
>>>>
>>>> Hence I'm reporting this issue.
>>>
>>> What about this? Is this passing static analysis?
>>
>> It will take me 2 hours to re-run the analysis, but from eyeballing the
>> code I think the assignments will fix this.
>
> It surprises me that tools in the 21st century can't run on a subset
> of the data.
>
> Had you filed a bug to the Coverity team that they will provide a way
> to rerun analysis on a subset of the data?
It can. However I need to keep copies of the entire build to do this and
I build many different kernels (hence lots of storage required) and
rarely do minor change + rebuilds, so I don't cater for this in my test
build environment.
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2021-03-12 12:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-11 10:57 pinctrl: core: Handling pinmux and pinconf separately Colin Ian King
2021-03-11 11:16 ` Michal Simek
2021-03-11 11:24 ` Colin Ian King
2021-03-11 11:28 ` Michal Simek
2021-03-11 14:03 ` Colin Ian King
2021-03-11 14:39 ` Linus Walleij
2021-03-12 12:45 ` Andy Shevchenko
2021-03-12 12:49 ` Colin Ian King
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.