Re: [RFC PATCH v5 17/27] qedn: Add qedn probe

Leon Romanovsky
2021-05-19T15:32:13Z

On Wed, May 19, 2021 at 05:29:32PM +0300, Shai Malin wrote:
> On Wed, 19 May 2021 at 15:31, Leon Romanovsky wrote:
> > On Wed, May 19, 2021 at 02:13:30PM +0300, Shai Malin wrote:
> > > This patch introduces the functionality of loading and unloading
> > > physical function.
> > > qedn_probe() loads the offload device PF(physical function), and
> > > initialize the HW and the FW with the PF parameters using the
> > > HW ops->qed_nvmetcp_ops, which are similar to other "qed_*_ops" which
> > > are used by the qede, qedr, qedf and qedi device drivers.
> > > qedn_remove() unloads the offload device PF, re-initialize the HW and
> > > the FW with the PF parameters.
> > >
> > > The struct qedn_ctx is per PF container for PF-specific attributes and
> > > resources.
> > >
> > > Acked-by: Igor Russkikh <irusskikh@marvell.com>
> > > Signed-off-by: Dean Balandin <dbalandin@marvell.com>
> > > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com>
> > > Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com>
> > > Signed-off-by: Michal Kalderon <mkalderon@marvell.com>
> > > Signed-off-by: Ariel Elior <aelior@marvell.com>
> > > Signed-off-by: Shai Malin <smalin@marvell.com>
> > > Reviewed-by: Hannes Reinecke <hare@suse.de>
> > > ---
> > > drivers/nvme/hw/Kconfig | 1 +
> > > drivers/nvme/hw/qedn/qedn.h | 35 +++++++
> > > drivers/nvme/hw/qedn/qedn_main.c | 159 ++++++++++++++++++++++++++++++-
> > > 3 files changed, 190 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/nvme/hw/Kconfig b/drivers/nvme/hw/Kconfig
> > > index 374f1f9dbd3d..91b1bd6f07d8 100644
> > > --- a/drivers/nvme/hw/Kconfig
> > > +++ b/drivers/nvme/hw/Kconfig
> > > @@ -2,6 +2,7 @@
> > > config NVME_QEDN
> > > tristate "Marvell NVM Express over Fabrics TCP offload"
> > > depends on NVME_TCP_OFFLOAD
> > > + select QED_NVMETCP
> > > help
> > > This enables the Marvell NVMe TCP offload support (qedn).
> > >
> > > diff --git a/drivers/nvme/hw/qedn/qedn.h b/drivers/nvme/hw/qedn/qedn.h
> > > index bcd0748a10fd..f13073afbced 100644
> > > --- a/drivers/nvme/hw/qedn/qedn.h
> > > +++ b/drivers/nvme/hw/qedn/qedn.h
> > > @@ -6,14 +6,49 @@
> > > #ifndef _QEDN_H_
> > > #define _QEDN_H_
> > >
> > > +#include <linux/qed/qed_if.h>
> > > +#include <linux/qed/qed_nvmetcp_if.h>
> > > +
> > > /* Driver includes */
> > > #include "../../host/tcp-offload.h"
> > >
> > > +#define QEDN_MAJOR_VERSION 8
> > > +#define QEDN_MINOR_VERSION 62
> > > +#define QEDN_REVISION_VERSION 10
> > > +#define QEDN_ENGINEERING_VERSION 0
> > > +#define DRV_MODULE_VERSION __stringify(QEDE_MAJOR_VERSION) "." \
> > > + __stringify(QEDE_MINOR_VERSION) "." \
> > > + __stringify(QEDE_REVISION_VERSION) "." \
> > > + __stringify(QEDE_ENGINEERING_VERSION)
> > > +
> >
> > This driver module version is not used in this series and more
> > important the module version have no meaning in upstream at all
> > and the community strongly against addition of new such code.
>
> Will be fixed.
>
> >
> > > #define QEDN_MODULE_NAME "qedn"
> >
> > And the general note, it will be great if you convert your probe/remove
> > flows to use auxiliary bus like other drivers that cross subsystems.
>
> qedn is simply fitting in with the existing design of qed/qede/qedr/qedf/qedi.

I know.

> Changing the entire multi-protocol design to auxiliary bus is being studied.

It will be required to do at some point of time.

Thanks