Date:   Sun, 25 Sep 2016 10:57:03 +0300
From:   Leon Romanovsky <leon@...nel.org>
To:     Adit Ranadive <aditr@...are.com>
Cc:     dledford@...hat.com, linux-rdma@...r.kernel.org,
        pv-drivers@...are.com, netdev@...r.kernel.org,
        linux-pci@...r.kernel.org, jhansen@...are.com, asarwade@...are.com,
        georgezhang@...are.com, bryantan@...are.com
Subject: Re: [PATCH v5 13/16] IB/pvrdma: Add the main driver module for PVRDMA

On Sat, Sep 24, 2016 at 04:21:37PM -0700, Adit Ranadive wrote:
> This patch adds the support to register a RDMA device with the kernel RDMA
> stack as well as a kernel module. This also initializes the underlying
> virtual PCI device.
>
> Reviewed-by: Yuval Shaia <yuval.shaia@...cle.com>
> Reviewed-by: Jorgen Hansen <jhansen@...are.com>
> Reviewed-by: George Zhang <georgezhang@...are.com>
> Reviewed-by: Aditya Sarwade <asarwade@...are.com>
> Reviewed-by: Bryan Tan <bryantan@...are.com>
> Signed-off-by: Adit Ranadive <aditr@...are.com>
> ---
> Changes v4->v5:
>  - Removed two unnecessary lines.
>  - Updated include for headers in UAPI folder.
>  - Update to pvrdma_cmd_post for add/delete GIDs.
>  - Add error code in dev_warn if pvrdma_cmd_post failed.
>
> Changes v3->v4:
>  - Fixed some checkpatch warnings.
>  - Added support for new get_dev_fw_str API.
>  - Added event workqueue for netdevice events.
>  - Restructured the pvrdma_pci_remove function a little bit.
>
> Changes v2->v3:
>  - Removed boolean in pvrdma_cmd_post.
>
> Changes v1->v2:
>  - Addressed 32-bit build errors
>  - Cosmetic change to avoid if else in intr0_handler
>  - Removed unnecessary return assignment.
> ---
>  drivers/infiniband/hw/pvrdma/pvrdma_main.c | 1220 ++++++++++++++++++++++++++++
>  1 file changed, 1220 insertions(+)
>  create mode 100644 drivers/infiniband/hw/pvrdma/pvrdma_main.c
>
> diff --git a/drivers/infiniband/hw/pvrdma/pvrdma_main.c b/drivers/infiniband/hw/pvrdma/pvrdma_main.c
> new file mode 100644
> index 0000000..94cbbb9
> --- /dev/null
> +++ b/drivers/infiniband/hw/pvrdma/pvrdma_main.c
> @@ -0,0 +1,1220 @@
> +/*
> + * Copyright (c) 2012-2016 VMware, Inc.  All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of EITHER the GNU General Public License
> + * version 2 as published by the Free Software Foundation or the BSD
> + * 2-Clause License. This program is distributed in the hope that it
> + * will be useful, but WITHOUT ANY WARRANTY; WITHOUT EVEN THE IMPLIED
> + * WARRANTY OF MERCHANTABILITY OR FITNESS FOR A PARTICULAR PURPOSE.
> + * See the GNU General Public License version 2 for more details at
> + * http://www.gnu.org/licenses/old-licenses/gpl-2.0.en.html.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program available in the file COPYING in the main
> + * directory of this source tree.
> + *
> + * The BSD 2-Clause License
> + *
> + *     Redistribution and use in source and binary forms, with or
> + *     without modification, are permitted provided that the following
> + *     conditions are met:
> + *
> + *      - Redistributions of source code must retain the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer.
> + *
> + *      - Redistributions in binary form must reproduce the above
> + *        copyright notice, this list of conditions and the following
> + *        disclaimer in the documentation and/or other materials
> + *        provided with the distribution.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + * "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + * LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS
> + * FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE
> + * COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT,
> + * INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES
> + * (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR
> + * SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION)
> + * HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
> + * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED
> + * OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +#include <linux/errno.h>
> +#include <linux/inetdevice.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <rdma/ib_addr.h>
> +#include <rdma/ib_smi.h>
> +#include <rdma/ib_user_verbs.h>
> +#include <net/addrconf.h>
> +#include <rdma/pvrdma-abi.h>
> +
> +#include "pvrdma.h"
> +
> +#define DRV_NAME	"pvrdma"
> +#define DRV_VERSION	"1.0"
> +#define DRV_RELDATE	"January 1, 2013"
> +
> +static const char pvrdma_version[] =
> +	DRV_NAME ": PVRDMA InfiniBand driver v"
> +	DRV_VERSION " (" DRV_RELDATE ")\n";

This is a good example why driver version and reldate are useless in
kernel. We are in 2016 and not in 2013. All these data are forgotten to
update right after the developer copied it from other pre-historic driver
(very common practice in netdev). It is worth to stop to use this totally
useless defines.

> +
> +static DEFINE_MUTEX(pvrdma_device_list_lock);
> +static LIST_HEAD(pvrdma_device_list);
> +static struct workqueue_struct *event_wq;
> +
> +static int pvrdma_add_gid(struct ib_device *ibdev,
> +			  u8 port_num,
> +			  unsigned int index,
> +			  const union ib_gid *gid,
> +			  const struct ib_gid_attr *attr,
> +			  void **context);
> +static int pvrdma_del_gid(struct ib_device *ibdev,
> +			  u8 port_num,
> +			  unsigned int index,
> +			  void **context);
> +
> +
> +static ssize_t show_hca(struct device *device, struct device_attribute *attr,
> +			char *buf)
> +{
> +	return sprintf(buf, "PVRDMA%s\n", DRV_VERSION);
> +}
> +
> +static ssize_t show_rev(struct device *device, struct device_attribute *attr,
> +			char *buf)
> +{
> +	return sprintf(buf, "%d\n", PVRDMA_REV_ID);
> +}
> +
> +static ssize_t show_board(struct device *device, struct device_attribute *attr,
> +			  char *buf)
> +{
> +	return sprintf(buf, "%d\n", PVRDMA_BOARD_ID);
> +}
> +
> +static DEVICE_ATTR(hw_rev,   S_IRUGO, show_rev,	   NULL);
> +static DEVICE_ATTR(hca_type, S_IRUGO, show_hca,	   NULL);
> +static DEVICE_ATTR(board_id, S_IRUGO, show_board,  NULL);
> +
> +static struct device_attribute *pvrdma_class_attributes[] = {
> +	&dev_attr_hw_rev,
> +	&dev_attr_hca_type,
> +	&dev_attr_board_id
> +};
> +
> +static void pvrdma_get_fw_ver_str(struct ib_device *device, char *str,
> +				  size_t str_len)
> +{
> +	struct pvrdma_dev *dev =
> +		container_of(device, struct pvrdma_dev, ib_dev);
> +	snprintf(str, str_len, "%d.%d.%d\n",
> +		 (int) (dev->dsr->caps.fw_ver >> 32),
> +		 (int) (dev->dsr->caps.fw_ver >> 16) & 0xffff,
> +		 (int) dev->dsr->caps.fw_ver & 0xffff);
> +}

Yuval already pointed it to you.
Thanks to Ira, we have general function in core to print FW version.
Please use them.

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists