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=-7.0 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,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 5E50CC433E6 for ; Mon, 13 Jul 2020 15:40:10 +0000 (UTC) Received: from lists.ozlabs.org (lists.ozlabs.org [203.11.71.2]) (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 4B90120771 for ; Mon, 13 Jul 2020 15:40:09 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4B90120771 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=arndb.de Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Received: from bilbo.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3]) by lists.ozlabs.org (Postfix) with ESMTP id 4B57BZ5g9fzDqQB for ; Tue, 14 Jul 2020 01:40:06 +1000 (AEST) Authentication-Results: lists.ozlabs.org; spf=none (no SPF record) smtp.mailfrom=arndb.de (client-ip=217.72.192.74; helo=mout.kundenserver.de; envelope-from=arnd@arndb.de; receiver=) Authentication-Results: lists.ozlabs.org; dmarc=none (p=none dis=none) header.from=arndb.de Received: from mout.kundenserver.de (mout.kundenserver.de [217.72.192.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4B56VF1X82zDqTQ for ; Tue, 14 Jul 2020 01:08:35 +1000 (AEST) Received: from mail-ej1-f41.google.com ([209.85.218.41]) by mrelayeu.kundenserver.de (mreue106 [212.227.15.145]) with ESMTPSA (Nemesis) id 1MVubb-1kKTWr3heX-00Rqwt for ; Mon, 13 Jul 2020 17:08:28 +0200 Received: by mail-ej1-f41.google.com with SMTP id lx13so17619660ejb.4 for ; Mon, 13 Jul 2020 08:08:27 -0700 (PDT) X-Gm-Message-State: AOAM530yhRomNGJu5W5NckTaVFTb/p4XV9P6MYFetXmUpb6FqpttOFLM J+NKRM3O1DByvMgCj/aOT94dHeloffD8V+4kTEE= X-Google-Smtp-Source: ABdhPJwTQltR//Qoy7CJjbYFWf5SE8su8bBzApvuOmTDy/uiiaL5a6kEWZGPL3gOs+ATNW8dGkRO2Pf0imsPrOKn0vM= X-Received: by 2002:ac2:51a1:: with SMTP id f1mr53585665lfk.173.1594652906936; Mon, 13 Jul 2020 08:08:26 -0700 (PDT) MIME-Version: 1.0 References: <20200713122247.10985-1-refactormyself@gmail.com> In-Reply-To: <20200713122247.10985-1-refactormyself@gmail.com> From: Arnd Bergmann Date: Mon, 13 Jul 2020 17:08:10 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86 To: "Saheed O. Bolarinwa" Content-Type: text/plain; charset="UTF-8" X-Provags-ID: V03:K1:NarOHKs64zhyCkyzN8W5lgAX1lhX3WRKQ2GDC1doYr5FkeR9DCa Bi8SbTX5C4OX8uINYaWOIUe4BwvsJkXKAyRx3MH0TmZfLnSUYsnNx4zFryhjyuQvL+W1C6+ /O5QQekoMypRr3uMNRZAZOQI1YxSLtgYFrJJn939KrroUCo/oVH97IxzjAiJ9rlxaCAH3T4 L60d+BxWj9CG5VERenzMA== X-UI-Out-Filterresults: notjunk:1;V03:K0:KQg/58nuSO8=:eJ9fRA8uFDi4ozZEfqHug5 tV6yeZ//e/VerCpXibl/dLTFHEWZgadxs5iJucTU9sNbb8CPeqcRm36K42wMOPVPfsaG3TAUP MP0sQ7egW75n3ryYLJfzo9aukI858pgPEbqOgwo4DCmczm/rNDtrbMxqb+9DauszG5ajwPOYr aUEXDJFdhr5SU/EJVBV6QeJii5uq/0Lda+UuTbAfItvrO3JgEDaS3Us1Pw8WAq0h2E3ktJcas bJ7p343dFyeSSE8k93n4/merF0Ph7TO2xQ/psLRSaIqWjNw+HBAS5MlycX2wjSntGteT97Y5c IA4dN9dJBB2lbi9PPM2y2aZNDJIHHvXfhxtuuGF0Wbi+V3sEWiXfRwOcBsh20USUG1EBXEltP sl5ZeTqi67wOwWBqJvNr7xSiOcO0VkAGEEnqjCrbdBn0sVcIss2nEjusxIeVuspb/pqmtHsxx PWfHbQgj6q4xonr7DTm1Wu6xiXJp60o+b/EYIxnAvNfkeKEyCatmAMzAcjZwhBsv3YHnql5IG uzpZv1L5lfohpiG4dyYjPs3Eyy7q6h9D431mP4Ih/jvNWJ8Fy/m6EU5q4XoQcsg0drZCpr1WS QovyAc9oGHxFMmpaIeXhIJDu6oUCOrQaYwJFOENQDovXjXkNjavcAycHHU32BMznfEz+3NxYA j3/ShyOYOGSYhFOqTIheZQKLmw0X6xgCUCKyIIFqnUDmFQ+JI/WyfgdL6tBQLpfESQE32s7+d a6aQT69lFoyeswUvXto+KZnWTaU5/l0XYufcsxPTgs4LnVJMQu2qKR4PS8MbNyG3Vbx9wszaC a2+2xDVyB4vF6IZ+zyvjuf1Cq8dr/YXiEraM9Hy/u6PTme8JYnnalwXLf2sowuegshPj3RRol biKyPIhJVwJmRH6588MttJy7krBeY3U3KTeV9u4ZE0h3ob9KND6K2a7XOObUogBSe64alUHV2 AHgfMr/pH6GEmCFD+4rRLrFSnspEcui+N45whkvtPMA8PM+gZEVtY X-Mailman-Approved-At: Tue, 14 Jul 2020 01:32:15 +1000 X-BeenThere: linuxppc-dev@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Rich Felker , "Martin K. Petersen" , Linux-sh list , linux-pci , linux-nvme@lists.infradead.org, Yicong Yang , sparclinux , Realtek linux nic maintainers , Paul Mackerras , Linux I2C , bcm-kernel-feedback-list , Bjorn Helgaas , rfi@lists.rocketboards.org, Toan Le , Greg Ungerer , Marek Vasut , Rob Herring , Stefano Stabellini , Sagi Grimberg , Yoshinori Sato , linux-scsi , Greg Kroah-Hartman , linux-atm-general@lists.sourceforge.net, Russell King , Ley Foon Tan , Christoph Hellwig , Geert Uytterhoeven , =?UTF-8?B?UmFmYcWCIE1pxYJlY2tp?= , Chas Williams <3chas3@gmail.com>, xen-devel , Matt Turner , "open list:BROADCOM NVRAM DRIVER" , linux-kernel-mentees@lists.linuxfoundation.org, Kevin Hilman , Guenter Roeck , linux-hwmon@vger.kernel.org, Jean Delvare , Andrew Donnellan , Ray Jui , "James E.J. Bottomley" , Linux-Renesas , Yue Wang , Jens Axboe , Jakub Kicinski , linux-m68k , Lorenzo Pieralisi , Ivan Kokshaysky , Michael Buesch , Shuah Khan , bjorn@helgaas.com, "open list:ARM/Amlogic Meson SoC support" , Boris Ostrovsky , Guan Xuetao , Linux ARM , Richard Henderson , Juergen Gross , Michal Simek , Thomas Bogendoerfer , Scott Branden , Bjorn Helgaas , Jingoo Han , Networking , Yoshihiro Shimoda , linux-wireless , "linux-kernel@vger.kernel.org" , Keith Busch , Brian King , Philipp Zabel , alpha , Frederic Barrat , Gustavo Pimentel , linuxppc-dev , "David S. Miller" , Heiner Kallweit Errors-To: linuxppc-dev-bounces+linuxppc-dev=archiver.kernel.org@lists.ozlabs.org Sender: "Linuxppc-dev" On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa wrote: > This goal of these series is to move the definition of *all* PCIBIOS* from > include/linux/pci.h to arch/x86 and limit their use within there. > All other tree specific definition will be left for intact. Maybe they can > be renamed. > > PCIBIOS* is an x86 concept as defined by the PCI spec. The returned error > codes of PCIBIOS* are positive values and this introduces some complexities > which other archs need not incur. I think the intention is good, but I find the series in its current form very hard to review, in particular the way you touch some functions three times with trivial changes. Instead of 1) replace PCIBIOS_SUCCESSFUL with 0 2) drop pointless 0-comparison 3) reformat whitespace I would suggest to combine the first two steps into one patch per subsystem and drop the third step. > PLAN: > > 1. [PATCH v0 1-36] Replace all PCIBIOS_SUCCESSFUL with 0 > > 2a. Audit all functions returning PCIBIOS_* error values directly or > indirectly and prevent possible bug coming in (2b) > > 2b. Make all functions returning PCIBIOS_* error values call > pcibios_err_to_errno(). *This will change their behaviour, for good.* > > 3. Clone a pcibios_err_to_errno() into arch/x86/pci/pcbios.c as _v2. > This handles the positive error codes directly and will not use any > PCIBIOS* definitions. So calls to it have no outside dependence. > > 4. Make all x86 codes that needs to convert to -E* values call the > cloned version - pcibios_err_to_errno_v2() > > 5. Assign PCIBIOS_* errors values directly to generic -E* errors > > 6. Refactor pcibios_err_to_errno() and mark it deprecated > > 7. Replace all calls to pcibios_err_to_errno() with the proper -E* value > or 0. > > 8. Remove all PCIBIOS* definitions in include/linux/pci.h and > pcibios_err_to_errno() too. > > 9. Redefine all PCIBIOS* definitions with original values inside > arch/x86/pci/pcbios.c > > 10. Redefine pcibios_err_to_errno() inside arch/x86/pci/pcbios.c > > 11. Replace pcibios_err_to_errno_v2() calls with pcibios_err_to_errno() > > 12. Remove pcibios_err_to_errno_v2() > > Suggested-by: Bjorn Helgaas > Suggested-by: Yicong Yang > Signed-off-by: "Saheed O. Bolarinwa" I would hope that there is a simpler procedure to get to good code than 12 steps that rename the same things multiple times. Maybe the work can be split up differently, with a similar end result but fewer and easier reviewed patches. The way I'd look at the problem, there are three main areas that can be dealt with one at a time: a) callers of the high-level config space accessors pci_{write,read}_config_{byte,word,dword}, mostly in device drivers. b) low-level implementation of the config space accessors through struct pci_ops c) all other occurrences of these constants Starting with a), my first question is whether any high-level drivers even need to care about errors from these functions. I see 4913 callers that ignore the return code, and 576 that actually check it, and almost none care about the specific error (as you found as well). Unless we conclude that most PCI drivers are wrong, could we just change the return type to 'void' and assume they never fail for valid arguments on a valid pci_device* ? For b), it might be nice to also change other aspects of the interface, e.g. passing a pci_host_bridge pointer plus bus number instead of a pci_bus pointer, or having the callback in the pci_host_bridge structure. > Bolarinwa Olayemi Saheed (35): > Change PCIBIOS_SUCCESSFUL to 0 > Change PCIBIOS_SUCCESSFUL to 0 > Change PCIBIOS_SUCCESSFUL to 0 > Tidy Success/Failure checks > Change PCIBIOS_SUCCESSFUL to 0 > Tidy Success/Failure checks > Change PCIBIOS_SUCCESSFUL to 0 Some patches have identical subject lines including the subsystem prefix, which you should avoid. Try to also fix the git request-pull output to not drop that prefix here so the list makes more sense. Arnd