Received-SPF: pass (google.com: domain of linux-kernel+bounces-214619-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Fri, 14 Jun 2024 11:15:53 +0200
From: Uwe =?utf-8?Q?Kleine-K=C3=B6nig?= <u.kleine-koenig@baylibre.com>
To: Geert Uytterhoeven <geert@linux-m68k.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
	"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
Subject: Re: [PATCH RFC v2 2/2] driver core: Don't allow passing a -ENOMEM to
 dev_err_probe()
Message-ID: <7xnzx6dlim2r5oujon3csocrljcvtvwz46yvmik62hpn4a2rlj@tiqrzlol7l5u>
References: <cover.1718311756.git.u.kleine-koenig@baylibre.com>
 <85a3d444f2943ebe9d64e722b1717a5f7d06ed48.1718311756.git.u.kleine-koenig@baylibre.com>
 <CAMuHMdUHi0Yu2Giccp7pHqpjLWG2CjkGFWehLV0iWBHB_V4oGw@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="7y5r7ee3ph6dfrbr"
Content-Disposition: inline
In-Reply-To: <CAMuHMdUHi0Yu2Giccp7pHqpjLWG2CjkGFWehLV0iWBHB_V4oGw@mail.gmail.com>


--7y5r7ee3ph6dfrbr
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Hello Geert,

On Fri, Jun 14, 2024 at 09:26:52AM +0200, Geert Uytterhoeven wrote:
> On Thu, Jun 13, 2024 at 11:24=E2=80=AFPM Uwe Kleine-K=C3=B6nig <ukleinek@=
kernel.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>
>=20
> Thanks for your patch!
>=20
> > 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".
>=20
> You can find several in public header files:
>=20
>     git grep -W "return\s*-ENOMEM\>" -- include/
>=20
> I expect there are more in static code all over the place.
>=20
> >  - 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.
>=20
> 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.

Huh, I didn't spot the dependency on kernel configuration. That makes it
quite bad.

> >  - 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.
>=20
> There's still the issue of increased kernel size, mainly due to the
> presence of the error message string.
>=20
> > Given these, the more complicated implementation for dev_err_probe()
> > isn't really justified IMHO.
>=20
> My initial reaction was quite positive, until I discovered the dummies...
>=20
> > --- 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, co=
nst char *fmt, ...);
> > +__printf(3, 4) int __dev_err_probe(const struct device *dev, int err, =
const 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_ */
>=20
> Looks like dev_err_probe() does not have a dummy for the !CONFIG_PRINTK
> case yet, while it could definitely use one.

Would you want to drop

	device_set_deferred_probe_reason(dev, &vaf);

=66rom dev_err_probe() for !CONFIG_PRINTK, too? If not, you can throw away
the string only if __builtin_constant_p(__err !=3D -EPROBE_DEFER) && __err
!=3D -EPROBE_DEFER. I agree such an improvement would be nice, but that's
orthogonal to this series.

Best regards
Uwe

--7y5r7ee3ph6dfrbr
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQEzBAABCgAdFiEEP4GsaTp6HlmJrf7Tj4D7WH0S/k4FAmZsCkUACgkQj4D7WH0S
/k7Qbgf9EV+28gU+jqlvkMzclGlUr7rW6QNrb9JQGrRrKvb31TFgjBN93XjNA1AO
gZP1hX4ObkhdmE8r6PYNsEtNyqpzIijNJu0LT3rFMUpXZLuGWxXLbfCJpGgdv9BN
BwSEk5Fxw1iZyHk8Wmv2Jv9vFb78n8Gv48DyjY3Rue4V0xtkJrqXjEq1CeRO9ErJ
0AB8AyPpYcRXmZQRccS97m8P6MWTZDz3SURYv8bvyRC2mOYRPTIZUKKIzKpglWnC
mgnXLdaq67Yw/HnnLsWoUSihzdez0ENhL21ZTMN4DiSjSbl8gM+Jt3rrZhMzG9Fi
5Sw1sgmMb5Gb4hx1BohzdtICvhBWEw==
=ZWF0
-----END PGP SIGNATURE-----

--7y5r7ee3ph6dfrbr--