On Mon, Dec 04, 2017 at 03:10:20PM -0500, Allen Hubbe <[email protected]> wrote: > From: Serge Semin > > The multi-port NTB API was introduced in kernel 4.13 as well as the > > first driver for the true multi-port devices of IDT PCIe-switches > > series. But the test drivers still were left almost unchanged. Yes, > > they didn't fail being used with new NTB API, but they only worked > > with two-ports NTB devices. This patchset is intended to fix the > > issue, by amending the NTB test drivers and script so they would be > > fully compatible with multi-port NTB API. > > > > Additionally I found a few NTB subsystem issues while developing the > > submitted patches. So they are also fixed in this patchset >
Hello Allen > Thanks for bringing the multiport support to the ntb tools and tests, and > getting it all tested with help of @pallam. I wondered about setting the dma > mask on the ntb device object, and it is better now that you have done that. > In addition to the contact information to the file comments, MODULE_AUTHOR > can also be specified more than once per module. > > As Logan said, some of the renaming was not really necessary and made those > patches more noisy than the needed to be. I am not as much bothered by it, > but it is a valid criticism. > Yeah, I know that. As I already said, it isn't usual of my patches being so complicated. The reason why I made so many small renamings was the code refactoring. I tried to set all the test drivers code up to the same naming convention, so it would be easier for a code reader to study it. Definitely, such the alterations should have been done within a separate patch. But I started refactoring the code and changing the logic for multi-portness all together, so it would be too painful to unpick one from another after the work was done. It especially concerned the ntb_tool and ntb_perf drivers. Next time the procedures will be separately. Sorry for inconvenience and thanks for understanding. > I can't find an earlier comment I thought I had made regarding the changes in > ntb_tool. Maybe it was only on IRC. I think the use of the anonymous unions > tool_mw is confusing. It seems to require the reader to first understand how > local and peer mw setup works, and then see that the anonymous unions are > accessed by the proper member names depending on the situation. In my > review, the use of those members appears to be correct in your code. Since > these drivers are also intended to be example code, I would have preferred > distinct types instead of a common type with anonymous union members. > Distinct types would make type checking by the compiler more effective at > catching improper use, and I think it would make the code more clear in its > use as an example ntb driver. Also, there might be some confusion in the > naming of members "mw", since it might refer to a tool_mw or a tool_mw_wrap, > and the name alone doesn't disclose the type, that requires reading and > understanding the surrounding context in the code. > Oops. We discussed it in the IRC. We agreed to add commentaries around the types definition, so a reader would better understand their purpose. I just forgot to move the change from my repo to the fork of Jon's one. I'll do it and resend the patch. > I am satisfied with these patches. This is important for getting the > multiport support established, and other than some comments about style and > presentation of the patch set, nothing looks obviously wrong. You should > probably also seek Dave's ack on at least ntb_perf. > > Acked-by: Allen Hubbe <[email protected]> > Thanks for your commentary and ack sign. Dave also made a great effort in testing the ntb_perf driver. Especially in helping Ujjal with our remote debugging procedure on the Intel platforms.) Anyway I'll be waiting for his explicit ack under the patches he agrees on. Thanks, -Sergey > > Serge Semin (15): > > NTB: Rename NTB messaging API methods > > NTB: Set dma mask and dma coherent mask to NTB devices > > NTB: Fix UB/bug in ntb_mw_get_align() > > NTB: ntb_pp: Add full multi-port NTB API support > > NTB: ntb_tool: Add full multi-port NTB API support > > NTB: ntb_perf: Add full multi-port NTB API support > > NTB: ntb_test: Safely use paths with whitespace > > NTB: ntb_test: Add ntb_tool port tests > > NTB: ntb_test: Update ntb_tool link tests > > NTB: ntb_test: Update ntb_tool DB tests > > NTB: ntb_test: Update ntb_tool Scratchpad tests > > NTB: ntb_test: Add ntb_tool Message tests > > NTB: ntb_test: Update ntb_tool MW tests > > NTB: ntb_test: Update ntb_perf tests > > NTB: ntb_hw_idt: Set NTB_TOPO_SWITCH topology > > > > drivers/ntb/hw/amd/ntb_hw_amd.c | 4 + > > drivers/ntb/hw/idt/ntb_hw_idt.c | 37 +- > > drivers/ntb/hw/intel/ntb_hw_intel.c | 4 + > > drivers/ntb/ntb.c | 1 - > > drivers/ntb/test/ntb_perf.c | 1826 > > +++++++++++++++++++++---------- > > drivers/ntb/test/ntb_pingpong.c | 450 +++++--- > > drivers/ntb/test/ntb_tool.c | 1805 > > ++++++++++++++++++++---------- > > include/linux/ntb.h | 36 +- > > tools/testing/selftests/ntb/ntb_test.sh | 307 ++++-- > > 9 files changed, 3013 insertions(+), 1457 deletions(-) > > > > -- > > 2.12.0 > > -- > You received this message because you are subscribed to the Google Groups > "linux-ntb" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to [email protected]. > To post to this group, send email to [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/linux-ntb/000001d36d3b%24e9f211d0%24bdd63570%24%40dell.com. > For more options, visit https://groups.google.com/d/optout.
