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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32D9FC433EF for ; Tue, 26 Oct 2021 15:11:02 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 0E9A860C40 for ; Tue, 26 Oct 2021 15:11:02 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234461AbhJZPNY (ORCPT ); Tue, 26 Oct 2021 11:13:24 -0400 Received: from mout.kundenserver.de ([212.227.17.13]:46725 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234877AbhJZPNV (ORCPT ); Tue, 26 Oct 2021 11:13:21 -0400 Received: from mail-wr1-f41.google.com ([209.85.221.41]) by mrelayeu.kundenserver.de (mreue109 [213.165.67.113]) with ESMTPSA (Nemesis) id 1N0o3X-1mu9l442GD-00wimC for ; Tue, 26 Oct 2021 17:10:56 +0200 Received: by mail-wr1-f41.google.com with SMTP id k7so15219453wrd.13 for ; Tue, 26 Oct 2021 08:10:55 -0700 (PDT) X-Gm-Message-State: AOAM530hpur5lOL5BicT2fqJ/OP8WZtwkjurhp9+MeXW8rvUntU+Anyv 0mCcD5+Hxkv75ChmhzE5MSjigrh5hoMHq2kjZiA= X-Google-Smtp-Source: ABdhPJy+7SUebQLbF0Q1W8y0RnNXZNYOKe2RjrD/OSPEJ3iGKSMB21RzxOjstxrdGrCGx79gQiShwPFj7/Z4vBk/6MU= X-Received: by 2002:a5d:47a3:: with SMTP id 3mr20048042wrb.336.1635261055644; Tue, 26 Oct 2021 08:10:55 -0700 (PDT) MIME-Version: 1.0 References: <20210901123557.1043953-1-liu.yun@linux.dev> In-Reply-To: <20210901123557.1043953-1-liu.yun@linux.dev> From: Arnd Bergmann Date: Tue, 26 Oct 2021 17:10:39 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] ARM: s3c: irq-s3c24xx: Fix return value check for s3c24xx_init_intc() To: Jackie Liu Cc: Krzysztof Kozlowski , "moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES" , Linux ARM , Jackie Liu Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:sKz8AK2s1fxijo8P28GDH0elMEtvsHDYQsnOhd2mAnmd+cukegB GAtSKc1rG4jfQjYqTmn+Wu0em7VijURKtmNOQJaiHkV+sgfJDRVUHtK9fpFcIu0AzMMUjs0 lS5rmMR7R+PkxjHgZaW4hC/eJ++h9zn6wMaGL1yUQFot+G15UjO4/W9uNRlYyj5bNNYI7kI x2M1DHt62aCk2bF8KgqMQ== X-UI-Out-Filterresults: notjunk:1;V03:K0:uzkNhT3m1ag=:7VIkFDUHr87xQzJV4ZPL7v 7iFJmyb6YyFTIvMdOjrjTBPeKzCOVWrh6kc1jzFZdT0dk9VrPbcRz/cApRQhFYWriFPjo0LIC 16eBCCAq9F3Z4tvMivrHTBOXFuY5IzsSmsygk+3JinDD2482BT3xXEZEsxdMMQ4hqew0tAFjo ofUPuRx9o7jpyq8VdOH90AiSuGK66KUKazzcjEthlezLtnOs1e8ZbkHwI3w6lUAGX++N91w7q YqmWs7X/RB9ju7oKnOM6Tg26LJpLuaf6FGgNN/HvsGy4USX6Ro7Sa++rmSifzDD4aMqH/Qb/X WKFIFSO8RSaoGMw34pa8ypMoUQmqRDpaleal7mklmXymeJ5K6hmZdtzParhQ6j6NxMLSNCmKh JgKwDyCzDFJdsqGXE4TiSYW21Ah6BTWuhghehL/qj0lxV03ur96aNHmQrgQ9bGpzFQg/OHoO2 3s7K5cQQrA3LVLaU+ev3zu1GE77BAUviNFkTx5S8NxahqoS/GuvmxdV5yNpSQ/M/E6JYwMcaY 1mZbwPVrcEqHXOqOe/ORzsG25NvieRgnwkZbgAMLMwbtVTKX9TFhKvuj6q6JDCYYE4FG7yKm+ Ih+Sw5XVJSgv+pAbsBjAAdpbkNt7W6rnmpFJQzuaTKQ+8UvbF04maPcmHjSgy+u/io/hwzq9j Dvja5qK3jEVcTtHf8YprYAwU382TnwMFrKfnms3+vvkMM2tyArjUOk2/b2wcyHwx9DlU= Precedence: bulk List-ID: X-Mailing-List: linux-samsung-soc@vger.kernel.org On Wed, Sep 1, 2021 at 2:35 PM Jackie Liu wrote: > + /* For platform based machines, neither ERR nor NULL can happen here. > + * The s3c24xx_handle_irq() will be set as IRQ handler iff this succeeds: > + * > + * s3c_intc[0] = s3c24xx_init_intc() > + * > + * If this fails, the next calls to s3c24xx_init_intc() won't be executed. > + * > + * For DT machine, s3c_init_intc_of() could set the IRQ handler without > + * setting s3c_intc[0] only if it was called with num_ctrl=0. There is no > + * such code path, so again the s3c_intc[0] will have a valid pointer if > + * set_handle_irq() is called. > + * > + * Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is always something. > + */ > + if (s3c24xx_handle_intc(s3c_intc[0], regs, 0)) > + continue; > > - if (s3c_intc[2]) > + if (!IS_ERR_OR_NULL(s3c_intc[2])) > if (s3c24xx_handle_intc(s3c_intc[2], regs, 64)) > continue; I just saw this in the pull request. I'm taking the pull request since it's a bugfix and the resulting code is technically correct, but I'd point out that this is particularly ugly. Any use of IS_ERR_OR_NULL() essentially means we have a misdefined interface, and it's clear that this is one of them. Nothing actually uses the return code of s3c24xx_init_intc(), so returning NULL on error there and changing all the checks to that would be a much more straightforward solution. Any chance you could send a follow-up to do that? Arnd 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 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 429FBC433EF for ; Tue, 26 Oct 2021 15:12:19 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 0A164600CD for ; Tue, 26 Oct 2021 15:12:19 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 0A164600CD Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arndb.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=8OiH2DRTCVdtWDk6oe5YI9TYCzHsZtsahApnNdQ8AFk=; b=x53RT7JytnE3yO f+koo/FFn8bdpH44S0uu58QIdBO06J0wgo/wOwTZUzbvgOy4WYvmo8nEgoeWv/U88JhxTKdBsZB+q QzNzsuPyIQuhyP5sXNYUtFBnXPdoIEGt9gD5TRibxOnCsmWLBLZCSLQrp4n3n34YaGfbh5/i+AY19 MGY+3Cx4cH5Bv4/wydTDyVkjGq2OWfn0vmu8jTenUpDFPNXvS5rcjmLXWGyNSCYMNq/cUN0JIXMA4 bYKRPvLJD+jlWli5TVtQjP68cqMSISfHHphHLUrXqo/RFY2ypjjbG2nLO7C51xAQBH5rHpnJ7O+JA a+vjW6SzRYCIHHJDINgQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfO6T-0027AI-Sb; Tue, 26 Oct 2021 15:11:06 +0000 Received: from mout.kundenserver.de ([212.227.126.135]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mfO6P-002796-RC for linux-arm-kernel@lists.infradead.org; Tue, 26 Oct 2021 15:11:03 +0000 Received: from mail-wr1-f47.google.com ([209.85.221.47]) by mrelayeu.kundenserver.de (mreue012 [213.165.67.97]) with ESMTPSA (Nemesis) id 1N4eOd-1mqM7T0x43-011iCe for ; Tue, 26 Oct 2021 17:10:56 +0200 Received: by mail-wr1-f47.google.com with SMTP id e12so19861976wra.4 for ; Tue, 26 Oct 2021 08:10:55 -0700 (PDT) X-Gm-Message-State: AOAM530oeDi8G1RtL/hrg/oSYexcNGlVNQ1KdTKgbhv7LaPFdROpHC7p bRVZluhXAYZxaqh5ShN4ODiXlhC9GJA5TMrLFew= X-Google-Smtp-Source: ABdhPJy+7SUebQLbF0Q1W8y0RnNXZNYOKe2RjrD/OSPEJ3iGKSMB21RzxOjstxrdGrCGx79gQiShwPFj7/Z4vBk/6MU= X-Received: by 2002:a5d:47a3:: with SMTP id 3mr20048042wrb.336.1635261055644; Tue, 26 Oct 2021 08:10:55 -0700 (PDT) MIME-Version: 1.0 References: <20210901123557.1043953-1-liu.yun@linux.dev> In-Reply-To: <20210901123557.1043953-1-liu.yun@linux.dev> From: Arnd Bergmann Date: Tue, 26 Oct 2021 17:10:39 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v2] ARM: s3c: irq-s3c24xx: Fix return value check for s3c24xx_init_intc() To: Jackie Liu Cc: Krzysztof Kozlowski , "moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES" , Linux ARM , Jackie Liu X-Provags-ID: V03:K1:yc/tOUdZwP1TKXAbVq/VMXpNhmrBqbYXzHhNeEZCm8qJxaxDFD8 gZ/7ZybCM4WXhhs8m9/a/Hkmv841GTvI28+Y0R0RVLFrlY9IUKj9IXCG57FbhBXYtpnR0TM WM+NtOSuuK043YEzZrUKia4vup04mPVfQkhmC+H9+regaBE7ErkoeQp6O5EqdZLgj0x9zQr PeXInANeeK12AIsYLY4Ew== X-UI-Out-Filterresults: notjunk:1;V03:K0:NolemJ0yyzM=:M+S5a/j/pjIiJ66YOiY1vS jlhtVUJL7A39KTmGeoDVllOq/8SWoEvagpv2DUaRbcwV7HcHR27u6Bt4KJ2a5XsNZsJ3uW7p7 dUpPTSgso15TnBqGpHaJhSY0x/M4/2ECrulynazF61colDfpIecvh3QuBUWp+HpQLauc4SgQn aRbB6pPbso6Tl2KWXinxIlv8BXuf/zhyxdA5zm8w+g7tH0D+OXRxZAJukHYKo/5DhZ1INwjZe Nxq1GLE6z3QOplXVRbzNUY29ci07JjzUZ2AcKmBo7PmjU4LMxI+wgAeSTAwcyCtrQwJZ5dJhU vPKGwlEuDsxfIMUbZrsDS65o/GkPyzfJHp+3kK93wKg3vTQrtMQ9Fl0qSo5WpJch9hLmMQBGD b4NYxlDBunkI8FrCWnZDfGUKofk7yt6OpSsBmGuLy8XDmRTJvL33tO77Sp0kc7hkVAnlty9rU j29iI0Wl91tWw86buv5arWe/ov/x8utAqxKPgNHwKwqe5396aDTWwwX/XPN3efV/x2Py9PLIp 9QyZoLiwSrNhKCjGKQ4eNXaSGDInD7i1wDrVcHqS3QbHTpl3XwYM6/+XgFCRE/zCzppPl9yEW QOy7diU5rs8Y6uddJuGsijB4NwvzLsttkQwBwnHFVQfNcXdEBObFJklPtsUCAczxjiqDZm0Cu a7vuscgZH1VP+qVbKHVgwmQSHPQBZWXWrHKncblfqWrjpBKGUcgVd5+hIiiG9loKo0es= X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20211026_081102_217557_36234524 X-CRM114-Status: GOOD ( 18.67 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Wed, Sep 1, 2021 at 2:35 PM Jackie Liu wrote: > + /* For platform based machines, neither ERR nor NULL can happen here. > + * The s3c24xx_handle_irq() will be set as IRQ handler iff this succeeds: > + * > + * s3c_intc[0] = s3c24xx_init_intc() > + * > + * If this fails, the next calls to s3c24xx_init_intc() won't be executed. > + * > + * For DT machine, s3c_init_intc_of() could set the IRQ handler without > + * setting s3c_intc[0] only if it was called with num_ctrl=0. There is no > + * such code path, so again the s3c_intc[0] will have a valid pointer if > + * set_handle_irq() is called. > + * > + * Therefore in s3c24xx_handle_irq(), the s3c_intc[0] is always something. > + */ > + if (s3c24xx_handle_intc(s3c_intc[0], regs, 0)) > + continue; > > - if (s3c_intc[2]) > + if (!IS_ERR_OR_NULL(s3c_intc[2])) > if (s3c24xx_handle_intc(s3c_intc[2], regs, 64)) > continue; I just saw this in the pull request. I'm taking the pull request since it's a bugfix and the resulting code is technically correct, but I'd point out that this is particularly ugly. Any use of IS_ERR_OR_NULL() essentially means we have a misdefined interface, and it's clear that this is one of them. Nothing actually uses the return code of s3c24xx_init_intc(), so returning NULL on error there and changing all the checks to that would be a much more straightforward solution. Any chance you could send a follow-up to do that? Arnd _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel