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

[Leon Romanovsky <leon-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 3500 bytes --]

On Sun, Sep 25, 2016 at 10:10:43PM -0700, Adit Ranadive wrote:
> 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-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Jorgen Hansen <jhansen-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: George Zhang <georgezhang-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Aditya Sarwade <asarwade-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Reviewed-by: Bryan Tan <bryantan-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
> > > Signed-off-by: Adit Ranadive <aditr-pghWNbHTmq7QT0dZR+AlfA@public.gmane.org>
>
> <...>
>
> > > +
> > > +#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.

They always promise to do it, but after year or two they are stopping.
The funny thing starts when they start debug customer issues in specific
distro with mix of backported fixes/features from old and new kernels.

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

You are right,
Sorry for that.

>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]