Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Nov 2022 10:00:47 +0530
From:   Pavan Kondeti <quic_pkondeti@quicinc.com>
To:     Maria Yu <quic_aiquny@quicinc.com>
CC:     <linus.walleij@linaro.org>, <linux-gpio@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] pinctrl: core: Make p->state in order in
 pinctrl_commit_state
Message-ID: <20221101043047.GA11893@hu-pkondeti-hyd.qualcomm.com>
References: <20221027065408.36977-1-quic_aiquny@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20221027065408.36977-1-quic_aiquny@quicinc.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

Hi Maria,

On Thu, Oct 27, 2022 at 02:54:08PM +0800, Maria Yu wrote:
> We've got a dump that current cpu is in pinctrl_commit_state, the
> old_state != p->state while the stack is still in the process of
> pinmux_disable_setting. So it means even if the current p->state is
> changed in new state, the settings are not yet up-to-date enabled
> complete yet.
> 
> Currently p->state in different value to synchronize the
> pinctrl_commit_state behaviors. The p->state will have transaction like
> old_state -> NULL -> new_state. When in old_state, it will try to
> disable all the all state settings. And when after new state settings
> enabled, p->state will changed to the new state after that. So use
> smp_mb to synchronize the p->state variable and the settings in order.
> ---
>  drivers/pinctrl/core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
> index 9e57f4c62e60..cd917a5b1a0a 100644
> --- a/drivers/pinctrl/core.c
> +++ b/drivers/pinctrl/core.c
> @@ -1256,6 +1256,7 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>  		}
>  	}
>  
> +	smp_mb();
>  	p->state = NULL;
>  
>  	/* Apply all the settings for the new state - pinmux first */
> @@ -1305,6 +1306,7 @@ static int pinctrl_commit_state(struct pinctrl *p, struct pinctrl_state *state)
>  			pinctrl_link_add(setting->pctldev, p->dev);
>  	}
>  
> +	smp_mb();
>  	p->state = state;
>  

From your commit description, are you inferring that this p->state assignment
re-ordered wrt pinmux_disable_setting()? btw, I don't see any locking that
protects concurrent access to p->state modifications. For whatever reasons, if
a client makes concurrent calls to pinctrl_select_state(), we can land up in
the situation, you are seeing. correct?

Thanks,
Pavan