Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Leon Romanovsky]

On Sun, Apr 11, 2021 at 07:34:55PM -0700, Dexuan Cui wrote:
> Add a VF driver for Microsoft Azure Network Adapter (MANA) that will be
> available in the future.
> 
> Co-developed-by: Haiyang Zhang <haiyangz@microsoft.com>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> Co-developed-by: Shachar Raindel <shacharr@microsoft.com>
> Signed-off-by: Shachar Raindel <shacharr@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>  MAINTAINERS                                   |    4 +-
>  drivers/net/ethernet/Kconfig                  |    1 +
>  drivers/net/ethernet/Makefile                 |    1 +
>  drivers/net/ethernet/microsoft/Kconfig        |   29 +
>  drivers/net/ethernet/microsoft/Makefile       |    5 +
>  drivers/net/ethernet/microsoft/mana/Makefile  |    6 +
>  drivers/net/ethernet/microsoft/mana/gdma.h    |  728 +++++++
>  .../net/ethernet/microsoft/mana/gdma_main.c   | 1525 +++++++++++++
>  .../net/ethernet/microsoft/mana/hw_channel.c  |  854 ++++++++
>  .../net/ethernet/microsoft/mana/hw_channel.h  |  190 ++
>  drivers/net/ethernet/microsoft/mana/mana.h    |  549 +++++
>  drivers/net/ethernet/microsoft/mana/mana_en.c | 1924 +++++++++++++++++
>  .../ethernet/microsoft/mana/mana_ethtool.c    |  252 +++
>  .../net/ethernet/microsoft/mana/shm_channel.c |  298 +++
>  .../net/ethernet/microsoft/mana/shm_channel.h |   21 +
>  15 files changed, 6386 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/net/ethernet/microsoft/Kconfig
>  create mode 100644 drivers/net/ethernet/microsoft/Makefile
>  create mode 100644 drivers/net/ethernet/microsoft/mana/Makefile
>  create mode 100644 drivers/net/ethernet/microsoft/mana/gdma.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/gdma_main.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/hw_channel.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/hw_channel.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana.h
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana_en.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/mana_ethtool.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/shm_channel.c
>  create mode 100644 drivers/net/ethernet/microsoft/mana/shm_channel.h
> 

<...>

> +/* Microsoft Azure Network Adapter (MANA)'s definitions
> + *
> + * Structures labeled with "HW DATA" are exchanged with the hardware. All of
> + * them are naturally aligned and hence don't need __packed.
> + */
> +
> +#define ANA_MAJOR_VERSION	0
> +#define ANA_MINOR_VERSION	1
> +#define ANA_MICRO_VERSION	1

Please don't introduce drier versions.

Thanks

RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Leon Romanovsky <leon@kernel.org>
> Sent: Monday, April 12, 2021 12:46 AM
> To: Dexuan Cui <decui@microsoft.com>
> > ...
> > +#define ANA_MAJOR_VERSION	0
> > +#define ANA_MINOR_VERSION	1
> > +#define ANA_MICRO_VERSION	1
> 
> Please don't introduce drier versions.

This is not the usual "driver version", though it's called  "drv version" :-)
As you can see, the driver does not use the macro MODULE_VERSION().

Here the "drv version" actually means the version of the VF-to-PF protocol,
with which the Azure Network Adapter ethernet NIC driver (i.e. the VF driver)
talks to the PF driver.  The protocol version determines the formats of the
messages that are sent from the VF driver to the PF driver, e.g. query the
MAC address, create Send/Receive queues, configure RSS, etc.

Currently the protocol versin is 0.1.1 You may ask why it's called
"drv version" rather than "protocol version" -- it's because the PF driver
calls it that way, so I think here the VF driver may as well use the same
name. BTW, the "drv ver" info is passed to the PF driver in the below
function:

static int mana_query_client_cfg(struct ana_context *ac, u32 drv_major_ver,
                                 u32 drv_minor_ver, u32 drv_micro_ver,
                                 u16 *max_num_vports)
{
        struct gdma_context *gc = ac->gdma_dev->gdma_context;
        struct ana_query_client_cfg_resp resp = {};
        struct ana_query_client_cfg_req req = {};
        struct device *dev = gc->dev;
        int err = 0;

        mana_gd_init_req_hdr(&req.hdr, ANA_QUERY_CLIENT_CONFIG,
                             sizeof(req), sizeof(resp));
        req.drv_major_ver = drv_major_ver;
        req.drv_minor_ver = drv_minor_ver;
        req.drv_micro_ver = drv_micro_ver;

        err = mana_send_request(ac, &req, sizeof(req), &resp, sizeof(resp));

Thanks,
Dexuan

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Leon Romanovsky]

On Mon, Apr 12, 2021 at 08:35:32AM +0000, Dexuan Cui wrote:
> > From: Leon Romanovsky <leon@kernel.org>
> > Sent: Monday, April 12, 2021 12:46 AM
> > To: Dexuan Cui <decui@microsoft.com>
> > > ...
> > > +#define ANA_MAJOR_VERSION	0
> > > +#define ANA_MINOR_VERSION	1
> > > +#define ANA_MICRO_VERSION	1
> > 
> > Please don't introduce drier versions.
> 
> This is not the usual "driver version", though it's called  "drv version" :-)
> As you can see, the driver does not use the macro MODULE_VERSION().
> 
> Here the "drv version" actually means the version of the VF-to-PF protocol,
> with which the Azure Network Adapter ethernet NIC driver (i.e. the VF driver)
> talks to the PF driver.  The protocol version determines the formats of the
> messages that are sent from the VF driver to the PF driver, e.g. query the
> MAC address, create Send/Receive queues, configure RSS, etc.
> 
> Currently the protocol versin is 0.1.1 You may ask why it's called
> "drv version" rather than "protocol version" -- it's because the PF driver
> calls it that way, so I think here the VF driver may as well use the same
> name. BTW, the "drv ver" info is passed to the PF driver in the below
> function:

Ohh, yes, the "driver version" is not the ideal name for that.

I already looked on it in previous patch, came to the conclusion about
the protocol and forgot :(.

> 
> static int mana_query_client_cfg(struct ana_context *ac, u32 drv_major_ver,
>                                  u32 drv_minor_ver, u32 drv_micro_ver,
>                                  u16 *max_num_vports)
> {
>         struct gdma_context *gc = ac->gdma_dev->gdma_context;
>         struct ana_query_client_cfg_resp resp = {};
>         struct ana_query_client_cfg_req req = {};
>         struct device *dev = gc->dev;
>         int err = 0;
> 
>         mana_gd_init_req_hdr(&req.hdr, ANA_QUERY_CLIENT_CONFIG,
>                              sizeof(req), sizeof(resp));
>         req.drv_major_ver = drv_major_ver;
>         req.drv_minor_ver = drv_minor_ver;
>         req.drv_micro_ver = drv_micro_ver;
> 
>         err = mana_send_request(ac, &req, sizeof(req), &resp, sizeof(resp));
> 
> Thanks,
> Dexuan
> 

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

> > Currently the protocol versin is 0.1.1 You may ask why it's called
> > "drv version" rather than "protocol version" -- it's because the PF driver
> > calls it that way, so I think here the VF driver may as well use the same
> > name. BTW, the "drv ver" info is passed to the PF driver in the below
> > function:
> 
> Ohh, yes, the "driver version" is not the ideal name for that.
> 
> I already looked on it in previous patch, came to the conclusion about
> the protocol and forgot :(.

Which suggests it needs renaming.

      Andrew

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

> +static inline bool is_gdma_msg(const void *req)
> +{
> +	struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> +
> +	if (hdr->req.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> +	    hdr->resp.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> +	    hdr->req.msg_size >= sizeof(struct gdma_req_hdr) &&
> +	    hdr->resp.msg_size >= sizeof(struct gdma_resp_hdr) &&
> +	    hdr->req.msg_type != 0 && hdr->resp.msg_type != 0)
> +		return true;
> +
> +	return false;
> +}
> +
> +static inline bool is_gdma_msg_len(const u32 req_len, const u32 resp_len,
> +				   const void *req)
> +{
> +	struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> +
> +	if (req_len >= sizeof(struct gdma_req_hdr) &&
> +	    resp_len >= sizeof(struct gdma_resp_hdr) &&
> +	    req_len >= hdr->req.msg_size && resp_len >= hdr->resp.msg_size &&
> +	    is_gdma_msg(req)) {
> +		return true;
> +	}
> +
> +	return false;
> +}

You missed adding the mana_ prefix here. There might be others.

> +#define CQE_POLLING_BUFFER 512
> +struct ana_eq {
> +	struct gdma_queue *eq;
> +	struct gdma_comp cqe_poll[CQE_POLLING_BUFFER];
> +};

> +static int ana_poll(struct napi_struct *napi, int budget)
> +{

You also have a few cases of ana_, not mana_. There might be others.

    Andrew

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Andrew Lunn]

> +static void mana_gd_deregiser_irq(struct gdma_queue *queue)
> +{
> +	struct gdma_dev *gd = queue->gdma_dev;
> +	struct gdma_irq_context *gic;
> +	struct gdma_context *gc;
> +	struct gdma_resource *r;
> +	unsigned int msix_index;
> +	unsigned long flags;
> +
> +	/* At most num_online_cpus() + 1 interrupts are used. */
> +	msix_index = queue->eq.msix_index;
> +	if (WARN_ON(msix_index > num_online_cpus()))
> +		return;

Do you handle hot{un}plug of CPUs?

> +static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
> +					struct gdma_event *event)
> +{
> +	struct hw_channel_context *hwc = ctx;
> +	struct gdma_dev *gd = hwc->gdma_dev;
> +	union hwc_init_type_data type_data;
> +	union hwc_init_eq_id_db eq_db;
> +	u32 type, val;
> +
> +	switch (event->type) {
> +	case GDMA_EQE_HWC_INIT_EQ_ID_DB:
> +		eq_db.as_uint32 = event->details[0];
> +		hwc->cq->gdma_eq->id = eq_db.eq_id;
> +		gd->doorbell = eq_db.doorbell;
> +		break;
> +
> +	case GDMA_EQE_HWC_INIT_DATA:
> +
> +		type_data.as_uint32 = event->details[0];
> +
> +	case GDMA_EQE_HWC_INIT_DONE:
> +		complete(&hwc->hwc_init_eqe_comp);
> +		break;

...

> +	default:
> +		WARN_ON(1);
> +		break;
> +	}

Are these events from the firmware? If you have newer firmware with an
older driver, are you going to spam the kernel log with WARN_ON dumps?

> +static int mana_move_wq_tail(struct gdma_queue *wq, u32 num_units)
> +{
> +	u32 used_space_old;
> +	u32 used_space_new;
> +
> +	used_space_old = wq->head - wq->tail;
> +	used_space_new = wq->head - (wq->tail + num_units);
> +
> +	if (used_space_new > used_space_old) {
> +		WARN_ON(1);
> +		return -ERANGE;
> +	}

You could replace the 1 by the condition. There are a couple of these.

    Andrew

RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, April 12, 2021 8:16 AM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: davem@davemloft.net; kuba@kernel.org; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> bernd@petrovitsch.priv.at; rdunlap@infradead.org; Shachar Raindel
> <shacharr@microsoft.com>; linux-kernel@vger.kernel.org; linux-
> hyperv@vger.kernel.org
> Subject: Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> > +static inline bool is_gdma_msg(const void *req) {
> > +	struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> > +
> > +	if (hdr->req.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> > +	    hdr->resp.hdr_type == GDMA_STANDARD_HEADER_TYPE &&
> > +	    hdr->req.msg_size >= sizeof(struct gdma_req_hdr) &&
> > +	    hdr->resp.msg_size >= sizeof(struct gdma_resp_hdr) &&
> > +	    hdr->req.msg_type != 0 && hdr->resp.msg_type != 0)
> > +		return true;
> > +
> > +	return false;
> > +}
> > +
> > +static inline bool is_gdma_msg_len(const u32 req_len, const u32 resp_len,
> > +				   const void *req)
> > +{
> > +	struct gdma_req_hdr *hdr = (struct gdma_req_hdr *)req;
> > +
> > +	if (req_len >= sizeof(struct gdma_req_hdr) &&
> > +	    resp_len >= sizeof(struct gdma_resp_hdr) &&
> > +	    req_len >= hdr->req.msg_size && resp_len >= hdr->resp.msg_size
> &&
> > +	    is_gdma_msg(req)) {
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> 
> You missed adding the mana_ prefix here. There might be others.
> 
> > +#define CQE_POLLING_BUFFER 512
> > +struct ana_eq {
> > +	struct gdma_queue *eq;
> > +	struct gdma_comp cqe_poll[CQE_POLLING_BUFFER]; };
> 
> > +static int ana_poll(struct napi_struct *napi, int budget) {
> 
> You also have a few cases of ana_, not mana_. There might be others.

We will rename the remaining ana_ to mana_.
Also the driver version to protocol version in your previous comments.

Thanks,
- Haiyang

RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Haiyang Zhang]

> -----Original Message-----
> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Monday, April 12, 2021 8:32 AM
> To: Dexuan Cui <decui@microsoft.com>
> Cc: davem@davemloft.net; kuba@kernel.org; KY Srinivasan
> <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; wei.liu@kernel.org; Wei Liu
> <liuwe@microsoft.com>; netdev@vger.kernel.org; leon@kernel.org;
> bernd@petrovitsch.priv.at; rdunlap@infradead.org; Shachar Raindel
> <shacharr@microsoft.com>; linux-kernel@vger.kernel.org; linux-
> hyperv@vger.kernel.org
> Subject: Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure
> Network Adapter (MANA)
> 
> > +static void mana_gd_deregiser_irq(struct gdma_queue *queue) {
> > +	struct gdma_dev *gd = queue->gdma_dev;
> > +	struct gdma_irq_context *gic;
> > +	struct gdma_context *gc;
> > +	struct gdma_resource *r;
> > +	unsigned int msix_index;
> > +	unsigned long flags;
> > +
> > +	/* At most num_online_cpus() + 1 interrupts are used. */
> > +	msix_index = queue->eq.msix_index;
> > +	if (WARN_ON(msix_index > num_online_cpus()))
> > +		return;
> 
> Do you handle hot{un}plug of CPUs?
We don't have hot{un}plug of CPU feature yet.

> 
> > +static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue
> *q_self,
> > +					struct gdma_event *event)
> > +{
> > +	struct hw_channel_context *hwc = ctx;
> > +	struct gdma_dev *gd = hwc->gdma_dev;
> > +	union hwc_init_type_data type_data;
> > +	union hwc_init_eq_id_db eq_db;
> > +	u32 type, val;
> > +
> > +	switch (event->type) {
> > +	case GDMA_EQE_HWC_INIT_EQ_ID_DB:
> > +		eq_db.as_uint32 = event->details[0];
> > +		hwc->cq->gdma_eq->id = eq_db.eq_id;
> > +		gd->doorbell = eq_db.doorbell;
> > +		break;
> > +
> > +	case GDMA_EQE_HWC_INIT_DATA:
> > +
> > +		type_data.as_uint32 = event->details[0];
> > +
> > +	case GDMA_EQE_HWC_INIT_DONE:
> > +		complete(&hwc->hwc_init_eqe_comp);
> > +		break;
> 
> ...
> 
> > +	default:
> > +		WARN_ON(1);
> > +		break;
> > +	}
> 
> Are these events from the firmware? If you have newer firmware with an
> older driver, are you going to spam the kernel log with WARN_ON dumps?
For protocol version mismatch, the host and guest will either negotiate the 
highest common version, or fail to probe. So this kind of warnings are not 
expected.

> 
> > +static int mana_move_wq_tail(struct gdma_queue *wq, u32 num_units) {
> > +	u32 used_space_old;
> > +	u32 used_space_new;
> > +
> > +	used_space_old = wq->head - wq->tail;
> > +	used_space_new = wq->head - (wq->tail + num_units);
> > +
> > +	if (used_space_new > used_space_old) {
> > +		WARN_ON(1);
> > +		return -ERANGE;
> > +	}
> 
> You could replace the 1 by the condition. There are a couple of these.
Will do.

Thanks,
- Haiyang

Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Jakub Kicinski]

On Sun, 11 Apr 2021 19:34:55 -0700 Dexuan Cui wrote:
> +	for (i = 0; i < ANA_INDIRECT_TABLE_SIZE; i++)
> +		apc->indir_table[i] = i % apc->num_queues;

ethtool_rxfh_indir_default()

> +	err = mana_cfg_vport_steering(apc, rx, true, update_hash, update_tab);
> +	return err;

return mana_...

please fix everywhere.

> +	netif_set_real_num_tx_queues(ndev, apc->num_queues);
> +
> +	err = mana_add_rx_queues(apc, ndev);
> +	if (err)
> +		goto destroy_vport;
> +
> +	apc->rss_state = apc->num_queues > 1 ? TRI_STATE_TRUE : TRI_STATE_FALSE;
> +
> +	netif_set_real_num_rx_queues(ndev, apc->num_queues);

netif_set_real_num_.. can fail.

> +	rtnl_lock();
> +
> +	netdev_lockdep_set_classes(ndev);
> +
> +	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM;
> +	ndev->hw_features |= NETIF_F_RXCSUM;
> +	ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
> +	ndev->hw_features |= NETIF_F_RXHASH;
> +	ndev->features = ndev->hw_features;
> +	ndev->vlan_features = 0;
> +
> +	err = register_netdevice(ndev);
> +	if (err) {
> +		netdev_err(ndev, "Unable to register netdev.\n");
> +		goto destroy_vport;
> +	}
> +
> +	rtnl_unlock();
> +
> +	return 0;
> +destroy_vport:
> +	rtnl_unlock();

Why do you take rtnl_lock() explicitly around this code?

> +static int mana_set_channels(struct net_device *ndev,
> +			     struct ethtool_channels *channels)
> +{
> +	struct ana_port_context *apc = netdev_priv(ndev);
> +	unsigned int new_count;
> +	unsigned int old_count;
> +	int err, err2;
> +
> +	new_count = channels->combined_count;
> +	old_count = apc->num_queues;
> +
> +	if (new_count < 1 || new_count > apc->max_queues ||
> +	    channels->rx_count || channels->tx_count || channels->other_count)

All these checks should be done by the core already.

> +		return -EINVAL;
> +
> +	if (new_count == old_count)
> +		return 0;

And so is this one.

> +	err = mana_detach(ndev);
> +	if (err) {
> +		netdev_err(ndev, "mana_detach failed: %d\n", err);
> +		return err;
> +	}

RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Haiyang Zhang <haiyangz@microsoft.com>
> Sent: Monday, April 12, 2021 7:40 AM
> > From: Andrew Lunn <andrew@lunn.ch>
> > Sent: Monday, April 12, 2021 8:32 AM
> > > ...
> > > +	/* At most num_online_cpus() + 1 interrupts are used. */
> > > +	msix_index = queue->eq.msix_index;
> > > +	if (WARN_ON(msix_index > num_online_cpus()))
> > > +		return;
> >
> > Do you handle hot{un}plug of CPUs?
> We don't have hot{un}plug of CPU feature yet.

Hyper-V doesn't support vCPU hot plug/unplug for VMs running on it,
but potentially the VM is able to offline and online its virtual CPUs,
though this is not a typical usage at all in production system (to offline
a vCPU, the VM also needs to unbind all the para-virtualized devices'
vmbus channels from that vCPU, which is kind of undoable in a VM
that has less than about 32 vCPUs, because typically all the vCPUs are
bound to one of the vmbus channels of the NetVSC device(s) and
StorVSC device(s), and can't be (easily) unbound.

So I think the driver does need to handle cpu online/offlining properly,
but I think we can do that in some future patch, because IMO that would
need more careful consideration. IMO here the WARN_ON() is good as
it at least lets us know something unexpected (if any) happens.

> > > +static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue
> > *q_self,
> > > +					struct gdma_event *event)
> > > +{
> > > +	struct hw_channel_context *hwc = ctx;
> > > +	struct gdma_dev *gd = hwc->gdma_dev;
> > > +	union hwc_init_type_data type_data;
> > > +	union hwc_init_eq_id_db eq_db;
> > > +	u32 type, val;
> > > +
> > > +	switch (event->type) {
> > > +	case GDMA_EQE_HWC_INIT_EQ_ID_DB:
> > > +		eq_db.as_uint32 = event->details[0];
> > > +		hwc->cq->gdma_eq->id = eq_db.eq_id;
> > > +		gd->doorbell = eq_db.doorbell;
> > > +		break;
> > > +
> > > +	case GDMA_EQE_HWC_INIT_DATA:
> > > +
> > > +		type_data.as_uint32 = event->details[0];
> > > +
> > > +	case GDMA_EQE_HWC_INIT_DONE:
> > > +		complete(&hwc->hwc_init_eqe_comp);
> > > +		break;
> >
> > ...
> >
> > > +	default:
> > > +		WARN_ON(1);
> > > +		break;
> > > +	}
> >
> > Are these events from the firmware? If you have newer firmware with an
> > older driver, are you going to spam the kernel log with WARN_ON dumps?
> For protocol version mismatch, the host and guest will either negotiate the
> highest common version, or fail to probe. So this kind of warnings are not
> expected.

I agree, but I think we'd better remove the WARN_ON(1), which was mainly
for debugging purposem, and was added just in case.

Thanks,
Dexuan

RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui]

> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, April 12, 2021 11:21 AM
> ... 
> On Sun, 11 Apr 2021 19:34:55 -0700 Dexuan Cui wrote:
> > +	for (i = 0; i < ANA_INDIRECT_TABLE_SIZE; i++)
> > +		apc->indir_table[i] = i % apc->num_queues;
> 
> ethtool_rxfh_indir_default()

Will use ethtool_rxfh_indir_default().

> > +	err = mana_cfg_vport_steering(apc, rx, true, update_hash, update_tab);
> > +	return err;
> 
> return mana_...
> 
> please fix everywhere.

Will fix this one, and will review if there is any similar issue.

> > +	netif_set_real_num_tx_queues(ndev, apc->num_queues);
> > +
> > +	err = mana_add_rx_queues(apc, ndev);
> > +	if (err)
> > +		goto destroy_vport;
> > +
> > +	apc->rss_state = apc->num_queues > 1 ? TRI_STATE_TRUE :
> TRI_STATE_FALSE;
> > +
> > +	netif_set_real_num_rx_queues(ndev, apc->num_queues);
> 
> netif_set_real_num_.. can fail.

Will fix the error handling.

> > +	rtnl_lock();
> > +
> > +	netdev_lockdep_set_classes(ndev);
> > +
> > +	ndev->hw_features = NETIF_F_SG | NETIF_F_IP_CSUM |
> NETIF_F_IPV6_CSUM;
> > +	ndev->hw_features |= NETIF_F_RXCSUM;
> > +	ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
> > +	ndev->hw_features |= NETIF_F_RXHASH;
> > +	ndev->features = ndev->hw_features;
> > +	ndev->vlan_features = 0;
> > +
> > +	err = register_netdevice(ndev);
> > +	if (err) {
> > +		netdev_err(ndev, "Unable to register netdev.\n");
> > +		goto destroy_vport;
> > +	}
> > +
> > +	rtnl_unlock();
> > +
> > +	return 0;
> > +destroy_vport:
> > +	rtnl_unlock();
> 
> Why do you take rtnl_lock() explicitly around this code?

It looks like there is no good reason, and I guess we just copied
the code from netvsc_probe(), where the RTNL lock is indeed
explicitly needed.

Will change to directly use register_netdev(), which gets and
release the RTNL lock automatically.

> > +static int mana_set_channels(struct net_device *ndev,
> > +			     struct ethtool_channels *channels)
> > +{
> > +	struct ana_port_context *apc = netdev_priv(ndev);
> > +	unsigned int new_count;
> > +	unsigned int old_count;
> > +	int err, err2;
> > +
> > +	new_count = channels->combined_count;
> > +	old_count = apc->num_queues;
> > +
> > +	if (new_count < 1 || new_count > apc->max_queues ||
> > +	    channels->rx_count || channels->tx_count ||
> channels->other_count)
> 
> All these checks should be done by the core already.
> 
> > +		return -EINVAL;
> > +
> > +	if (new_count == old_count)
> > +		return 0;
> 
> And so is this one.

Will change the code to avoid unnecessary checking.

Thanks,
Dexuan