Received-SPF: pass (google.com: domain of linux-crypto+bounces-2938-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Wed, 27 Mar 2024 14:39:21 +0300
From: Dan Carpenter <dan.carpenter@linaro.org>
To: Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.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?Q?Geant=C4=83?= <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: <b70ec580-dfac-48bc-87d6-b30f2e91f723@moroto.mountain>
References: <20240323164359.21642-1-kabel@kernel.org>
 <20240323164359.21642-7-kabel@kernel.org>
 <72bf31c3-337d-4747-8353-639492507a7b@moroto.mountain>
 <20240327103419.3918953a@dellmb>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20240327103419.3918953a@dellmb>

On Wed, Mar 27, 2024 at 10:34:19AM +0100, Marek Beh?n wrote:
> 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?n wrote:
> > > +/**
> > > + * devm_irq_create_mapping - Resource managed version of irq_create_mapping()
> > > + * @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 disposed 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 created, or  
> >                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > > + * 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 = irq_create_mapping(domain, hwirq);
> > > +
> > > +	if (!virq)
> > > +		return 0;  
> > 
> > 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.
> > 
> > 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.
> > 
> > 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 = devm_irq_create_mapping(dev, mcu->gc.irq.domain, irq_idx);
> +	if (irq <= 0)
> +		return dev_err_probe(dev, irq ?: -ENXIO,
> +				     "Cannot map MESSAGE_SIGNED IRQ\n");
> 
> So it handles !irq as -ENXIO.

Yeah.  But imagine how much easier it would be if devm_irq_create_mapping()
returned -ENXIO directly.

	irq = devm_irq_create_mapping();
	if (irq < 0)
		return dev_err_probe(dev, irq,
				     "Cannot map MESSAGE_SIGNED IRQ\n");

> 
> I looked into several users who do
>   virq = irq_create_mapping()
> and then reutrn errno if !virq:
> 
>   git grep -A 3 'virq = irq_create_mapping'
> 
> Some return -ENOMEM, some -ENXIO, some -EINVAL.
> 
> What do you think?

-ENXIO is fine.

regards,
dan carpenter