Τη Κυριακή, 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!
>
>> +
>> + // 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/4c10827e-abd2-40f8-a98f-88dbdf9b3634%40googlegroups.com.
