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@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.
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
