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@xxxxxx> 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@xxxxxx>
>> ---
>>  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