Received-SPF: pass (google.com: domain of linux-crypto+bounces-2937-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 27 Mar 2024 10:34:19 +0100
From: Marek =?UTF-8?B?QmVow7pu?= <kabel@kernel.org>
To: Dan Carpenter <dan.carpenter@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>, Gregory CLEMENT
 <gregory.clement@bootlin.com>, soc@kernel.org, arm@kernel.org, Hans de
 Goede <hdegoede@redhat.com>, Matti Vaittinen <mazziesaccount@gmail.com>,
 Horia =?UTF-8?B?R2VhbnTEgw==?= <horia.geanta@nxp.com>, Pankaj Gupta
 <pankaj.gupta@nxp.com>, Gaurav Jain <gaurav.jain@nxp.com>,
 linux-crypto@vger.kernel.org, Herbert Xu <herbert@gondor.apana.org.au>
Subject: Re: [PATCH v5 06/11] devm-helpers: Add resource managed version of
 irq_create_mapping()
Message-ID: <20240327103419.3918953a@dellmb>
In-Reply-To: <72bf31c3-337d-4747-8353-639492507a7b@moroto.mountain>
References: <20240323164359.21642-1-kabel@kernel.org>
	<20240323164359.21642-7-kabel@kernel.org>
	<72bf31c3-337d-4747-8353-639492507a7b@moroto.mountain>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Tue, 26 Mar 2024 12:00:25 +0300
Dan Carpenter <dan.carpenter@linaro.org> wrote:

> On Sat, Mar 23, 2024 at 05:43:54PM +0100, Marek Beh=C3=BAn wrote:
> > +/**
> > + * devm_irq_create_mapping - Resource managed version of irq_create_ma=
pping()
> > + * @dev:	Device which lifetime the mapping is bound to
> > + * @domain:	domain owning this hardware interrupt or NULL for default =
domain
> > + * @hwirq:	hardware irq number in that domain space
> > + *
> > + * Create an irq mapping to linux irq space which is automatically dis=
posed when
> > + * the driver is detached.
> > + * devm_irq_create_mapping() can be used to omit the explicit
> > + * irq_dispose_mapping() call when driver is detached.
> > + *
> > + * Returns a linux irq number on success, 0 if mapping could not be cr=
eated, or =20
>                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^=
^^^^^
>=20
> > + * a negative error number if devm action could not be added.
> > + */
> > +static inline int devm_irq_create_mapping(struct device *dev,
> > +					  struct irq_domain *domain,
> > +					  irq_hw_number_t hwirq)
> > +{
> > +	unsigned int virq =3D irq_create_mapping(domain, hwirq);
> > +
> > +	if (!virq)
> > +		return 0; =20
>=20
> What is the point of returning zero instead of an error code?  Neither
> of the callers that are introduced later in the patchset use this.
>=20
> I understand that it matches some of the other legacy irq function
> behaviors, but I think we are trying to move away from that because it
> just leads to bugs.
>=20
> Since we don't need the zero now, let's wait until we have a user before
> introducing this behavior.  Then we can add a new function that returns
> zero, but we'll still encourage people to use the standard error code
> function where possible.  And at the same time, when we do introduce the
> zero is an error code, function you should contact
> kernel-janitors@vger.kernel.org so someone an write a static checker
> rule to detect the bugs that result from it.

Hi Dan,

the first user of this function is the very next patch of this series,
and it does this:

+	irq =3D devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
+	if (irq <=3D 0)
+		return dev_err_probe(dev, irq ?: -ENXIO,
+				     "Cannot map MESSAGE_SIGNED IRQ\n");

So it handles !irq as -ENXIO.

I looked into several users who do
  virq =3D irq_create_mapping()
and then reutrn errno if !virq:

  git grep -A 3 'virq =3D irq_create_mapping'

Some return -ENOMEM, some -ENXIO, some -EINVAL.

What do you think?

Or should I send this driver without introducing this helper for now?

Marek