From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BDFBCC47247 for ; Tue, 5 May 2020 13:35:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9C665206FA for ; Tue, 5 May 2020 13:35:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588685759; bh=27eegcfN4OCPFz8Eg3QszwZbxwkuazdL8Z4GFlVGgsc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:List-ID:From; b=HKZOO0YEE4P+XVg0zby/oJ0GwWSbkIX4sCF6x2rR2eAM06xD4rRo/qBIZ2pHGDsyu s94JhxDDIVGdeERGjcuiXq3nt6qY/Lm5VumHupD0xRRbfTgWxIEqlK9jMj7u5Y///S QprReuZvaOAOx8l3+P+sveuf2sjMyrsd7COv/hV8= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729087AbgEENf7 (ORCPT ); Tue, 5 May 2020 09:35:59 -0400 Received: from mail.kernel.org ([198.145.29.99]:52568 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728954AbgEENf6 (ORCPT ); Tue, 5 May 2020 09:35:58 -0400 Received: from localhost (83-86-89-107.cable.dynamic.v4.ziggo.nl [83.86.89.107]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C4E62206A5; Tue, 5 May 2020 13:35:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1588685758; bh=27eegcfN4OCPFz8Eg3QszwZbxwkuazdL8Z4GFlVGgsc=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=vqtExeeaUnSviGJmZkNmbJ6Gn/s2RKKtdHtQdFsbPTHdXw+HyZ9OVK5RAdxltmRQG jVJjsOGMfvCI/WGbVQ5zHdvfKgTgaX2fnfB2z5y9vZhl75mVcFbfArkyRk0aowa72w jhu7JDiZmh5qT097JYnQM5NPjFPBnbFY2zULRZrc= Date: Tue, 5 May 2020 15:35:55 +0200 From: Greg Kroah-Hartman To: Bjorn Helgaas Cc: Thomas Gleixner , "Rafael J . Wysocki" , Aman Sharma , linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org, Bjorn Helgaas Subject: Re: [PATCH v2 1/2] driver core: platform: Clarify that IRQ 0 is invalid Message-ID: <20200505133555.GA529259@kroah.com> References: <20200504190721.GA2810934@kroah.com> <20200504222659.GA296947@bjorn-Precision-5520> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20200504222659.GA296947@bjorn-Precision-5520> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, May 04, 2020 at 05:26:59PM -0500, Bjorn Helgaas wrote: > On Mon, May 04, 2020 at 09:07:21PM +0200, Greg Kroah-Hartman wrote: > > On Mon, May 04, 2020 at 01:08:22PM -0500, Bjorn Helgaas wrote: > > > On Sat, May 02, 2020 at 08:15:37AM +0200, Greg Kroah-Hartman wrote: > > > > On Fri, May 01, 2020 at 05:40:41PM -0500, Bjorn Helgaas wrote: > > > > > From: Bjorn Helgaas > > > > > > > > > > These interfaces return a negative error number or an IRQ: > > > > > > > > > > platform_get_irq() > > > > > platform_get_irq_optional() > > > > > platform_get_irq_byname() > > > > > platform_get_irq_byname_optional() > > > > > > > > > > The function comments suggest checking for error like this: > > > > > > > > > > irq = platform_get_irq(...); > > > > > if (irq < 0) > > > > > return irq; > > > > > > > > > > which is what most callers (~900 of 1400) do, so it's implicit > > > > > that IRQ 0 is invalid. But some callers check for "irq <= 0", > > > > > and it's not obvious from the source that we never return an > > > > > IRQ 0. > > > > > > > > > > Make this more explicit by updating the comments to say that > > > > > an IRQ number is always non-zero and adding a WARN() if we > > > > > ever do return zero. If we do return IRQ 0, it likely > > > > > indicates a bug in the arch-specific parts of > > > > > platform_get_irq(). > > > > > > > > I worry about adding WARN() as there are systems that do > > > > panic_on_warn() and syzbot trips over this as well. I don't > > > > think that for this issue it would be a problem, but what really > > > > is this warning about that someone could do anything with? > > > > > > > > Other than that minor thing, this looks good to me, thanks for > > > > finally clearing this up. > > > > > > What I'm concerned about is an arch that returns 0. Most drivers > > > don't check for 0 so they'll just try to use it, and things will > > > fail in some obscure way. My assumption is that if there really > > > is no IRQ, we should return -ENOENT or similar instead of 0. > > > > > > I could be convinced that it's not worth warning about at all, or > > > we could do something like the following: > > > > > > diff --git a/drivers/base/platform.c b/drivers/base/platform.c > > > index 084cf1d23d3f..4afa5875e14d 100644 > > > --- a/drivers/base/platform.c > > > +++ b/drivers/base/platform.c > > > @@ -220,7 +220,11 @@ int platform_get_irq_optional(struct platform_device *dev, unsigned int num) > > > ret = -ENXIO; > > > #endif > > > out: > > > - WARN(ret == 0, "0 is an invalid IRQ number\n"); > > > + /* Returning zero here is likely a bug in the arch IRQ code */ > > > + if (ret == 0) { > > > + pr_warn("0 is an invalid IRQ number\n"); > > > + dump_stack(); > > > + } > > > return ret; > > > } > > > ... > > > I like that, but you said this is something that the platform people > > should only see when bringing up a new system, so maybe the WARN() is > > fine. It's not user-triggerable, so your original is ok. > > Is that an ack? Thomas, any thoughts? Sorry, yes: Acked-by: Greg Kroah-Hartman