On Tue, 26 Mar 2024 12:00:25 +0300
Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxxxxxxx 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
