netdev - Re: [PATCH v5 13/16] IB/pvrdma: Add the main driver module for PVRDMA

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <da4a09fc-bef1-60ea-f9af-7c505a9af6d6@vmware.com>
Date:   Sun, 25 Sep 2016 22:10:43 -0700
From:   Adit Ranadive <aditr@...are.com>
To:     Leon Romanovsky <leon@...nel.org>
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 sun, Sep 25 2016 at 10:57:03AM +0300, Leon Romanovsky wrote:
> 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>

<...>

> > +
> > +#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.
> 

Sorry, in our case it was the Mellanox mlx4 driver :). Yeah, I can remove
DRV_VERSION and DRV_RELDATE. I'll keep the MODULE_VERSION since we keep
track of our virtual driver versions that way. So when there are any changes
to our driver we typically bump up the version. Not sure if that's a netdev
or misc device practice and if thats followed here.

<...>

> > +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.
> 

If you are referring to commit 5fa76c20458518ed6181adddef2e31c5afc0745c
then isnt this is exactly what Ira did for the other providers?
Here, the pvrdma_get_fw_ver_str function is registered as a callback for
the get_dev_fw_str API. Please our pvrdma_register_device function where
we register our callbacks with ib_core:
https://patchwork.kernel.org/patch/9349357/

> > +	dev->ib_dev.get_dev_fw_str = pvrdma_get_fw_ver_str;