| Message ID | 20200713122247.10985-1-refactormyself@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Move all PCIBIOS* definitions into arch/x86 | expand |
On Mon, Jul 13, 2020 at 3:22 PM Saheed O. Bolarinwa <refactormyself@gmail.com> 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 <bjorn@helgaas.com> > Suggested-by: Yicong Yang <yangyicong@hisilicon.com> > Signed-off-by: "Saheed O. Bolarinwa" <refactormyself@gmail.com> 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
On Mon, Jul 13, 2020 at 02:22:12PM +0200, 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. More comments later, but a few trivial whitespace issues you can clean up in the meantime. Don't repost for at least a few days to avoid spamming everybody. I found these with: $ b4 am -om/ 20200713122247.10985-1-refactormyself@gmail.com $ git am m/20200713_refactormyself_move_all_pcibios_definitions_into_arch_x86.mbx Applying: atm: Change PCIBIOS_SUCCESSFUL to 0 .git/rebase-apply/patch:11: trailing whitespace. iadev = INPH_IA_DEV(dev); .git/rebase-apply/patch:12: trailing whitespace. for(i=0; i<64; i++) .git/rebase-apply/patch:13: trailing whitespace. if ((error = pci_read_config_dword(iadev->pci, .git/rebase-apply/patch:16: trailing whitespace, space before tab in indent. return error; .git/rebase-apply/patch:17: trailing whitespace. writel(0, iadev->reg+IPHASE5575_EXT_RESET); warning: squelched 5 whitespace errors warning: 10 lines add whitespace errors. Applying: atm: Tidy Success/Failure checks .git/rebase-apply/patch:13: trailing whitespace. .git/rebase-apply/patch:14: trailing whitespace. iadev = INPH_IA_DEV(dev); .git/rebase-apply/patch:15: trailing whitespace. for(i=0; i<64; i++) .git/rebase-apply/patch:21: trailing whitespace. writel(0, iadev->reg+IPHASE5575_EXT_RESET); .git/rebase-apply/patch:22: trailing whitespace. for(i=0; i<64; i++) warning: squelched 3 whitespace errors warning: 8 lines add whitespace errors. Applying: atm: Fix Style ERROR- assignment in if condition .git/rebase-apply/patch:12: trailing whitespace. unsigned int pci[64]; .git/rebase-apply/patch:13: trailing whitespace. .git/rebase-apply/patch:14: trailing whitespace. iadev = INPH_IA_DEV(dev); .git/rebase-apply/patch:23: trailing whitespace. writel(0, iadev->reg+IPHASE5575_EXT_RESET); .git/rebase-apply/patch:32: trailing whitespace. udelay(5); warning: squelched 2 whitespace errors warning: 7 lines add whitespace errors. Applying: PCI: Change PCIBIOS_SUCCESSFUL to 0 .git/rebase-apply/patch:37: trailing whitespace. struct pci_ops apecs_pci_ops = .git/rebase-apply/patch:50: trailing whitespace. static int .git/rebase-apply/patch:59: trailing whitespace. struct pci_ops cia_pci_ops = .git/rebase-apply/patch:94: trailing whitespace. static int .git/rebase-apply/patch:103: trailing whitespace. struct pci_ops lca_pci_ops = warning: squelched 10 whitespace errors warning: 15 lines add whitespace errors.
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. 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 <bjorn@helgaas.com> Suggested-by: Yicong Yang <yangyicong@hisilicon.com> Signed-off-by: "Saheed O. Bolarinwa" <refactormyself@gmail.com> 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 Tidy Success/Failure checks Change PCIBIOS_SUCCESSFUL to 0 Tidy Success/Failure checks Change PCIBIOS_SUCCESSFUL to 0 Tidy Success/Failure checks 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 Change PCIBIOS_SUCCESSFUL to 0 Tidy Success/Failure checks Fix Style ERROR: assignment in if condition 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 Tidy Success/Failure checks 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 arch/alpha/kernel/core_apecs.c | 4 +-- arch/alpha/kernel/core_cia.c | 4 +-- arch/alpha/kernel/core_irongate.c | 4 +-- arch/alpha/kernel/core_lca.c | 4 +-- arch/alpha/kernel/core_marvel.c | 4 +-- arch/alpha/kernel/core_mcpcia.c | 4 +-- arch/alpha/kernel/core_polaris.c | 4 +-- arch/alpha/kernel/core_t2.c | 4 +-- arch/alpha/kernel/core_titan.c | 4 +-- arch/alpha/kernel/core_tsunami.c | 4 +-- arch/alpha/kernel/core_wildfire.c | 4 +-- arch/alpha/kernel/sys_miata.c | 2 +- arch/arm/common/it8152.c | 4 +-- arch/arm/mach-cns3xxx/pcie.c | 2 +- arch/arm/mach-footbridge/dc21285.c | 4 +-- arch/arm/mach-iop32x/pci.c | 6 ++-- arch/arm/mach-ixp4xx/common-pci.c | 8 ++--- arch/arm/mach-orion5x/pci.c | 4 +-- arch/arm/plat-orion/pcie.c | 8 ++--- arch/m68k/coldfire/pci.c | 8 ++--- arch/microblaze/pci/indirect_pci.c | 4 +-- arch/mips/pci/fixup-ath79.c | 2 +- arch/mips/pci/ops-bcm63xx.c | 14 ++++---- arch/mips/pci/ops-bonito64.c | 4 +-- arch/mips/pci/ops-gt64xxx_pci0.c | 4 +-- arch/mips/pci/ops-lantiq.c | 4 +-- arch/mips/pci/ops-loongson2.c | 4 +-- arch/mips/pci/ops-mace.c | 4 +-- arch/mips/pci/ops-msc.c | 4 +-- arch/mips/pci/ops-rc32434.c | 6 ++-- arch/mips/pci/ops-sni.c | 4 +-- arch/mips/pci/ops-tx3927.c | 2 +- arch/mips/pci/ops-tx4927.c | 2 +- arch/mips/pci/ops-vr41xx.c | 4 +-- arch/mips/pci/pci-alchemy.c | 6 ++-- arch/mips/pci/pci-ar2315.c | 5 ++- arch/mips/pci/pci-ar71xx.c | 4 +-- arch/mips/pci/pci-ar724x.c | 6 ++-- arch/mips/pci/pci-bcm1480.c | 4 +-- arch/mips/pci/pci-bcm1480ht.c | 4 +-- arch/mips/pci/pci-mt7620.c | 4 +-- arch/mips/pci/pci-octeon.c | 12 +++---- arch/mips/pci/pci-rt2880.c | 4 +-- arch/mips/pci/pci-rt3883.c | 4 +-- arch/mips/pci/pci-sb1250.c | 4 +-- arch/mips/pci/pci-virtio-guest.c | 4 +-- arch/mips/pci/pci-xlp.c | 4 +-- arch/mips/pci/pci-xlr.c | 4 +-- arch/mips/pci/pci-xtalk-bridge.c | 14 ++++---- arch/mips/pci/pcie-octeon.c | 4 +-- arch/mips/txx9/generic/pci.c | 5 ++- arch/powerpc/kernel/rtas_pci.c | 4 +-- arch/powerpc/platforms/4xx/pci.c | 4 +-- arch/powerpc/platforms/52xx/efika.c | 4 +-- arch/powerpc/platforms/52xx/mpc52xx_pci.c | 4 +-- arch/powerpc/platforms/82xx/pq2.c | 2 +- arch/powerpc/platforms/85xx/mpc85xx_cds.c | 2 +- arch/powerpc/platforms/85xx/mpc85xx_ds.c | 2 +- arch/powerpc/platforms/86xx/mpc86xx_hpcn.c | 2 +- arch/powerpc/platforms/chrp/pci.c | 8 ++--- arch/powerpc/platforms/embedded6xx/holly.c | 2 +- .../platforms/embedded6xx/mpc7448_hpc2.c | 2 +- arch/powerpc/platforms/fsl_uli1575.c | 2 +- arch/powerpc/platforms/maple/pci.c | 18 +++++----- arch/powerpc/platforms/pasemi/pci.c | 6 ++-- arch/powerpc/platforms/powermac/pci.c | 8 ++--- arch/powerpc/platforms/powernv/eeh-powernv.c | 4 +-- arch/powerpc/platforms/powernv/pci.c | 4 +-- arch/powerpc/platforms/pseries/eeh_pseries.c | 4 +-- arch/powerpc/sysdev/fsl_pci.c | 2 +- arch/powerpc/sysdev/indirect_pci.c | 4 +-- arch/powerpc/sysdev/tsi108_pci.c | 4 +-- arch/sh/drivers/pci/common.c | 3 +- arch/sh/drivers/pci/ops-dreamcast.c | 4 +-- arch/sh/drivers/pci/ops-sh4.c | 4 +-- arch/sh/drivers/pci/ops-sh7786.c | 8 ++--- arch/sh/drivers/pci/pci.c | 2 +- arch/sparc/kernel/pci_common.c | 28 +++++++-------- arch/unicore32/kernel/pci.c | 4 +-- drivers/atm/iphase.c | 20 ++++++----- drivers/atm/lanai.c | 8 ++--- drivers/bcma/driver_pci_host.c | 4 +-- drivers/hwmon/sis5595.c | 13 +++---- drivers/hwmon/via686a.c | 13 +++---- drivers/hwmon/vt8231.c | 13 +++---- drivers/i2c/busses/i2c-ali15x3.c | 5 ++- drivers/i2c/busses/i2c-nforce2.c | 3 +- drivers/i2c/busses/i2c-sis5595.c | 15 +++----- drivers/misc/cxl/vphb.c | 4 +-- drivers/net/ethernet/realtek/r8169_main.c | 2 +- drivers/nvme/host/pci.c | 2 +- drivers/pci/access.c | 14 ++++---- drivers/pci/controller/dwc/pci-meson.c | 4 +-- .../pci/controller/dwc/pcie-designware-host.c | 2 +- drivers/pci/controller/dwc/pcie-designware.c | 4 +-- drivers/pci/controller/dwc/pcie-hisi.c | 4 +-- drivers/pci/controller/dwc/pcie-tegra194.c | 4 +-- .../pci/controller/mobiveil/pcie-mobiveil.c | 4 +-- drivers/pci/controller/pci-aardvark.c | 4 +-- drivers/pci/controller/pci-ftpci100.c | 4 +-- drivers/pci/controller/pci-hyperv.c | 8 ++--- drivers/pci/controller/pci-mvebu.c | 4 +-- drivers/pci/controller/pci-thunder-ecam.c | 36 +++++++++---------- drivers/pci/controller/pci-thunder-pem.c | 4 +-- drivers/pci/controller/pci-xgene.c | 5 ++- drivers/pci/controller/pcie-altera.c | 16 ++++----- drivers/pci/controller/pcie-iproc.c | 10 +++--- drivers/pci/controller/pcie-mediatek.c | 4 +-- drivers/pci/controller/pcie-rcar-host.c | 8 ++--- drivers/pci/controller/pcie-rockchip-host.c | 10 +++--- drivers/pci/pci-bridge-emul.c | 14 ++++---- drivers/pci/pci.c | 8 ++--- drivers/pci/pcie/bw_notification.c | 4 +-- drivers/pci/probe.c | 4 +-- drivers/pci/quirks.c | 4 +-- drivers/pci/syscall.c | 8 ++--- drivers/pci/xen-pcifront.c | 2 +- drivers/scsi/ipr.c | 16 ++++----- drivers/scsi/pmcraid.c | 6 ++-- drivers/ssb/driver_gige.c | 4 +-- drivers/ssb/driver_pcicore.c | 4 +-- drivers/xen/xen-pciback/conf_space.c | 2 +- 122 files changed, 347 insertions(+), 369 deletions(-)