RE: [PATCH net-next, 3/6] net/mlx5: Add wrappers for HyperV PCIe operations

[Haiyang Zhang <haiyangz@microsoft.com>]

> -----Original Message-----
> From: Eran Ben Elisha <eranbe@mellanox.com>
> Sent: Thursday, August 15, 2019 7:35 AM
> To: Mark Bloch <markb@mellanox.com>; Haiyang Zhang
> <haiyangz@microsoft.com>; sashal@kernel.org; davem@davemloft.net;
> Saeed Mahameed <saeedm@mellanox.com>; leon@kernel.org;
> lorenzo.pieralisi@arm.com; bhelgaas@google.com; linux-
> pci@vger.kernel.org; linux-hyperv@vger.kernel.org; netdev@vger.kernel.org
> Cc: KY Srinivasan <kys@microsoft.com>; Stephen Hemminger
> <sthemmin@microsoft.com>; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/6] net/mlx5: Add wrappers for HyperV PCIe
> operations
> 
> 
> 
> On 8/14/2019 11:41 PM, Mark Bloch wrote:
> >
> >
> > On 8/14/19 12:08 PM, Haiyang Zhang wrote:
> >> From: Eran Ben Elisha <eranbe@mellanox.com>
> >>
> >> Add wrapper functions for HyperV PCIe read / write /
> >> block_invalidate_register operations.  This will be used as an
> >> infrastructure in the downstream patch for software communication.
> >>
> >> This will be enabled by default if CONFIG_PCI_HYPERV_MINI is set.
> >>
> >> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
> >> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> >> ---
> >>   drivers/net/ethernet/mellanox/mlx5/core/Makefile |  1 +
> >>   drivers/net/ethernet/mellanox/mlx5/core/lib/hv.c | 64
> ++++++++++++++++++++++++
> >>   drivers/net/ethernet/mellanox/mlx5/core/lib/hv.h | 22 ++++++++
> >>   3 files changed, 87 insertions(+)
> >>   create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/hv.c
> >>   create mode 100644 drivers/net/ethernet/mellanox/mlx5/core/lib/hv.h
> >>
> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> >> index 8b7edaa..a8950b1 100644
> >> --- a/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/Makefile
> >> @@ -45,6 +45,7 @@ mlx5_core-$(CONFIG_MLX5_ESWITCH)   +=
> eswitch.o eswitch_offloads.o eswitch_offlo
> >>   mlx5_core-$(CONFIG_MLX5_MPFS)      += lib/mpfs.o
> >>   mlx5_core-$(CONFIG_VXLAN)          += lib/vxlan.o
> >>   mlx5_core-$(CONFIG_PTP_1588_CLOCK) += lib/clock.o
> >> +mlx5_core-$(CONFIG_PCI_HYPERV_MINI)     += lib/hv.o
> >>
> >>   #
> >>   # Ipoib netdev
> >> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv.c
> b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv.c
> >> new file mode 100644
> >> index 0000000..cf08d02
> >> --- /dev/null
> >> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv.c
> >> @@ -0,0 +1,64 @@
> >> +// SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB
> >> +// Copyright (c) 2018 Mellanox Technologies
> >> +
> >> +#include <linux/hyperv.h>
> >> +#include "mlx5_core.h"
> >> +#include "lib/hv.h"
> >> +
> >> +static int mlx5_hv_config_common(struct mlx5_core_dev *dev, void
> *buf, int len,
> >> +				 int offset, bool read)
> >> +{
> >> +	int rc = -EOPNOTSUPP;
> >> +	int bytes_returned;
> >> +	int block_id;
> >> +
> >> +	if (offset % HV_CONFIG_BLOCK_SIZE_MAX || len %
> HV_CONFIG_BLOCK_SIZE_MAX)
> >> +		return -EINVAL;
> >> +
> >> +	block_id = offset / HV_CONFIG_BLOCK_SIZE_MAX;
> >> +
> >> +	rc = read ?
> >> +	     hyperv_read_cfg_blk(dev->pdev, buf,
> >> +				 HV_CONFIG_BLOCK_SIZE_MAX, block_id,
> >> +				 &bytes_returned) :
> >> +	     hyperv_write_cfg_blk(dev->pdev, buf,
> >> +				  HV_CONFIG_BLOCK_SIZE_MAX, block_id);
> >> +
> >> +	/* Make sure len bytes were read successfully  */
> >> +	if (read)
> >> +		rc |= !(len == bytes_returned);
> >> +
> >> +	if (rc) {
> >> +		mlx5_core_err(dev, "Failed to %s hv config, err = %d, len
> = %d, offset = %d\n",
> >> +			      read ? "read" : "write", rc, len,
> >> +			      offset);
> >> +		return rc;
> >> +	}
> >> +
> >> +	return 0;
> >> +}
> >
> > This seems out of place why not expose this function as part of hyperv and
> mlx5
> > will just pass the pdev.
> >
> The HV driver works with block chunks. I found it less convenience to do
> so directly, so I add a small wrapper for mlx5 core.
> 
> Haiyangz,
> Do you see a reason to export this callback style from the HYPERV level
> instead?
I don’t think the wrapper has to be in the hv interface. 
One function for read, another for write, are pretty straight forward. 
Users (other drivers) may use or wrap them in the way they like to.

Thanks,
- Haiyang