Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
       [not found] <20210412023455.45594-1-decui@microsoft.com>
@ 2021-04-12  7:45 ` Leon Romanovsky
  2021-04-12  8:35   ` Dexuan Cui
  2021-04-12 12:15 ` Andrew Lunn
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2021-04-12  7:45 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: davem, kuba, kys, haiyangz, sthemmin, wei.liu, liuwe, netdev,
	andrew, bernd, rdunlap, shacharr, linux-kernel, linux-hyperv

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12  7:45 ` [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA) Leon Romanovsky
@ 2021-04-12  8:35   ` Dexuan Cui
  2021-04-12  8:52     ` Leon Romanovsky
  0 siblings, 1 reply; 11+ messages in thread
From: Dexuan Cui @ 2021-04-12  8:35 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: davem, kuba, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	wei.liu, Wei Liu, netdev, andrew, bernd, rdunlap,
	Shachar Raindel, linux-kernel, linux-hyperv

> 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12  8:35   ` Dexuan Cui
@ 2021-04-12  8:52     ` Leon Romanovsky
  2021-04-12 12:10       ` Andrew Lunn
  0 siblings, 1 reply; 11+ messages in thread
From: Leon Romanovsky @ 2021-04-12  8:52 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: davem, kuba, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
	wei.liu, Wei Liu, netdev, andrew, bernd, rdunlap,
	Shachar Raindel, linux-kernel, linux-hyperv

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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12  8:52     ` Leon Romanovsky
@ 2021-04-12 12:10       ` Andrew Lunn
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Lunn @ 2021-04-12 12:10 UTC (permalink / raw)
  To: Leon Romanovsky
  Cc: Dexuan Cui, davem, kuba, KY Srinivasan, Haiyang Zhang,
	Stephen Hemminger, wei.liu, Wei Liu, netdev, bernd, rdunlap,
	Shachar Raindel, linux-kernel, linux-hyperv

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
       [not found] <20210412023455.45594-1-decui@microsoft.com>
  2021-04-12  7:45 ` [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA) Leon Romanovsky
@ 2021-04-12 12:15 ` Andrew Lunn
  2021-04-12 14:29   ` Haiyang Zhang
  2021-04-12 12:32 ` Andrew Lunn
  2021-04-12 18:21 ` Jakub Kicinski
  3 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2021-04-12 12:15 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: davem, kuba, kys, haiyangz, sthemmin, wei.liu, liuwe, netdev,
	leon, bernd, rdunlap, shacharr, linux-kernel, linux-hyperv

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
       [not found] <20210412023455.45594-1-decui@microsoft.com>
  2021-04-12  7:45 ` [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA) Leon Romanovsky
  2021-04-12 12:15 ` Andrew Lunn
@ 2021-04-12 12:32 ` Andrew Lunn
  2021-04-12 14:39   ` Haiyang Zhang
  2021-04-12 18:21 ` Jakub Kicinski
  3 siblings, 1 reply; 11+ messages in thread
From: Andrew Lunn @ 2021-04-12 12:32 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: davem, kuba, kys, haiyangz, sthemmin, wei.liu, liuwe, netdev,
	leon, bernd, rdunlap, shacharr, linux-kernel, linux-hyperv

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12 12:15 ` Andrew Lunn
@ 2021-04-12 14:29   ` Haiyang Zhang
  0 siblings, 0 replies; 11+ messages in thread
From: Haiyang Zhang @ 2021-04-12 14:29 UTC (permalink / raw)
  To: Andrew Lunn, Dexuan Cui
  Cc: davem, kuba, KY Srinivasan, Stephen Hemminger, wei.liu, Wei Liu,
	netdev, leon, bernd, rdunlap, Shachar Raindel, linux-kernel,
	linux-hyperv



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12 12:32 ` Andrew Lunn
@ 2021-04-12 14:39   ` Haiyang Zhang
  2021-04-12 20:30     ` Dexuan Cui
  0 siblings, 1 reply; 11+ messages in thread
From: Haiyang Zhang @ 2021-04-12 14:39 UTC (permalink / raw)
  To: Andrew Lunn, Dexuan Cui
  Cc: davem, kuba, KY Srinivasan, Stephen Hemminger, wei.liu, Wei Liu,
	netdev, leon, bernd, rdunlap, Shachar Raindel, linux-kernel,
	linux-hyperv



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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
       [not found] <20210412023455.45594-1-decui@microsoft.com>
                   ` (2 preceding siblings ...)
  2021-04-12 12:32 ` Andrew Lunn
@ 2021-04-12 18:21 ` Jakub Kicinski
  2021-04-12 20:43   ` Dexuan Cui
  3 siblings, 1 reply; 11+ messages in thread
From: Jakub Kicinski @ 2021-04-12 18:21 UTC (permalink / raw)
  To: Dexuan Cui
  Cc: davem, kys, haiyangz, sthemmin, wei.liu, liuwe, netdev, leon,
	andrew, bernd, rdunlap, shacharr, linux-kernel, linux-hyperv

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12 14:39   ` Haiyang Zhang
@ 2021-04-12 20:30     ` Dexuan Cui
  0 siblings, 0 replies; 11+ messages in thread
From: Dexuan Cui @ 2021-04-12 20:30 UTC (permalink / raw)
  To: Haiyang Zhang, Andrew Lunn
  Cc: davem, kuba, KY Srinivasan, Stephen Hemminger, wei.liu, Wei Liu,
	netdev, leon, bernd, rdunlap, Shachar Raindel, linux-kernel,
	linux-hyperv

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* RE: [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)
  2021-04-12 18:21 ` Jakub Kicinski
@ 2021-04-12 20:43   ` Dexuan Cui
  0 siblings, 0 replies; 11+ messages in thread
From: Dexuan Cui @ 2021-04-12 20:43 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: davem, KY Srinivasan, Haiyang Zhang, Stephen Hemminger, wei.liu,
	Wei Liu, netdev, leon, andrew, bernd, rdunlap, Shachar Raindel,
	linux-kernel, linux-hyperv

> 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

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <20210412023455.45594-1-decui@microsoft.com>
2021-04-12  7:45 ` [PATCH v4 net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA) Leon Romanovsky
2021-04-12  8:35   ` Dexuan Cui
2021-04-12  8:52     ` Leon Romanovsky
2021-04-12 12:10       ` Andrew Lunn
2021-04-12 12:15 ` Andrew Lunn
2021-04-12 14:29   ` Haiyang Zhang
2021-04-12 12:32 ` Andrew Lunn
2021-04-12 14:39   ` Haiyang Zhang
2021-04-12 20:30     ` Dexuan Cui
2021-04-12 18:21 ` Jakub Kicinski
2021-04-12 20:43   ` Dexuan Cui

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git