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 <fo...@windowslive.com <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 osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/be7abe3b-c4f5-42b4-99bf-5f3fd7ce7f40%40googlegroups.com.
