Received-SPF: pass (google.com: domain of linux-kernel+bounces-128435-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 2 Apr 2024 13:07:15 -0400
From: Hugo Villeneuve <hugo@hugovil.com>
To: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
 linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org, Jiri Slaby
 <jirislaby@kernel.org>
Message-Id: <20240402130715.7f1292b774ee0b2f056a6e95@hugovil.com>
In-Reply-To: <20240402154219.3583679-2-andriy.shevchenko@linux.intel.com>
References: <20240402154219.3583679-1-andriy.shevchenko@linux.intel.com>
	<20240402154219.3583679-2-andriy.shevchenko@linux.intel.com>
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Subject: Re: [PATCH v1 01/16] serial: max3100: Lock port->lock when calling
 uart_handle_cts_change()

On Tue,  2 Apr 2024 18:38:07 +0300
Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:

Hi Andy,

> uart_handle_cts_change() has to be called with port lock taken,
> Since we run it in a separate work, the lcok maybe not taken at

lcok -> lock

and possibly: "may not be taken" ?


> the time of running. Make sure that it's taken by explicitly doing
> that. Without it we got a splat:
> 
>   WARNING: CPU: 0 PID: 10 at drivers/tty/serial/serial_core.c:3491 uart_handle_cts_change+0xa6/0xb0
>   ...
>   Workqueue: max3100-0 max3100_work [max3100]
>   RIP: 0010:uart_handle_cts_change+0xa6/0xb0
>   ...
>    max3100_handlerx+0xc5/0x110 [max3100]
>    max3100_work+0x12a/0x340 [max3100]
> 
> Fixes: 7831d56b0a35 ("tty: MAX3100")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/tty/serial/max3100.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
> index 5efb2b593be3..45022f2909f0 100644
> --- a/drivers/tty/serial/max3100.c
> +++ b/drivers/tty/serial/max3100.c
> @@ -213,7 +213,7 @@ static int max3100_sr(struct max3100_port *s, u16 tx, u16 *rx)
>  	return 0;
>  }
>  
> -static int max3100_handlerx(struct max3100_port *s, u16 rx)
> +static int max3100_handlerx_unlocked(struct max3100_port *s, u16 rx)
>  {
>  	unsigned int status = 0;
>  	int ret = 0, cts;
> @@ -254,6 +254,17 @@ static int max3100_handlerx(struct max3100_port *s, u16 rx)
>  	return ret;
>  }
>  
> +static int max3100_handlerx(struct max3100_port *s, u16 rx)
> +{
> +	unsigned long flags;
> +	int ret;
> +
> +	uart_port_lock_irqsave(&s->port, &flags);
> +	ret = max3100_handlerx_unlocked(s, rx);
> +	uart_port_unlock_irqrestore(&s->port, flags);
> +	return ret;
> +}
> +
>  static void max3100_work(struct work_struct *w)
>  {
>  	struct max3100_port *s = container_of(w, struct max3100_port, work);
> -- 
> 2.43.0.rc1.1.gbec44491f096
> 
> 


-- 
Hugo Villeneuve