Like it overall but see my suggestion below.
On Sunday, March 15, 2020 at 7:43:49 AM UTC-4, Fotis Xenakis wrote:
>
> Signed-off-by: Fotis Xenakis <[email protected] <javascript:>>
> ---
> drivers/pci-function.cc | 39 +++++++++++++++++++++++++++++++++------
> drivers/pci-function.hh | 1 +
> 2 files changed, 34 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/pci-function.cc b/drivers/pci-function.cc
> index 369f22e9..2cc8b837 100644
> --- a/drivers/pci-function.cc
> +++ b/drivers/pci-function.cc
> @@ -807,36 +807,63 @@ namespace pci {
> write_pci_config(_bus, _device, _func, offset, val);
> }
>
> + // Returns the offset of the first capability with id matching
> @cap_id, or
> + // 0xFF if none found.
> u8 function::find_capability(u8 cap_id)
> {
> - return this->find_capability(cap_id, [](function *fun, u8 off) {
> return true; } );
> + return find_capability(cap_id, [](function *fun, u8 off) { return
> true; } );
> }
>
> + // Returns the offset of the first capability with id matching
> @cap_id and
> + // satisfying @predicate (if specified). If none found, returns 0xFF.
> u8 function::find_capability(u8 cap_id, std::function<bool
> (function*, u8)> predicate)
> + {
> + u8 bad_offset = 0xFF;
> + std::vector<u8> cap_offs;
> +
> + if (!find_capabilities(cap_offs, cap_id)) {
> + return bad_offset;
> + }
> +
> + if (!predicate) {
> + predicate = [](function *fun, u8 off) { return true; };
> + }
> + for (auto off: cap_offs) {
> + if (predicate(this, off)) {
> + return off;
> + }
> + }
> + return bad_offset;
> + }
Let us keep the original implementation of "u8 function::find_capability(u8
cap_id, std::function<bool (function*, u8)> predicate)" as it is now. I
think the current implementation of it is easier to understand and I do not
see much benefit of it delegating to the new find_capabilities() function
below.
I like your comments so please keep them of course.
>
> +
> + // Append to @cap_offs the offsets of all capabilities with id
> matching
> + // @cap_id. Returns whether any such capabilities were found.
> + bool function::find_capabilities(std::vector<u8>& cap_offs, u8
> cap_id)
> {
> u8 capabilities_base = pci_readb(PCI_CAPABILITIES_PTR);
> u8 off = capabilities_base;
> - u8 bad_offset = 0xFF;
> u8 max_capabilities = 0xF0;
> u8 ctr = 0;
> + bool found = false;
>
> while (off != 0) {
> // Read capability
> u8 capability = pci_readb(off + PCI_CAP_OFF_ID);
> - if (capability == cap_id && predicate(this, off)) {
> - return off;
> + if (capability == cap_id) {
> + cap_offs.push_back(off);
> + found = true;
> }
>
> ctr++;
> if (ctr > max_capabilities) {
> - return bad_offset;
> + return found;
> }
>
> // Next
> off = pci_readb(off + PCI_CAP_OFF_NEXT);
> }
>
> - return bad_offset;
> + return found;
> }
>
> bar * function::get_bar(int idx)
> diff --git a/drivers/pci-function.hh b/drivers/pci-function.hh
> index 59d57a1e..a8904cb0 100644
> --- a/drivers/pci-function.hh
> +++ b/drivers/pci-function.hh
> @@ -342,6 +342,7 @@ namespace pci {
> // Capability parsing
> u8 find_capability(u8 cap_id);
> u8 find_capability(u8 cap_id, std::function<bool (function*, u8)>
> predicate);
> + bool find_capabilities(std::vector<u8>& caps, u8 cap_id);
>
> bar * get_bar(int idx);
> void add_bar(int idx, bar* bar);
> --
> 2.25.1
>
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/be7abe3b-c4f5-42b4-99bf-5f3fd7ce7f40%40googlegroups.com.
