Re: [PATCH 1/2] platform: make platform_get_irq_optional() optional

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Uwe,

On Mon, Jan 17, 2022 at 12:49 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Mon, Jan 17, 2022 at 11:35:52AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Jan 17, 2022 at 10:24 AM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > > On Mon, Jan 17, 2022 at 09:41:42AM +0100, Geert Uytterhoeven wrote:
> > > > On Sat, Jan 15, 2022 at 9:22 PM Sergey Shtylyov <s.shtylyov@omp.ru> wrote:
> > > > > On 1/14/22 11:22 PM, Uwe Kleine-König wrote:
> > > > > > You have to understand that for clk (and regulator and gpiod) NULL is a
> > > > > > valid descriptor that can actually be used, it just has no effect. So
> > > > > > this is a convenience value for the case "If the clk/regulator/gpiod in
> > > > > > question isn't available, there is nothing to do". This is what makes
> > > > > > clk_get_optional() and the others really useful and justifies their
> > > > > > existence. This doesn't apply to platform_get_irq_optional().
> > > > >
> > > > >    I do understand that. However, IRQs are a different beast with their
> > > > > own justifications...
> > > >
> > > > > > clk_get_optional() is sane and sensible for cases where the clk might be
> > > > > > absent and it helps you because you don't have to differentiate between
> > > > > > "not found" and "there is an actual resource".
> > > > > >
> > > > > > The reason for platform_get_irq_optional()'s existence is just that
> > > > > > platform_get_irq() emits an error message which is wrong or suboptimal
> > > > >
> > > > >    I think you are very wrong here. The real reason is to simplify the
> > > > > callers.
> > > >
> > > > Indeed.
> > >
> > > The commit that introduced platform_get_irq_optional() said:
> > >
> > >         Introduce a new platform_get_irq_optional() that works much like
> > >         platform_get_irq() but does not output an error on failure to
> > >         find the interrupt.
> > >
> > > So the author of 8973ea47901c81a1912bd05f1577bed9b5b52506 failed to
> > > mention the real reason? Or look at
> > > 31a8d8fa84c51d3ab00bf059158d5de6178cf890:
> > >
> > >         [...] use platform_get_irq_optional() to get second/third IRQ
> > >         which are optional to avoid below error message during probe:
> > >         [...]
> > >
> > > Look through the output of
> > >
> > >         git log -Splatform_get_irq_optional
> > >
> > > to find several more of these.
> >
> > Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> > platform_get_irq_optional()") and the various fixups fixed the ugly
> > printing of error messages that were not applicable.
> > In hindsight, probably commit 7723f4c5ecdb8d83 ("driver core:
> > platform: Add an error message to platform_get_irq*()") should have
> > been reverted instead, until a platform_get_irq_optional() with proper
> > semantics was introduced.
>
> ack.
>
> > But as we were all in a hurry to kill the non-applicable error
> > message, we went for the quick and dirty fix.
> >
> > > Also I fail to see how a caller of (today's) platform_get_irq_optional()
> > > is simpler than a caller of platform_get_irq() given that there is no
> > > semantic difference between the two. Please show me a single
> > > conversion from platform_get_irq to platform_get_irq_optional that
> > > yielded a simplification.
> >
> > That's exactly why we want to change the latter to return 0 ;-)
>
> OK. So you agree to my statement "The reason for
> platform_get_irq_optional()'s existence is just that platform_get_irq()
> emits an error message [...]". Actually you don't want to oppose but
> say: It's unfortunate that the silent variant of platform_get_irq() took
> the obvious name of a function that could have an improved return code
> semantic.
>
> So my suggestion to rename todays platform_get_irq_optional() to
> platform_get_irq_silently() and then introducing
> platform_get_irq_optional() with your suggested semantic seems
> intriguing and straigt forward to me.

I don't really see the point of needing platform_get_irq_silently(),
unless as an intermediary step, where it's going to be removed again
once the conversion has completed.
Still, the rename would touch all users at once anyway.

> Another thought: platform_get_irq emits an error message for all
> problems. Wouldn't it be consistent to let platform_get_irq_optional()
> emit an error message for all problems but "not found"?
> Alternatively remove the error printk from platform_get_irq().

Yes, all problems but not found are real errors.

> > > So you need some more effort to convince me of your POV.
> > >
> > > > Even for clocks, you cannot assume that you can always blindly use
> > > > the returned dummy (actually a NULL pointer) to call into the clk
> > > > API.  While this works fine for simple use cases, where you just
> > > > want to enable/disable an optional clock (clk_prepare_enable() and
> > > > clk_disable_unprepare()), it does not work for more complex use cases.
> > >
> > > Agreed. But for clks and gpiods and regulators the simple case is quite
> > > usual. For irqs it isn't.
> >
> > It is for devices that can have either separate interrupts, or a single
> > multiplexed interrupt.
> >
> > The logic in e.g. drivers/tty/serial/sh-sci.c and
> > drivers/spi/spi-rspi.c could be simplified and improved (currently
> > it doesn't handle deferred probe) if platform_get_irq_optional()
> > would return 0 instead of -ENXIO.
>
> Looking at sh-sci.c the irq handling logic could be improved even
> without a changed platform_get_irq_optional():
>
> diff --git a/drivers/tty/serial/sh-sci.c b/drivers/tty/serial/sh-sci.c
> index 968967d722d4..c7dc9fb84844 100644
> --- a/drivers/tty/serial/sh-sci.c
> +++ b/drivers/tty/serial/sh-sci.c
> @@ -2873,11 +2873,13 @@ static int sci_init_single(struct platform_device *dev,
>          * interrupt ID numbers, or muxed together with another interrupt.
>          */
>         if (sci_port->irqs[0] < 0)
> -               return -ENXIO;
> +               return sci_port->irqs[0];
>
> -       if (sci_port->irqs[1] < 0)
> +       if (sci_port->irqs[1] == -ENXIO)
>                 for (i = 1; i < ARRAY_SIZE(sci_port->irqs); i++)
>                         sci_port->irqs[i] = sci_port->irqs[0];
> +       else if (sci_port->irqs[1] < 0)
> +               return sci_port->irqs[1];
>
>         sci_port->params = sci_probe_regmap(p);
>         if (unlikely(sci_port->params == NULL))
>
> And then the code flow is actively irritating. sci_init_single() copies
> irqs[0] to all other irqs[i] and then sci_request_irq() loops over the
> already requested irqs and checks for duplicates. A single place that
> identifies the exact set of required irqs would already help a lot.

Yeah, it's ugly and convoluted, like the wide set of hardware the
driver supports.

> Also for spi-rspi.c I don't see how platform_get_irq_byname_optional()
> returning 0 instead of -ENXIO would help. Please talk in patches.

--- a/drivers/spi/spi-rspi.c
+++ b/drivers/spi/spi-rspi.c
@@ -1420,17 +1420,25 @@ static int rspi_probe(struct platform_device *pdev)
        ctlr->max_native_cs = rspi->ops->num_hw_ss;

        ret = platform_get_irq_byname_optional(pdev, "rx");
-       if (ret < 0) {
+       if (ret < 0)
+               goto error2;
+
+       if (!ret) {
                ret = platform_get_irq_byname_optional(pdev, "mux");
-               if (ret < 0)
+               if (!ret)
                        ret = platform_get_irq(pdev, 0);
+               if (ret < 0)
+                       goto error2;
+
                if (ret >= 0)
                        rspi->rx_irq = rspi->tx_irq = ret;
        } else {
                rspi->rx_irq = ret;
                ret = platform_get_irq_byname(pdev, "tx");
-               if (ret >= 0)
-                       rspi->tx_irq = ret;
+               if (ret < 0)
+                       goto error2;
+
+               rspi->tx_irq = ret;
        }

        if (rspi->rx_irq == rspi->tx_irq) {

I like it when the "if (ret < ) ..." error handling is the first check to do.
With -ENXIO, it becomes more convoluted. and looks less nice (IMHO).

> Preferably first simplify in-driver logic to make the conversion to the
> new platform_get_irq_optional() actually reviewable.

So I have to choose between

    if (ret < 0 && ret != -ENXIO)
            return ret;

    if (ret) {
            ...
    }

and

    if (ret == -ENXIO) {
            ...
    } else if (ret < 0)
            return ret;
    }

with the final target being

    if (ret < 0)
            return ret;

    if (ret) {
            ...
    }

So the first option means the final change is smaller, but it looks less
nice than the second option (IMHO).
But the second option means more churn.

> > So there are three reasons: because the absence of an optional IRQ
> > is not an error, and thus that should not cause (a) an error code
> > to be returned, and (b) an error message to be printed, and (c)
> > because it can simplify the logic in device drivers.
>
> I don't agree to (a). If the value signaling not-found is -ENXIO or 0
> (or -ENODEV) doesn't matter much. I wouldn't deviate from the return
> code semantics of platform_get_irq() just for having to check against 0
> instead of -ENXIO. Zero is then just another magic value.

Zero is a natural magic value (also for pointers).
Errors are always negative.
Positive values are cookies (or pointers) associated with success.

> (c) still has to be proven, see above.
>
> > Commit 8973ea47901c81a1 ("driver core: platform: Introduce
> > platform_get_irq_optional()") fixed (b), but didn't address (a) and
> > (c).
>
> Yes, it fixed (b) and picked a bad name for that.

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