linux-kernel - Re: [PATCH v3] serial: sc16is7xx: Fix IRQ number check behavior

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] [day] [month] [year] [list]

Message-Id: <20250120103851.6518f01800ed946d02bfb81b@hugovil.com>
Date: Mon, 20 Jan 2025 10:38:51 -0500
From: Hugo Villeneuve <hugo@...ovil.com>
To: Andre Werner <andre.werner@...tec-electronic.com>
Cc: gregkh@...uxfoundation.org, jirislaby@...nel.org,
 hvilleneuve@...onoff.com, andy@...nel.org, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org, lech.perczak@...lingroup.com
Subject: Re: [PATCH v3] serial: sc16is7xx: Fix IRQ number check behavior

On Mon, 20 Jan 2025 08:32:34 +0100
Andre Werner <andre.werner@...tec-electronic.com> wrote:

Hi,

> The logical meaning of the previous version is wrong due to a typo.

Ok.

> The rest of the patch expects polling = true for irq = 0;
> 
> The behaviour is fixed for irq = 0 as well as extended to negative

What do you mean by "is fixed"? and also isnt it the same explanation
as above for the "=0" case ("The rest of the patch..."?

> values because the irq parameter is an integer such that negative values
> are not completely impossible. So negative values will also be treated as
> a fallback to polling mode.

Please try to rephrase the whole commit message so that it is
more concise and clearer to understand, and takes into account the
previous comments from Andy/Jiri/Marteen?

If I understood these comments correctly, maybe something like:

------
When IRQ = 0, this means that no interrupt is used, and activate
polling mode.

And since documentation still says that negative IRQ values cannot be
absolutely ruled-out, add a check for negative IRQ values to
increase robustness.
------


Hugo


> 
> Fixes: 104c1b9dde9d859dd01bd2d ("serial: sc16is7xx: Add polling mode if no IRQ pin is available")
> 
> Signed-off-by: Andre Werner <andre.werner@...tec-electronic.com>
> ---
> V2:
> There are no changes to the patch itself. The previous patch submission
> had a very weird structure within the discussion thread:
> https://lore.kernel.org/all/20250116093203.460215-1-andre.werner@systec-electronic.com/
> This is simply a new thread opened for better handling.
> V3:
> Add Fixes tag and update commit message description.
> ---
>  drivers/tty/serial/sc16is7xx.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 7b51cdc274fd..560f45ed19ae 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1561,7 +1561,7 @@ int sc16is7xx_probe(struct device *dev, const struct sc16is7xx_devtype *devtype,
>  	/* Always ask for fixed clock rate from a property. */
>  	device_property_read_u32(dev, "clock-frequency", &uartclk);
>  
> -	s->polling = !!irq;
> +	s->polling = (irq <= 0);
>  	if (s->polling)
>  		dev_dbg(dev,
>  			"No interrupt pin definition, falling back to polling mode\n");
> -- 
> 2.48.0
> 
> 
>