Message-ID: <20171205155454.GA1701@mobilestation>
Date:   Tue, 5 Dec 2017 18:54:54 +0300
From:   Serge Semin <fancer.lancer@...il.com>
To:     Allen Hubbe <Allen.Hubbe@...l.com>
Cc:     jdmason@...zu.us, dave.jiang@...el.com, Shyam-sundar.S-k@....com,
        Xiangliang.Yu@....com, gary.hook@....com,
        Sergey.Semin@...latforms.ru, linux-ntb@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 00/15] NTB: Add full multi-port API support to the
 test drivers

On Mon, Dec 04, 2017 at 03:10:20PM -0500, Allen Hubbe <Allen.Hubbe@...l.com> 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 <Allen.Hubbe@...l.com>
> 

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 linux-ntb+unsubscribe@...glegroups.com.
> To post to this group, send email to linux-ntb@...glegroups.com.
> 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.

Powered by blists - more mailing lists