Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add GetDefaultPkeyFromPci and GetPKeyByIndexFromPci functions #69

Merged
merged 1 commit into from
Jan 28, 2024

Conversation

almaslennikov
Copy link
Contributor

@almaslennikov almaslennikov commented Dec 15, 2023

The functions read the from the PCI device's
infiniband dir and returns either the PKey for index0 or given index

sriovnet.go Outdated
@@ -520,3 +520,32 @@ func GetPciFromNetDevice(name string) (string, error) {
}
return base, nil
}

// GetPKeyFromPci returns the index0 PKey for the IB PCI device
func GetPKeyFromPci(pciAddress string) (string, error) {
Copy link
Collaborator

@adrianchiris adrianchiris Jan 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this works for both PF and VFs ? (just making sure, my memory is dusty around these parts :D)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are several indexes for pkey membership, index0 is the default membership.

should we have two functions ?

func GetPkeyByIndexFromPci(pciAddress string) (string, error)

and

func GetDefaultlPkeyFromPci(pciAddress string) (string, error

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works for both PFs and VFs as the mechanism around how PKeys are stored is the same for both.
I'm not opposed having two functions to retrieve different PKeys. Let me address that in the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split into two new functions

Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

later on we might want to split infiniband specific functionality to its own file if deemed nessecary.

@adrianchiris adrianchiris self-requested a review January 8, 2024 16:02
@almaslennikov almaslennikov changed the title Add GetPkeyFromPci function Add GetDefaultPkeyFromPci and GetPKeyByIndexFromPci functions Jan 9, 2024
@almaslennikov
Copy link
Contributor Author

@SchSeba @moshe010 Could you take a look at this PR?

@adrianchiris
Copy link
Collaborator

@bn222 PTAL, quick one :)

Copy link
Contributor

@bn222 bn222 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two small nits that will aid debugging. Once those are addressed, it's an LGTM from me.

sriovnet.go Outdated Show resolved Hide resolved
sriovnet.go Outdated Show resolved Hide resolved
@almaslennikov
Copy link
Contributor Author

almaslennikov commented Jan 22, 2024

Hi @bn222, thank you for the review! addressed your comments.
@adrianchiris I think we can merge now

sriovnet.go Outdated Show resolved Hide resolved
sriovnet.go Outdated Show resolved Hide resolved
sriovnet.go Show resolved Hide resolved
The functions read the from the PCI device's
infiniband dir and returns either the PKey for index0 or given index

Signed-off-by: amaslennikov <[email protected]>
Copy link
Collaborator

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM,
@bn222 PTAL i believe we can merge this one

@bn222
Copy link
Contributor

bn222 commented Jan 26, 2024

LGTM.

@adrianchiris adrianchiris merged commit 3ca5e43 into k8snetworkplumbingwg:master Jan 28, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants