This patch has the changes to enable ntb_hw_amd driver for Device ID 0x148B and associated changes.
Details
Details
Diff Detail
Diff Detail
- Repository
- rS FreeBSD src repository - subversion
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
The general idea of the change looks good to me, thanks!
In the future, please include full context with diffs. You can use the arc (Arcanist) tool (recommended for convenience), or generate plain diffs with diff -U9999999 if you prefer to avoid the Arcanist tool.
Some stylistic comments below.
| sys/dev/ntb/ntb_hw/ntb_hw_amd.c | ||
|---|---|---|
| 84–85 | I think style-wise we'd leave these on the previous line, to match the spacing for the { It would also be fine to encode the START_IDX and CNT in this table as data. | |
| 339–340 | I'd add a period at the end of the second sentence, and maybe "The remaining cases all use 64-bit bars." | |
| 342 | It might also make more sense to encode this in the PCI device table, as a quirk flag or something. QUIRK_BAR0_32BIT or something. Or code off the deviceid. Unless there is some inherent connection between mw_count==3 devices and a 32-bit BAR0. | |
| 384 | style(9) includes a blank first line for multi-line comments, like: /* * set and verify ... * * For Device ... */ | |
Comment Actions
@cem, sorry about the delayed response.
I was trying to use the "arc" tool for updating the patch. But it asks to create a new commit as below. I am unsure whether to accept it. So for now, I have created the patch with "git diff -U9" and attached here. Usually I will do "git diff -p", which will have 3 lines of context. Now, I changed to include 9 lines of context with "git diff -U9".
Logs I get when using "arc". Once I am clear about this, I will use arc for my future review submission
~/rajesh/freebsd_ntb # arc diff --update D20892
You have uncommitted changes in this working copy.
Working copy: /root/rajesh/freebsd_ntb/ Unstaged changes in working copy: sys/dev/ntb/ntb_hw/ntb_hw_amd.c sys/dev/ntb/ntb_hw/ntb_hw_amd.h Do you want to create a new commit with these changes? [y/N] N
Usage Exception: You can not continue with uncommitted changes. Commit or discard them before proceeding.
Comment Actions
The workflow for arc and a git repository is to create a commit with your proposed changes, then use arc diff --update D20892 HEAD^ (for example).
So for now, I have created the patch with "git diff -U9" and attached here. Usually I will do "git diff -p", which will have 3 lines of context. Now, I changed to include 9 lines of context with "git diff -U9".
Thanks, 9 lines is better than 3. If you use -U99999, diff will grab the entire file, which is what arc does.
Comment Actions
@cem,
The workflow for arc and a git repository is to create a commit with your proposed changes, then use arc diff --update D20892 HEAD^ (for example).
Starting next diff, I will use the arc approach to update the diffs. I assume HEAD^ creates a commit on top of the current branch where I have my changes. Not sure what is ^ for?
Is there any other comments you have on top of the second diff updated?
| sys/dev/ntb/ntb_hw/ntb_hw_amd.c | ||
|---|---|---|
| 84–85 | I assume, first line of the comments states to have the closing "}" in the same line rather than a new line. If so, that is done now. Comment mentioned in second line is addressed now. | |
| 339–340 | Do you mean the following way? If so, it's made now. But it doesn't make any much difference right? /*
| |
| 342 | This comment is addressed now. A quirk has been added to take care of this. | |
| 384 | Corrected it now. Also, changed all single line comments to use "//" instead of "/* */" | |
Comment Actions
Thanks!
I assume HEAD^ creates a commit on top of the current branch where I have my changes. Not sure what is ^ for?
Nope. arc diff implicitly compares some earlier version, specified in this example as HEAD^ (the revision before HEAD), to the latest commit (HEAD). arc diff --create HEAD^ means, create a Phabricator differential/review with my latest commit.
Is there any other comments you have on top of the second diff updated?
Yes, a few comments below. Thanks!
| sys/dev/ntb/ntb_hw/ntb_hw_amd.c | ||
|---|---|---|
| 12 | style: please add a space between ) and } This table should also be const. | |
| 339–340 | Not quite fixed, no. "uses" should be "use" and the 2nd sentence is still missing a period at the end. | |
| 342 | LGTM, thanks. | |
| 384 | Comments should be english sentences, so maybe: "Set and verify setting the limit registers." Like before, "uses" -> "use" and the last sentence should end in a period. // comments are discouraged; /* */ is preferred for single-line comments. | |
| 1030 | We should not be modifying the shared hw_info table. If we need to track spad_count specially for some kinds of connection, it should be copied into the softc. | |
| sys/dev/ntb/ntb_hw/ntb_hw_amd.h | ||
| 75–81 | I don't think it makes much sense to define these as enums anymore. They're only used once, in the amd_ntb_hw_info_list table instantiation. Instead, the numbers can just be placed in the table directly. | |
| 97 | It doesn't make a lot of sense to declare a variable (amd_ntb_hw_info_list) in a header, since it will be instantiated each time the header is included. I also don't think it makes sense to store this data separately, but referenced, from the amd_ntb_devs table below. I would just add the needed PCI VID:DID/description fields to struct amd_ntb_hw_info and use that structure for amd_ntb_devs. amd_ntb_devs is not required to be a struct pci_device_table. This table should be const. | |
Comment Actions
@cem, sorry about the delayed response.
V3 patch updated with the commented addressed. Please let me know your comments.
| sys/dev/ntb/ntb_hw/ntb_hw_amd.c | ||
|---|---|---|
| 339–340 | Done. | |
| 384 | Done. Single line comments moved back to /* */ format. | |
| 1030 | Copied the spad_count alone to softc and modifying it. Rest of the elements are constants, so using them directly. | |
| sys/dev/ntb/ntb_hw/ntb_hw_amd.h | ||
| 75–81 | I believe, you mentioned about AMD_MW_CNT1/2, AMD_BART_START_IDX1/2, AMD_DB_CNT and AMD_SPADS_CNT. If so, they are removed now and the numbers are used directly. | |
| 97 | Moved the amd_ntb_hw_info_list definition to ntb_hw_amd.c file itself and marked it as static const. Also marked the amd_ntb_devs to be static const. I understand, you mention to remove two different structures (amd_ntb_hw_info -> amd_ntb_hw_info_list) and (pci_device_table -> amd_ntb_devs) and make them as single structure directly with all details. If so, the reason, why I implemented the pci_device_table is to use the PCI_MATCH macro and PCI_PNP_INFO from the pci subsystem. This was one of the suggested changes when this file is submitted for the first time. So I retain those two structures but with some of your suggested changes. If I understand wrongly, please correct me. | |
Comment Actions
Looks good to me. I'll commit it as soon as test build completes.
Not related to this specific change, but looking through the driver I got a question: why in case of revert to INTx and single interrupt you reset number of doorbells to 1? I guess it should not be needed if AMD_DBSTAT_OFFSET read in amd_ntb_db_read() return status for all doorbells. Am I wrong? On my tests with PLX NTB, supporting only one INTx interrupt, I found that multiple doorbells are still useful. Above levels of NTB code should handle it just fine. You just need to provide valid amd_ntb_db_vector_mask() for that case.
| sys/dev/ntb/ntb_hw/ntb_hw_amd.h | ||
|---|---|---|
| 97 | The PCI_PNP_INFO macro could still be used on a complete table so long as the struct pci_device_table is the first member of each table entry. Yes, it breaks PCI_MATCH. I don't think there is any real reason to prefer these matching/pnp macros. | |
Comment Actions
And could you also write small man page so that people would know about the driver and to which hardware it applies.
Comment Actions
@mav and @cem, Thanks for your time to review and push it upstream.
Sorry about the delayed response.
Theoretically, I don't see any trouble in retaining the db_count in case of MSI and INTx fallback. But, how will it be helpful to have multiple doorbell with single interrupt? Can you give an idea please? Also, When you say,
You just need to provide valid amd_ntb_db_vector_mask() for that case.
db_valid_mask will be set as (db_count - 1 ) always (I assume, this should not be changed). So, if we don't reduce the db_count, how amd_ntb_db_vector_mask will create an impact?
We kept it this way to 1x1 map between doorbell and interrupt vector and to be in sync with our Linux driver. I will try to test the discussed scenario to see how it behaves. Currently, my machines are in use for some other testing. I will get them freed by mid september. I will do these tests then and update you on the same.
Regarding the man page, I have created a ntb_hw_amd.4 file (with most of the contents similar to ntb_hw_intel with changes specific to AMD hardware). I tested them by zipping and placing them in /usr/share/man/man4 and doing "man ntb_hw_amd". It looks fine. Is that all I need to do? or am I missing something? Should I put that ntb_hw_amd.4 file for review as a separate one here? or any other process involved? Please advice.
Also, I am backporting this driver for stable/11 and stable/12 branch. Can I create a separate review for each branch?
I used PCI_PNP_INFO as it is suggested in the initial review. Internally it uses MODULE_PNP_INFO only. Also, PCI_MATCH looks more simpler to look for a match in the table (rather than looping in our code). So, do you still feel them to be removed. Please advice.
Comment Actions
Right now amd_ntb_db_vector_mask() maps interrupts to doorbells 1:1. But it is not required to do so. It should just tell what doorbells are signaled by each interrupt vector. You may define any reasonable mapping. For example, you may see Intel NTB driver, depending on hardware, may use 1:15, 4:15, 1:1, 1:34, etc. PLX driver always uses 1:16. Support for multiple doorbells gives more flexibility to upper layers, for example, on FreeBSD they may be spilt between different drivers, or single driver, such as ntb_transport may utilize them to distribute workload between multiple CPU threads, that is good for if_ntb performance.
We kept it this way to 1x1 map between doorbell and interrupt vector and to be in sync with our Linux driver. I will try to test the discussed scenario to see how it behaves. Currently, my machines are in use for some other testing. I will get them freed by mid september. I will do these tests then and update you on the same.
Regarding the man page, I have created a ntb_hw_amd.4 file (with most of the contents similar to ntb_hw_intel with changes specific to AMD hardware). I tested them by zipping and placing them in /usr/share/man/man4 and doing "man ntb_hw_amd". It looks fine. Is that all I need to do? or am I missing something? Should I put that ntb_hw_amd.4 file for review as a separate one here? or any other process involved? Please advice.
Generally it has to be mentioned in respective Makefile to be built. It is also good to crosslink it with other related manpage(s), so that user would have a chance to find its existence. Also there are style requirements how man pages should be properly formatted, that is why it is generally recommended to get some review from some doc committer. Though if you just copied and slightly edited existing page it is not required. Also IIRC there was some tool to check formatting, but I forgot what it is. If you send me your draft I may do the linkage. Considering that FreeBSD 12.1 release cycle should start in September, it should better be done sooner to be included.
Also, I am backporting this driver for stable/11 and stable/12 branch. Can I create a separate review for each branch?
Just last night I merged the driver into stable/12. Merges do not usually require review, unless merged code for some good reason is different from head.
Comment Actions
Thanks @mav for merging with stable/12.
on FreeBSD they may be spilt between different drivers, or single driver, such as ntb_transport may utilize them to distribute workload between multiple CPU threads, that is good for if_ntb performance.
I will do these changes and test it. Will update once it's done.
I have submitted the manpage for Review (https://reviews.freebsd.org/D21462). Please have a look and let me know your comments.
Comment Actions
Thank you. Committed.
But reading the man page I've noticed it says that only NTB-to-Root Port mode is supported. Are you planning to add NTB-to-NTB/Back-to-Back mode support also? We at iXsystems use both Intel and PLX NTBs exactly in B2B mode to make nodes configuration identical and to not bother with handling PCIe hot-plug events by the OS.
Comment Actions
Thanks @mav for committing the changes.
Are you planning to add NTB-to-NTB/Back-to-Back mode support also? We at iXsystems use both Intel and PLX NTBs exactly in B2B mode to make nodes configuration identical and to not bother with handling PCIe hot-plug events by the OS.
Currently, we don't have plans to support NTB-to-NTB mode. I will do the necessary updates as and when we plan to do the same.