[PATCH v2 0/8] PCI/treewide: Cleanup/streamline PCI error code handling

[PATCH v2 0/8] PCI/treewide: Cleanup/streamline PCI error code handling

[Ilpo Järvinen]

As the first step towards converting PCI accessor function return codes
into normal errnos this series cleans up related code paths which have
complicated multi-line construct to handle the PCI error checking.

v2:
- Moved ret local var to the inner block (I2C: ali15x3)
- Removed already accepted patches

Ilpo Järvinen (8):
  alpha: Streamline convoluted PCI error handling
  MIPS: TXx9: Do PCI error checks on own line
  sh: pci: Do PCI error check on own line
  atm: iphase: Do PCI error checks on own line
  I2C: ali15x3: Do PCI error checks on own line
  PCI: Do error check on own line to split long if conditions
  PCI: xgene: Do PCI error check on own line
  scsi: ipr: Do PCI error checks on own line

 arch/alpha/kernel/sys_miata.c      | 17 ++++++------
 arch/mips/txx9/generic/pci.c       | 43 ++++++++++++++++--------------
 arch/sh/drivers/pci/common.c       |  7 ++---
 drivers/atm/iphase.c               | 20 +++++++-------
 drivers/i2c/busses/i2c-ali15x3.c   | 11 ++++----
 drivers/pci/controller/pci-xgene.c |  5 ++--
 drivers/pci/pci.c                  |  9 ++++---
 drivers/pci/probe.c                |  6 ++---
 drivers/pci/quirks.c               |  6 ++---
 drivers/scsi/ipr.c                 | 12 ++++++---
 10 files changed, 76 insertions(+), 60 deletions(-)

-- 
2.30.2

[PATCH v2 1/8] alpha: Streamline convoluted PCI error handling

[Ilpo Järvinen]

miata_map_irq() handles PCI device and read config related errors in a
conditional block that is more complex than necessary.

Streamline the code flow and error handling.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/alpha/kernel/sys_miata.c | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/alpha/kernel/sys_miata.c b/arch/alpha/kernel/sys_miata.c
index e1bee8f84c58..33b2798de8fc 100644
--- a/arch/alpha/kernel/sys_miata.c
+++ b/arch/alpha/kernel/sys_miata.c
@@ -183,16 +183,17 @@ miata_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
            the 2nd 8259 controller. So we have to check for it first. */
 
 	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) {
-			pci_dev_put(pdev);
+		u8 irq = 0;
+		int ret;
+
+		if (!pdev)
 			return -1;
-		}
-		else	{
-			pci_dev_put(pdev);
-			return irq;
-		}
+
+		ret = pci_read_config_byte(pdev, 0x40, &irq);
+		pci_dev_put(pdev);
+
+		return ret == PCIBIOS_SUCCESSFUL ? irq : -1;
 	}
 
 	return COMMON_TABLE_LOOKUP;
-- 
2.30.2

[PATCH v2 2/8] MIPS: TXx9: Do PCI error checks on own line

[Ilpo Järvinen]

Instead of if conditions with line splits, use the usual error handling
pattern with a separate variable to improve readability.

The second check can use reverse logic which reduces indentation level.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
---
 arch/mips/txx9/generic/pci.c | 43 +++++++++++++++++++-----------------
 1 file changed, 23 insertions(+), 20 deletions(-)

diff --git a/arch/mips/txx9/generic/pci.c b/arch/mips/txx9/generic/pci.c
index e98845543b77..5ae30b78d38d 100644
--- a/arch/mips/txx9/generic/pci.c
+++ b/arch/mips/txx9/generic/pci.c
@@ -51,6 +51,7 @@ int __init txx9_pci66_check(struct pci_controller *hose, int top_bus,
 	unsigned short vid;
 	int cap66 = -1;
 	u16 stat;
+	int ret;
 
 	/* It seems SLC90E66 needs some time after PCI reset... */
 	mdelay(80);
@@ -60,9 +61,9 @@ int __init txx9_pci66_check(struct pci_controller *hose, int top_bus,
 	for (pci_devfn = 0; pci_devfn < 0xff; pci_devfn++) {
 		if (PCI_FUNC(pci_devfn))
 			continue;
-		if (early_read_config_word(hose, top_bus, current_bus,
-					   pci_devfn, PCI_VENDOR_ID, &vid) !=
-		    PCIBIOS_SUCCESSFUL)
+		ret = early_read_config_word(hose, top_bus, current_bus,
+					     pci_devfn, PCI_VENDOR_ID, &vid);
+		if (ret != PCIBIOS_SUCCESSFUL)
 			continue;
 		if (vid == 0xffff)
 			continue;
@@ -343,26 +344,28 @@ static void tc35815_fixup(struct pci_dev *dev)
 
 static void final_fixup(struct pci_dev *dev)
 {
+	unsigned long timeout;
 	unsigned char bist;
+	int ret;
 
 	/* Do build-in self test */
-	if (pci_read_config_byte(dev, PCI_BIST, &bist) == PCIBIOS_SUCCESSFUL &&
-	    (bist & PCI_BIST_CAPABLE)) {
-		unsigned long timeout;
-		pci_set_power_state(dev, PCI_D0);
-		pr_info("PCI: %s BIST...", pci_name(dev));
-		pci_write_config_byte(dev, PCI_BIST, PCI_BIST_START);
-		timeout = jiffies + HZ * 2;	/* timeout after 2 sec */
-		do {
-			pci_read_config_byte(dev, PCI_BIST, &bist);
-			if (time_after(jiffies, timeout))
-				break;
-		} while (bist & PCI_BIST_START);
-		if (bist & (PCI_BIST_CODE_MASK | PCI_BIST_START))
-			pr_cont("failed. (0x%x)\n", bist);
-		else
-			pr_cont("OK.\n");
-	}
+	ret = pci_read_config_byte(dev, PCI_BIST, &bist);
+	if ((ret != PCIBIOS_SUCCESSFUL) || !(bist & PCI_BIST_CAPABLE))
+		return;
+
+	pci_set_power_state(dev, PCI_D0);
+	pr_info("PCI: %s BIST...", pci_name(dev));
+	pci_write_config_byte(dev, PCI_BIST, PCI_BIST_START);
+	timeout = jiffies + HZ * 2;	/* timeout after 2 sec */
+	do {
+		pci_read_config_byte(dev, PCI_BIST, &bist);
+		if (time_after(jiffies, timeout))
+			break;
+	} while (bist & PCI_BIST_START);
+	if (bist & (PCI_BIST_CODE_MASK | PCI_BIST_START))
+		pr_cont("failed. (0x%x)\n", bist);
+	else
+		pr_cont("OK.\n");
 }
 
 #ifdef CONFIG_TOSHIBA_FPCIB0
-- 
2.30.2

Re: [PATCH v2 2/8] MIPS: TXx9: Do PCI error checks on own line

[Thomas Bogendoerfer]

On Sun, Aug 27, 2023 at 04:36:59PM +0300, Ilpo Järvinen wrote:
> Instead of if conditions with line splits, use the usual error handling
> pattern with a separate variable to improve readability.
> 
> The second check can use reverse logic which reduces indentation level.
> 
> No functional changes intended.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> ---
>  arch/mips/txx9/generic/pci.c | 43 +++++++++++++++++++-----------------
>  1 file changed, 23 insertions(+), 20 deletions(-)

applied to mips-next.

Thomas.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessarily a
good idea.                                                [ RFC1925, 2.3 ]

[PATCH v2 3/8] sh: pci: Do PCI error check on own line

[Ilpo Järvinen]

Instead of a if condition with a line split, use the usual error
handling pattern with a separate variable to improve readability.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Acked-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
---
 arch/sh/drivers/pci/common.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/arch/sh/drivers/pci/common.c b/arch/sh/drivers/pci/common.c
index 2fd2b77e12ce..f59e5b9a6a80 100644
--- a/arch/sh/drivers/pci/common.c
+++ b/arch/sh/drivers/pci/common.c
@@ -53,15 +53,16 @@ int __init pci_is_66mhz_capable(struct pci_channel *hose,
 	unsigned short vid;
 	int cap66 = -1;
 	u16 stat;
+	int ret;
 
 	pr_info("PCI: Checking 66MHz capabilities...\n");
 
 	for (pci_devfn = 0; pci_devfn < 0xff; pci_devfn++) {
 		if (PCI_FUNC(pci_devfn))
 			continue;
-		if (early_read_config_word(hose, top_bus, current_bus,
-					   pci_devfn, PCI_VENDOR_ID, &vid) !=
-		    PCIBIOS_SUCCESSFUL)
+		ret = early_read_config_word(hose, top_bus, current_bus,
+					     pci_devfn, PCI_VENDOR_ID, &vid);
+		if (ret != PCIBIOS_SUCCESSFUL)
 			continue;
 		if (vid == 0xffff)
 			continue;
-- 
2.30.2

[PATCH v2 4/8] atm: iphase: Do PCI error checks on own line

[Ilpo Järvinen]

In get_esi() PCI errors are checked inside line-split if conditions (in
addition to the file not following the coding style). To make the code
in get_esi() more readable, fix the coding style and use the usual
error handling pattern with a separate variable.

In addition, initialization of 'error' variable at declaration is not
needed.

No function changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/atm/iphase.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/atm/iphase.c b/drivers/atm/iphase.c
index 324148686953..9bba8f280a4d 100644
--- a/drivers/atm/iphase.c
+++ b/drivers/atm/iphase.c
@@ -2291,19 +2291,21 @@ static int get_esi(struct atm_dev *dev)
 static int reset_sar(struct atm_dev *dev)  
 {  
 	IADEV *iadev;  
-	int i, error = 1;  
+	int i, error;
 	unsigned int pci[64];  
 	  
 	iadev = INPH_IA_DEV(dev);  
-	for(i=0; i<64; i++)  
-	  if ((error = pci_read_config_dword(iadev->pci,  
-				i*4, &pci[i])) != PCIBIOS_SUCCESSFUL)  
-  	      return error;  
+	for (i = 0; i < 64; i++) {
+		error = pci_read_config_dword(iadev->pci, i * 4, &pci[i]);
+		if (error != PCIBIOS_SUCCESSFUL)
+			return error;
+	}
 	writel(0, iadev->reg+IPHASE5575_EXT_RESET);  
-	for(i=0; i<64; i++)  
-	  if ((error = pci_write_config_dword(iadev->pci,  
-					i*4, pci[i])) != PCIBIOS_SUCCESSFUL)  
-	    return error;  
+	for (i = 0; i < 64; i++) {
+		error = pci_write_config_dword(iadev->pci, i * 4, pci[i]);
+		if (error != PCIBIOS_SUCCESSFUL)
+			return error;
+	}
 	udelay(5);  
 	return 0;  
 }  
-- 
2.30.2

[PATCH v2 5/8] I2C: ali15x3: Do PCI error checks on own line

[Ilpo Järvinen]

Instead of if conditions with line splits, use the usual error handling
pattern with a separate variable to improve readability.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/i2c/busses/i2c-ali15x3.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-ali15x3.c b/drivers/i2c/busses/i2c-ali15x3.c
index cc58feacd082..0231c5be6354 100644
--- a/drivers/i2c/busses/i2c-ali15x3.c
+++ b/drivers/i2c/busses/i2c-ali15x3.c
@@ -165,14 +165,15 @@ static int ali15x3_setup(struct pci_dev *ALI15X3_dev)
 	}
 
 	if(force_addr) {
+		int ret;
+
 		dev_info(&ALI15X3_dev->dev, "forcing ISA address 0x%04X\n",
 			ali15x3_smba);
-		if (PCIBIOS_SUCCESSFUL != pci_write_config_word(ALI15X3_dev,
-								SMBBA,
-								ali15x3_smba))
+		ret = pci_write_config_word(ALI15X3_dev, SMBBA, ali15x3_smba);
+		if (ret != PCIBIOS_SUCCESSFUL)
 			goto error;
-		if (PCIBIOS_SUCCESSFUL != pci_read_config_word(ALI15X3_dev,
-								SMBBA, &a))
+		ret = pci_read_config_word(ALI15X3_dev, SMBBA, &a);
+		if (ret != PCIBIOS_SUCCESSFUL)
 			goto error;
 		if ((a & ~(ALI15X3_SMB_IOSIZE - 1)) != ali15x3_smba) {
 			/* make sure it works */
-- 
2.30.2

Re: [PATCH v2 5/8] I2C: ali15x3: Do PCI error checks on own line

[Andi Shyti]

Hi Ilpo,

On Sun, Aug 27, 2023 at 04:37:02PM +0300, Ilpo Järvinen wrote:
> Instead of if conditions with line splits, use the usual error handling
> pattern with a separate variable to improve readability.
> 
> No functional changes intended.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Reviewed-by: Andi Shyti <andi.shyti@kernel.org> 

Andi

> ---
>  drivers/i2c/busses/i2c-ali15x3.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ali15x3.c b/drivers/i2c/busses/i2c-ali15x3.c
> index cc58feacd082..0231c5be6354 100644
> --- a/drivers/i2c/busses/i2c-ali15x3.c
> +++ b/drivers/i2c/busses/i2c-ali15x3.c
> @@ -165,14 +165,15 @@ static int ali15x3_setup(struct pci_dev *ALI15X3_dev)
>  	}
>  
>  	if(force_addr) {
> +		int ret;
> +
>  		dev_info(&ALI15X3_dev->dev, "forcing ISA address 0x%04X\n",
>  			ali15x3_smba);
> -		if (PCIBIOS_SUCCESSFUL != pci_write_config_word(ALI15X3_dev,
> -								SMBBA,
> -								ali15x3_smba))
> +		ret = pci_write_config_word(ALI15X3_dev, SMBBA, ali15x3_smba);
> +		if (ret != PCIBIOS_SUCCESSFUL)
>  			goto error;
> -		if (PCIBIOS_SUCCESSFUL != pci_read_config_word(ALI15X3_dev,
> -								SMBBA, &a))
> +		ret = pci_read_config_word(ALI15X3_dev, SMBBA, &a);
> +		if (ret != PCIBIOS_SUCCESSFUL)
>  			goto error;
>  		if ((a & ~(ALI15X3_SMB_IOSIZE - 1)) != ali15x3_smba) {
>  			/* make sure it works */
> -- 
> 2.30.2
> 

Re: [PATCH v2 5/8] I2C: ali15x3: Do PCI error checks on own line

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 342 bytes --]

On Sun, Aug 27, 2023 at 04:37:02PM +0300, Ilpo Järvinen wrote:
> Instead of if conditions with line splits, use the usual error handling
> pattern with a separate variable to improve readability.
> 
> No functional changes intended.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 6/8] PCI: Do error check on own line to split long if conditions

[Ilpo Järvinen]

Placing PCI error code check inside if condition usually results in
need to split lines. Combined with additional conditions the if
condition becomes messy.

Convert to the usual error handling pattern with an additional variable
to improve code readability. In addition, reverse the logic in
pci_find_vsec_capability() to get rid of &&.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/pci.c    | 9 ++++++---
 drivers/pci/probe.c  | 6 +++---
 drivers/pci/quirks.c | 6 +++---
 3 files changed, 12 insertions(+), 9 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 60230da957e0..e5a58f0fe58d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -732,15 +732,18 @@ u16 pci_find_vsec_capability(struct pci_dev *dev, u16 vendor, int cap)
 {
 	u16 vsec = 0;
 	u32 header;
+	int ret;
 
 	if (vendor != dev->vendor)
 		return 0;
 
 	while ((vsec = pci_find_next_ext_capability(dev, vsec,
 						     PCI_EXT_CAP_ID_VNDR))) {
-		if (pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER,
-					  &header) == PCIBIOS_SUCCESSFUL &&
-		    PCI_VNDR_HEADER_ID(header) == cap)
+		ret = pci_read_config_dword(dev, vsec + PCI_VNDR_HEADER, &header);
+		if (ret != PCIBIOS_SUCCESSFUL)
+			continue;
+
+		if (PCI_VNDR_HEADER_ID(header) == cap)
 			return vsec;
 	}
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 8bac3ce02609..0717ff62e54f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1652,15 +1652,15 @@ static void pci_set_removable(struct pci_dev *dev)
 static bool pci_ext_cfg_is_aliased(struct pci_dev *dev)
 {
 #ifdef CONFIG_PCI_QUIRKS
-	int pos;
+	int pos, ret;
 	u32 header, tmp;
 
 	pci_read_config_dword(dev, PCI_VENDOR_ID, &header);
 
 	for (pos = PCI_CFG_SPACE_SIZE;
 	     pos < PCI_CFG_SPACE_EXP_SIZE; pos += PCI_CFG_SPACE_SIZE) {
-		if (pci_read_config_dword(dev, pos, &tmp) != PCIBIOS_SUCCESSFUL
-		    || header != tmp)
+		ret = pci_read_config_dword(dev, pos, &tmp);
+		if ((ret != PCIBIOS_SUCCESSFUL) || (header != tmp))
 			return false;
 	}
 
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 321156ca273d..863b9a64c1fe 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -5381,7 +5381,7 @@ int pci_dev_specific_disable_acs_redir(struct pci_dev *dev)
  */
 static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
 {
-	int pos, i = 0;
+	int pos, i = 0, ret;
 	u8 next_cap;
 	u16 reg16, *cap;
 	struct pci_cap_saved_state *state;
@@ -5427,8 +5427,8 @@ static void quirk_intel_qat_vf_cap(struct pci_dev *pdev)
 		pdev->pcie_mpss = reg16 & PCI_EXP_DEVCAP_PAYLOAD;
 
 		pdev->cfg_size = PCI_CFG_SPACE_EXP_SIZE;
-		if (pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status) !=
-		    PCIBIOS_SUCCESSFUL || (status == 0xffffffff))
+		ret = pci_read_config_dword(pdev, PCI_CFG_SPACE_SIZE, &status);
+		if ((ret != PCIBIOS_SUCCESSFUL) || (status == 0xffffffff))
 			pdev->cfg_size = PCI_CFG_SPACE_SIZE;
 
 		if (pci_find_saved_cap(pdev, PCI_CAP_ID_EXP))
-- 
2.30.2

[PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

[Ilpo Järvinen]

Instead of a if condition with a line split, use the usual error
handling pattern with a separate variable to improve readability.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/pci/controller/pci-xgene.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
index 887b4941ff32..b7f338de160b 100644
--- a/drivers/pci/controller/pci-xgene.c
+++ b/drivers/pci/controller/pci-xgene.c
@@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
 				    int where, int size, u32 *val)
 {
 	struct xgene_pcie *port = pcie_bus_to_port(bus);
+	int ret;
 
-	if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
-	    PCIBIOS_SUCCESSFUL)
+	ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val);
+	if (ret != PCIBIOS_SUCCESSFUL)
 		return PCIBIOS_DEVICE_NOT_FOUND;
 
 	/*
-- 
2.30.2

Re: [PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

[Rob Herring]

On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
>
> Instead of a if condition with a line split, use the usual error
> handling pattern with a separate variable to improve readability.
>
> No functional changes intended.
>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/pci/controller/pci-xgene.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
> index 887b4941ff32..b7f338de160b 100644
> --- a/drivers/pci/controller/pci-xgene.c
> +++ b/drivers/pci/controller/pci-xgene.c
> @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
>                                     int where, int size, u32 *val)
>  {
>         struct xgene_pcie *port = pcie_bus_to_port(bus);
> +       int ret;
>
> -       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> -           PCIBIOS_SUCCESSFUL)
> +       ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val);
> +       if (ret != PCIBIOS_SUCCESSFUL)

Long term I think we want to replace these error codes with standard
linux ones. As PCIBIOS_SUCCESSFUL is 0, I would change this to just:

if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val))
    return PCIBIOS_DEVICE_NOT_FOUND;


>                 return PCIBIOS_DEVICE_NOT_FOUND;
>
>         /*
> --
> 2.30.2
>

Re: [PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1935 bytes --]

On Tue, 29 Aug 2023, Rob Herring wrote:

> On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > Instead of a if condition with a line split, use the usual error
> > handling pattern with a separate variable to improve readability.
> >
> > No functional changes intended.
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > ---
> >  drivers/pci/controller/pci-xgene.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
> > index 887b4941ff32..b7f338de160b 100644
> > --- a/drivers/pci/controller/pci-xgene.c
> > +++ b/drivers/pci/controller/pci-xgene.c
> > @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> >                                     int where, int size, u32 *val)
> >  {
> >         struct xgene_pcie *port = pcie_bus_to_port(bus);
> > +       int ret;
> >
> > -       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> > -           PCIBIOS_SUCCESSFUL)
> > +       ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val);
> > +       if (ret != PCIBIOS_SUCCESSFUL)
> 
> Long term I think we want to replace these error codes with standard
> linux ones.

This series is preparatory work for this very goal you stated!

> As PCIBIOS_SUCCESSFUL is 0, I would change this to just:
> 
> if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val))
>     return PCIBIOS_DEVICE_NOT_FOUND;

I'm not so sure about this suggestion as it will overwrite the original 
error code (like the current approach unfortunately also does). To me it 
would seem more appropriate is to return the original error code instead. 
But more discussion is needed before making such changes to values these 
functions return. (And there are plenty of similar examples besides this 
one.)


-- 
 i.

Re: [PATCH v2 7/8] PCI: xgene: Do PCI error check on own line

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2278 bytes --]

On Tue, 29 Aug 2023, Ilpo Järvinen wrote:

> On Tue, 29 Aug 2023, Rob Herring wrote:
> 
> > On Sun, Aug 27, 2023 at 8:37 AM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > Instead of a if condition with a line split, use the usual error
> > > handling pattern with a separate variable to improve readability.
> > >
> > > No functional changes intended.
> > >
> > > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> > > ---
> > >  drivers/pci/controller/pci-xgene.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/pci/controller/pci-xgene.c b/drivers/pci/controller/pci-xgene.c
> > > index 887b4941ff32..b7f338de160b 100644
> > > --- a/drivers/pci/controller/pci-xgene.c
> > > +++ b/drivers/pci/controller/pci-xgene.c
> > > @@ -163,9 +163,10 @@ static int xgene_pcie_config_read32(struct pci_bus *bus, unsigned int devfn,
> > >                                     int where, int size, u32 *val)
> > >  {
> > >         struct xgene_pcie *port = pcie_bus_to_port(bus);
> > > +       int ret;
> > >
> > > -       if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val) !=
> > > -           PCIBIOS_SUCCESSFUL)
> > > +       ret = pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val);
> > > +       if (ret != PCIBIOS_SUCCESSFUL)
> > 
> > Long term I think we want to replace these error codes with standard
> > linux ones.
> 
> This series is preparatory work for this very goal you stated!
> 
> > As PCIBIOS_SUCCESSFUL is 0, I would change this to just:
> > 
> > if (pci_generic_config_read32(bus, devfn, where & ~0x3, 4, val))
> >     return PCIBIOS_DEVICE_NOT_FOUND;
> 
> I'm not so sure about this suggestion as it will overwrite the original 
> error code (like the current approach unfortunately also does). To me it 
> would seem more appropriate is to return the original error code instead. 
> But more discussion is needed before making such changes to values these 
> functions return. (And there are plenty of similar examples besides this 
> one.)

Actually, it looks in this case I could do that transformation safely now
since pci_generic_config_read32() can only return either 
PCIBIOS_DEVICE_NOT_FOUND or PCIBIOS_SUCCESSFUL so it won't alter anything.

-- 
 i.

[PATCH v2 8/8] scsi: ipr: Do PCI error checks on own line

[Ilpo Järvinen]

Instead of if conditions with line splits, use the usual error handling
pattern with a separate variable to improve readability.

No functional changes intended.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/scsi/ipr.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/ipr.c b/drivers/scsi/ipr.c
index 4e13797b2a4a..81e3d464d1f6 100644
--- a/drivers/scsi/ipr.c
+++ b/drivers/scsi/ipr.c
@@ -761,12 +761,14 @@ static void ipr_mask_and_clear_interrupts(struct ipr_ioa_cfg *ioa_cfg,
 static int ipr_save_pcix_cmd_reg(struct ipr_ioa_cfg *ioa_cfg)
 {
 	int pcix_cmd_reg = pci_find_capability(ioa_cfg->pdev, PCI_CAP_ID_PCIX);
+	int rc;
 
 	if (pcix_cmd_reg == 0)
 		return 0;
 
-	if (pci_read_config_word(ioa_cfg->pdev, pcix_cmd_reg + PCI_X_CMD,
-				 &ioa_cfg->saved_pcix_cmd_reg) != PCIBIOS_SUCCESSFUL) {
+	rc = pci_read_config_word(ioa_cfg->pdev, pcix_cmd_reg + PCI_X_CMD,
+				  &ioa_cfg->saved_pcix_cmd_reg);
+	if (rc != PCIBIOS_SUCCESSFUL) {
 		dev_err(&ioa_cfg->pdev->dev, "Failed to save PCI-X command register\n");
 		return -EIO;
 	}
@@ -785,10 +787,12 @@ static int ipr_save_pcix_cmd_reg(struct ipr_ioa_cfg *ioa_cfg)
 static int ipr_set_pcix_cmd_reg(struct ipr_ioa_cfg *ioa_cfg)
 {
 	int pcix_cmd_reg = pci_find_capability(ioa_cfg->pdev, PCI_CAP_ID_PCIX);
+	int rc;
 
 	if (pcix_cmd_reg) {
-		if (pci_write_config_word(ioa_cfg->pdev, pcix_cmd_reg + PCI_X_CMD,
-					  ioa_cfg->saved_pcix_cmd_reg) != PCIBIOS_SUCCESSFUL) {
+		rc = pci_write_config_word(ioa_cfg->pdev, pcix_cmd_reg + PCI_X_CMD,
+					   ioa_cfg->saved_pcix_cmd_reg);
+		if (rc != PCIBIOS_SUCCESSFUL) {
 			dev_err(&ioa_cfg->pdev->dev, "Failed to setup PCI-X command register\n");
 			return -EIO;
 		}
-- 
2.30.2