Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Received-SPF: Pass (protection.outlook.com: domain of xilinx.com designates
 149.199.62.198 as permitted sender) receiver=protection.outlook.com;
 client-ip=149.199.62.198; helo=xsj-pvapexch02.xlnx.xilinx.com;
Envelope-to: linux-kernel@vger.kernel.org,
 linux-gpio@vger.kernel.org,
 linus.walleij@linaro.org,
 colin.king@canonical.com
Subject: Re: pinctrl: core: Handling pinmux and pinconf separately
To:     Colin Ian King <colin.king@canonical.com>,
        Michal Simek <michal.simek@xilinx.com>
CC:     Linus Walleij <linus.walleij@linaro.org>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
References: <d66e78e3-2000-611b-cd74-8a61461153e8@canonical.com>
From:   Michal Simek <michal.simek@xilinx.com>
Message-ID: <5c08bd61-688f-e95b-5fa3-584f190ed4bf@xilinx.com>
Date:   Thu, 11 Mar 2021 12:16:56 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <d66e78e3-2000-611b-cd74-8a61461153e8@canonical.com>
Content-Type: text/plain; charset="utf-8"
Content-Language: en-US
Content-Transfer-Encoding: 7bit
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Mar 2021 11:17:04.2752
 (UTC)
X-MS-Exchange-CrossTenant-Network-Message-Id: 943b0537-eb32-4a38-63fe-08d8e47f332b
X-MS-Exchange-CrossTenant-Id: 657af505-d5df-48d0-8300-c31994686c5c
X-MS-Exchange-CrossTenant-OriginalAttributedTenantConnectingIp: TenantId=657af505-d5df-48d0-8300-c31994686c5c;Ip=[149.199.62.198];Helo=[xsj-pvapexch02.xlnx.xilinx.com]
X-MS-Exchange-CrossTenant-AuthSource: CY1NAM02FT059.eop-nam02.prod.protection.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Anonymous
X-MS-Exchange-CrossTenant-FromEntityHeader: HybridOnPrem
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR02MB2741
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


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