Re: pinctrl: core: Handling pinmux and pinconf separately
On 12/03/2021 12:45, Andy Shevchenko wrote: > On Thu, Mar 11, 2021 at 1:26 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: > 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. > >
Re: pinctrl: core: Handling pinmux and pinconf separately
On Thu, Mar 11, 2021 at 1:26 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: > >> 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
Re: pinctrl: core: Handling pinmux and pinconf separately
On Thu, Mar 11, 2021 at 12:28 PM Michal Simek 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
Re: pinctrl: core: Handling pinmux and pinconf separately
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
>>>> 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 {
>>>> 1241struct pinctrl_setting *setting, *setting2;
>>>> 1242struct pinctrl_state *old_state = p->state;
>>>>
>>>> 1. var_decl: Declaring variable ret without initializer.
>>>>
>>>> 1243int ret;
>>>> 1244
>>>>
>>>> 2. Condition p->state, taking true branch.
>>>>
>>>> 1245if (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 !(>node == >state->settings), taking true
>>>> branch.
>>>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 8. Condition !(>node == >state->settings), taking true
>>>> branch.
>>>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>>> !__builtin_types_compatible_p()) */, taking false branch.
>>>> 12. Condition !(>node == >state->settings), taking false
>>>> branch.
>>>>
>>>> 1252list_for_each_entry(setting, >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.
>>>> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>>> 6. Continuing loop.
>>>> 10. Continuing loop.
>>>>
>>>> 1254continue;
>>>> 1255pinmux_disable_setting(setting);
>>>> 1256}
>>>> 1257}
>>>> 1258
>>>> 1259p->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 !(>node == >settings), taking true
>>>> branch.
>>>> 1262list_for_each_entry(setting, >settings, node) {
>>>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>>>
>>>> 1263switch (setting->type) {
>>>> 1264case PIN_MAP_TYPE_MUX_GROUP:
>>>> 1265ret = pinmux_enable_setting(setting);
>>>> 1266break;
>>>> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
>>>> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>>>>
>>>> 16. Breaking from switch.
>>>>
>>>> 1269
Re: pinctrl: core: Handling pinmux and pinconf separately
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
>>> 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 {
>>> 1241struct pinctrl_setting *setting, *setting2;
>>> 1242struct pinctrl_state *old_state = p->state;
>>>
>>> 1. var_decl: Declaring variable ret without initializer.
>>>
>>> 1243int ret;
>>> 1244
>>>
>>> 2. Condition p->state, taking true branch.
>>>
>>> 1245if (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 !(>node == >state->settings), taking true
>>> branch.
>>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 8. Condition !(>node == >state->settings), taking true
>>> branch.
>>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>>> !__builtin_types_compatible_p()) */, taking false branch.
>>> 12. Condition !(>node == >state->settings), taking false
>>> branch.
>>>
>>> 1252list_for_each_entry(setting, >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.
>>> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>>> 6. Continuing loop.
>>> 10. Continuing loop.
>>>
>>> 1254continue;
>>> 1255pinmux_disable_setting(setting);
>>> 1256}
>>> 1257}
>>> 1258
>>> 1259p->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 !(>node == >settings), taking true branch.
>>> 1262list_for_each_entry(setting, >settings, node) {
>>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>>
>>> 1263switch (setting->type) {
>>> 1264case PIN_MAP_TYPE_MUX_GROUP:
>>> 1265ret = pinmux_enable_setting(setting);
>>> 1266break;
>>> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
>>> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>>>
>>> 16. Breaking from switch.
>>>
>>> 1269break;
>>> 1270default:
>>> 1271ret = -EINVAL;
>>> 1272break;
>>> 1273}
>>> 1274
>>>
>>> Uninitialized scalar variable (UNINIT)
>>> 17. uninit_use: Using uninitialized value ret.
>>>
>>> 1275if (ret < 0)
>>> 1276goto 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
Re: pinctrl: core: Handling pinmux and pinconf separately
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
>> 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 {
>> 1241struct pinctrl_setting *setting, *setting2;
>> 1242struct pinctrl_state *old_state = p->state;
>>
>> 1. var_decl: Declaring variable ret without initializer.
>>
>> 1243int ret;
>> 1244
>>
>> 2. Condition p->state, taking true branch.
>>
>> 1245if (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 !(>node == >state->settings), taking true
>> branch.
>> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 8. Condition !(>node == >state->settings), taking true
>> branch.
>> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
>> !__builtin_types_compatible_p()) */, taking false branch.
>> 12. Condition !(>node == >state->settings), taking false
>> branch.
>>
>> 1252list_for_each_entry(setting, >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.
>> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
>> 6. Continuing loop.
>> 10. Continuing loop.
>>
>> 1254continue;
>> 1255pinmux_disable_setting(setting);
>> 1256}
>> 1257}
>> 1258
>> 1259p->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 !(>node == >settings), taking true branch.
>> 1262list_for_each_entry(setting, >settings, node) {
>> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>>
>> 1263switch (setting->type) {
>> 1264case PIN_MAP_TYPE_MUX_GROUP:
>> 1265ret = pinmux_enable_setting(setting);
>> 1266break;
>> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
>> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>>
>> 16. Breaking from switch.
>>
>> 1269break;
>> 1270default:
>> 1271ret = -EINVAL;
>> 1272break;
>> 1273}
>> 1274
>>
>> Uninitialized scalar variable (UNINIT)
>> 17. uninit_use: Using uninitialized value ret.
>>
>> 1275if (ret < 0)
>> 1276goto 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
>> th
Re: pinctrl: core: Handling pinmux and pinconf separately
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
> 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 {
> 1241struct pinctrl_setting *setting, *setting2;
> 1242struct pinctrl_state *old_state = p->state;
>
> 1. var_decl: Declaring variable ret without initializer.
>
> 1243int ret;
> 1244
>
> 2. Condition p->state, taking true branch.
>
> 1245if (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 !(>node == >state->settings), taking true
> branch.
> 7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 8. Condition !(>node == >state->settings), taking true
> branch.
> 11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
> !__builtin_types_compatible_p()) */, taking false branch.
> 12. Condition !(>node == >state->settings), taking false
> branch.
>
> 1252list_for_each_entry(setting, >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.
> 1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
> 6. Continuing loop.
> 10. Continuing loop.
>
> 1254continue;
> 1255pinmux_disable_setting(setting);
> 1256}
> 1257}
> 1258
> 1259p->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 !(>node == >settings), taking true branch.
> 1262list_for_each_entry(setting, >settings, node) {
> 15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
>
> 1263switch (setting->type) {
> 1264case PIN_MAP_TYPE_MUX_GROUP:
> 1265ret = pinmux_enable_setting(setting);
> 1266break;
> 1267case PIN_MAP_TYPE_CONFIGS_PIN:
> 1268case PIN_MAP_TYPE_CONFIGS_GROUP:
>
> 16. Breaking from switch.
>
> 1269break;
> 1270default:
> 1271ret = -EINVAL;
> 1272break;
> 1273}
> 1274
>
> Uninitialized scalar variable (UNINIT)
> 17. uninit_use: Using uninitialized value ret.
>
> 1275if (ret < 0)
> 1276goto 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(st
re: pinctrl: core: Handling pinmux and pinconf separately
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
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 {
1241struct pinctrl_setting *setting, *setting2;
1242struct pinctrl_state *old_state = p->state;
1. var_decl: Declaring variable ret without initializer.
1243int ret;
1244
2. Condition p->state, taking true branch.
1245if (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 !(>node == >state->settings), taking true
branch.
7. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
8. Condition !(>node == >state->settings), taking true
branch.
11. Condition 0 /* !!(!__builtin_types_compatible_p() &&
!__builtin_types_compatible_p()) */, taking false branch.
12. Condition !(>node == >state->settings), taking false
branch.
1252list_for_each_entry(setting, >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.
1253if (setting->type != PIN_MAP_TYPE_MUX_GROUP)
6. Continuing loop.
10. Continuing loop.
1254continue;
1255pinmux_disable_setting(setting);
1256}
1257}
1258
1259p->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 !(>node == >settings), taking true branch.
1262list_for_each_entry(setting, >settings, node) {
15. Switch case value PIN_MAP_TYPE_CONFIGS_PIN.
1263switch (setting->type) {
1264case PIN_MAP_TYPE_MUX_GROUP:
1265ret = pinmux_enable_setting(setting);
1266break;
1267case PIN_MAP_TYPE_CONFIGS_PIN:
1268case PIN_MAP_TYPE_CONFIGS_GROUP:
16. Breaking from switch.
1269break;
1270default:
1271ret = -EINVAL;
1272break;
1273}
1274
Uninitialized scalar variable (UNINIT)
17. uninit_use: Using uninitialized value ret.
1275if (ret < 0)
1276goto 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
Re: [PATCH] pinctrl: core: Handling pinmux and pinconf separately
On Wed, Mar 10, 2021 at 9:16 AM Michal Simek wrote: > Right now the handling order depends on how entries are coming which is > corresponding with order in DT. We have reached the case with DT overlays > where conf and mux descriptions are exchanged which ends up in sequence > that firmware has been asked to perform configuration before requesting the > pin. > The patch is enforcing the order that pin is requested all the time first > followed by pin configuration. This change will ensure that firmware gets > requests in the right order. > > Signed-off-by: Michal Simek This looks right to me so I simply applied the patch so it gets some testing in linux-next. If there are problems on some platform(s) we will get to know. Yours, Linus Walleij
[PATCH] pinctrl: core: Handling pinmux and pinconf separately
Right now the handling order depends on how entries are coming which is
corresponding with order in DT. We have reached the case with DT overlays
where conf and mux descriptions are exchanged which ends up in sequence
that firmware has been asked to perform configuration before requesting the
pin.
The patch is enforcing the order that pin is requested all the time first
followed by pin configuration. This change will ensure that firmware gets
requests in the right order.
Signed-off-by: Michal Simek
---
drivers/pinctrl/core.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 7d3370289938..f5c32d2a3c91 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1258,13 +1258,34 @@ static int pinctrl_commit_state(struct pinctrl *p,
struct pinctrl_state *state)
p->state = NULL;
- /* Apply all the settings for the new state */
+ /* Apply all the settings for the new state - pinmux first */
list_for_each_entry(setting, >settings, node) {
switch (setting->type) {
case PIN_MAP_TYPE_MUX_GROUP:
ret = pinmux_enable_setting(setting);
break;
case PIN_MAP_TYPE_CONFIGS_PIN:
+ case PIN_MAP_TYPE_CONFIGS_GROUP:
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ if (ret < 0)
+ goto unapply_new_state;
+
+ /* Do not link hogs (circular dependency) */
+ if (p != setting->pctldev->p)
+ pinctrl_link_add(setting->pctldev, p->dev);
+ }
+
+ /* Apply all the settings for the new state - pinconf after */
+ list_for_each_entry(setting, >settings, node) {
+ switch (setting->type) {
+ case PIN_MAP_TYPE_MUX_GROUP:
+ break;
+ case PIN_MAP_TYPE_CONFIGS_PIN:
case PIN_MAP_TYPE_CONFIGS_GROUP:
ret = pinconf_apply_setting(setting);
break;
--
2.30.1
