[PATCH 0/3] PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices

[PATCH 0/3] PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices

[Kishon Vijay Abraham I]

AM64 has an issue in that it doesn't trigger interrupt if the address
in the *pre_its_window* is not aligned to 8-bytes (this is due to an
invalid bridge configuration in HW).

This means there will not be interrupts for devices with PCIe
requestor ID 0x1, 0x3, 0x5..., as the address in the pre-ITS window
would be 4 (1 << 2), 12 (3 << 2), 20 (5 << 2) respectively which are
not aligned to 8-bytes.

The DT binding has specified "msi-map-mask" using which multiple PCIe
devices could be made to use the same ITS device ID.

Add support in irq-gic-v3-its-pci-msi.c for such cases where multiple
PCIe devices are using the same ITS device ID.

Kishon Vijay Abraham I (3):
  PCI: Add support in pci_walk_bus() to invoke callback matching RID
  PCI: Export find_pci_root_bus()
  irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs

 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
 drivers/pci/bus.c                        | 13 +++++++++----
 drivers/pci/host-bridge.c                |  3 ++-
 include/linux/pci.h                      |  8 ++++++--
 4 files changed, 37 insertions(+), 8 deletions(-)

-- 
2.17.1

[PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Kishon Vijay Abraham I]

Add two arguments to pci_walk_bus() [requestorID and mask], and add
support in pci_walk_bus() to invoke the *callback* only for devices
whose RequestorID after applying *mask* matches with *requestorID*
passed as argument.

This is done in preparation for calculating the total number of
interrupt vectors that has to be supported by a specific GIC ITS device ID,
specifically when "msi-map-mask" is populated in device tree.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/bus.c   | 13 +++++++++----
 include/linux/pci.h |  7 +++++--
 2 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
index 3cef835b375f..e381e639ceaa 100644
--- a/drivers/pci/bus.c
+++ b/drivers/pci/bus.c
@@ -358,10 +358,12 @@ void pci_bus_add_devices(const struct pci_bus *bus)
 }
 EXPORT_SYMBOL(pci_bus_add_devices);
 
-/** pci_walk_bus - walk devices on/under bus, calling callback.
+/** __pci_walk_bus - walk devices on/under bus matching requestor ID, calling callback.
  *  @top      bus whose devices should be walked
  *  @cb       callback to be called for each device found
  *  @userdata arbitrary pointer to be passed to callback.
+ *  @rid      Requestor ID that has to be matched for the callback to be invoked
+ *  @mask     Mask that has to be applied to pci_dev_id(), before compating it with @rid
  *
  *  Walk the given bus, including any bridged devices
  *  on buses under this bus.  Call the provided callback
@@ -371,8 +373,8 @@ EXPORT_SYMBOL(pci_bus_add_devices);
  *  other than 0, we break out.
  *
  */
-void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
-		  void *userdata)
+void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
+		    void *userdata, u32 rid, u32 mask)
 {
 	struct pci_dev *dev;
 	struct pci_bus *bus;
@@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 		} else
 			next = dev->bus_list.next;
 
+		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
+			continue;
+
 		retval = cb(dev, userdata);
 		if (retval)
 			break;
 	}
 	up_read(&pci_bus_sem);
 }
-EXPORT_SYMBOL_GPL(pci_walk_bus);
+EXPORT_SYMBOL_GPL(__pci_walk_bus);
 
 struct pci_bus *pci_bus_get(struct pci_bus *bus)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index cd8aa6fce204..8500fec56e50 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1473,14 +1473,17 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
 int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 		    int pass);
 
-void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
-		  void *userdata);
+void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
+		    void *userdata, u32 rid, u32 mask);
 int pci_cfg_space_size(struct pci_dev *dev);
 unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 void pci_setup_bridge(struct pci_bus *bus);
 resource_size_t pcibios_window_alignment(struct pci_bus *bus,
 					 unsigned long type);
 
+#define pci_walk_bus(top, cb, userdata) \
+	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
+
 #define PCI_VGA_STATE_CHANGE_BRIDGE (1 << 0)
 #define PCI_VGA_STATE_CHANGE_DECODES (1 << 1)
 
-- 
2.17.1

[PATCH 2/3] PCI: Export find_pci_root_bus()

[Kishon Vijay Abraham I]

Export find_pci_root_bus() in order for other subsystems (like
IRQCHIP) to find the root bus of a particual PCIe device.

This is done in preparation for GIC ITS to walk the PCIe bus for
calculating the total number of interrupt vectors that has to be
supported by a specific GIC ITS device ID, specifically when
"msi-map-mask" is populated in device tree.

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/pci/host-bridge.c | 3 ++-
 include/linux/pci.h       | 1 +
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
index afa50b446567..4ec34d040c02 100644
--- a/drivers/pci/host-bridge.c
+++ b/drivers/pci/host-bridge.c
@@ -9,13 +9,14 @@
 
 #include "pci.h"
 
-static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
+struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
 {
 	while (bus->parent)
 		bus = bus->parent;
 
 	return bus;
 }
+EXPORT_SYMBOL_GPL(find_pci_root_bus);
 
 struct pci_host_bridge *pci_find_host_bridge(struct pci_bus *bus)
 {
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 8500fec56e50..b33ef3e08a2f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1475,6 +1475,7 @@ int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
 
 void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
 		    void *userdata, u32 rid, u32 mask);
+struct pci_bus *find_pci_root_bus(struct pci_bus *bus);
 int pci_cfg_space_size(struct pci_dev *dev);
 unsigned char pci_bus_max_busnr(struct pci_bus *bus);
 void pci_setup_bridge(struct pci_bus *bus);
-- 
2.17.1

[PATCH 3/3] irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs

[Kishon Vijay Abraham I]

Using "msi-map-mask" in device tree lets multiple PCIe requestor ID to
use the same GIC ITS device ID. So while creating the Interrupt
Translation Table (ITT) for a specific GIC ITS device ID, the total number
of interrupts required by all the PCIe requestor ID that maps to the
same GIC ITS device ID should be calculated

Add support for gic-v3-its to include "msi-map-mask" property in device
tree for calculating the total number of MSI interrupts in
its_pci_msi_prepare().

Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
index ad2810c017ed..c79bca1a5787 100644
--- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
+++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
@@ -54,9 +54,13 @@ static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
 static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 			       int nvec, msi_alloc_info_t *info)
 {
+	int alias_count = 0, map_count = 0, minnvec = 1, ret;
 	struct pci_dev *pdev, *alias_dev;
 	struct msi_domain_info *msi_info;
-	int alias_count = 0, minnvec = 1;
+	struct device *parent_dev;
+	struct pci_bus *root_bus;
+	struct device_node *np;
+	u32 map_mask, rid;
 
 	if (!dev_is_pci(dev))
 		return -EINVAL;
@@ -78,6 +82,21 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
 		info->flags |= MSI_ALLOC_FLAGS_PROXY_DEVICE;
 	}
 
+	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {
+		np = parent_dev->of_node;
+		if (!np)
+			continue;
+
+		ret = of_property_read_u32(np, "msi-map-mask", &map_mask);
+		if (!ret && map_mask != 0xffff) {
+			rid = pci_dev_id(pdev) & map_mask;
+			root_bus = find_pci_root_bus(pdev->bus);
+			__pci_walk_bus(root_bus, its_pci_msi_vec_count, &map_count, rid, map_mask);
+			break;
+		}
+	}
+	alias_count = max(map_count, alias_count);
+
 	/* ITS specific DeviceID, as the core ITS ignores dev. */
 	info->scratchpad[0].ul = pci_msi_domain_get_msi_rid(domain, pdev);
 
-- 
2.17.1

Re: [PATCH 3/3] irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs

[Marc Zyngier]

+ Robin, who has dealt with a lot of the stuff here.

On Mon, 20 Sep 2021 07:41:33 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Using "msi-map-mask" in device tree lets multiple PCIe requestor ID to
> use the same GIC ITS device ID. So while creating the Interrupt
> Translation Table (ITT) for a specific GIC ITS device ID, the total number
> of interrupts required by all the PCIe requestor ID that maps to the
> same GIC ITS device ID should be calculated
> 
> Add support for gic-v3-its to include "msi-map-mask" property in device
> tree for calculating the total number of MSI interrupts in
> its_pci_msi_prepare().
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
>  1 file changed, 20 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its-pci-msi.c b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> index ad2810c017ed..c79bca1a5787 100644
> --- a/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> +++ b/drivers/irqchip/irq-gic-v3-its-pci-msi.c
> @@ -54,9 +54,13 @@ static int its_get_pci_alias(struct pci_dev *pdev, u16 alias, void *data)
>  static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  			       int nvec, msi_alloc_info_t *info)
>  {
> +	int alias_count = 0, map_count = 0, minnvec = 1, ret;
>  	struct pci_dev *pdev, *alias_dev;
>  	struct msi_domain_info *msi_info;
> -	int alias_count = 0, minnvec = 1;
> +	struct device *parent_dev;
> +	struct pci_bus *root_bus;
> +	struct device_node *np;
> +	u32 map_mask, rid;
>  
>  	if (!dev_is_pci(dev))
>  		return -EINVAL;
> @@ -78,6 +82,21 @@ static int its_pci_msi_prepare(struct irq_domain *domain, struct device *dev,
>  		info->flags |= MSI_ALLOC_FLAGS_PROXY_DEVICE;
>  	}
>  
> +	for (parent_dev = dev; parent_dev; parent_dev = parent_dev->parent) {

Move the declaration of np, ret and map_count here.

> +		np = parent_dev->of_node;
> +		if (!np)
> +			continue;
> +
> +		ret = of_property_read_u32(np, "msi-map-mask", &map_mask);
> +		if (!ret && map_mask != 0xffff) {
> +			rid = pci_dev_id(pdev) & map_mask;
> +			root_bus = find_pci_root_bus(pdev->bus);
> +			__pci_walk_bus(root_bus, its_pci_msi_vec_count, &map_count, rid, map_mask);
> +			break;
> +		}
> +	}
> +	alias_count = max(map_count, alias_count);

Move this after the call to __pci_walk_bus().

> +
>  	/* ITS specific DeviceID, as the core ITS ignores dev. */
>  	info->scratchpad[0].ul = pci_msi_domain_get_msi_rid(domain, pdev);

If you don't set the PROXY_DEVICE flag, bad thing will happen when the
canonical device is removed.

Ultimately, this should be moved into a separate helper, along the
line of pci_for_each_dma_alias(). It probably also needs to have an
ACPI counterpart.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 2/3] PCI: Export find_pci_root_bus()

[Marc Zyngier]

On Mon, 20 Sep 2021 07:41:32 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Export find_pci_root_bus() in order for other subsystems (like
> IRQCHIP) to find the root bus of a particual PCIe device.
> 
> This is done in preparation for GIC ITS to walk the PCIe bus for
> calculating the total number of interrupt vectors that has to be
> supported by a specific GIC ITS device ID, specifically when
> "msi-map-mask" is populated in device tree.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/host-bridge.c | 3 ++-
>  include/linux/pci.h       | 1 +
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/host-bridge.c b/drivers/pci/host-bridge.c
> index afa50b446567..4ec34d040c02 100644
> --- a/drivers/pci/host-bridge.c
> +++ b/drivers/pci/host-bridge.c
> @@ -9,13 +9,14 @@
>  
>  #include "pci.h"
>  
> -static struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
> +struct pci_bus *find_pci_root_bus(struct pci_bus *bus)
>  {
>  	while (bus->parent)
>  		bus = bus->parent;
>  
>  	return bus;
>  }
> +EXPORT_SYMBOL_GPL(find_pci_root_bus);

There is no need for this export as the ITS cannot be made modular.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Marc Zyngier]

On Mon, 20 Sep 2021 07:41:31 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Add two arguments to pci_walk_bus() [requestorID and mask], and add
> support in pci_walk_bus() to invoke the *callback* only for devices
> whose RequestorID after applying *mask* matches with *requestorID*
> passed as argument.
> 
> This is done in preparation for calculating the total number of
> interrupt vectors that has to be supported by a specific GIC ITS device ID,
> specifically when "msi-map-mask" is populated in device tree.
> 
> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/pci/bus.c   | 13 +++++++++----
>  include/linux/pci.h |  7 +++++--
>  2 files changed, 14 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> index 3cef835b375f..e381e639ceaa 100644
> --- a/drivers/pci/bus.c
> +++ b/drivers/pci/bus.c
> @@ -358,10 +358,12 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>  }
>  EXPORT_SYMBOL(pci_bus_add_devices);
>  
> -/** pci_walk_bus - walk devices on/under bus, calling callback.
> +/** __pci_walk_bus - walk devices on/under bus matching requestor ID, calling callback.
>   *  @top      bus whose devices should be walked
>   *  @cb       callback to be called for each device found
>   *  @userdata arbitrary pointer to be passed to callback.
> + *  @rid      Requestor ID that has to be matched for the callback to be invoked
> + *  @mask     Mask that has to be applied to pci_dev_id(), before compating it with @rid
>   *
>   *  Walk the given bus, including any bridged devices
>   *  on buses under this bus.  Call the provided callback
> @@ -371,8 +373,8 @@ EXPORT_SYMBOL(pci_bus_add_devices);
>   *  other than 0, we break out.
>   *
>   */
> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> -		  void *userdata)
> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata, u32 rid, u32 mask)
>  {
>  	struct pci_dev *dev;
>  	struct pci_bus *bus;
> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>  		} else
>  			next = dev->bus_list.next;
>  
> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))

Why the check for the mask? I also wonder whether the mask should apply
to the rid as well:

		if ((pci_dev_id(dev) & mask) != (rid & mask))

> +			continue;
> +
>  		retval = cb(dev, userdata);
>  		if (retval)
>  			break;
>  	}
>  	up_read(&pci_bus_sem);
>  }
> -EXPORT_SYMBOL_GPL(pci_walk_bus);
> +EXPORT_SYMBOL_GPL(__pci_walk_bus);
>  
>  struct pci_bus *pci_bus_get(struct pci_bus *bus)
>  {
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index cd8aa6fce204..8500fec56e50 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1473,14 +1473,17 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>  		    int pass);
>  
> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> -		  void *userdata);
> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> +		    void *userdata, u32 rid, u32 mask);
>  int pci_cfg_space_size(struct pci_dev *dev);
>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>  void pci_setup_bridge(struct pci_bus *bus);
>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
>  					 unsigned long type);
>  
> +#define pci_walk_bus(top, cb, userdata) \
> +	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)

Please keep this close to the helper it replaces. I also really
dislike the use of this raw 0xffff. Don't we already have a named
constant that represents the mask for a RID?

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices

[Marc Zyngier]

On Mon, 20 Sep 2021 07:41:30 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> AM64 has an issue in that it doesn't trigger interrupt if the address
> in the *pre_its_window* is not aligned to 8-bytes (this is due to an
> invalid bridge configuration in HW).
> 
> This means there will not be interrupts for devices with PCIe
> requestor ID 0x1, 0x3, 0x5..., as the address in the pre-ITS window
> would be 4 (1 << 2), 12 (3 << 2), 20 (5 << 2) respectively which are
> not aligned to 8-bytes.
> 
> The DT binding has specified "msi-map-mask" using which multiple PCIe
> devices could be made to use the same ITS device ID.
> 
> Add support in irq-gic-v3-its-pci-msi.c for such cases where multiple
> PCIe devices are using the same ITS device ID.
> 
> Kishon Vijay Abraham I (3):
>   PCI: Add support in pci_walk_bus() to invoke callback matching RID
>   PCI: Export find_pci_root_bus()
>   irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs
> 
>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
>  drivers/pci/bus.c                        | 13 +++++++++----
>  drivers/pci/host-bridge.c                |  3 ++-
>  include/linux/pci.h                      |  8 ++++++--
>  4 files changed, 37 insertions(+), 8 deletions(-)

What I don't see in this series is how you address the other part of
the problem, which is your reuse of the Socionext hack. Please post a
complete series addressing all the issues for this HW.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 0/3] PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices

[Kishon Vijay Abraham I]

Hi Marc,

On 20/09/21 2:44 pm, Marc Zyngier wrote:
> On Mon, 20 Sep 2021 07:41:30 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> AM64 has an issue in that it doesn't trigger interrupt if the address
>> in the *pre_its_window* is not aligned to 8-bytes (this is due to an
>> invalid bridge configuration in HW).
>>
>> This means there will not be interrupts for devices with PCIe
>> requestor ID 0x1, 0x3, 0x5..., as the address in the pre-ITS window
>> would be 4 (1 << 2), 12 (3 << 2), 20 (5 << 2) respectively which are
>> not aligned to 8-bytes.
>>
>> The DT binding has specified "msi-map-mask" using which multiple PCIe
>> devices could be made to use the same ITS device ID.
>>
>> Add support in irq-gic-v3-its-pci-msi.c for such cases where multiple
>> PCIe devices are using the same ITS device ID.
>>
>> Kishon Vijay Abraham I (3):
>>   PCI: Add support in pci_walk_bus() to invoke callback matching RID
>>   PCI: Export find_pci_root_bus()
>>   irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs
>>
>>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
>>  drivers/pci/bus.c                        | 13 +++++++++----
>>  drivers/pci/host-bridge.c                |  3 ++-
>>  include/linux/pci.h                      |  8 ++++++--
>>  4 files changed, 37 insertions(+), 8 deletions(-)
> 
> What I don't see in this series is how you address the other part of
> the problem, which is your reuse of the Socionext hack. Please post a
> complete series addressing all the issues for this HW.

No additional patches are pending. Socionext configuration is used as is
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am64-main.dtsi#n72

FWIW the issue that I address in this series is not observed with standalone USB
cards or NVMe cards. The issue was observed when I tried with a multi-function
PCIe card.

Thank You,
Kishon

Re: [PATCH 0/3] PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices

[Marc Zyngier]

On Mon, 20 Sep 2021 12:22:48 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Hi Marc,
> 
> On 20/09/21 2:44 pm, Marc Zyngier wrote:
> > On Mon, 20 Sep 2021 07:41:30 +0100,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> AM64 has an issue in that it doesn't trigger interrupt if the address
> >> in the *pre_its_window* is not aligned to 8-bytes (this is due to an
> >> invalid bridge configuration in HW).
> >>
> >> This means there will not be interrupts for devices with PCIe
> >> requestor ID 0x1, 0x3, 0x5..., as the address in the pre-ITS window
> >> would be 4 (1 << 2), 12 (3 << 2), 20 (5 << 2) respectively which are
> >> not aligned to 8-bytes.
> >>
> >> The DT binding has specified "msi-map-mask" using which multiple PCIe
> >> devices could be made to use the same ITS device ID.
> >>
> >> Add support in irq-gic-v3-its-pci-msi.c for such cases where multiple
> >> PCIe devices are using the same ITS device ID.
> >>
> >> Kishon Vijay Abraham I (3):
> >>   PCI: Add support in pci_walk_bus() to invoke callback matching RID
> >>   PCI: Export find_pci_root_bus()
> >>   irqchip/gic-v3-its: Include "msi-map-mask" for calculating nvecs
> >>
> >>  drivers/irqchip/irq-gic-v3-its-pci-msi.c | 21 ++++++++++++++++++++-
> >>  drivers/pci/bus.c                        | 13 +++++++++----
> >>  drivers/pci/host-bridge.c                |  3 ++-
> >>  include/linux/pci.h                      |  8 ++++++--
> >>  4 files changed, 37 insertions(+), 8 deletions(-)
> > 
> > What I don't see in this series is how you address the other part of
> > the problem, which is your reuse of the Socionext hack. Please post a
> > complete series addressing all the issues for this HW.
> 
> No additional patches are pending. Socionext configuration is used as is
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/ti/k3-am64-main.dtsi#n72

Hmmm. You are pretty lucky that the Socionext machine is also using a
GIC-500, and that your pre-ITS widget falls into the low 4GB...

> FWIW the issue that I address in this series is not observed with
> standalone USB cards or NVMe cards. The issue was observed when I
> tried with a multi-function PCIe card.

That's indeed what I understood of the bug.

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Kishon Vijay Abraham I]

Hi Marc,

On 20/09/21 2:26 pm, Marc Zyngier wrote:
> On Mon, 20 Sep 2021 07:41:31 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Add two arguments to pci_walk_bus() [requestorID and mask], and add
>> support in pci_walk_bus() to invoke the *callback* only for devices
>> whose RequestorID after applying *mask* matches with *requestorID*
>> passed as argument.
>>
>> This is done in preparation for calculating the total number of
>> interrupt vectors that has to be supported by a specific GIC ITS device ID,
>> specifically when "msi-map-mask" is populated in device tree.
>>
>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>> ---
>>  drivers/pci/bus.c   | 13 +++++++++----
>>  include/linux/pci.h |  7 +++++--
>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>> index 3cef835b375f..e381e639ceaa 100644
>> --- a/drivers/pci/bus.c
>> +++ b/drivers/pci/bus.c
>> @@ -358,10 +358,12 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>>  }
>>  EXPORT_SYMBOL(pci_bus_add_devices);
>>  
>> -/** pci_walk_bus - walk devices on/under bus, calling callback.
>> +/** __pci_walk_bus - walk devices on/under bus matching requestor ID, calling callback.
>>   *  @top      bus whose devices should be walked
>>   *  @cb       callback to be called for each device found
>>   *  @userdata arbitrary pointer to be passed to callback.
>> + *  @rid      Requestor ID that has to be matched for the callback to be invoked
>> + *  @mask     Mask that has to be applied to pci_dev_id(), before compating it with @rid
>>   *
>>   *  Walk the given bus, including any bridged devices
>>   *  on buses under this bus.  Call the provided callback
>> @@ -371,8 +373,8 @@ EXPORT_SYMBOL(pci_bus_add_devices);
>>   *  other than 0, we break out.
>>   *
>>   */
>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>> -		  void *userdata)
>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>> +		    void *userdata, u32 rid, u32 mask)
>>  {
>>  	struct pci_dev *dev;
>>  	struct pci_bus *bus;
>> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>  		} else
>>  			next = dev->bus_list.next;
>>  
>> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
> 
> Why the check for the mask? I also wonder whether the mask should apply
> to the rid as well:

If the mask is set for all 16bits, then there is not going to be two PCIe
devices which gets the same ITS device ID right? So no need for calculating
total number of vectors?
> 
> 		if ((pci_dev_id(dev) & mask) != (rid & mask))
> 
>> +			continue;
>> +
>>  		retval = cb(dev, userdata);
>>  		if (retval)
>>  			break;
>>  	}
>>  	up_read(&pci_bus_sem);
>>  }
>> -EXPORT_SYMBOL_GPL(pci_walk_bus);
>> +EXPORT_SYMBOL_GPL(__pci_walk_bus);
>>  
>>  struct pci_bus *pci_bus_get(struct pci_bus *bus)
>>  {
>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>> index cd8aa6fce204..8500fec56e50 100644
>> --- a/include/linux/pci.h
>> +++ b/include/linux/pci.h
>> @@ -1473,14 +1473,17 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>>  		    int pass);
>>  
>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>> -		  void *userdata);
>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>> +		    void *userdata, u32 rid, u32 mask);
>>  int pci_cfg_space_size(struct pci_dev *dev);
>>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>>  void pci_setup_bridge(struct pci_bus *bus);
>>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
>>  					 unsigned long type);
>>  
>> +#define pci_walk_bus(top, cb, userdata) \
>> +	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
> 
> Please keep this close to the helper it replaces. I also really
> dislike the use of this raw 0xffff. Don't we already have a named
> constant that represents the mask for a RID?

I didn't find one on quick look but let me check.

Thanks,
Kishon

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Marc Zyngier]

On Mon, 20 Sep 2021 15:28:52 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> Hi Marc,
> 
> On 20/09/21 2:26 pm, Marc Zyngier wrote:
> > On Mon, 20 Sep 2021 07:41:31 +0100,
> > Kishon Vijay Abraham I <kishon@ti.com> wrote:
> >>
> >> Add two arguments to pci_walk_bus() [requestorID and mask], and add
> >> support in pci_walk_bus() to invoke the *callback* only for devices
> >> whose RequestorID after applying *mask* matches with *requestorID*
> >> passed as argument.
> >>
> >> This is done in preparation for calculating the total number of
> >> interrupt vectors that has to be supported by a specific GIC ITS device ID,
> >> specifically when "msi-map-mask" is populated in device tree.
> >>
> >> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
> >> ---
> >>  drivers/pci/bus.c   | 13 +++++++++----
> >>  include/linux/pci.h |  7 +++++--
> >>  2 files changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
> >> index 3cef835b375f..e381e639ceaa 100644
> >> --- a/drivers/pci/bus.c
> >> +++ b/drivers/pci/bus.c
> >> @@ -358,10 +358,12 @@ void pci_bus_add_devices(const struct pci_bus *bus)
> >>  }
> >>  EXPORT_SYMBOL(pci_bus_add_devices);
> >>  
> >> -/** pci_walk_bus - walk devices on/under bus, calling callback.
> >> +/** __pci_walk_bus - walk devices on/under bus matching requestor ID, calling callback.
> >>   *  @top      bus whose devices should be walked
> >>   *  @cb       callback to be called for each device found
> >>   *  @userdata arbitrary pointer to be passed to callback.
> >> + *  @rid      Requestor ID that has to be matched for the callback to be invoked
> >> + *  @mask     Mask that has to be applied to pci_dev_id(), before compating it with @rid
> >>   *
> >>   *  Walk the given bus, including any bridged devices
> >>   *  on buses under this bus.  Call the provided callback
> >> @@ -371,8 +373,8 @@ EXPORT_SYMBOL(pci_bus_add_devices);
> >>   *  other than 0, we break out.
> >>   *
> >>   */
> >> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >> -		  void *userdata)
> >> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >> +		    void *userdata, u32 rid, u32 mask)
> >>  {
> >>  	struct pci_dev *dev;
> >>  	struct pci_bus *bus;
> >> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >>  		} else
> >>  			next = dev->bus_list.next;
> >>  
> >> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
> > 
> > Why the check for the mask? I also wonder whether the mask should apply
> > to the rid as well:
> 
> If the mask is set for all 16bits, then there is not going to be two PCIe
> devices which gets the same ITS device ID right? So no need for calculating
> total number of vectors?

Are we really arguing about the cost of a compare+branch vs some
readability? Or is there an actual correctness issue here?

> > 
> > 		if ((pci_dev_id(dev) & mask) != (rid & mask))

Because I think the above expression is a lot more readable (and
likely more correct) than what you are suggesting.

> > 
> >> +			continue;
> >> +
> >>  		retval = cb(dev, userdata);
> >>  		if (retval)
> >>  			break;
> >>  	}
> >>  	up_read(&pci_bus_sem);
> >>  }
> >> -EXPORT_SYMBOL_GPL(pci_walk_bus);
> >> +EXPORT_SYMBOL_GPL(__pci_walk_bus);
> >>  
> >>  struct pci_bus *pci_bus_get(struct pci_bus *bus)
> >>  {
> >> diff --git a/include/linux/pci.h b/include/linux/pci.h
> >> index cd8aa6fce204..8500fec56e50 100644
> >> --- a/include/linux/pci.h
> >> +++ b/include/linux/pci.h
> >> @@ -1473,14 +1473,17 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
> >>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
> >>  		    int pass);
> >>  
> >> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >> -		  void *userdata);
> >> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >> +		    void *userdata, u32 rid, u32 mask);
> >>  int pci_cfg_space_size(struct pci_dev *dev);
> >>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
> >>  void pci_setup_bridge(struct pci_bus *bus);
> >>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
> >>  					 unsigned long type);
> >>  
> >> +#define pci_walk_bus(top, cb, userdata) \
> >> +	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
> > 
> > Please keep this close to the helper it replaces. I also really
> > dislike the use of this raw 0xffff. Don't we already have a named
> > constant that represents the mask for a RID?
> 
> I didn't find one on quick look but let me check.

Worse case, you could create your own.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Kishon Vijay Abraham I]

Hi Marc,

On 20/09/21 11:31 pm, Marc Zyngier wrote:
> On Mon, 20 Sep 2021 15:28:52 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>> Hi Marc,
>>
>> On 20/09/21 2:26 pm, Marc Zyngier wrote:
>>> On Mon, 20 Sep 2021 07:41:31 +0100,
>>> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>>>
>>>> Add two arguments to pci_walk_bus() [requestorID and mask], and add
>>>> support in pci_walk_bus() to invoke the *callback* only for devices
>>>> whose RequestorID after applying *mask* matches with *requestorID*
>>>> passed as argument.
>>>>
>>>> This is done in preparation for calculating the total number of
>>>> interrupt vectors that has to be supported by a specific GIC ITS device ID,
>>>> specifically when "msi-map-mask" is populated in device tree.
>>>>
>>>> Signed-off-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>> ---
>>>>  drivers/pci/bus.c   | 13 +++++++++----
>>>>  include/linux/pci.h |  7 +++++--
>>>>  2 files changed, 14 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/pci/bus.c b/drivers/pci/bus.c
>>>> index 3cef835b375f..e381e639ceaa 100644
>>>> --- a/drivers/pci/bus.c
>>>> +++ b/drivers/pci/bus.c
>>>> @@ -358,10 +358,12 @@ void pci_bus_add_devices(const struct pci_bus *bus)
>>>>  }
>>>>  EXPORT_SYMBOL(pci_bus_add_devices);
>>>>  
>>>> -/** pci_walk_bus - walk devices on/under bus, calling callback.
>>>> +/** __pci_walk_bus - walk devices on/under bus matching requestor ID, calling callback.
>>>>   *  @top      bus whose devices should be walked
>>>>   *  @cb       callback to be called for each device found
>>>>   *  @userdata arbitrary pointer to be passed to callback.
>>>> + *  @rid      Requestor ID that has to be matched for the callback to be invoked
>>>> + *  @mask     Mask that has to be applied to pci_dev_id(), before compating it with @rid
>>>>   *
>>>>   *  Walk the given bus, including any bridged devices
>>>>   *  on buses under this bus.  Call the provided callback
>>>> @@ -371,8 +373,8 @@ EXPORT_SYMBOL(pci_bus_add_devices);
>>>>   *  other than 0, we break out.
>>>>   *
>>>>   */
>>>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>> -		  void *userdata)
>>>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>> +		    void *userdata, u32 rid, u32 mask)
>>>>  {
>>>>  	struct pci_dev *dev;
>>>>  	struct pci_bus *bus;
>>>> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>>  		} else
>>>>  			next = dev->bus_list.next;
>>>>  
>>>> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
>>>
>>> Why the check for the mask? I also wonder whether the mask should apply
>>> to the rid as well:
>>
>> If the mask is set for all 16bits, then there is not going to be two PCIe
>> devices which gets the same ITS device ID right? So no need for calculating
>> total number of vectors?
> 
> Are we really arguing about the cost of a compare+branch vs some
> readability? Or is there an actual correctness issue here?

It is for correctness. So existing pci_walk_bus() doesn't invoke cb based on
rid. So when we convert to __pci_walk_bus(), existing callers of pci_walk_bus()
might not invoke cb for some devices without the check.
> 
>>>
>>> 		if ((pci_dev_id(dev) & mask) != (rid & mask))
> 
> Because I think the above expression is a lot more readable (and
> likely more correct) than what you are suggesting.

That would result in existing pci_walk_bus() behave differently from what was
before this patch no?

I'm having something like this below
	+#define pci_walk_bus(top, cb, userdata) \
	+	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)

So if we add only "if ((pci_dev_id(dev) & mask) != (rid & mask))", the callback
will not be invoked for any devices (other than one with rid = 0)

> 
>>>
>>>> +			continue;
>>>> +
>>>>  		retval = cb(dev, userdata);
>>>>  		if (retval)
>>>>  			break;
>>>>  	}
>>>>  	up_read(&pci_bus_sem);
>>>>  }
>>>> -EXPORT_SYMBOL_GPL(pci_walk_bus);
>>>> +EXPORT_SYMBOL_GPL(__pci_walk_bus);
>>>>  
>>>>  struct pci_bus *pci_bus_get(struct pci_bus *bus)
>>>>  {
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index cd8aa6fce204..8500fec56e50 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -1473,14 +1473,17 @@ const struct pci_device_id *pci_match_id(const struct pci_device_id *ids,
>>>>  int pci_scan_bridge(struct pci_bus *bus, struct pci_dev *dev, int max,
>>>>  		    int pass);
>>>>  
>>>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>> -		  void *userdata);
>>>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>> +		    void *userdata, u32 rid, u32 mask);
>>>>  int pci_cfg_space_size(struct pci_dev *dev);
>>>>  unsigned char pci_bus_max_busnr(struct pci_bus *bus);
>>>>  void pci_setup_bridge(struct pci_bus *bus);
>>>>  resource_size_t pcibios_window_alignment(struct pci_bus *bus,
>>>>  					 unsigned long type);
>>>>  
>>>> +#define pci_walk_bus(top, cb, userdata) \
>>>> +	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
>>>
>>> Please keep this close to the helper it replaces. I also really
>>> dislike the use of this raw 0xffff. Don't we already have a named
>>> constant that represents the mask for a RID?
>>
>> I didn't find one on quick look but let me check.
> 
> Worse case, you could create your own.

sure.

Thanks,
Kishon

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Marc Zyngier]

On Wed, 22 Sep 2021 02:26:09 +0100,
Kishon Vijay Abraham I <kishon@ti.com> wrote:
> 
> >>>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >>>> -		  void *userdata)
> >>>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >>>> +		    void *userdata, u32 rid, u32 mask)
> >>>>  {
> >>>>  	struct pci_dev *dev;
> >>>>  	struct pci_bus *bus;
> >>>> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
> >>>>  		} else
> >>>>  			next = dev->bus_list.next;
> >>>>  
> >>>> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
> >>>
> >>> Why the check for the mask? I also wonder whether the mask should apply
> >>> to the rid as well:
> >>
> >> If the mask is set for all 16bits, then there is not going to be two PCIe
> >> devices which gets the same ITS device ID right? So no need for calculating
> >> total number of vectors?
> > 
> > Are we really arguing about the cost of a compare+branch vs some
> > readability? Or is there an actual correctness issue here?
> 
> It is for correctness. So existing pci_walk_bus() doesn't invoke cb based on
> rid. So when we convert to __pci_walk_bus(), existing callers of pci_walk_bus()
> might not invoke cb for some devices without the check.
> > 
> >>>
> >>> 		if ((pci_dev_id(dev) & mask) != (rid & mask))
> > 
> > Because I think the above expression is a lot more readable (and
> > likely more correct) than what you are suggesting.
> 
> That would result in existing pci_walk_bus() behave differently from what was
> before this patch no?
> 
> I'm having something like this below
> 	+#define pci_walk_bus(top, cb, userdata) \
> 	+	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
> 
> So if we add only "if ((pci_dev_id(dev) & mask) != (rid & mask))",
> the callback will not be invoked for any devices (other than one
> with rid = 0)

But that *is* the bug, isn't it? If you want to parse all the devices,
a mask of 0 is what you need. The mask defines the bits that you want
to match against the RID you passed as a parameter. If you don't want
to check any bit, don't pass any!

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.

Re: [PATCH 1/3] PCI: Add support in pci_walk_bus() to invoke callback matching RID

[Kishon Vijay Abraham I]

Hi Marc,

On 27/09/21 8:15 pm, Marc Zyngier wrote:
> On Wed, 22 Sep 2021 02:26:09 +0100,
> Kishon Vijay Abraham I <kishon@ti.com> wrote:
>>
>>>>>> -void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>>>> -		  void *userdata)
>>>>>> +void __pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>>>> +		    void *userdata, u32 rid, u32 mask)
>>>>>>  {
>>>>>>  	struct pci_dev *dev;
>>>>>>  	struct pci_bus *bus;
>>>>>> @@ -399,13 +401,16 @@ void pci_walk_bus(struct pci_bus *top, int (*cb)(struct pci_dev *, void *),
>>>>>>  		} else
>>>>>>  			next = dev->bus_list.next;
>>>>>>  
>>>>>> +		if (mask != 0xffff && ((pci_dev_id(dev) & mask) != rid))
>>>>>
>>>>> Why the check for the mask? I also wonder whether the mask should apply
>>>>> to the rid as well:
>>>>
>>>> If the mask is set for all 16bits, then there is not going to be two PCIe
>>>> devices which gets the same ITS device ID right? So no need for calculating
>>>> total number of vectors?
>>>
>>> Are we really arguing about the cost of a compare+branch vs some
>>> readability? Or is there an actual correctness issue here?
>>
>> It is for correctness. So existing pci_walk_bus() doesn't invoke cb based on
>> rid. So when we convert to __pci_walk_bus(), existing callers of pci_walk_bus()
>> might not invoke cb for some devices without the check.
>>>
>>>>>
>>>>> 		if ((pci_dev_id(dev) & mask) != (rid & mask))
>>>
>>> Because I think the above expression is a lot more readable (and
>>> likely more correct) than what you are suggesting.
>>
>> That would result in existing pci_walk_bus() behave differently from what was
>> before this patch no?
>>
>> I'm having something like this below
>> 	+#define pci_walk_bus(top, cb, userdata) \
>> 	+	 __pci_walk_bus((top), (cb), (userdata), 0x0, 0xffff)
>>
>> So if we add only "if ((pci_dev_id(dev) & mask) != (rid & mask))",
>> the callback will not be invoked for any devices (other than one
>> with rid = 0)
> 
> But that *is* the bug, isn't it? If you want to parse all the devices,
> a mask of 0 is what you need. The mask defines the bits that you want
> to match against the RID you passed as a parameter. If you don't want
> to check any bit, don't pass any!

Indeed! Thanks for explaining.

Regards,
Kishon