linux-kernel - Re: [PATCH v2 1/1] serial: sh-sci: Respect deferred probe when getting 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: <33a5ee25-d4c9-b5c2-b5f9-05316b1139c0@roeck-us.net>
Date:   Wed, 7 Apr 2021 06:11:46 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org
Cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>
Subject: Re: [PATCH v2 1/1] serial: sh-sci: Respect deferred probe when
 getting IRQ

On 4/7/21 3:17 AM, Andy Shevchenko wrote:
> With platform_get_irq() and its optional variant it's possible to get
> a deferred probe error code. Since the commit ed7027fdf4ec ("driver core:
> platform: Make platform_get_irq_optional() optional") the error code
> can be distinguished from no IRQ case. With this, rewrite IRQ resource
> handling in sh-sci driver to follow above and allow to respect deferred
> probe.
> 
> Fixes: ed7027fdf4ec ("driver core: platform: Make platform_get_irq_optional() optional")
> Reported-by: Guenter Roeck <linux@...ck-us.net>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>

This patch alone causes a hard hang early during boot. It works if applied
together with ed7027fdf4ec. Ultimately that means that ed7027fdf4ec introduces
a functional change, and will need to be applied very carefully. A cursory
glance through callers of platform_get_irq_optional() shows that many
do not handle this correctly: various drivers handle a return value of 0
as valid interrupt, and others treat errors other than -ENXIO as fatal.

Also, each patch on its own causes failures on sh, which is problematic
when applying them even as series. See below for an idea how to
address that.

> ---
> v2: fixed a typo: i -> 0
>  drivers/tty/serial/sh-sci.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index ad2c189e8fc8..574f68ba50ff 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2899,13 +2899,6 @@ static int sci_init_single(struct platform_device *dev,
>  	port->mapbase = res->start;
>  	sci_port->reg_size = resource_size(res);
>  
> -	for (i = 0; i < ARRAY_SIZE(sci_port->irqs); ++i) {
> -		if (i)
> -			sci_port->irqs[i] = platform_get_irq_optional(dev, i);
> -		else
> -			sci_port->irqs[i] = platform_get_irq(dev, i);
> -	}
> -
>  	/* The SCI generates several interrupts. They can be muxed together or
>  	 * connected to different interrupt lines. In the muxed case only one
>  	 * interrupt resource is specified as there is only one interrupt ID.
> @@ -2913,12 +2906,17 @@ static int sci_init_single(struct platform_device *dev,
>  	 * from the SCI, however those signals might have their own individual
>  	 * interrupt ID numbers, or muxed together with another interrupt.
>  	 */
> +	sci_port->irqs[0] = platform_get_irq(dev, 0);
>  	if (sci_port->irqs[0] < 0)
> -		return -ENXIO;
> +		return sci_port->irqs[0];
>  
> -	if (sci_port->irqs[1] < 0)
> -		for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
> +	for (i = 1; i < ARRAY_SIZE(sci_port->irqs); ++i) {
> +		sci_port->irqs[i] = platform_get_irq_optional(dev, i);
> +		if (sci_port->irqs[i] < 0)
> +			return sci_port->irqs[i];
> +		if (sci_port->irqs[i] == 0)
>  			sci_port->irqs[i] = sci_port->irqs[0];

Since sh never gets -EPROBE_DEFER, the following code can be applied
on its own and does not depend on ed7027fdf4ec.

	sci_port->irqs[i] = platform_get_irq_optional(dev, i);
	if (sci_port->irqs[i] <= 0)
		sci_port->irqs[i] = sci_port->irqs[0];

With this change, sh images boot in qemu both with and without ed7027fdf4ec.

Thanks,
Guenter

> +	}
>  
>  	sci_port->params = sci_probe_regmap(p);
>  	if (unlikely(sci_port->params == NULL))
>