Re: [PATCH] NTB: ntb_tool: Add full multi-port NTB API support

[Logan Gunthorpe]

On 08/08/17 04:11 AM, Serge Semin wrote:
> NTB API has been updated to support multi-port devices like IDT
> 89HPESx series or Microsemi Switchtec. Message registers
> functionality has also been added to new API. In order to keep
> the new hardware and corresponding capabilities well tested, NTB
> tool driver is accordingly altered.

These patches are all incredibly hard to read as they are very long and
also contain a bunch of unrelated and unnecessary changes in them. I've
commented on a subset of these issues below for this patch only but the
subsequent patches also have a lot of these issues.

The patches are so convoluted and hard to read that if I were in Jon's
position I could not in good conscious merge them. It's way too
difficult to review them for changes we don't want to have in the
kernel. However, this is only my opinion and in the end will be up to Jon.

Also, because you are changing the ntb_tool API significantly, we also
need to update the ntb_test script in tools/testing/selftests/ntb/ at
the same time.


> -#define DRIVER_NAME			"ntb_tool"
> -#define DRIVER_DESCRIPTION		"PCIe NTB Debugging Tool"
> +#define DRIVER_NAME		"ntb_tool"
> +#define DRIVER_DESCRIPTION	"PCIe NTB Debugging Tool"

Do we need to add more noise here by changing the tabbing? I think
cosmetic changes like this need to be in a separate patch and argued for
separately.

> -/* It is rare to have hadrware with greater than six MWs */
> -#define MAX_MWS	6
> -/* Only two-ports devices are supported */
> -#define PIDX	NTB_DEF_PEER_IDX

I'm glad we're getting rid of this restriction but this should really be
a separate patch. I noticed only after your last series got included
that you reduced the restriction from 16 to 6 which was mildly annoying
seeing switchtec can actually have up to ~128 mws. At the _very_ least,
all changes to semantics, features and restrictions need to be mentioned
in the commit message.

> -	ssize_t rc;
> +	ssize_t ret;

These diffs would be much easier to read if names of unrelated variables
were not needlessly changed. This is done in a lot of places and it just
adds to the noise we have to filter through to try and understand what
is actually being changed.


>  static struct ntb_client tool_client = {
>  	.ops = {
>  		.probe = tool_probe,
>  		.remove = tool_remove,
> -	},
> +	}

More cosmetic unnecessary changes.



>  static int __init tool_init(void)
>  {
> -	int rc;
> +	int ret;
>  
>  	if (debugfs_initialized())
> -		tool_dbgfs = debugfs_create_dir(KBUILD_MODNAME, NULL);
> +		tool_dbgfs_topdir = debugfs_create_dir(KBUILD_MODNAME, NULL);
>  
> -	rc = ntb_register_client(&tool_client);
> -	if (rc)
> -		goto err_client;
> +	ret = ntb_register_client(&tool_client);
> +	if (ret)
> +		debugfs_remove_recursive(tool_dbgfs_topdir);
>  
> -	return 0;
> -
> -err_client:
> -	debugfs_remove_recursive(tool_dbgfs);
> -	return rc;
> +	return ret;
>  }
>  module_init(tool_init);
>  
>  static void __exit tool_exit(void)
>  {
>  	ntb_unregister_client(&tool_client);
> -	debugfs_remove_recursive(tool_dbgfs);
> +	debugfs_remove_recursive(tool_dbgfs_topdir);
>  }
>  module_exit(tool_exit);
> +

I don't think there are actually any real changes in this hunk. Just
cosmetic changes to variable names. If you absolutely feel these need to
be changed it should be in a separate patch that specifically says this
is happening and provides justifications.

I'll be happy to review and test the actual content of this series more
once the patches are cleaned up and more easily read.

Logan