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

[Greg Ungerer <gerg@linux-m68k.org>]

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