Re: [PATCH 27/33] serial: 8250: extract serial_get_or_create_irq_info()

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2668 bytes --]

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@kernel.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@linux.intel.com>

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

-- 
 i.