diff mbox series

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

Message ID 20210920064133.14115-2-kishon@ti.com (mailing list archive)
State Changes Requested
Delegated to: Bjorn Helgaas
Headers show
Series PCI/gic-v3-its: Add support for same ITS device ID for multiple PCIe devices | expand

Commit Message

Kishon Vijay Abraham I Sept. 20, 2021, 6:41 a.m. UTC
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(-)

Comments

Marc Zyngier Sept. 20, 2021, 8:56 a.m. UTC | #1
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.
Kishon Vijay Abraham I Sept. 20, 2021, 2:28 p.m. UTC | #2
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
Marc Zyngier Sept. 20, 2021, 6:01 p.m. UTC | #3
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.
Kishon Vijay Abraham I Sept. 22, 2021, 1:26 a.m. UTC | #4
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
Marc Zyngier Sept. 27, 2021, 2:45 p.m. UTC | #5
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.
Kishon Vijay Abraham I Sept. 27, 2021, 2:56 p.m. UTC | #6
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
diff mbox series

Patch

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)