Received-SPF: pass (google.com: domain of linux-kernel+bounces-70470-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sun, 18 Feb 2024 18:59:37 +0100
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
To: Bartosz Golaszewski <brgl@bgdev.pl>
Cc: Patrice Chotard <patrice.chotard@foss.st.com>,
	Jiri Slaby <jirislaby@kernel.org>,
	Dan Carpenter <dan.carpenter@linaro.org>,
	Linus Walleij <linus.walleij@linaro.org>,
	linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org,
	Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Subject: Re: [PATCH] serial: st-asc: don't get/put GPIOs in atomic context
Message-ID: <2024021851-amniotic-trimester-89af@gregkh>
References: <20240214092438.10785-1-brgl@bgdev.pl>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240214092438.10785-1-brgl@bgdev.pl>

On Wed, Feb 14, 2024 at 10:24:38AM +0100, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> 
> Since commit 1f2bcb8c8ccd ("gpio: protect the descriptor label with
> SRCU") gpiod_set_consumer_name() calls synchronize_srcu() which led to
> a "sleeping in atomic context" smatch warning.
> 
> This function (along with gpiod_get/put() and all other GPIO APIs apart
> from gpiod_get/set_value() and gpiod_direction_input/output()) should
> have never been called with a spinlock taken. We're only fixing this now
> as GPIOLIB has been rebuilt to use SRCU for access serialization which
> uncovered this problem.
> 
> Move the calls to gpiod_get/put() outside the spinlock critical section.
> 
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-gpio/deee1438-efc1-47c4-8d80-0ab2cf01d60a@moroto.mountain/
> Signed-off-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> ---
>  drivers/tty/serial/st-asc.c | 40 ++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/tty/serial/st-asc.c b/drivers/tty/serial/st-asc.c
> index bbb5595d7e24..52a20277df98 100644
> --- a/drivers/tty/serial/st-asc.c
> +++ b/drivers/tty/serial/st-asc.c
> @@ -467,6 +467,7 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  	struct asc_port *ascport = to_asc_port(port);
>  	struct gpio_desc *gpiod;
>  	unsigned int baud;
> +	bool manual_rts;
>  	u32 ctrl_val;
>  	tcflag_t cflag;
>  	unsigned long flags;
> @@ -517,26 +518,12 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  		ctrl_val |= ASC_CTL_CTSENABLE;
>  
>  		/* If flow-control selected, stop handling RTS manually */
> -		if (ascport->rts) {
> -			devm_gpiod_put(port->dev, ascport->rts);
> -			ascport->rts = NULL;
> -
> -			pinctrl_select_state(ascport->pinctrl,
> -					     ascport->states[DEFAULT]);
> -		}
> +		if (ascport->rts)
> +			manual_rts = false;
>  	} else {
>  		/* If flow-control disabled, it's safe to handle RTS manually */
> -		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL]) {
> -			pinctrl_select_state(ascport->pinctrl,
> -					     ascport->states[NO_HW_FLOWCTRL]);
> -
> -			gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
> -			if (!IS_ERR(gpiod)) {
> -				gpiod_set_consumer_name(gpiod,
> -						port->dev->of_node->name);
> -				ascport->rts = gpiod;
> -			}
> -		}
> +		if (!ascport->rts && ascport->states[NO_HW_FLOWCTRL])
> +			manual_rts = true;
>  	}
>  
>  	if ((baud < 19200) && !ascport->force_m1) {
> @@ -595,6 +582,23 @@ static void asc_set_termios(struct uart_port *port, struct ktermios *termios,
>  	asc_out(port, ASC_CTL, (ctrl_val | ASC_CTL_RUN));
>  
>  	uart_port_unlock_irqrestore(port, flags);
> +
> +	if (manual_rts) {
> +		pinctrl_select_state(ascport->pinctrl,
> +				     ascport->states[NO_HW_FLOWCTRL]);
> +
> +		gpiod = devm_gpiod_get(port->dev, "rts", GPIOD_OUT_LOW);
> +		if (!IS_ERR(gpiod)) {
> +			gpiod_set_consumer_name(gpiod,
> +						port->dev->of_node->name);
> +			ascport->rts = gpiod;
> +		} else {
> +			devm_gpiod_put(port->dev, ascport->rts);
> +			ascport->rts = NULL;
> +			pinctrl_select_state(ascport->pinctrl,
> +					     ascport->states[DEFAULT]);
> +		}
> +	}
>  }

The 0-day bot rightly points out that manual_rts could be uninitialized
by this change, so I'm dropping it from my tree.  Please fix up and
resend.

thanks,

greg k-h