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

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

[Andy Shevchenko]

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>
---
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];
+	}
 
 	sci_port->params = sci_probe_regmap(p);
 	if (unlikely(sci_port->params == NULL))
-- 
2.30.2

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

[Greg Kroah-Hartman]

On Wed, Apr 07, 2021 at 01:17:13PM +0300, 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")

I've already reverted this commit, sorry.  Please feel free to resend
this as a patch series.

thanks,

greg k-h

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

[Andy Shevchenko]

On Wed, Apr 07, 2021 at 12:23:21PM +0200, Greg Kroah-Hartman wrote:
> On Wed, Apr 07, 2021 at 01:17:13PM +0300, 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")
> 
> I've already reverted this commit, sorry.  Please feel free to resend
> this as a patch series.

Thanks!

I think I will not hurry in this cycle. Perhaps I have to check plenty of
places if they could have the same issue before resending. Sorry for the
troubles.


-- 
With Best Regards,
Andy Shevchenko

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

[Guenter Roeck]

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))
> 

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

[Andy Shevchenko]

On Wed, Apr 07, 2021 at 06:11:46AM -0700, Guenter Roeck wrote:
> 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.

Right, that's why I think I have to slow down with it (as I answered to Greg).

> 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.

Yeah, thanks! But I think we still can avoid double loops there.

-- 
With Best Regards,
Andy Shevchenko

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

[Geert Uytterhoeven]

Hi Günter,

On Wed, Apr 7, 2021 at 10:58 PM Guenter Roeck <linux@roeck-us.net> wrote:
> 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.

Note that the sh-sci driver is also used on ARM32/64 and H8/300.
On ARM, I don't expect GIC interrupts causing -EPROBE_DEFER, though.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds