linux-kernel - Re: DT overlay applied via pinctrl description

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <45cea3bb-6e5d-4005-ef2a-67b08772e0d7@xilinx.com>
Date:   Mon, 1 Mar 2021 10:31:19 +0100
From:   Michal Simek <michal.simek@...inx.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Michal Simek <michal.simek@...inx.com>
CC:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        Sai Krishna Potthuri <lakshmi.sai.krishna.potthuri@...inx.com>
Subject: Re: DT overlay applied via pinctrl description

Hi Linus,

On 3/1/21 10:19 AM, Linus Walleij wrote:
> On Tue, Feb 16, 2021 at 4:35 PM Michal Simek <michal.simek@...inx.com> wrote:
> 
>> I have a question about expectations when pinctrl setting is applied. In
>> DTS all nodes are described in the order available in DT.
>>
>> uart-default {
>>         mux {
>>                 ...
>>         };
>>
>>         conf {
>>                 ...
>>         };
>> };
>>
>> I don't know if this standard description or not. I definitely see other
>> pinctrl drivers which are using different structure.
>>
>> Anyway when overlay is applied the order has changed to
>> uart-default {
>>         conf {
>>                 ...
>>         };
>>
>>         mux {
>>                 ...
>>         };
>> };
>>
>> which is causing issue because pin is configured first via conf node
>> before it is requested via mux. This is something what firmware is
>> checking and error out.
> 
> As Frank says the DT ordering has no semantic meaning, it is essentially
> a functional language, describes object relations not sequences.
> 
> The Linux kernel applies the mux and conf in that order because of how
> the code is implemented (this order also makes a lot of sense for the
> hardware). I would recommend to trace the execution of an overlay
> being applied and try to find the reason conf goes before mux and fix
> the bug there. I think it is a bug in how pinctrl handles DT overlays.

Does this mean that you prefer to fix how dt overlay applying instead of
fixing code to apply mux configs first before conf one?

Something like this? (just c&p patch below)

Thanks,
Michal

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 7d3370289938..cf56ee5d7e02 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -1258,13 +1258,35 @@ 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, &state->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, &state->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;