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

feat: fix Mellanox Build & add missing Kernel Flags #1048

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

feniarus
Copy link

@feniarus feniarus commented Oct 5, 2024

This PR is for fixing the Mellanox OFED Driver Package and adding the missing Kernel Flags for it to build.
The NVME Part of the Dirver is disabled because it has Source Code related problems.

Signed-off-by: Feniarus [email protected]

@dsseng
Copy link
Member

dsseng commented Oct 6, 2024

The NVME Part of the Dirver is disabled because it has Source Code related problems

Which precisely?

Also, your commit doesn't contain a second body line (well, just add description there) and is missing a signed-off-by line, which indicates accepting Developer Certificate of Origin. Please add it using git commit --amend -s

Copy link
Member

@dsseng dsseng left a comment

Choose a reason for hiding this comment

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

Thank you for contributing! Could you please elaborate a bit on some parts unclear to me?

@@ -1,5 +1,5 @@
name: mellanox-ofed-pkg
variant: scratch
variant: alpine
Copy link
Member

Choose a reason for hiding this comment

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

Not sure whether or not we should do so. Why doesn't it work without variant: alpine, what's missing in our tools, and why not add it there?

Comment on lines +16 to +17
rm /bin/mktemp
ln -s /toolchain/bin/mktemp /bin/
Copy link
Member

Choose a reason for hiding this comment

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

Could you please add a comment why is this replacement done?

mellanox_ofed_version: 5.9-0.5.6.0
mellanox_ofed_sha256: 4503258cbe92b00c734e612c3a7ad1d71e023fdffae2a2c119f7b537505e499c
mellanox_ofed_sha512: 58604ea89aa8351727532c48f0c59b4e533ba8bfcef9533f45d94e15ffdcf3a5c464398706cad14ebf3826b132972bd044fadbf7f047e60bdb0c2a454c96acd7
mellanox_ofed_version: 24.07-0.6.1.0
Copy link
Member

Choose a reason for hiding this comment

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

Please also include a note about the version update in the commit message

@@ -84,6 +84,7 @@ TARGETS += xfsprogs
TARGETS += kernel
TARGETS += drbd-pkg
TARGETS += gasket-driver-pkg
TARGETS += mellanox-ofed-pkg
Copy link
Member

Choose a reason for hiding this comment

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

this file is generated, instead .kres.yaml should be updated with make rekres after that

@smira smira requested a review from frezbo October 7, 2024 12:05
@feniarus
Copy link
Author

feniarus commented Oct 8, 2024

The NVME Part of the Dirver is disabled because it has Source Code related problems

Which precisely?

Also, your commit doesn't contain a second body line (well, just add description there) and is missing a signed-off-by line, which indicates accepting Developer Certificate of Origin. Please add it using git commit --amend -s

When trying to build with full NVME support, the build fails with the following error:

image

@frezbo
Copy link
Member

frezbo commented Oct 15, 2024

This PR is for fixing the Mellanox OFED Driver Package and adding the missing Kernel Flags for it to build. The NVME Part of the Dirver is disabled because it has Source Code related problems.

Signed-off-by: Feniarus [email protected]

does this means it's better than the in-tree kernel mlx5 drivers, just trying to gauge if we need to build and maintain this? 🤔

@frezbo
Copy link
Member

frezbo commented Oct 15, 2024

also wants to know if this co-exist with in-tree kernel drivers, might need to denylist udev loading mlx5 by default I guess

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