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.
Changes in v2:
- Reverse checks in pci_iomap_range,pci_iomap_wc_range.
- Refactor loops in vfio_pci to keep PCI_STD_RESOURCES.
- Add 2 new patches to replace the magic constant with new define.
- Split net patch in v1 to separate stmmac and dwc-xlgmac patches.
Denis Efremov (10):
PCI: Add define for the number of standard PCI BARs
s390/pci: Loop using PCI_STD_NUM_BARS
x86/PCI: Loop using PCI_STD_NUM_BARS
stmmac: pci: Loop using PCI_STD_NUM_BARS
net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
rapidio/tsi721: Loop using PCI_STD_NUM_BARS
efifb: Loop using PCI_STD_NUM_BARS
vfio_pci: Loop using PCI_STD_NUM_BARS
PCI: hv: Use PCI_STD_NUM_BARS
PCI: Use PCI_STD_NUM_BARS
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/controller/pci-hyperv.c | 10 +++++-----
drivers/pci/pci.c | 11 ++++++-----
drivers/pci/quirks.c | 4 ++--
drivers/rapidio/devices/tsi721.c | 2 +-
drivers/vfio/pci/vfio_pci.c | 11 +++++++----
drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
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 +
17 files changed, 51 insertions(+), 47 deletions(-)
--
2.21.0
Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
'i <= PCI_STD_RESOURCE_END'.
Signed-off-by: Denis Efremov <[email protected]>
---
drivers/vfio/pci/vfio_pci.c | 11 +++++++----
drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
drivers/vfio/pci/vfio_pci_private.h | 4 ++--
3 files changed, 15 insertions(+), 10 deletions(-)
diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
index 703948c9fbe1..cb7d220d3246 100644
--- a/drivers/vfio/pci/vfio_pci.c
+++ b/drivers/vfio/pci/vfio_pci.c
@@ -110,13 +110,15 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
static void vfio_pci_probe_mmaps(struct vfio_pci_device *vdev)
{
struct resource *res;
- int bar;
+ int i;
struct vfio_pci_dummy_resource *dummy_res;
INIT_LIST_HEAD(&vdev->dummy_resources_list);
- for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
- res = vdev->pdev->resource + bar;
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ int bar = i + PCI_STD_RESOURCES;
+
+ res = &vdev->pdev->resource[bar];
if (!IS_ENABLED(CONFIG_VFIO_PCI_MMAP))
goto no_mmap;
@@ -399,7 +401,8 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
vfio_config_free(vdev);
- for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
+ bar = i + PCI_STD_RESOURCES;
if (!vdev->barmap[bar])
continue;
pci_iounmap(pdev, vdev->barmap[bar]);
diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
index f0891bd8444c..df8772395219 100644
--- a/drivers/vfio/pci/vfio_pci_config.c
+++ b/drivers/vfio/pci/vfio_pci_config.c
@@ -455,16 +455,18 @@ static void vfio_bar_fixup(struct vfio_pci_device *vdev)
bar = (__le32 *)&vdev->vconfig[PCI_BASE_ADDRESS_0];
- for (i = PCI_STD_RESOURCES; i <= PCI_STD_RESOURCE_END; i++, bar++) {
- if (!pci_resource_start(pdev, i)) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++, bar++) {
+ int ibar = i + PCI_STD_RESOURCES;
+
+ if (!pci_resource_start(pdev, ibar)) {
*bar = 0; /* Unmapped by host = unimplemented to user */
continue;
}
- mask = ~(pci_resource_len(pdev, i) - 1);
+ mask = ~(pci_resource_len(pdev, ibar) - 1);
*bar &= cpu_to_le32((u32)mask);
- *bar |= vfio_generate_bar_flags(pdev, i);
+ *bar |= vfio_generate_bar_flags(pdev, ibar);
if (*bar & cpu_to_le32(PCI_BASE_ADDRESS_MEM_TYPE_64)) {
bar++;
diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
index ee6ee91718a4..8a2c7607d513 100644
--- a/drivers/vfio/pci/vfio_pci_private.h
+++ b/drivers/vfio/pci/vfio_pci_private.h
@@ -86,8 +86,8 @@ struct vfio_pci_reflck {
struct vfio_pci_device {
struct pci_dev *pdev;
- void __iomem *barmap[PCI_STD_RESOURCE_END + 1];
- bool bar_mmap_supported[PCI_STD_RESOURCE_END + 1];
+ void __iomem *barmap[PCI_STD_NUM_BARS];
+ bool bar_mmap_supported[PCI_STD_NUM_BARS];
u8 *pci_config_map;
u8 *vconfig;
struct perm_bits *msi_perm;
--
2.21.0
Replace the magic constant with define PCI_STD_NUM_BARS.
Signed-off-by: Denis Efremov <[email protected]>
---
drivers/pci/pci.c | 11 ++++++-----
drivers/pci/quirks.c | 2 +-
2 files changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 1b27b5af3d55..a9005c9eee6c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -3768,7 +3768,7 @@ void pci_release_selected_regions(struct pci_dev *pdev, int bars)
{
int i;
- for (i = 0; i < 6; i++)
+ for (i = 0; i < PCI_STD_NUM_BARS; i++)
if (bars & (1 << i))
pci_release_region(pdev, i);
}
@@ -3779,7 +3779,7 @@ static int __pci_request_selected_regions(struct pci_dev *pdev, int bars,
{
int i;
- for (i = 0; i < 6; i++)
+ for (i = 0; i < PCI_STD_NUM_BARS; i++)
if (bars & (1 << i))
if (__pci_request_region(pdev, i, res_name, excl))
goto err_out;
@@ -3827,7 +3827,7 @@ EXPORT_SYMBOL(pci_request_selected_regions_exclusive);
void pci_release_regions(struct pci_dev *pdev)
{
- pci_release_selected_regions(pdev, (1 << 6) - 1);
+ pci_release_selected_regions(pdev, (1 << PCI_STD_NUM_BARS) - 1);
}
EXPORT_SYMBOL(pci_release_regions);
@@ -3846,7 +3846,8 @@ EXPORT_SYMBOL(pci_release_regions);
*/
int pci_request_regions(struct pci_dev *pdev, const char *res_name)
{
- return pci_request_selected_regions(pdev, ((1 << 6) - 1), res_name);
+ return pci_request_selected_regions(pdev,
+ ((1 << PCI_STD_NUM_BARS) - 1), res_name);
}
EXPORT_SYMBOL(pci_request_regions);
@@ -3868,7 +3869,7 @@ EXPORT_SYMBOL(pci_request_regions);
int pci_request_regions_exclusive(struct pci_dev *pdev, const char *res_name)
{
return pci_request_selected_regions_exclusive(pdev,
- ((1 << 6) - 1), res_name);
+ ((1 << PCI_STD_NUM_BARS) - 1), res_name);
}
EXPORT_SYMBOL(pci_request_regions_exclusive);
diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
index 02bdf3a0231e..51caa61e6112 100644
--- a/drivers/pci/quirks.c
+++ b/drivers/pci/quirks.c
@@ -1810,7 +1810,7 @@ static void quirk_alder_ioapic(struct pci_dev *pdev)
* The next five BARs all seem to be rubbish, so just clean
* them out.
*/
- for (i = 1; i < 6; i++)
+ for (i = 1; i < PCI_STD_NUM_BARS; i++)
memset(&pdev->resource[i], 0, sizeof(pdev->resource[i]));
}
DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_EESSC, quirk_alder_ioapic);
--
2.21.0
Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
'i <= PCI_STD_RESOURCE_END'.
Signed-off-by: Denis Efremov <[email protected]>
---
arch/x86/pci/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/pci/common.c b/arch/x86/pci/common.c
index 9acab6ac28f5..1e59df041456 100644
--- a/arch/x86/pci/common.c
+++ b/arch/x86/pci/common.c
@@ -135,7 +135,7 @@ static void pcibios_fixup_device_resources(struct pci_dev *dev)
* resource so the kernel doesn't attempt to assign
* it later on in pci_assign_unassigned_resources
*/
- for (bar = 0; bar <= PCI_STD_RESOURCE_END; bar++) {
+ for (bar = 0; bar < PCI_STD_NUM_BARS; bar++) {
bar_r = &dev->resource[bar];
if (bar_r->start == 0 && bar_r->end != 0) {
bar_r->flags = 0;
--
2.21.0
Replace the magic constant with define PCI_STD_NUM_BARS.
Signed-off-by: Denis Efremov <[email protected]>
---
drivers/pci/controller/pci-hyperv.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
index 40b625458afa..1665c23b649f 100644
--- a/drivers/pci/controller/pci-hyperv.c
+++ b/drivers/pci/controller/pci-hyperv.c
@@ -307,7 +307,7 @@ struct pci_bus_relations {
struct pci_q_res_req_response {
struct vmpacket_descriptor hdr;
s32 status; /* negative values are failures */
- u32 probed_bar[6];
+ u32 probed_bar[PCI_STD_NUM_BARS];
} __packed;
struct pci_set_power {
@@ -503,7 +503,7 @@ struct hv_pci_dev {
* What would be observed if one wrote 0xFFFFFFFF to a BAR and then
* read it back, for each of the BAR offsets within config space.
*/
- u32 probed_bar[6];
+ u32 probed_bar[PCI_STD_NUM_BARS];
};
struct hv_pci_compl {
@@ -1327,7 +1327,7 @@ static void survey_child_resources(struct hv_pcibus_device *hbus)
* so it's sufficient to just add them up without tracking alignment.
*/
list_for_each_entry(hpdev, &hbus->children, list_entry) {
- for (i = 0; i < 6; i++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
if (hpdev->probed_bar[i] & PCI_BASE_ADDRESS_SPACE_IO)
dev_err(&hbus->hdev->device,
"There's an I/O BAR in this list!\n");
@@ -1401,7 +1401,7 @@ static void prepopulate_bars(struct hv_pcibus_device *hbus)
/* Pick addresses for the BARs. */
do {
list_for_each_entry(hpdev, &hbus->children, list_entry) {
- for (i = 0; i < 6; i++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
bar_val = hpdev->probed_bar[i];
if (bar_val == 0)
continue;
@@ -1558,7 +1558,7 @@ static void q_resource_requirements(void *context, struct pci_response *resp,
"query resource requirements failed: %x\n",
resp->status);
} else {
- for (i = 0; i < 6; i++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
completion->hpdev->probed_bar[i] =
q_res_req->probed_bar[i];
}
--
2.21.0
Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
'i <= PCI_STD_RESOURCE_END'.
Signed-off-by: Denis Efremov <[email protected]>
---
drivers/rapidio/devices/tsi721.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rapidio/devices/tsi721.c b/drivers/rapidio/devices/tsi721.c
index 125a173bed45..4dd31dd9feea 100644
--- a/drivers/rapidio/devices/tsi721.c
+++ b/drivers/rapidio/devices/tsi721.c
@@ -2755,7 +2755,7 @@ static int tsi721_probe(struct pci_dev *pdev,
{
int i;
- for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
tsi_debug(INIT, &pdev->dev, "res%d %pR",
i, &pdev->resource[i]);
}
--
2.21.0
Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
'i <= PCI_STD_RESOURCE_END'.
Signed-off-by: Denis Efremov <[email protected]>
---
drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c b/drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c
index 386bafe74c3f..fa8604d7b797 100644
--- a/drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c
+++ b/drivers/net/ethernet/synopsys/dwc-xlgmac-pci.c
@@ -34,7 +34,7 @@ static int xlgmac_probe(struct pci_dev *pcidev, const struct pci_device_id *id)
return ret;
}
- for (i = 0; i <= PCI_STD_RESOURCE_END; i++) {
+ for (i = 0; i < PCI_STD_NUM_BARS; i++) {
if (pci_resource_len(pcidev, i) == 0)
continue;
ret = pcim_iomap_regions(pcidev, BIT(i), XLGMAC_DRV_NAME);
--
2.21.0
On Fri, 16 Aug 2019, Denis Efremov wrote:
> Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
> 'i <= PCI_STD_RESOURCE_END'.
Please describe the WHY not the WHAT. I can see the WHAT from the patch
itself, but I can't figure out WHY.
Thanks,
tglx
On Fri, Aug 16, 2019 at 12:24:27PM +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.
>
> Changes in v2:
> - Reverse checks in pci_iomap_range,pci_iomap_wc_range.
> - Refactor loops in vfio_pci to keep PCI_STD_RESOURCES.
> - Add 2 new patches to replace the magic constant with new define.
> - Split net patch in v1 to separate stmmac and dwc-xlgmac patches.
>
> Denis Efremov (10):
> PCI: Add define for the number of standard PCI BARs
> s390/pci: Loop using PCI_STD_NUM_BARS
> x86/PCI: Loop using PCI_STD_NUM_BARS
> stmmac: pci: Loop using PCI_STD_NUM_BARS
> net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
> rapidio/tsi721: Loop using PCI_STD_NUM_BARS
> efifb: Loop using PCI_STD_NUM_BARS
> vfio_pci: Loop using PCI_STD_NUM_BARS
> PCI: hv: Use PCI_STD_NUM_BARS
> PCI: Use PCI_STD_NUM_BARS
>
> 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/controller/pci-hyperv.c | 10 +++++-----
> drivers/pci/pci.c | 11 ++++++-----
> drivers/pci/quirks.c | 4 ++--
> drivers/rapidio/devices/tsi721.c | 2 +-
> drivers/vfio/pci/vfio_pci.c | 11 +++++++----
> drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
> 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 +
> 17 files changed, 51 insertions(+), 47 deletions(-)
I've come across a few more places where this change can be made. There
may be multiple instances in the same file, but only the first is shown
below:
drivers/misc/pci_endpoint_test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
drivers/net/ethernet/intel/e1000/e1000_main.c: for (i = BAR_1; i <= BAR_5; i++) {
drivers/net/ethernet/intel/ixgb/ixgb_main.c: for (i = BAR_1; i <= BAR_5; i++) {
drivers/pci/controller/dwc/pci-dra7xx.c: for (bar = BAR_0; bar <= BAR_5; bar++)
drivers/pci/controller/dwc/pci-layerscape-ep.c: for (bar = BAR_0; bar <= BAR_5; bar++)
drivers/pci/controller/dwc/pcie-artpec6.c: for (bar = BAR_0; bar <= BAR_5; bar++)
drivers/pci/controller/dwc/pcie-designware-plat.c: for (bar = BAR_0; bar <= BAR_5; bar++)
drivers/pci/endpoint/functions/pci-epf-test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
include/linux/pci-epc.h: u64 bar_fixed_size[BAR_5 + 1];
drivers/scsi/pm8001/pm8001_hwi.c: for (bar = 0; bar < 6; bar++) {
drivers/scsi/pm8001/pm8001_init.c: for (bar = 0; bar < 6; bar++) {
drivers/ata/sata_nv.c: for (bar = 0; bar < 6; bar++)
drivers/video/fbdev/core/fbmem.c: for (idx = 0, bar = 0; bar < PCI_ROM_RESOURCE; bar++) {
drivers/staging/gasket/gasket_core.c: for (i = 0; i < GASKET_NUM_BARS; i++) {
drivers/tty/serial/8250/8250_pci.c: for (i = 0; i < PCI_NUM_BAR_RESOURCES; i++) { <-----------
It looks like BARs are often iterated with PCI_NUM_BAR_RESOURCES, there
are a load of these too found with:
git grep PCI_ROM_RESOURCE | grep "< "
I'm happy to share patches if preferred.
Thanks,
Andrew Murray
>
> --
> 2.21.0
>
On Fri, Aug 16, 2019 at 11:32:41AM +0200, Thomas Gleixner wrote:
> On Fri, 16 Aug 2019, Denis Efremov wrote:
>
> > Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
> > 'i <= PCI_STD_RESOURCE_END'.
>
> Please describe the WHY not the WHAT. I can see the WHAT from the patch
> itself, but I can't figure out WHY.
Good point; the WHY is to use idiomatic C style and avoid
the fencepost error of using "i < PCI_STD_RESOURCE_END"
when "i <= PCI_STD_RESOURCE_END" is required, e.g.,
2f686f1d9bee ("PCI: Correct PCI_STD_RESOURCE_END usage")
Denis, can you include something along those lines in the next
version?
On Fri, Aug 16, 2019 at 11:51:28AM +0100, Andrew Murray wrote:
> On Fri, Aug 16, 2019 at 12:24:27PM +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.
> >
> > Changes in v2:
> > - Reverse checks in pci_iomap_range,pci_iomap_wc_range.
> > - Refactor loops in vfio_pci to keep PCI_STD_RESOURCES.
> > - Add 2 new patches to replace the magic constant with new define.
> > - Split net patch in v1 to separate stmmac and dwc-xlgmac patches.
> >
> > Denis Efremov (10):
> > PCI: Add define for the number of standard PCI BARs
> > s390/pci: Loop using PCI_STD_NUM_BARS
> > x86/PCI: Loop using PCI_STD_NUM_BARS
> > stmmac: pci: Loop using PCI_STD_NUM_BARS
> > net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
> > rapidio/tsi721: Loop using PCI_STD_NUM_BARS
> > efifb: Loop using PCI_STD_NUM_BARS
> > vfio_pci: Loop using PCI_STD_NUM_BARS
> > PCI: hv: Use PCI_STD_NUM_BARS
> > PCI: Use PCI_STD_NUM_BARS
> >
> > 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/controller/pci-hyperv.c | 10 +++++-----
> > drivers/pci/pci.c | 11 ++++++-----
> > drivers/pci/quirks.c | 4 ++--
> > drivers/rapidio/devices/tsi721.c | 2 +-
> > drivers/vfio/pci/vfio_pci.c | 11 +++++++----
> > drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
> > 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 +
> > 17 files changed, 51 insertions(+), 47 deletions(-)
>
> I've come across a few more places where this change can be made. There
> may be multiple instances in the same file, but only the first is shown
> below:
>
> drivers/misc/pci_endpoint_test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
> drivers/net/ethernet/intel/e1000/e1000_main.c: for (i = BAR_1; i <= BAR_5; i++) {
> drivers/net/ethernet/intel/ixgb/ixgb_main.c: for (i = BAR_1; i <= BAR_5; i++) {
> drivers/pci/controller/dwc/pci-dra7xx.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pci-layerscape-ep.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pcie-artpec6.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pcie-designware-plat.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/endpoint/functions/pci-epf-test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
> include/linux/pci-epc.h: u64 bar_fixed_size[BAR_5 + 1];
> drivers/scsi/pm8001/pm8001_hwi.c: for (bar = 0; bar < 6; bar++) {
> drivers/scsi/pm8001/pm8001_init.c: for (bar = 0; bar < 6; bar++) {
> drivers/ata/sata_nv.c: for (bar = 0; bar < 6; bar++)
> drivers/video/fbdev/core/fbmem.c: for (idx = 0, bar = 0; bar < PCI_ROM_RESOURCE; bar++) {
> drivers/staging/gasket/gasket_core.c: for (i = 0; i < GASKET_NUM_BARS; i++) {
> drivers/tty/serial/8250/8250_pci.c: for (i = 0; i < PCI_NUM_BAR_RESOURCES; i++) { <-----------
Thanks, I agree, these look like good candidates as well.
> It looks like BARs are often iterated with PCI_NUM_BAR_RESOURCES, there
> are a load of these too found with:
>
> git grep PCI_ROM_RESOURCE | grep "< "
Good point, those are slightly questionable and I'd change those too.
Bjorn
On Fri, 16 Aug 2019 12:24:35 +0300
Denis Efremov <[email protected]> wrote:
> Refactor loops to use 'i < PCI_STD_NUM_BARS' instead of
> 'i <= PCI_STD_RESOURCE_END'.
>
> Signed-off-by: Denis Efremov <[email protected]>
> ---
> drivers/vfio/pci/vfio_pci.c | 11 +++++++----
> drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
> drivers/vfio/pci/vfio_pci_private.h | 4 ++--
> 3 files changed, 15 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c
> index 703948c9fbe1..cb7d220d3246 100644
> --- a/drivers/vfio/pci/vfio_pci.c
> +++ b/drivers/vfio/pci/vfio_pci.c
> @@ -110,13 +110,15 @@ static inline bool vfio_pci_is_vga(struct pci_dev *pdev)
> static void vfio_pci_probe_mmaps(struct vfio_pci_device *vdev)
> {
> struct resource *res;
> - int bar;
> + int i;
> struct vfio_pci_dummy_resource *dummy_res;
>
> INIT_LIST_HEAD(&vdev->dummy_resources_list);
>
> - for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
> - res = vdev->pdev->resource + bar;
> + for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> + int bar = i + PCI_STD_RESOURCES;
> +
> + res = &vdev->pdev->resource[bar];
>
> if (!IS_ENABLED(CONFIG_VFIO_PCI_MMAP))
> goto no_mmap;
> @@ -399,7 +401,8 @@ static void vfio_pci_disable(struct vfio_pci_device *vdev)
>
> vfio_config_free(vdev);
>
> - for (bar = PCI_STD_RESOURCES; bar <= PCI_STD_RESOURCE_END; bar++) {
> + for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> + bar = i + PCI_STD_RESOURCES;
> if (!vdev->barmap[bar])
> continue;
> pci_iounmap(pdev, vdev->barmap[bar]);
> diff --git a/drivers/vfio/pci/vfio_pci_config.c b/drivers/vfio/pci/vfio_pci_config.c
> index f0891bd8444c..df8772395219 100644
> --- a/drivers/vfio/pci/vfio_pci_config.c
> +++ b/drivers/vfio/pci/vfio_pci_config.c
> @@ -455,16 +455,18 @@ static void vfio_bar_fixup(struct vfio_pci_device *vdev)
>
> bar = (__le32 *)&vdev->vconfig[PCI_BASE_ADDRESS_0];
>
> - for (i = PCI_STD_RESOURCES; i <= PCI_STD_RESOURCE_END; i++, bar++) {
> - if (!pci_resource_start(pdev, i)) {
> + for (i = 0; i < PCI_STD_NUM_BARS; i++, bar++) {
> + int ibar = i + PCI_STD_RESOURCES;
> +
> + if (!pci_resource_start(pdev, ibar)) {
> *bar = 0; /* Unmapped by host = unimplemented to user */
> continue;
> }
>
> - mask = ~(pci_resource_len(pdev, i) - 1);
> + mask = ~(pci_resource_len(pdev, ibar) - 1);
>
> *bar &= cpu_to_le32((u32)mask);
> - *bar |= vfio_generate_bar_flags(pdev, i);
> + *bar |= vfio_generate_bar_flags(pdev, ibar);
>
> if (*bar & cpu_to_le32(PCI_BASE_ADDRESS_MEM_TYPE_64)) {
> bar++;
It might be a bit cleaner to rename the 'bar' variable to 'vbar', then
we have 'bar' available to use as the BAR number. It seems more
consistent with other uses. Otherwise the logic looks fine. Thanks,
Alex
> diff --git a/drivers/vfio/pci/vfio_pci_private.h b/drivers/vfio/pci/vfio_pci_private.h
> index ee6ee91718a4..8a2c7607d513 100644
> --- a/drivers/vfio/pci/vfio_pci_private.h
> +++ b/drivers/vfio/pci/vfio_pci_private.h
> @@ -86,8 +86,8 @@ struct vfio_pci_reflck {
>
> struct vfio_pci_device {
> struct pci_dev *pdev;
> - void __iomem *barmap[PCI_STD_RESOURCE_END + 1];
> - bool bar_mmap_supported[PCI_STD_RESOURCE_END + 1];
> + void __iomem *barmap[PCI_STD_NUM_BARS];
> + bool bar_mmap_supported[PCI_STD_NUM_BARS];
> u8 *pci_config_map;
> u8 *vconfig;
> struct perm_bits *msi_perm;
On 16.08.2019 13:51, Andrew Murray wrote:
> On Fri, Aug 16, 2019 at 12:24:27PM +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.
>>
>> Changes in v2:
>> - Reverse checks in pci_iomap_range,pci_iomap_wc_range.
>> - Refactor loops in vfio_pci to keep PCI_STD_RESOURCES.
>> - Add 2 new patches to replace the magic constant with new define.
>> - Split net patch in v1 to separate stmmac and dwc-xlgmac patches.
>>
>> Denis Efremov (10):
>> PCI: Add define for the number of standard PCI BARs
>> s390/pci: Loop using PCI_STD_NUM_BARS
>> x86/PCI: Loop using PCI_STD_NUM_BARS
>> stmmac: pci: Loop using PCI_STD_NUM_BARS
>> net: dwc-xlgmac: Loop using PCI_STD_NUM_BARS
>> rapidio/tsi721: Loop using PCI_STD_NUM_BARS
>> efifb: Loop using PCI_STD_NUM_BARS
>> vfio_pci: Loop using PCI_STD_NUM_BARS
>> PCI: hv: Use PCI_STD_NUM_BARS
>> PCI: Use PCI_STD_NUM_BARS
>>
>> 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/controller/pci-hyperv.c | 10 +++++-----
>> drivers/pci/pci.c | 11 ++++++-----
>> drivers/pci/quirks.c | 4 ++--
>> drivers/rapidio/devices/tsi721.c | 2 +-
>> drivers/vfio/pci/vfio_pci.c | 11 +++++++----
>> drivers/vfio/pci/vfio_pci_config.c | 10 ++++++----
>> 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 +
>> 17 files changed, 51 insertions(+), 47 deletions(-)
>
> I've come across a few more places where this change can be made. There
> may be multiple instances in the same file, but only the first is shown
> below:
>
> drivers/misc/pci_endpoint_test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
> drivers/net/ethernet/intel/e1000/e1000_main.c: for (i = BAR_1; i <= BAR_5; i++) {
> drivers/net/ethernet/intel/ixgb/ixgb_main.c: for (i = BAR_1; i <= BAR_5; i++) {
> drivers/pci/controller/dwc/pci-dra7xx.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pci-layerscape-ep.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pcie-artpec6.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/controller/dwc/pcie-designware-plat.c: for (bar = BAR_0; bar <= BAR_5; bar++)
> drivers/pci/endpoint/functions/pci-epf-test.c: for (bar = BAR_0; bar <= BAR_5; bar++) {
> include/linux/pci-epc.h: u64 bar_fixed_size[BAR_5 + 1];
> drivers/scsi/pm8001/pm8001_hwi.c: for (bar = 0; bar < 6; bar++) {
> drivers/scsi/pm8001/pm8001_init.c: for (bar = 0; bar < 6; bar++) {
> drivers/ata/sata_nv.c: for (bar = 0; bar < 6; bar++)
> drivers/video/fbdev/core/fbmem.c: for (idx = 0, bar = 0; bar < PCI_ROM_RESOURCE; bar++) {
> drivers/staging/gasket/gasket_core.c: for (i = 0; i < GASKET_NUM_BARS; i++) {
> drivers/tty/serial/8250/8250_pci.c: for (i = 0; i < PCI_NUM_BAR_RESOURCES; i++) { <-----------
>
> It looks like BARs are often iterated with PCI_NUM_BAR_RESOURCES, there
> are a load of these too found with:
>
> git grep PCI_ROM_RESOURCE | grep "< "
>
> I'm happy to share patches if preferred.
>
I'm not sure about lib/devres.c
265:#define PCIM_IOMAP_MAX PCI_ROM_RESOURCE
268: void __iomem *table[PCIM_IOMAP_MAX];
277: for (i = 0; i < PCIM_IOMAP_MAX; i++)
324: BUG_ON(bar >= PCIM_IOMAP_MAX);
352: for (i = 0; i < PCIM_IOMAP_MAX; i++)
455: for (i = 0; i < PCIM_IOMAP_MAX; i++) {
Is it worth changing?
#define PCIM_IOMAP_MAX PCI_STD_NUM_BARS
Thanks,
Denis