* [RFC PATCH 34/35] PCI: Change PCIBIOS_SUCCESSFUL to 0
2020-07-13 12:22 [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86 Saheed O. Bolarinwa
@ 2020-07-13 12:22 ` Saheed O. Bolarinwa
2020-07-13 12:22 ` [RFC PATCH 35/35] alpha: Tidy Success/Failure checks Saheed O. Bolarinwa
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Saheed O. Bolarinwa @ 2020-07-13 12:22 UTC (permalink / raw)
To: helgaas, Richard Henderson, Ivan Kokshaysky, Matt Turner
Cc: Saheed O. Bolarinwa, bjorn, skhan, linux-pci,
linux-kernel-mentees, linux-alpha, linux-kernel
In reference to the PCI spec (Chapter 2), PCIBIOS* is an x86 concept.
Their scope should be limited within arch/x86.
Change all PCIBIOS_SUCCESSFUL to 0
Signed-off-by: "Saheed O. Bolarinwa" <refactormyself@gmail.com>
---
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 +-
12 files changed, 23 insertions(+), 23 deletions(-)
diff --git a/arch/alpha/kernel/core_apecs.c b/arch/alpha/kernel/core_apecs.c
index 6df765ff2b10..d74d78d92434 100644
--- a/arch/alpha/kernel/core_apecs.c
+++ b/arch/alpha/kernel/core_apecs.c
@@ -287,7 +287,7 @@ apecs_read_config(struct pci_bus *bus, unsigned int devfn, int where,
shift = (where & 3) * 8;
addr = (pci_addr << 5) + mask + APECS_CONF;
*value = conf_read(addr, type1) >> (shift);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -304,7 +304,7 @@ apecs_write_config(struct pci_bus *bus, unsigned int devfn, int where,
mask = (size - 1) * 8;
addr = (pci_addr << 5) + mask + APECS_CONF;
conf_write(addr, value << ((where & 3) * 8), type1);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops apecs_pci_ops =
diff --git a/arch/alpha/kernel/core_cia.c b/arch/alpha/kernel/core_cia.c
index f489170201c3..25300bc19c48 100644
--- a/arch/alpha/kernel/core_cia.c
+++ b/arch/alpha/kernel/core_cia.c
@@ -221,7 +221,7 @@ cia_read_config(struct pci_bus *bus, unsigned int devfn, int where, int size,
shift = (where & 3) * 8;
addr = (pci_addr << 5) + mask + CIA_CONF;
*value = conf_read(addr, type1) >> (shift);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -238,7 +238,7 @@ cia_write_config(struct pci_bus *bus, unsigned int devfn, int where, int size,
mask = (size - 1) * 8;
addr = (pci_addr << 5) + mask + CIA_CONF;
conf_write(addr, value << ((where & 3) * 8), type1);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops cia_pci_ops =
diff --git a/arch/alpha/kernel/core_irongate.c b/arch/alpha/kernel/core_irongate.c
index a9fd133a7fb2..858a2293c786 100644
--- a/arch/alpha/kernel/core_irongate.c
+++ b/arch/alpha/kernel/core_irongate.c
@@ -121,7 +121,7 @@ irongate_read_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -152,7 +152,7 @@ irongate_write_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops irongate_pci_ops =
diff --git a/arch/alpha/kernel/core_lca.c b/arch/alpha/kernel/core_lca.c
index 57e0750419f2..a7a00d73e2c5 100644
--- a/arch/alpha/kernel/core_lca.c
+++ b/arch/alpha/kernel/core_lca.c
@@ -213,7 +213,7 @@ lca_read_config(struct pci_bus *bus, unsigned int devfn, int where,
mask = (size - 1) * 8;
addr = (pci_addr << 5) + mask + LCA_CONF;
*value = conf_read(addr) >> (shift);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -229,7 +229,7 @@ lca_write_config(struct pci_bus *bus, unsigned int devfn, int where, int size,
mask = (size - 1) * 8;
addr = (pci_addr << 5) + mask + LCA_CONF;
conf_write(addr, value << ((where & 3) * 8));
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops lca_pci_ops =
diff --git a/arch/alpha/kernel/core_marvel.c b/arch/alpha/kernel/core_marvel.c
index 1db9d0eb2922..c076b97a9961 100644
--- a/arch/alpha/kernel/core_marvel.c
+++ b/arch/alpha/kernel/core_marvel.c
@@ -561,7 +561,7 @@ marvel_read_config(struct pci_bus *bus, unsigned int devfn, int where,
return PCIBIOS_FUNC_NOT_SUPPORTED;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -593,7 +593,7 @@ marvel_write_config(struct pci_bus *bus, unsigned int devfn, int where,
return PCIBIOS_FUNC_NOT_SUPPORTED;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops marvel_pci_ops =
diff --git a/arch/alpha/kernel/core_mcpcia.c b/arch/alpha/kernel/core_mcpcia.c
index 74b1d018124c..fdb6d055bcc0 100644
--- a/arch/alpha/kernel/core_mcpcia.c
+++ b/arch/alpha/kernel/core_mcpcia.c
@@ -216,7 +216,7 @@ mcpcia_read_config(struct pci_bus *bus, unsigned int devfn, int where,
*value = w;
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -233,7 +233,7 @@ mcpcia_write_config(struct pci_bus *bus, unsigned int devfn, int where,
addr |= (size - 1) * 8;
value = __kernel_insql(value, where & 3);
conf_write(addr, value, type1, hose);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops mcpcia_pci_ops =
diff --git a/arch/alpha/kernel/core_polaris.c b/arch/alpha/kernel/core_polaris.c
index 75d622d96ff2..345b9d5a116f 100644
--- a/arch/alpha/kernel/core_polaris.c
+++ b/arch/alpha/kernel/core_polaris.c
@@ -102,7 +102,7 @@ polaris_read_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
@@ -134,7 +134,7 @@ polaris_write_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops polaris_pci_ops =
diff --git a/arch/alpha/kernel/core_t2.c b/arch/alpha/kernel/core_t2.c
index 98d5b6ff8a76..0bbf9b028c11 100644
--- a/arch/alpha/kernel/core_t2.c
+++ b/arch/alpha/kernel/core_t2.c
@@ -296,7 +296,7 @@ t2_read_config(struct pci_bus *bus, unsigned int devfn, int where,
shift = (where & 3) * 8;
addr = (pci_addr << 5) + mask + T2_CONF;
*value = conf_read(addr, type1) >> (shift);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -313,7 +313,7 @@ t2_write_config(struct pci_bus *bus, unsigned int devfn, int where, int size,
mask = (size - 1) * 8;
addr = (pci_addr << 5) + mask + T2_CONF;
conf_write(addr, value << ((where & 3) * 8), type1);
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops t2_pci_ops =
diff --git a/arch/alpha/kernel/core_titan.c b/arch/alpha/kernel/core_titan.c
index 2a2820fb1be6..aac94708a226 100644
--- a/arch/alpha/kernel/core_titan.c
+++ b/arch/alpha/kernel/core_titan.c
@@ -158,7 +158,7 @@ titan_read_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -189,7 +189,7 @@ titan_write_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops titan_pci_ops =
diff --git a/arch/alpha/kernel/core_tsunami.c b/arch/alpha/kernel/core_tsunami.c
index fc1ab73f23de..88fe80a8b41a 100644
--- a/arch/alpha/kernel/core_tsunami.c
+++ b/arch/alpha/kernel/core_tsunami.c
@@ -134,7 +134,7 @@ tsunami_read_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -165,7 +165,7 @@ tsunami_write_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops tsunami_pci_ops =
diff --git a/arch/alpha/kernel/core_wildfire.c b/arch/alpha/kernel/core_wildfire.c
index e8d3b033018d..012ec2f5b675 100644
--- a/arch/alpha/kernel/core_wildfire.c
+++ b/arch/alpha/kernel/core_wildfire.c
@@ -400,7 +400,7 @@ wildfire_read_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
static int
@@ -431,7 +431,7 @@ wildfire_write_config(struct pci_bus *bus, unsigned int devfn, int where,
break;
}
- return PCIBIOS_SUCCESSFUL;
+ return 0;
}
struct pci_ops wildfire_pci_ops =
diff --git a/arch/alpha/kernel/sys_miata.c b/arch/alpha/kernel/sys_miata.c
index e1bee8f84c58..1b4c03ac34d8 100644
--- a/arch/alpha/kernel/sys_miata.c
+++ b/arch/alpha/kernel/sys_miata.c
@@ -185,7 +185,7 @@ miata_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
if((slot == 7) && (PCI_FUNC(dev->devfn) == 3)) {
u8 irq=0;
struct pci_dev *pdev = pci_get_slot(dev->bus, dev->devfn & ~7);
- if(pdev == NULL || pci_read_config_byte(pdev, 0x40,&irq) != PCIBIOS_SUCCESSFUL) {
+ if (pdev == NULL || pci_read_config_byte(pdev, 0x40, &irq) != 0) {
pci_dev_put(pdev);
return -1;
}
--
2.18.2
^ permalink raw reply related [flat|nested] 5+ messages in thread* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
2020-07-13 12:22 [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86 Saheed O. Bolarinwa
2020-07-13 12:22 ` [RFC PATCH 34/35] PCI: Change PCIBIOS_SUCCESSFUL to 0 Saheed O. Bolarinwa
2020-07-13 12:22 ` [RFC PATCH 35/35] alpha: Tidy Success/Failure checks Saheed O. Bolarinwa
@ 2020-07-13 15:08 ` Arnd Bergmann
2020-07-13 22:01 ` Bjorn Helgaas
3 siblings, 0 replies; 5+ messages in thread
From: Arnd Bergmann @ 2020-07-13 15:08 UTC (permalink / raw)
To: Saheed O. Bolarinwa
Cc: Rich Felker, Martin K. Petersen, Linux-sh list, linux-pci,
linux-nvme, Yicong Yang, sparclinux,
Realtek linux nic maintainers, Paul Mackerras, Linux I2C,
bcm-kernel-feedback-list, Bjorn Helgaas, rfi, Toan Le,
Greg Ungerer, Marek Vasut, Rob Herring, Stefano Stabellini,
Sagi Grimberg, Yoshinori Sato, linux-scsi, Greg Kroah-Hartman
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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86
2020-07-13 12:22 [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86 Saheed O. Bolarinwa
` (2 preceding siblings ...)
2020-07-13 15:08 ` [RFC PATCH 00/35] Move all PCIBIOS* definitions into arch/x86 Arnd Bergmann
@ 2020-07-13 22:01 ` Bjorn Helgaas
3 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2020-07-13 22:01 UTC (permalink / raw)
To: Saheed O. Bolarinwa
Cc: Rich Felker, Martin K. Petersen, linux-sh, linux-pci, linux-nvme,
Yicong Yang, Keith Busch, Realtek linux nic maintainers,
Paul Mackerras, linux-i2c, bcm-kernel-feedback-list, sparclinux,
rfi, Toan Le, Greg Ungerer, Marek Vasut, Rob Herring,
Stefano Stabellini, Sagi Grimberg, Yoshinori Sato, linux-scsi,
Greg Kroah-Hartman, Michael Ellerman, linux-atm-general
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.
^ permalink raw reply [flat|nested] 5+ messages in thread