lkml.org 
[lkml]   [2021]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: pinctrl: core: Handling pinmux and pinconf separately
From
Date


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



\
 
 \ /
  Last update: 2021-03-11 12:30    [W:0.087 / U:0.392 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site