Received-SPF: pass (google.com: domain of linux-kernel+bounces-214446-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Precedence: bulk
MIME-Version: 1.0
References: <cover.1718311756.git.u.kleine-koenig@baylibre.com> <85a3d444f2943ebe9d64e722b1717a5f7d06ed48.1718311756.git.u.kleine-koenig@baylibre.com>
In-Reply-To: <85a3d444f2943ebe9d64e722b1717a5f7d06ed48.1718311756.git.u.kleine-koenig@baylibre.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Fri, 14 Jun 2024 09:26:52 +0200
Message-ID: <CAMuHMdUHi0Yu2Giccp7pHqpjLWG2CjkGFWehLV0iWBHB_V4oGw@mail.gmail.com>
Subject: Re: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to dev_err_probe()
To: =?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <ukleinek@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	=?UTF-8?Q?Uwe_Kleine=2DK=C3=B6nig?= <u.kleine-koenig@baylibre.com>, 
	"Rafael J. Wysocki" <rafael@kernel.org>, Petr Mladek <pmladek@suse.com>, 
	Andy Shevchenko <andriy.shevchenko@linux.intel.com>, Xiubo Li <xiubli@redhat.com>, 
	Geert Uytterhoeven <geert+renesas@glider.be>, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Uwe,

On Thu, Jun 13, 2024 at 11:24=E2=80=AFPM Uwe Kleine-K=C3=B6nig <ukleinek@ke=
rnel.org> wrote:
> From: Uwe Kleine-K=C3=B6nig <u.kleine-koenig@baylibre.com>
>
> If a function returns the error code -ENOMEM, there should be no error
> output, because a failing allocation is already quite talkative and
> adding another indication only makes it harder to determine the actual
> problem.
>
> So the construct:
>
>         ret =3D some_function(...);
>         if (ret)
>                 return dev_err_probe(dev, ret, ...);
>
> is questionable if some_function() can only succeed or return -ENODEV.
>
> Catch some of these failures during compile time.
>
> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Uwe Kleine-K=C3=B6nig <u.kleine-koenig@baylibre.com>

Thanks for your patch!

> I have some concerns about this patch, I only implemented it because in
> reply to the first submission of patch #1 Andy wrote that he thinks this
> should be done, too. So the idea of this patch is only to keep the
> discussion about handling a constant -ENOMEM to dev_err_probe() away
> from patch 1, in the hope to make application of patch 1 more likely :-)
>
> So, I think this patch 2/2 is a bad idea, because:
>
>  - Let's assume there are functions, that return either success or
>    -ENOMEM. (I'm not aware of such a function, but I didn't search for
>    one and probably something like that exists.) Probably the compiler
>    won't be able to know that, and so doesn't catch that "problem".

You can find several in public header files:

    git grep -W "return\s*-ENOMEM\>" -- include/

I expect there are more in static code all over the place.

>  - Using dev_err_probe() to handle the return code of some_function() is
>    convenient. First to make error handling in the calling function
>    uniform, and second, to not create a patch opportunity for all
>    callers when some_function() might return another error code in the
>    future. So dev_err_probe() can just be used without caring for the
>    details of the handled error.

IMHO this is the only drawback.
And things may change: a static (inline) function that can only return
zero or -ENOMEM now, can return other error codes tomorrow.
Also, some dummies (e.g. dma_mapping_error()) return -ENOMEM, so it
depends on kernel configuration too.

>  - In the presence of patch #1, there is no real problem with calling
>    dev_err_probe(dev, -ENOMEM, ...), because this is an error path and
>    so not performance critical, and no error message is emitted.

There's still the issue of increased kernel size, mainly due to the
presence of the error message string.

> Given these, the more complicated implementation for dev_err_probe()
> isn't really justified IMHO.

My initial reaction was quite positive, until I discovered the dummies...

> --- a/include/linux/dev_printk.h
> +++ b/include/linux/dev_printk.h
> @@ -275,6 +275,12 @@ do {                                                =
                       \
>         WARN_ONCE(condition, "%s %s: " format, \
>                         dev_driver_string(dev), dev_name(dev), ## arg)
>
> -__printf(3, 4) int dev_err_probe(const struct device *dev, int err, cons=
t char *fmt, ...);
> +__printf(3, 4) int __dev_err_probe(const struct device *dev, int err, co=
nst char *fmt, ...);
> +#define dev_err_probe(dev, err, ...)                                    =
       \
> +       ({                                                               =
       \
> +               int __err =3D (err);                                     =
         \
> +               BUILD_BUG_ON(__builtin_constant_p(__err) && __err =3D=3D =
-ENOMEM);  \
> +               __dev_err_probe((dev), __err, __VA_ARGS__);              =
       \
> +        })
>
>  #endif /* _DEVICE_PRINTK_H_ */

Looks like dev_err_probe() does not have a dummy for the !CONFIG_PRINTK
case yet, while it could definitely use one.

Gr{oetje,eeting}s,

                        Geert

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

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