On Sunday, March 15, 2020 at 5:38:27 PM UTC-4, Fotis Xenakis wrote:
>
> Τη Κυριακή, 15 Μαρτίου 2020 - 10:14:46 μ.μ. UTC+2, ο χρήστης Waldek
> Kozaczuk έγραψε:
>>
>> 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]>
>>> ---
>>> 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.
>>
> My rationale behind this change was to avoid duplication, but truth is the
> re-implementation turned out more complex than I imagined. I am reverting
> it to the original (with an added check for predicate) and will send a v3.
> Another option I considered when thinking of how to avoid the duplication
> was to remove this function altogether (it is currently unused), but I
> turned it down. Since this was brought up though, I 'd like your opinion on
> it (pluralism vs smaller code base).
> Thank you!
>
I think that sometimes a little duplication is not bad especially when it
brings clarity to the code. No worries.
>
>>> +
>>> + // 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/b230452d-9b0e-4a55-bd30-c26bfee0caf0%40googlegroups.com.
