Re: [PATCH 01/13] RDMA: Split the alloc_hw_stats() ops to port and device variants

From: Mark Zhang <markzhang@nvidia.com>
To: Jason Gunthorpe <jgg@nvidia.com>,
	Potnuri Bharat Teja <bharat@chelsio.com>,
	Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Devesh Sharma <devesh.sharma@broadcom.com>,
	Doug Ledford <dledford@redhat.com>,
	"Faisal Latif" <faisal.latif@intel.com>,
	Gal Pressman <galpress@amazon.com>,
	"Leon Romanovsky" <leon@kernel.org>, <linux-rdma@vger.kernel.org>,
	Mike Marciniszyn <mike.marciniszyn@cornelisnetworks.com>,
	Naresh Kumar PBS <nareshkumar.pbs@broadcom.com>,
	Selvin Xavier <selvin.xavier@broadcom.com>,
	Shiraz Saleem <shiraz.saleem@intel.com>,
	Yossi Leybovich <sleybo@amazon.com>,
	Somnath Kotur <somnath.kotur@broadcom.com>,
	Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>,
	Yishai Hadas <yishaih@nvidia.com>,
	"Zhu Yanjun" <zyjzyj2000@gmail.com>
Cc: Greg KH <gregkh@linuxfoundation.org>,
	Kees Cook <keescook@chromium.org>,
	Nathan Chancellor <nathan@kernel.org>
Subject: Re: [PATCH 01/13] RDMA: Split the alloc_hw_stats() ops to port and device variants
Date: Tue, 18 May 2021 11:49:57 +0800	[thread overview]
Message-ID: <b39b0550-6602-be80-7343-349a6f6f30a9@nvidia.com> (raw)
In-Reply-To: <1-v1-34c90fa45f1c+3c7b0-port_sysfs_jgg@nvidia.com>

On 5/18/2021 12:47 AM, Jason Gunthorpe wrote:
> External email: Use caution opening links or attachments
> 
> 
> This is being used to implement both the port and device global stats,
> which is causing some confusion in the drivers. For instance EFA and i40iw
> both seem to be misusing the device stats.
> 
> Split it into two ops so drivers that don't support one or the other can
> leave the op NULL'd, making the calling code a little simpler to
> understand.
> 
> Signed-off-by: Jason Gunthorpe <jgg@nvidia.com>
> ---
>   drivers/infiniband/core/counters.c          |  4 +-
>   drivers/infiniband/core/device.c            |  3 +-
>   drivers/infiniband/core/nldev.c             |  2 +-
>   drivers/infiniband/core/sysfs.c             | 15 +++-
>   drivers/infiniband/hw/bnxt_re/hw_counters.c |  7 +-
>   drivers/infiniband/hw/bnxt_re/hw_counters.h |  4 +-
>   drivers/infiniband/hw/bnxt_re/main.c        |  2 +-
>   drivers/infiniband/hw/cxgb4/provider.c      |  9 +--
>   drivers/infiniband/hw/efa/efa.h             |  3 +-
>   drivers/infiniband/hw/efa/efa_main.c        |  3 +-
>   drivers/infiniband/hw/efa/efa_verbs.c       | 11 ++-
>   drivers/infiniband/hw/hfi1/verbs.c          | 86 ++++++++++-----------
>   drivers/infiniband/hw/i40iw/i40iw_verbs.c   | 19 ++++-
>   drivers/infiniband/hw/mlx4/main.c           | 25 ++++--
>   drivers/infiniband/hw/mlx5/counters.c       | 42 +++++++---
>   drivers/infiniband/sw/rxe/rxe_hw_counters.c |  7 +-
>   drivers/infiniband/sw/rxe/rxe_hw_counters.h |  4 +-
>   drivers/infiniband/sw/rxe/rxe_verbs.c       |  2 +-
>   include/rdma/ib_verbs.h                     | 13 ++--
>   19 files changed, 158 insertions(+), 103 deletions(-)
> 

[...]

> diff --git a/drivers/infiniband/core/sysfs.c b/drivers/infiniband/core/sysfs.c
> index 05b702de00e89b..29082d8d45fc4f 100644
> --- a/drivers/infiniband/core/sysfs.c
> +++ b/drivers/infiniband/core/sysfs.c
> @@ -981,8 +981,15 @@ static void setup_hw_stats(struct ib_device *device, struct ib_port *port,
>          struct rdma_hw_stats *stats;
>          int i, ret;
> 
> -       stats = device->ops.alloc_hw_stats(device, port_num);
> -
> +       if (port_num) {
> +               if (!device->ops.alloc_hw_port_stats)
> +                       return;

Do we need this "if (!device->ops.alloc_hw_port_stats)" here?

> +               stats = device->ops.alloc_hw_port_stats(device, port_num);
> +       } else {
> +               if (!device->ops.alloc_hw_device_stats)
> +                       return;

And here?

> +               stats = device->ops.alloc_hw_device_stats(device);
> +       }
>          if (!stats)
>                  return;
> 
> @@ -1165,7 +1172,7 @@ static int add_port(struct ib_core_device *coredev, int port_num)
>           * port, so holder should be device. Therefore skip per port conunter
>           * initialization.
>           */
> -       if (device->ops.alloc_hw_stats && port_num && is_full_dev)
> +       if (device->ops.alloc_hw_port_stats && port_num && is_full_dev)
>                  setup_hw_stats(device, p, port_num);
> 
>          list_add_tail(&p->kobj.entry, &coredev->port_list);
> @@ -1409,7 +1416,7 @@ int ib_device_register_sysfs(struct ib_device *device)
>          if (ret)
>                  return ret;
> 
> -       if (device->ops.alloc_hw_stats)
> +       if (device->ops.alloc_hw_device_stats)
>                  setup_hw_stats(device, NULL, 0);
> 
>          return 0;

[...]

> diff --git a/drivers/infiniband/hw/cxgb4/provider.c b/drivers/infiniband/hw/cxgb4/provider.c
> index 3f1893e180ddf3..c0f01799f4a0b9 100644
> --- a/drivers/infiniband/hw/cxgb4/provider.c
> +++ b/drivers/infiniband/hw/cxgb4/provider.c
> @@ -377,14 +377,11 @@ static const char * const names[] = {
>          [IP6OUTRSTS] = "ip6OutRsts"
>   };
> 
> -static struct rdma_hw_stats *c4iw_alloc_stats(struct ib_device *ibdev,
> -                                             u32 port_num)
> +static struct rdma_hw_stats *c4iw_alloc_port_stats(struct ib_device *ibdev,
> +                                                  u32 port_num)
>   {
>          BUILD_BUG_ON(ARRAY_SIZE(names) != NR_COUNTERS);
> 
> -       if (port_num != 0)
> -               return NULL;
> -

I'm not familiar with this driver, but if port_num must be 0 here, does 
it mean this is per-device not per-port?