Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq()

[Greg Ungerer]

On 26/2/20 4:39 pm, Finn Thain wrote:
> On Wed, 26 Feb 2020, Greg Ungerer wrote:
>
> > > That error would almost always be -EBUSY, right?
> >
> > I expect it will never fail this early in boot.
>
> If so, it suggests to me that tweaking the error message string is just
> bikeshedding and that adding these error messages across the tree is just
> bloat.
>
> > But how will you know if it really is EBUSY if you don't print it out?
> >
> > > Moreover, compare this change,
> > >
> > > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > > +	request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > >
> > > with this change,
> > >
> > > +	int err;
> > >
> > > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > > +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > > +	if (err)
> > > +		return err;
> > >
> > > Isn't the latter change the more common pattern? It prints nothing.
> >
> > Hmm, in my experience the much more common pattern is:
> >
> > > +	int err;
> > >
> > > -	setup_irq(TMR_IRQ_NUM, &m68328_timer_irq);
> > > +	err = request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL);
> > > +	if (err) {
> > > +             pr_err("timer: request_irq() failed with err=%d\n", err);
> > > +		return err;
> > > +     }
> >
> > Where the pr_err() could be one of pr_err, printk, dev_err, ...
>
> A rough poll using 'git grep' seems to agree with your assessment.
>
> If -EBUSY means the end user has misconfigured something, printing
> "request_irq failed" would be helpful. But does that still happen?

I have seen it many times. Its not at all difficult to get interrupt
assignments wrong, duplicated, or otherwise mistaken when creating
device trees. Not so much m68k/coldfire platforms where they are
most commonly hard coded.


> Printing any error message for -ENOMEM is frowned upon, and printing -12
> is really unhelpful. So the most popular pattern isn't that great, though
> it is usually less verbose than the example you've given.
>
> Besides, introducing local variables and altering control flow seems well
> out-of-scope for this kind of refactoring, right?

I don't agree with the local variable part. Adding a local variable to
keep track of the error return code doesn't seem out of scope for this change.
The patch as Afzal sent it doesn't change the control flow - and
that is the right thing to do here.


> Anyway, if you're going to add an error message,
> pr_err("%s: request_irq failed", foo) is unavoidable whenever foo isn't a
> string constant, so one can't expect to grep the source code for the
> literal error message from the log.
>
> BTW, one of the benefits of "%s: request_irq failed" is that a compilation
> unit with multiple request_irq calls permits the compiler to coalesce all
> duplicated format strings. Whereas, that's not possible with
> "foo: request_irq failed" and "bar: request_irq failed".

Given the wide variety of message text used with failed request_irq() calls
it would be shear luck that this matched anything else. A quick grep shows
that "%s: request_irq() failed\n" has no other exact matches in the current
kernel source.

Regards
Greg