Re: [PATCH v2 1/1] serial: sh-sci: Respect deferred probe when getting IRQ

[Guenter Roeck <linux@roeck-us.net>]

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@roeck-us.net>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.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))
>