linux-kernel - Re: [PATCH 27/33] serial: 8250: extract serial_get_or_create_irq

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: <5643b834-b9be-a70b-7669-a54be24845ad@linux.intel.com>
Date: Wed, 11 Jun 2025 16:09:08 +0300 (EEST)
From: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To: "Jiri Slaby (SUSE)" <jirislaby@...nel.org>
cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, 
    linux-serial <linux-serial@...r.kernel.org>, 
    LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 27/33] serial: 8250: extract
 serial_get_or_create_irq_info()

On Wed, 11 Jun 2025, Jiri Slaby (SUSE) wrote:

> This find-or-create-irq part of the serial_link_irq_chain()'s code is
> logically bounded and self-standing. For easier-to-follow code flow,
> extract the code to a separate function:
> serial_get_or_create_irq_info().
> 
> This allows for an easier found-an-irq handling -- simple jump to the
> 'unlock' label and return. That results in one less 'if' levels.
> 
> Note when using guard()s in the upcoming patchset, the label can dropped
> altogether.
> 
> Signed-off-by: Jiri Slaby (SUSE) <jirislaby@...nel.org>
> ---
>  drivers/tty/serial/8250/8250_core.c | 37 ++++++++++++++++++++---------
>  1 file changed, 26 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
> index 7a6050f1c094..d42ceb6ffdc2 100644
> --- a/drivers/tty/serial/8250/8250_core.c
> +++ b/drivers/tty/serial/8250/8250_core.c
> @@ -129,11 +129,15 @@ static void serial_do_unlink(struct irq_info *i, struct uart_8250_port *up)
>  	}
>  }
>  
> -static int serial_link_irq_chain(struct uart_8250_port *up)
> +/*
> + * Either:
> + * - find the corresponding info in the hashtable and return it, or
> + * - allocate a new one, add it to the hashtable and return it.
> + */
> +static struct irq_info *serial_get_or_create_irq_info(const struct uart_8250_port *up)
>  {
>  	struct hlist_head *h;
>  	struct irq_info *i;
> -	int ret;
>  
>  	mutex_lock(&hash_mutex);
>  
> @@ -141,20 +145,31 @@ static int serial_link_irq_chain(struct uart_8250_port *up)
>  
>  	hlist_for_each_entry(i, h, node)
>  		if (i->irq == up->port.irq)
> -			break;
> +			goto unlock;
>  
> +	i = kzalloc(sizeof(*i), GFP_KERNEL);
>  	if (i == NULL) {
> -		i = kzalloc(sizeof(struct irq_info), GFP_KERNEL);
> -		if (i == NULL) {
> -			mutex_unlock(&hash_mutex);
> -			return -ENOMEM;
> -		}
> -		spin_lock_init(&i->lock);
> -		i->irq = up->port.irq;
> -		hlist_add_head(&i->node, h);
> +		i = ERR_PTR(-ENOMEM);
> +		goto unlock;
>  	}
> +	spin_lock_init(&i->lock);
> +	i->irq = up->port.irq;
> +	hlist_add_head(&i->node, h);
> +unlock:
>  	mutex_unlock(&hash_mutex);
>  
> +	return i;
> +}
> +
> +static int serial_link_irq_chain(struct uart_8250_port *up)
> +{
> +	struct irq_info *i;
> +	int ret;
> +
> +	i = serial_get_or_create_irq_info(up);
> +	if (IS_ERR(i))
> +		return PTR_ERR(i);
> +
>  	spin_lock_irq(&i->lock);
>  
>  	if (i->head) {
> 

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>

Unrelated to the patch, I didn't like the use of 'i' as a variable name 
to store a struct pointer.

-- 
 i.