Re: [PATCH v5 06/11] devm-helpers: Add resource managed version of irq_create_mapping()

["Marek Behún" <kabel@kernel.org>]

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.

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?

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

Marek