Received: by 2002:a25:1506:0:0:0:0:0 with SMTP id 6csp4342423ybv; Tue, 25 Feb 2020 18:12:45 -0800 (PST) X-Google-Smtp-Source: APXvYqxc/B56t5GN9dJeMrXyMBY1t63fv3NsSPtVIBZjq6eRyd3Xa11oV703DcvL4KGYRiRTo/yx X-Received: by 2002:aca:2419:: with SMTP id n25mr1424073oic.13.1582683164920; Tue, 25 Feb 2020 18:12:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1582683164; cv=none; d=google.com; s=arc-20160816; b=vpgoKegrNBxW2mjAlN+8hjJR+LJu/oBjhoxwnWCEmRWOq6j+EK6kHvI/48WApB8/jz pyjXcwNkrnwKHqzC1aAerKvaOXY/TyM72tBYjRGTD7eifcsMSHtNExqf5J6bxveG8YNn 5+LYsIc2tWamtCUDYMVNPpXvjXoU7e3TXFSnrv0QQ9Pb4N7N8yh4fojVt1Xbk0QdOlKl lGaBJewH5+TTFUVnKIxxw+Db9F3UKQq/w6spVJdMFpstvJ9D+W0Ow87ssC84jQFouW3K aMfaP8qNXAjqbrCW9eHqH+5Ah2JDbktqM8o4r+UEgxC9UNdhtBNBG6Uuig96ACYznAua RKnw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject; bh=bgt2rHOD7zhcGbiJ18ZixbigenUvmxVYgKECbeqlMfc=; b=x8rSs5H13wp1l9KrBY819J+AgQototRm0lNC6PRdNVRpUYkDy31AZfPCYW9zLQSbCO AxnjglIHxrPwYGm5ykbgoOTaK+hcozbtWsx7c5IY91lDE9tMKMj5N28mwW6Uk3bRNxts U/ovgKAVOVEBmSeyZ5hPifU0+aaPL1SoredyyRYDUzCF+ZGMRKXshxhtGXs1tJ4NWAsj 26K4rS4mo7jTNPMgWy4vlqIGTHzxbz+zv6wsYwSTGJ7Z40Cz/7OPAMig+l2IRYBHmuCi OwqebBIAcipiu5NGcRaNcxVKcfhdETtMH4YUE6MyjgAmHua94sGK75WV1yTx9Ta229X9 biag== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id u63si334383oif.88.2020.02.25.18.12.32; Tue, 25 Feb 2020 18:12:44 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730170AbgBZCL5 (ORCPT + 99 others); Tue, 25 Feb 2020 21:11:57 -0500 Received: from mail.kernel.org ([198.145.29.99]:40970 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1730008AbgBZCL4 (ORCPT ); Tue, 25 Feb 2020 21:11:56 -0500 Received: from [10.44.0.22] (unknown [103.48.210.53]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 030E521744; Wed, 26 Feb 2020 02:11:50 +0000 (UTC) Subject: Re: [PATCH v2 06/18] m68k: Replace setup_irq() by request_irq() To: Finn Thain Cc: afzal mohammed , linux-m68k@lists.linux-m68k.org, linux-kernel@vger.kernel.org, Thomas Gleixner , Geert Uytterhoeven References: <00b0bf964278dd0bb3e093283994399ff796cca5.1582471508.git.afzal.mohd.ma@gmail.com> <73c3ad08-963d-fea2-91d7-b06e4ef8d3ef@linux-m68k.org> From: Greg Ungerer Message-ID: Date: Wed, 26 Feb 2020 12:11:38 +1000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 26/2/20 11:11 am, Finn Thain wrote: > On Wed, 26 Feb 2020, Greg Ungerer wrote: >> On 24/2/20 10:50 am, afzal mohammed wrote: >>> request_irq() is preferred over setup_irq(). The early boot setup_irq() >>> invocations happen either via 'init_IRQ()' or 'time_init()', while >>> memory allocators are ready by 'mm_init()'. >>> >>> Per tglx[1], setup_irq() existed in olden days when allocators were not >>> ready by the time early interrupts were initialized. >>> >>> Hence replace setup_irq() by request_irq(). >>> >>> Seldom remove_irq() usage has been observed coupled with setup_irq(), >>> wherever that has been found, it too has been replaced by free_irq(). >>> >>> [1] https://lkml.kernel.org/r/alpine.DEB.2.20.1710191609480.1971@nanos >>> >>> Signed-off-by: afzal mohammed >>> Tested-by: Greg Ungerer # ColdFire >>> --- >>> >>> v2: >>> * Replace pr_err("request_irq() on %s failed" by >>> pr_err("%s: request_irq() failed" >>> * Commit message massage >>> * remove now irrelevant comment lines at 3 places >>> >>> arch/m68k/68000/timers.c | 11 ++--------- >>> arch/m68k/coldfire/pit.c | 11 ++--------- >>> arch/m68k/coldfire/sltimers.c | 19 +++++-------------- >>> arch/m68k/coldfire/timers.c | 21 +++++---------------- >>> 4 files changed, 14 insertions(+), 48 deletions(-) >>> >>> diff --git a/arch/m68k/68000/timers.c b/arch/m68k/68000/timers.c >>> index 71ddb4c98726..55a76a2d3d58 100644 >>> --- a/arch/m68k/68000/timers.c >>> +++ b/arch/m68k/68000/timers.c >>> @@ -68,14 +68,6 @@ static irqreturn_t hw_tick(int irq, void *dummy) >>> /***************************************************************************/ >>> -static struct irqaction m68328_timer_irq = { >>> - .name = "timer", >>> - .flags = IRQF_TIMER, >>> - .handler = hw_tick, >>> -}; >>> - >>> >>> -/***************************************************************************/ >>> - >>> static u64 m68328_read_clk(struct clocksource *cs) >>> { >>> unsigned long flags; >>> @@ -106,7 +98,8 @@ void hw_timer_init(irq_handler_t handler) >>> TCTL = 0; >>> /* set ISR */ >>> - setup_irq(TMR_IRQ_NUM, &m68328_timer_irq); >>> + if (request_irq(TMR_IRQ_NUM, hw_tick, IRQF_TIMER, "timer", NULL)) >>> + pr_err("%s: request_irq() failed\n", "timer"); >> >> Why not just: >> >> pr_err("timer: request_irq() failed\n"); >> > > I believe that the compiler would coalesce the two "timer" string > constants in the patch from Afzal (as per my suggestion).> > I suspect that your version costs a few extra bytes everywhere it appears > (but I didn't check). Maybe. It costs some extra code for another argument push and a bunch of cycles to process the %s at run time though (if triggered). The profile timer setup is not commonly used, so in most typical builds there is no scope for coalescing the same string. So in the end most builds will be a few bytes larger with the separated strings. But really that is not the point. It just seems simpler and clearer to me to put the string in place - all in one. >> And maybe would it be useful to print out the error return code from a >> failed request_irq()? What about displaying the requested IRQ number as >> well? Just a thought. >> > > That error would almost always be -EBUSY, right? I expect it will never fail this early in boot. 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, ... Regards Greg