[PATCH 0/7] Add definition for the number of standard PCI BARs

[PATCH 0/7] Add definition for the number of standard PCI BARs

[Denis Efremov]

Code that iterates over all standard PCI BARs typically uses
PCI_STD_RESOURCE_END, but this is error-prone because it requires
"i <= PCI_STD_RESOURCE_END" rather than something like
"i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
way PCI_SRIOV_NUM_BARS is used. There is already the definition
PCI_BAR_COUNT for s390 only. Thus, this patchset introduces it globally.

The patch is splitted into 7 parts for different drivers/subsystems for
easy readability.

Denis Efremov (7):
  PCI: Add define for the number of standard PCI BARs
  s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS
  x86/PCI: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
  PCI/net: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
  rapidio/tsi721: use PCI_STD_NUM_BARS in loops instead of
    PCI_STD_RESOURCE_END
  efifb: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
  vfio_pci: Use PCI_STD_NUM_BARS in loops instead of
    PCI_STD_RESOURCE_END

 arch/s390/include/asm/pci.h                      |  5 +----
 arch/s390/include/asm/pci_clp.h                  |  6 +++---
 arch/s390/pci/pci.c                              | 16 ++++++++--------
 arch/s390/pci/pci_clp.c                          |  6 +++---
 arch/x86/pci/common.c                            |  2 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |  4 ++--
 drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c   |  2 +-
 drivers/pci/quirks.c                             |  2 +-
 drivers/rapidio/devices/tsi721.c                 |  2 +-
 drivers/vfio/pci/vfio_pci.c                      |  4 ++--
 drivers/vfio/pci/vfio_pci_config.c               |  2 +-
 drivers/vfio/pci/vfio_pci_private.h              |  4 ++--
 drivers/video/fbdev/efifb.c                      |  2 +-
 include/linux/pci.h                              |  2 +-
 include/uapi/linux/pci_regs.h                    |  1 +
 15 files changed, 29 insertions(+), 31 deletions(-)

-- 
2.21.0

[PATCH 1/7] PCI: Add define for the number of standard PCI BARs

[Denis Efremov]

Code that iterates over all standard PCI BARs typically uses
PCI_STD_RESOURCE_END. However, it requires the "unusual" loop condition
"i <= PCI_STD_RESOURCE_END" rather than something more standard like
"i < PCI_STD_NUM_BARS".

This patch adds the definition PCI_STD_NUM_BARS which is equivalent to
"PCI_STD_RESOURCE_END + 1" and updates loop conditions to use it.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 drivers/pci/quirks.c          | 2 +-
 include/linux/pci.h           | 2 +-
 include/uapi/linux/pci_regs.h | 1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 208aacf39329..02bdf3a0231e 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -475,7 +475,7 @@ static void quirk_extend_bar_to_page(struct pci_dev *dev)
 {
 	int i;
 
-	for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		struct resource *r = &dev->resource[i];
 
 		if (r->flags & IORESOURCE_MEM && resource_size(r) < PAGE_SIZE) {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9e700d9f9f28..7b9590d5dc2d 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -76,7 +76,7 @@ enum pci_mmap_state {
 enum {
 	/* #0-5: standard PCI resources */
 	PCI_STD_RESOURCES,
-	PCI_STD_RESOURCE_END = 5,
+	PCI_STD_RESOURCE_END = PCI_STD_RESOURCES + PCI_STD_NUM_BARS - 1,
 
 	/* #6: expansion ROM resource */
 	PCI_ROM_RESOURCE,
diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index f28e562d7ca8..68b571d491eb 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -34,6 +34,7 @@
  * of which the first 64 bytes are standardized as follows:
  */
 #define PCI_STD_HEADER_SIZEOF	64
+#define PCI_STD_NUM_BARS	6	/* Number of standard BARs */
 #define PCI_VENDOR_ID		0x00	/* 16 bits */
 #define PCI_DEVICE_ID		0x02	/* 16 bits */
 #define PCI_COMMAND		0x04	/* 16 bits */
-- 
2.21.0

[PATCH 2/7] s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS

[Denis Efremov]

This patch replaces local define PCI_BAR_COUNT with global
PCI_STD_NUM_BARS.

Signed-off-by: Denis Efremov <efremov@linux.com>
---
 arch/s390/include/asm/pci.h     |  5 +----
 arch/s390/include/asm/pci_clp.h |  6 +++---
 arch/s390/pci/pci.c             | 16 ++++++++--------
 arch/s390/pci/pci_clp.c         |  6 +++---
 4 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
index a2399eff84ca..3a06c264ea53 100644
--- a/arch/s390/include/asm/pci.h
+++ b/arch/s390/include/asm/pci.h
@@ -2,9 +2,6 @@
 #ifndef __ASM_S390_PCI_H
 #define __ASM_S390_PCI_H
 
-/* must be set before including pci_clp.h */
-#define PCI_BAR_COUNT	6
-
 #include <linux/pci.h>
 #include <linux/mutex.h>
 #include <linux/iommu.h>
@@ -138,7 +135,7 @@ struct zpci_dev {
 
 	char res_name[16];
 	bool mio_capable;
-	struct zpci_bar_struct bars[PCI_BAR_COUNT];
+	struct zpci_bar_struct bars[PCI_STD_NUM_BARS];
 
 	u64		start_dma;	/* Start of available DMA addresses */
 	u64		end_dma;	/* End of available DMA addresses */
diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
index 50359172cc48..bd2cb4ea7d93 100644
--- a/arch/s390/include/asm/pci_clp.h
+++ b/arch/s390/include/asm/pci_clp.h
@@ -77,7 +77,7 @@ struct mio_info {
 	struct {
 		u64 wb;
 		u64 wt;
-	} addr[PCI_BAR_COUNT];
+	} addr[PCI_STD_NUM_BARS];
 	u32 reserved[6];
 } __packed;
 
@@ -98,9 +98,9 @@ struct clp_rsp_query_pci {
 	u16 util_str_avail	:  1;	/* utility string available? */
 	u16 pfgid		:  8;	/* pci function group id */
 	u32 fid;			/* pci function id */
-	u8 bar_size[PCI_BAR_COUNT];
+	u8 bar_size[PCI_STD_NUM_BARS];
 	u16 pchid;
-	__le32 bar[PCI_BAR_COUNT];
+	__le32 bar[PCI_STD_NUM_BARS];
 	u8 pfip[CLP_PFIP_NR_SEGMENTS];	/* pci function internal path */
 	u32			: 16;
 	u8 fmb_len;
diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
index b0e3b9a0e488..c57ccac25c02 100644
--- a/arch/s390/pci/pci.c
+++ b/arch/s390/pci/pci.c
@@ -43,7 +43,7 @@ static DECLARE_BITMAP(zpci_domain, ZPCI_NR_DEVICES);
 static DEFINE_SPINLOCK(zpci_domain_lock);
 
 #define ZPCI_IOMAP_ENTRIES						\
-	min(((unsigned long) ZPCI_NR_DEVICES * PCI_BAR_COUNT / 2),	\
+	min(((unsigned long) ZPCI_NR_DEVICES * PCI_STD_NUM_BARS / 2),	\
 	    ZPCI_IOMAP_MAX_ENTRIES)
 
 static DEFINE_SPINLOCK(zpci_iomap_lock);
@@ -294,7 +294,7 @@ static void __iomem *pci_iomap_range_mio(struct pci_dev *pdev, int bar,
 void __iomem *pci_iomap_range(struct pci_dev *pdev, int bar,
 			      unsigned long offset, unsigned long max)
 {
-	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
+	if (!pci_resource_len(pdev, bar) || bar >= PCI_STD_NUM_BARS)
 		return NULL;
 
 	if (static_branch_likely(&have_mio))
@@ -324,7 +324,7 @@ static void __iomem *pci_iomap_wc_range_mio(struct pci_dev *pdev, int bar,
 void __iomem *pci_iomap_wc_range(struct pci_dev *pdev, int bar,
 				 unsigned long offset, unsigned long max)
 {
-	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
+	if (!pci_resource_len(pdev, bar) || bar >= PCI_STD_NUM_BARS)
 		return NULL;
 
 	if (static_branch_likely(&have_mio))
@@ -416,7 +416,7 @@ static void zpci_map_resources(struct pci_dev *pdev)
 	resource_size_t len;
 	int i;
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		len = pci_resource_len(pdev, i);
 		if (!len)
 			continue;
@@ -451,7 +451,7 @@ static void zpci_unmap_resources(struct pci_dev *pdev)
 	if (zpci_use_mio(zdev))
 		return;
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		len = pci_resource_len(pdev, i);
 		if (!len)
 			continue;
@@ -514,7 +514,7 @@ static int zpci_setup_bus_resources(struct zpci_dev *zdev,
 	snprintf(zdev->res_name, sizeof(zdev->res_name),
 		 "PCI Bus %04x:%02x", zdev->domain, ZPCI_BUS_NR);
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		if (!zdev->bars[i].size)
 			continue;
 		entry = zpci_alloc_iomap(zdev);
@@ -551,7 +551,7 @@ static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
 {
 	int i;
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		if (!zdev->bars[i].size || !zdev->bars[i].res)
 			continue;
 
@@ -573,7 +573,7 @@ int pcibios_add_device(struct pci_dev *pdev)
 	pdev->dev.dma_ops = &s390_pci_dma_ops;
 	zpci_map_resources(pdev);
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		res = &pdev->resource[i];
 		if (res->parent || !res->flags)
 			continue;
diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
index 9bdff4defef1..8b729b5f2972 100644
--- a/arch/s390/pci/pci_clp.c
+++ b/arch/s390/pci/pci_clp.c
@@ -145,7 +145,7 @@ static int clp_store_query_pci_fn(struct zpci_dev *zdev,
 {
 	int i;
 
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
 		zdev->bars[i].val = le32_to_cpu(response->bar[i]);
 		zdev->bars[i].size = response->bar_size[i];
 	}
@@ -164,8 +164,8 @@ static int clp_store_query_pci_fn(struct zpci_dev *zdev,
 		       sizeof(zdev->util_str));
 	}
 	zdev->mio_capable = response->mio_addr_avail;
-	for (i = 0; i < PCI_BAR_COUNT; i++) {
-		if (!(response->mio.valid & (1 << (PCI_BAR_COUNT - i - 1))))
+	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+		if (!(response->mio.valid & (1 << (PCI_STD_NUM_BARS - i - 1))))
 			continue;
 
 		zdev->bars[i].mio_wb = (void __iomem *) response->mio.addr[i].wb;
-- 
2.21.0

Re: [PATCH 0/7] Add definition for the number of standard PCI BARs

[Andrew Murray]

On Sun, Aug 11, 2019 at 06:07:55PM +0300, Denis Efremov wrote:
> Code that iterates over all standard PCI BARs typically uses
> PCI_STD_RESOURCE_END, but this is error-prone because it requires
> "i <= PCI_STD_RESOURCE_END" rather than something like
> "i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
> way PCI_SRIOV_NUM_BARS is used. There is already the definition
> PCI_BAR_COUNT for s390 only. Thus, this patchset introduces it globally.
> 
> The patch is splitted into 7 parts for different drivers/subsystems for
> easy readability.
> 
> Denis Efremov (7):
>   PCI: Add define for the number of standard PCI BARs
>   s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS
>   x86/PCI: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   PCI/net: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   rapidio/tsi721: use PCI_STD_NUM_BARS in loops instead of
>     PCI_STD_RESOURCE_END
>   efifb: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   vfio_pci: Use PCI_STD_NUM_BARS in loops instead of
>     PCI_STD_RESOURCE_END
> 
>  arch/s390/include/asm/pci.h                      |  5 +----
>  arch/s390/include/asm/pci_clp.h                  |  6 +++---
>  arch/s390/pci/pci.c                              | 16 ++++++++--------
>  arch/s390/pci/pci_clp.c                          |  6 +++---
>  arch/x86/pci/common.c                            |  2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |  4 ++--
>  drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c   |  2 +-
>  drivers/pci/quirks.c                             |  2 +-
>  drivers/rapidio/devices/tsi721.c                 |  2 +-
>  drivers/vfio/pci/vfio_pci.c                      |  4 ++--
>  drivers/vfio/pci/vfio_pci_config.c               |  2 +-
>  drivers/vfio/pci/vfio_pci_private.h              |  4 ++--
>  drivers/video/fbdev/efifb.c                      |  2 +-
>  include/linux/pci.h                              |  2 +-
>  include/uapi/linux/pci_regs.h                    |  1 +

Hi Denis,

You could also fix up a few cases where the number of BARs is hard coded in
loops, e.g.

drivers/pci/controller/pci-hyperv.c - look for uses of probed_bar in loops
drivers/pci/pci.c - pci_release_selected_regions and __pci_request_selected_regions
drivers/pci/quirks.c - quirk_alder_ioapic

Thanks,

Andrew Murray

>  15 files changed, 29 insertions(+), 31 deletions(-)
> 
> -- 
> 2.21.0
> 

Re: [PATCH 0/7] Add definition for the number of standard PCI BARs

[Bjorn Helgaas]

On Sun, Aug 11, 2019 at 06:07:55PM +0300, Denis Efremov wrote:
> Code that iterates over all standard PCI BARs typically uses
> PCI_STD_RESOURCE_END, but this is error-prone because it requires
> "i <= PCI_STD_RESOURCE_END" rather than something like
> "i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
> way PCI_SRIOV_NUM_BARS is used. There is already the definition
> PCI_BAR_COUNT for s390 only. Thus, this patchset introduces it globally.
> 
> The patch is splitted into 7 parts for different drivers/subsystems for
> easy readability.

This looks good.  I can take all these together, since they all depend
on the first patch.  I have a few comments on the individual patches.

> Denis Efremov (7):
>   PCI: Add define for the number of standard PCI BARs
>   s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS
>   x86/PCI: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   PCI/net: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   rapidio/tsi721: use PCI_STD_NUM_BARS in loops instead of
>     PCI_STD_RESOURCE_END
>   efifb: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END
>   vfio_pci: Use PCI_STD_NUM_BARS in loops instead of
>     PCI_STD_RESOURCE_END
> 
>  arch/s390/include/asm/pci.h                      |  5 +----
>  arch/s390/include/asm/pci_clp.h                  |  6 +++---
>  arch/s390/pci/pci.c                              | 16 ++++++++--------
>  arch/s390/pci/pci_clp.c                          |  6 +++---
>  arch/x86/pci/common.c                            |  2 +-
>  drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |  4 ++--
>  drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c   |  2 +-
>  drivers/pci/quirks.c                             |  2 +-
>  drivers/rapidio/devices/tsi721.c                 |  2 +-
>  drivers/vfio/pci/vfio_pci.c                      |  4 ++--
>  drivers/vfio/pci/vfio_pci_config.c               |  2 +-
>  drivers/vfio/pci/vfio_pci_private.h              |  4 ++--
>  drivers/video/fbdev/efifb.c                      |  2 +-
>  include/linux/pci.h                              |  2 +-
>  include/uapi/linux/pci_regs.h                    |  1 +
>  15 files changed, 29 insertions(+), 31 deletions(-)
> 
> -- 
> 2.21.0
> 

Re: [PATCH 2/7] s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS

[Bjorn Helgaas]

On Sun, Aug 11, 2019 at 06:07:57PM +0300, Denis Efremov wrote:
> This patch replaces local define PCI_BAR_COUNT with global
> PCI_STD_NUM_BARS.
> 
> Signed-off-by: Denis Efremov <efremov@linux.com>
> ---
>  arch/s390/include/asm/pci.h     |  5 +----
>  arch/s390/include/asm/pci_clp.h |  6 +++---
>  arch/s390/pci/pci.c             | 16 ++++++++--------
>  arch/s390/pci/pci_clp.c         |  6 +++---
>  4 files changed, 15 insertions(+), 18 deletions(-)
> 
> diff --git a/arch/s390/include/asm/pci.h b/arch/s390/include/asm/pci.h
> index a2399eff84ca..3a06c264ea53 100644
> --- a/arch/s390/include/asm/pci.h
> +++ b/arch/s390/include/asm/pci.h
> @@ -2,9 +2,6 @@
>  #ifndef __ASM_S390_PCI_H
>  #define __ASM_S390_PCI_H
>  
> -/* must be set before including pci_clp.h */
> -#define PCI_BAR_COUNT	6
> -
>  #include <linux/pci.h>
>  #include <linux/mutex.h>
>  #include <linux/iommu.h>
> @@ -138,7 +135,7 @@ struct zpci_dev {
>  
>  	char res_name[16];
>  	bool mio_capable;
> -	struct zpci_bar_struct bars[PCI_BAR_COUNT];
> +	struct zpci_bar_struct bars[PCI_STD_NUM_BARS];
>  
>  	u64		start_dma;	/* Start of available DMA addresses */
>  	u64		end_dma;	/* End of available DMA addresses */
> diff --git a/arch/s390/include/asm/pci_clp.h b/arch/s390/include/asm/pci_clp.h
> index 50359172cc48..bd2cb4ea7d93 100644
> --- a/arch/s390/include/asm/pci_clp.h
> +++ b/arch/s390/include/asm/pci_clp.h
> @@ -77,7 +77,7 @@ struct mio_info {
>  	struct {
>  		u64 wb;
>  		u64 wt;
> -	} addr[PCI_BAR_COUNT];
> +	} addr[PCI_STD_NUM_BARS];
>  	u32 reserved[6];
>  } __packed;
>  
> @@ -98,9 +98,9 @@ struct clp_rsp_query_pci {
>  	u16 util_str_avail	:  1;	/* utility string available? */
>  	u16 pfgid		:  8;	/* pci function group id */
>  	u32 fid;			/* pci function id */
> -	u8 bar_size[PCI_BAR_COUNT];
> +	u8 bar_size[PCI_STD_NUM_BARS];
>  	u16 pchid;
> -	__le32 bar[PCI_BAR_COUNT];
> +	__le32 bar[PCI_STD_NUM_BARS];
>  	u8 pfip[CLP_PFIP_NR_SEGMENTS];	/* pci function internal path */
>  	u32			: 16;
>  	u8 fmb_len;
> diff --git a/arch/s390/pci/pci.c b/arch/s390/pci/pci.c
> index b0e3b9a0e488..c57ccac25c02 100644
> --- a/arch/s390/pci/pci.c
> +++ b/arch/s390/pci/pci.c
> @@ -43,7 +43,7 @@ static DECLARE_BITMAP(zpci_domain, ZPCI_NR_DEVICES);
>  static DEFINE_SPINLOCK(zpci_domain_lock);
>  
>  #define ZPCI_IOMAP_ENTRIES						\
> -	min(((unsigned long) ZPCI_NR_DEVICES * PCI_BAR_COUNT / 2),	\
> +	min(((unsigned long) ZPCI_NR_DEVICES * PCI_STD_NUM_BARS / 2),	\
>  	    ZPCI_IOMAP_MAX_ENTRIES)
>  
>  static DEFINE_SPINLOCK(zpci_iomap_lock);
> @@ -294,7 +294,7 @@ static void __iomem *pci_iomap_range_mio(struct pci_dev *pdev, int bar,
>  void __iomem *pci_iomap_range(struct pci_dev *pdev, int bar,
>  			      unsigned long offset, unsigned long max)
>  {
> -	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
> +	if (!pci_resource_len(pdev, bar) || bar >= PCI_STD_NUM_BARS)

The two conditions in this test should be reversed so we bounds check
"bar" before using it, i.e.,:

        if (bar >= PCI_STD_NUM_BARS || !pci_resource_len(pdev, bar))

That's not strictly related to the purpose of this patch, but it's so
trivial that I don't think it's worth splitting to a separate patch.

>  		return NULL;
>  
>  	if (static_branch_likely(&have_mio))
> @@ -324,7 +324,7 @@ static void __iomem *pci_iomap_wc_range_mio(struct pci_dev *pdev, int bar,
>  void __iomem *pci_iomap_wc_range(struct pci_dev *pdev, int bar,
>  				 unsigned long offset, unsigned long max)
>  {
> -	if (!pci_resource_len(pdev, bar) || bar >= PCI_BAR_COUNT)
> +	if (!pci_resource_len(pdev, bar) || bar >= PCI_STD_NUM_BARS)

Same here.

>  		return NULL;
>  
>  	if (static_branch_likely(&have_mio))
> @@ -416,7 +416,7 @@ static void zpci_map_resources(struct pci_dev *pdev)
>  	resource_size_t len;
>  	int i;
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		len = pci_resource_len(pdev, i);
>  		if (!len)
>  			continue;

There's a loop just below here that should be rewritten in the typical
style:

        i = PCI_IOV_RESOURCES;
        for (; i < PCI_SRIOV_NUM_BARS + PCI_IOV_RESOURCES; i++) {
                len = pci_resource_len(pdev, i);

Again, not strictly related to this patch, but probably trivial enough
to do in the same patch.

> @@ -451,7 +451,7 @@ static void zpci_unmap_resources(struct pci_dev *pdev)
>  	if (zpci_use_mio(zdev))
>  		return;
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		len = pci_resource_len(pdev, i);
>  		if (!len)
>  			continue;

Completely unrelated to this patch, but the fact that
zpci_map_resources() deals with both standard BARs and SR-IOV BARs
while zpci_unmap_resources() only deals with standard BARs makes me
wonder why they're not symmetric.

It is true that zpci_map_resources() doesn't handle standard and
SR-IOV BARs similarly, so maybe this is intended.  Maybe the s390
folks know.

> @@ -514,7 +514,7 @@ static int zpci_setup_bus_resources(struct zpci_dev *zdev,
>  	snprintf(zdev->res_name, sizeof(zdev->res_name),
>  		 "PCI Bus %04x:%02x", zdev->domain, ZPCI_BUS_NR);
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		if (!zdev->bars[i].size)
>  			continue;
>  		entry = zpci_alloc_iomap(zdev);
> @@ -551,7 +551,7 @@ static void zpci_cleanup_bus_resources(struct zpci_dev *zdev)
>  {
>  	int i;
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		if (!zdev->bars[i].size || !zdev->bars[i].res)
>  			continue;
>  
> @@ -573,7 +573,7 @@ int pcibios_add_device(struct pci_dev *pdev)
>  	pdev->dev.dma_ops = &s390_pci_dma_ops;
>  	zpci_map_resources(pdev);
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		res = &pdev->resource[i];
>  		if (res->parent || !res->flags)
>  			continue;
> diff --git a/arch/s390/pci/pci_clp.c b/arch/s390/pci/pci_clp.c
> index 9bdff4defef1..8b729b5f2972 100644
> --- a/arch/s390/pci/pci_clp.c
> +++ b/arch/s390/pci/pci_clp.c
> @@ -145,7 +145,7 @@ static int clp_store_query_pci_fn(struct zpci_dev *zdev,
>  {
>  	int i;
>  
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>  		zdev->bars[i].val = le32_to_cpu(response->bar[i]);
>  		zdev->bars[i].size = response->bar_size[i];
>  	}
> @@ -164,8 +164,8 @@ static int clp_store_query_pci_fn(struct zpci_dev *zdev,
>  		       sizeof(zdev->util_str));
>  	}
>  	zdev->mio_capable = response->mio_addr_avail;
> -	for (i = 0; i < PCI_BAR_COUNT; i++) {
> -		if (!(response->mio.valid & (1 << (PCI_BAR_COUNT - i - 1))))
> +	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> +		if (!(response->mio.valid & (1 << (PCI_STD_NUM_BARS - i - 1))))
>  			continue;
>  
>  		zdev->bars[i].mio_wb = (void __iomem *) response->mio.addr[i].wb;
> -- 
> 2.21.0
> 

Re: [PATCH 0/7] Add definition for the number of standard PCI BARs

[Thomas Gleixner]

On Mon, 12 Aug 2019, Bjorn Helgaas wrote:

> On Sun, Aug 11, 2019 at 06:07:55PM +0300, Denis Efremov wrote:
> > Code that iterates over all standard PCI BARs typically uses
> > PCI_STD_RESOURCE_END, but this is error-prone because it requires
> > "i <= PCI_STD_RESOURCE_END" rather than something like
> > "i < PCI_STD_NUM_BARS". We could add such a definition and use it the same
> > way PCI_SRIOV_NUM_BARS is used. There is already the definition
> > PCI_BAR_COUNT for s390 only. Thus, this patchset introduces it globally.
> > 
> > The patch is splitted into 7 parts for different drivers/subsystems for
> > easy readability.
> 
> This looks good.  I can take all these together, since they all depend
> on the first patch.  I have a few comments on the individual patches.
> 
> > Denis Efremov (7):
> >   PCI: Add define for the number of standard PCI BARs
> >   s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS
> >   x86/PCI: Use PCI_STD_NUM_BARS in loops instead of PCI_STD_RESOURCE_END

Fine with me for the x86 part. That's your turf anyway :)

Thanks,

	tglx

Re: [PATCH 0/7] Add definition for the number of standard PCI BARs

[Denis Efremov]

On 12.08.2019 12:06, Andrew Murray wrote:
> 
> Hi Denis,

Hi!

> 
> You could also fix up a few cases where the number of BARs is hard coded in
> loops, e.g.
> 
> drivers/pci/controller/pci-hyperv.c - look for uses of probed_bar in loops
> drivers/pci/pci.c - pci_release_selected_regions and __pci_request_selected_regions
> drivers/pci/quirks.c - quirk_alder_ioapic
> 

Thanks for pointing me on that. I will take this into account in v2.

Denis

Re: [PATCH 2/7] s390/pci: Replace PCI_BAR_COUNT with PCI_STD_NUM_BARS

[Denis Efremov]

Hi!

> 
> There's a loop just below here that should be rewritten in the typical
> style:
> 
>         i = PCI_IOV_RESOURCES;
>         for (; i < PCI_SRIOV_NUM_BARS + PCI_IOV_RESOURCES; i++) {
>                 len = pci_resource_len(pdev, i);
> 
> Again, not strictly related to this patch, but probably trivial enough
> to do in the same patch.
> 

I think it's already fixed in a separate patch https://lkml.org/lkml/2019/8/6/694
Thanks for the review, I will send v2.

Denis