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

Remove pci vendor check, check only pci class #621

Merged
merged 1 commit into from
Apr 23, 2021

Conversation

rmr-silicom
Copy link
Contributor

No description provided.


if len(os.Getenv("VID")) == 0 && pci.Vendor != vendorIntel {
return errors.Errorf("unsupported PCI device %s VID=%s PID=%s Class=%s", pci.BDF, pci.Vendor, pci.Device, pci.Class)
} else if pci.Vendor != os.Getenv("VID") && pci.Vendor != vendorIntel {
Copy link
Contributor

Choose a reason for hiding this comment

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

@rmr-silicom are you looking to import pkg/fpga in your project?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I am adding support for the FPGA n5010 card. It uses the kernel drivers from here https://github.com/OPAE/linux-dfl-backport, but it seems the sysfs entries are mostly the same.

Copy link
Member

Choose a reason for hiding this comment

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

@rmr-silicom can you share output of lspci for this card, to see how it is reported in terms IDs/class/etc?
Also, it would be great to see how kernel DFL drivers sysfs reports about that device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mythi It has a vendor id of 0x1c2c and product ids of 0x1001, 0x1000, and the 0x12000 fpga class, of course

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, didn't catch your sysfs question... I added a few, there are a lot more...

[core@worker1 ~]$ find /sys/class/fpga_*
/sys/class/fpga_bridge
/sys/class/fpga_bridge/br0
/sys/class/fpga_manager
/sys/class/fpga_manager/fpga0
/sys/class/fpga_region
/sys/class/fpga_region/region0
/sys/class/fpga_region/region1
/sys/class/fpga_sec_mgr
/sys/class/fpga_sec_mgr/fpga_sec0

FME.0
/sys/class/fpga_region/region0/dfl-fme.0/uevent
/sys/class/fpga_region/region0/dfl-fme.0/bitstream_id
/sys/class/fpga_region/region0/dfl-fme.0/bitstream_metadata

PORT.0
/sys/class/fpga_region/region0/dfl-port.0
/sys/class/fpga_region/region0/dfl-port.0/uevent
/sys/class/fpga_region/region0/dfl-port.0/afu_id
/sys/class/fpga_region/region0/dfl-port.0/power_state
/sys/class/fpga_region/region0/dfl-port.0/ap2_event
/sys/class/fpga_region/region0/dfl-port.0/ltr

Copy link
Contributor

Choose a reason for hiding this comment

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

removing vendor check by overall, but make sure that DFL APIs are compatible still.

@rmr-silicom can you update this PR to drop the vendor check completely and rename the function.

@kad would the existing reg expressions suffice or did you have something else in your mind?

Copy link
Member

Choose a reason for hiding this comment

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

@rmr-silicom you can squash commits and update PR title. beside that, lgtm.

@mythi the current patch is ok from me for now. However just existing regexp for device node names might not be enough. I'd like later to implement validation of kernel driver behind the PCI device.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kad isn't the merge squash thing on your end? If I squash the commits, the branch changes and this request is lost.

https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/about-pull-request-merges

Copy link
Contributor

Choose a reason for hiding this comment

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

@rmr-silicom we don't squash commits during merge but ask the developers to do that. the ask is you do git rebase -i HEAD~2, combine the two commits in our branch to one and force push to remote

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the tip!

@rmr-silicom rmr-silicom changed the title Add environment variables to support other VIDs than Intel Remove pci vendor check, check only pci class Apr 21, 2021
Copy link
Contributor

@mythi mythi left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@bart0sh bart0sh left a comment

Choose a reason for hiding this comment

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

/lgtm

@bart0sh bart0sh merged commit edd2672 into intel:main Apr 23, 2021
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.

None yet

5 participants