Re: [PATCH for bnxt_re V3 03/21] bnxt_re: register with the NIC driver

Leon Romanovsky
2017-01-15T19:41:44Z

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

On Tue, Dec 20, 2016 at 01:13:13AM -0800, Selvin Xavier wrote:
> This patch handles the registration with the bnxt_en driver.
> The bnxt_re driver first registers with netdev notifier chain and upon
> receiving the NETDEV_REGISTER event, it registers with bnxt_en driver.
>
> 1. bnxt_en's ulp_probe function returns a structure that contains
> information about the device and additional entry points.
> 2. bnxt_en driver returns 'struct bnxt_eth_dev' that contains set
> of operation vectors that bnxt_re driver invokes later.
> 3. bnxt_request_msix() allows the bnxt_re driver to specify the
> number of MSI-X vectors that are needed.
> 4. bnxt_send_fw_msg () is used to send messages to the FW
> 5. bnxt_register_async_events() is used to register for async
> event callbacks.
>
> v2: Remove some sparse warning. Also, remove some unused code from unreg
> path.
> v3: Removed condition checks for rdev reported during static code analysis.
> Check the return value of try_module_get while getting bnxt_en
> reference.
>
> Signed-off-by: Eddie Wai <eddie.wai@broadcom.com>
> Signed-off-by: Devesh Sharma <devesh.sharma@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.kotur@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapatna@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xavier@broadcom.com>
> ---
> drivers/infiniband/hw/bnxtre/bnxt_re.h | 51 ++++
> drivers/infiniband/hw/bnxtre/bnxt_re_main.c | 448 ++++++++++++++++++++++++++++
> 2 files changed, 499 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxtre/bnxt_re.h b/drivers/infiniband/hw/bnxtre/bnxt_re.h
> index f9b8542..8b73e3d 100644
> --- a/drivers/infiniband/hw/bnxtre/bnxt_re.h
> +++ b/drivers/infiniband/hw/bnxtre/bnxt_re.h
> @@ -42,5 +42,56 @@
> #define ROCE_DRV_MODULE_NAME "bnxt_re"
> #define ROCE_DRV_MODULE_VERSION "1.0.0"
>
> +#define BNXT_RE_REF_WAIT_COUNT 10
> #define BNXT_RE_DESC "Broadcom NetXtreme-C/E RoCE Driver"
> +
> +struct bnxt_re_work {
> + struct work_struct work;
> + unsigned long event;
> + struct bnxt_re_dev *rdev;
> + struct net_device *vlan_dev;
> +};
> +
> +#define BNXT_RE_MIN_MSIX 2
> +#define BNXT_RE_MAX_MSIX 16
> +struct bnxt_re_dev {
> + struct ib_device ibdev;
> + struct list_head list;
> + atomic_t ref_count;
> + unsigned long flags;
> +#define BNXT_RE_FLAG_NETDEV_REGISTERED 0
> +#define BNXT_RE_FLAG_IBDEV_REGISTERED 1
> +#define BNXT_RE_FLAG_GOT_MSIX 2
> +#define BNXT_RE_FLAG_RCFW_CHANNEL_EN 8
> +#define BNXT_RE_FLAG_QOS_WORK_REG 16
> + struct net_device *netdev;
> + unsigned int version, major, minor;
> + struct bnxt_en_dev *en_dev;
> + struct bnxt_msix_entry msix_entries[BNXT_RE_MAX_MSIX];
> + int num_msix;
> +
> + int id;
> +
> + atomic_t qp_count;
> + struct mutex qp_lock; /* protect qp list */
> + struct list_head qp_list;
> +
> + atomic_t cq_count;
> + atomic_t srq_count;
> + atomic_t mr_count;
> + atomic_t mw_count;
> + /* Max of 2 lossless traffic class supported per port */
> + u16 cosq[2];
> +};
> +
> +#define to_bnxt_re_dev(ptr, member) \
> + container_of((ptr), struct bnxt_re_dev, member)
> +
> +static inline struct device *rdev_to_dev(struct bnxt_re_dev *rdev)
> +{
> + if (rdev)
> + return &rdev->ibdev.dev;
> + return NULL;
> +}
> +
> #endif
> diff --git a/drivers/infiniband/hw/bnxtre/bnxt_re_main.c b/drivers/infiniband/hw/bnxtre/bnxt_re_main.c
> index 6c22d51..fbd2ad3 100644
> --- a/drivers/infiniband/hw/bnxtre/bnxt_re_main.c
> +++ b/drivers/infiniband/hw/bnxtre/bnxt_re_main.c
> @@ -38,10 +38,24 @@
>
> #include <linux/module.h>
> #include <linux/netdevice.h>
> +#include <linux/ethtool.h>
> #include <linux/mutex.h>
> #include <linux/list.h>
> #include <linux/rculist.h>
> +#include <linux/spinlock.h>
> +#include <linux/pci.h>
> +#include <net/ipv6.h>
> +#include <net/addrconf.h>
> +
> +#include <rdma/ib_verbs.h>
> +#include <rdma/ib_user_verbs.h>
> +#include <rdma/ib_umem.h>
> +#include <rdma/ib_addr.h>
> +
> +#include "bnxt_ulp.h"
> +#include "bnxt_re_hsi.h"
> #include "bnxt_re.h"
> +#include "bnxt.h"
> static char version[] =
> BNXT_RE_DESC " v" ROCE_DRV_MODULE_VERSION "\n";
>
> @@ -55,6 +69,384 @@ static struct list_head bnxt_re_dev_list = LIST_HEAD_INIT(bnxt_re_dev_list);
> /* Mutex to protect the list of bnxt_re devices added */
> static DEFINE_MUTEX(bnxt_re_dev_lock);
> static struct workqueue_struct *bnxt_re_wq;
> +
> +/* for handling bnxt_en callbacks later */
> +static void bnxt_re_stop(void *p)
> +{
> +}
> +
> +static void bnxt_re_start(void *p)
> +{
> +}
> +
> +static void bnxt_re_sriov_config(void *p, int num_vfs)
> +{
> +}
> +
> +static struct bnxt_ulp_ops bnxt_re_ulp_ops = {
> + .ulp_async_notifier = NULL,
> + .ulp_stop = bnxt_re_stop,
> + .ulp_start = bnxt_re_start,
> + .ulp_sriov_config = bnxt_re_sriov_config
> +};
> +
> +/* The rdev ref_count is to protect immature removal of the device */
> +static inline void bnxt_re_hold(struct bnxt_re_dev *rdev)
> +{
> + atomic_inc(&rdev->ref_count);
> +}
> +
> +static inline void bnxt_re_put(struct bnxt_re_dev *rdev)
> +{
> + atomic_dec(&rdev->ref_count);
> +}

Recently, in one of our submission to netdev and rdma, we got
a reminder that inline functions shouldn't be in *.c. Let for
the compiler to decide.

IMHO, it should be open-coded without wrappers and honestly I failed to
understand why do you need so many wrappers for one line standard kernel
functions.

Thanks

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