RE: linux-next: Tree for Jul 26

[Rosen, Rami]

Hi Sergey,
Paolo Abeni had sent a patch:
https://www.mail-archive.com/netdev@vger.kernel.org/msg179192.html

Regards,
Rami Rosen

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Sergey Senozhatsky
Sent: Wednesday, July 26, 2017 13:49
To: Paolo Abeni ; Stephen Rothwell 
Cc: Linux-Next Mailing List ; Linux Kernel Mailing 
List ; Paul Moore ; David S. 
Miller ; net...@vger.kernel.org
Subject: Re: linux-next: Tree for Jul 26

Hello,

On (07/26/17 16:12), Stephen Rothwell wrote:
> Hi all,
> 
> Changes since 20170725:
> 
> Non-merge commits (relative to Linus' tree): 2358
>  2466 files changed, 86994 insertions(+), 44655 deletions(-)


dce4551cb2adb1ac ("udp: preserve head state for IP_CMSG_PASSSEC") causes a 
build error

net/ipv4/udp.c: In function ‘__udp_queue_rcv_skb’:
net/ipv4/udp.c:1789:49: error: ‘struct sk_buff’ has no member named ‘sp’; did 
you mean ‘sk’?
  if (likely(IPCB(skb)->opt.optlen == 0 && !skb->sp))
 ^

-ss

RE: linux-next: Tree for Jul 26

[Rosen, Rami]

Hi Sergey,
Paolo Abeni had sent a patch:
https://www.mail-archive.com/netdev@vger.kernel.org/msg179192.html

Regards,
Rami Rosen

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Sergey Senozhatsky
Sent: Wednesday, July 26, 2017 13:49
To: Paolo Abeni ; Stephen Rothwell 
Cc: Linux-Next Mailing List ; Linux Kernel Mailing 
List ; Paul Moore ; David S. 
Miller ; net...@vger.kernel.org
Subject: Re: linux-next: Tree for Jul 26

Hello,

On (07/26/17 16:12), Stephen Rothwell wrote:
> Hi all,
> 
> Changes since 20170725:
> 
> Non-merge commits (relative to Linus' tree): 2358
>  2466 files changed, 86994 insertions(+), 44655 deletions(-)


dce4551cb2adb1ac ("udp: preserve head state for IP_CMSG_PASSSEC") causes a 
build error

net/ipv4/udp.c: In function ‘__udp_queue_rcv_skb’:
net/ipv4/udp.c:1789:49: error: ‘struct sk_buff’ has no member named ‘sp’; did 
you mean ‘sk’?
  if (likely(IPCB(skb)->opt.optlen == 0 && !skb->sp))
 ^

-ss

RE: [ANNOUNCE] bridge-utils 1.6 release

[Rosen, Rami]

Hi, Alexander, 

>This link seems to be broken.

Seems that it should be:
https://www.kernel.org/pub/linux/utils/net/bridge-utils/bridge-utils-1.6.tar.gz
Instead of:
>   
> http://www.kernel.org/pub/linux/utils/net/bridge-utils/bridge-utils.1.6.tar.gz

Regards,
Rami Rosen
Intel Corporation

RE: [ANNOUNCE] bridge-utils 1.6 release

[Rosen, Rami]

Hi, Alexander, 

>This link seems to be broken.

Seems that it should be:
https://www.kernel.org/pub/linux/utils/net/bridge-utils/bridge-utils-1.6.tar.gz
Instead of:
>   
> http://www.kernel.org/pub/linux/utils/net/bridge-utils/bridge-utils.1.6.tar.gz

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

[Rosen, Rami]

Hi, Amir,

>This patch is the communication with the FW.
>The network functionality is added in the next patches in the series 
>and with it, more messages from FW.
>Indeed this function always returns true in this patch, 
>but while writing it, I predicted that the network functionality will 
>use the send_event flag differently.
>You can see the documentation of send_event - currently unused in this patch.
>I don't see any harm that in this patch, this function will always return true,
>while applying the rest of the series will change it.

Ok, seems reasonable.

>The prototype of suspend is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

>The prototype of resume is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

You are right about this, I missed the macro invocation in your code, 
SET_SYSTEM_SLEEP_PM_OPS(nhi_suspend, nhi_resume),
apologies.

 Regards,
 Rami Rosen
 Intel Corporation

RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

[Rosen, Rami]

Hi, Amir,

>This patch is the communication with the FW.
>The network functionality is added in the next patches in the series 
>and with it, more messages from FW.
>Indeed this function always returns true in this patch, 
>but while writing it, I predicted that the network functionality will 
>use the send_event flag differently.
>You can see the documentation of send_event - currently unused in this patch.
>I don't see any harm that in this patch, this function will always return true,
>while applying the rest of the series will change it.

Ok, seems reasonable.

>The prototype of suspend is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

>The prototype of resume is return int:
>http://lxr.free-electrons.com/source/include/linux/pm.h#L295

You are right about this, I missed the macro invocation in your code, 
SET_SYSTEM_SLEEP_PM_OPS(nhi_suspend, nhi_resume),
apologies.

 Regards,
 Rami Rosen
 Intel Corporation

RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

[Rosen, Rami]

Hi Amir,
Here are my 2 cents:

This method always returns true, should be void (unless you will change 
PDF_ERROR_NOTIFICATION  or other pdf values to return false), and  likewise its 
invocation should not check return value.

> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> + enum pdf_value pdf,
> + const u8 *msg, u32 msg_len)
> +{
> + /*
> +  * preparation for messages that won't be sent,
> +  * currently unused in this patch.
> +  */
> + bool send_event = true;
> +
> + switch (pdf) {
> + case PDF_ERROR_NOTIFICATION:
> + dev_err(_ctxt->pdev->dev,
> + "controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> + nhi_ctxt->id, msg[11], msg_len);
> + /* fallthrough */
> + case PDF_WRITE_CONFIGURATION_REGISTERS:
> + /* fallthrough */
> + case PDF_READ_CONFIGURATION_REGISTERS:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(_ctxt->send_sem);
> + }
> + break;
> +
> + case PDF_FW_TO_SW_RESPONSE:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(_ctxt->send_sem);
> + }
> + break;
> +
> + default:
> + dev_warn(_ctxt->pdev->dev,
> +  "controller id %#x pdf %u isn't handled/expected\n",
> +  nhi_ctxt->id, pdf);
> + break;
> + }
> +
> + return send_event;
> +}
> +

This methods always returns 0, should be void.

> +static int nhi_suspend(struct device *dev) __releases(_ctxt-
> >send_sem)
> +{
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + void __iomem *rx_reg, *tx_reg;
> + u32 rx_reg_val, tx_reg_val;
> +
> + /* must be after negotiation_events, since messages might be sent
> */
> + nhi_ctxt->d0_exit = true;
> +
> + rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> +  (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> + tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> +  (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> + /* disable RX flow control  */
> + iowrite32(rx_reg_val, rx_reg);
> + /* disable TX flow control  */
> + iowrite32(tx_reg_val, tx_reg);
> + /* disable RX ring  */
> + iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> + mutex_lock(_ctxt->d0_exit_mailbox_mutex);
> + mutex_lock(_ctxt->d0_exit_send_mutex);
> +
> + cancel_work_sync(_ctxt->icm_msgs_work);
> +
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + nhi_ctxt->ignore_icm_resp = false;
> + /*
> +  * if there is response, it is lost, so unlock the send
> +  * for the next resume.
> +  */
> + up(_ctxt->send_sem);
> + }
> +
> + mutex_unlock(_ctxt->d0_exit_send_mutex);
> + mutex_unlock(_ctxt->d0_exit_mailbox_mutex);
> +
> + /* wait for all TX to finish  */
> + usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> + /* disable all interrupts */
> + iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> + /* disable TX ring  */
> + iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> + return 0;
> +}
> +

This methods also always returns 0, should be void.

> +static int nhi_resume(struct device *dev) __acquires(_ctxt-
> >send_sem)
> +{
> + dma_addr_t phys;
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + struct tbt_buf_desc *desc;
> + void __iomem *iobase = nhi_ctxt->iobase;
> + void __iomem *reg;
> + int i;
> +
> + if (nhi_ctxt->msix_entries) {
> + iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
>   REG_DMA_MISC_INT_AUTO_CLEAR,
> +   iobase + REG_DMA_MISC);
> + /*
> +  * Vector #0, which is TX complete to ICM,
> +  * isn't been used currently.
> +  */
> + nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> + for (i = 2; i < nhi_ctxt->num_vectors; i++)
> + nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> + i);
> + }
> +
> + /* configure TX descriptors */
> + for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> +  i < TBT_ICM_RING_NUM_TX_BUFS;
> +  i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> + desc = _ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> + desc->phys = cpu_to_le64(phys);
> + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> + }
> + /* configure RX 

RE: [PATCH v3 5/8] thunderbolt: Communication with the ICM (firmware)

[Rosen, Rami]

Hi Amir,
Here are my 2 cents:

This method always returns true, should be void (unless you will change 
PDF_ERROR_NOTIFICATION  or other pdf values to return false), and  likewise its 
invocation should not check return value.

> +static bool nhi_msg_from_icm_analysis(struct tbt_nhi_ctxt *nhi_ctxt,
> + enum pdf_value pdf,
> + const u8 *msg, u32 msg_len)
> +{
> + /*
> +  * preparation for messages that won't be sent,
> +  * currently unused in this patch.
> +  */
> + bool send_event = true;
> +
> + switch (pdf) {
> + case PDF_ERROR_NOTIFICATION:
> + dev_err(_ctxt->pdev->dev,
> + "controller id %#x PDF_ERROR_NOTIFICATION %hhu
> msg len %u\n",
> + nhi_ctxt->id, msg[11], msg_len);
> + /* fallthrough */
> + case PDF_WRITE_CONFIGURATION_REGISTERS:
> + /* fallthrough */
> + case PDF_READ_CONFIGURATION_REGISTERS:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(_ctxt->send_sem);
> + }
> + break;
> +
> + case PDF_FW_TO_SW_RESPONSE:
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + up(_ctxt->send_sem);
> + }
> + break;
> +
> + default:
> + dev_warn(_ctxt->pdev->dev,
> +  "controller id %#x pdf %u isn't handled/expected\n",
> +  nhi_ctxt->id, pdf);
> + break;
> + }
> +
> + return send_event;
> +}
> +

This methods always returns 0, should be void.

> +static int nhi_suspend(struct device *dev) __releases(_ctxt-
> >send_sem)
> +{
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + void __iomem *rx_reg, *tx_reg;
> + u32 rx_reg_val, tx_reg_val;
> +
> + /* must be after negotiation_events, since messages might be sent
> */
> + nhi_ctxt->d0_exit = true;
> +
> + rx_reg = nhi_ctxt->iobase + REG_RX_OPTIONS_BASE +
> +  (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + rx_reg_val = ioread32(rx_reg) & ~REG_OPTS_E2E_EN;
> + tx_reg = nhi_ctxt->iobase + REG_TX_OPTIONS_BASE +
> +  (TBT_ICM_RING_NUM * REG_OPTS_STEP);
> + tx_reg_val = ioread32(tx_reg) & ~REG_OPTS_E2E_EN;
> + /* disable RX flow control  */
> + iowrite32(rx_reg_val, rx_reg);
> + /* disable TX flow control  */
> + iowrite32(tx_reg_val, tx_reg);
> + /* disable RX ring  */
> + iowrite32(rx_reg_val & ~REG_OPTS_VALID, rx_reg);
> +
> + mutex_lock(_ctxt->d0_exit_mailbox_mutex);
> + mutex_lock(_ctxt->d0_exit_send_mutex);
> +
> + cancel_work_sync(_ctxt->icm_msgs_work);
> +
> + if (nhi_ctxt->wait_for_icm_resp) {
> + nhi_ctxt->wait_for_icm_resp = false;
> + nhi_ctxt->ignore_icm_resp = false;
> + /*
> +  * if there is response, it is lost, so unlock the send
> +  * for the next resume.
> +  */
> + up(_ctxt->send_sem);
> + }
> +
> + mutex_unlock(_ctxt->d0_exit_send_mutex);
> + mutex_unlock(_ctxt->d0_exit_mailbox_mutex);
> +
> + /* wait for all TX to finish  */
> + usleep_range(5 * USEC_PER_MSEC, 7 * USEC_PER_MSEC);
> +
> + /* disable all interrupts */
> + iowrite32(0, nhi_ctxt->iobase + REG_RING_INTERRUPT_BASE);
> + /* disable TX ring  */
> + iowrite32(tx_reg_val & ~REG_OPTS_VALID, tx_reg);
> +
> + return 0;
> +}
> +

This methods also always returns 0, should be void.

> +static int nhi_resume(struct device *dev) __acquires(_ctxt-
> >send_sem)
> +{
> + dma_addr_t phys;
> + struct tbt_nhi_ctxt *nhi_ctxt = pci_get_drvdata(to_pci_dev(dev));
> + struct tbt_buf_desc *desc;
> + void __iomem *iobase = nhi_ctxt->iobase;
> + void __iomem *reg;
> + int i;
> +
> + if (nhi_ctxt->msix_entries) {
> + iowrite32(ioread32(iobase + REG_DMA_MISC) |
> +
>   REG_DMA_MISC_INT_AUTO_CLEAR,
> +   iobase + REG_DMA_MISC);
> + /*
> +  * Vector #0, which is TX complete to ICM,
> +  * isn't been used currently.
> +  */
> + nhi_set_int_vec(nhi_ctxt, 0, 1);
> +
> + for (i = 2; i < nhi_ctxt->num_vectors; i++)
> + nhi_set_int_vec(nhi_ctxt, nhi_ctxt->num_paths -
> (i/2),
> + i);
> + }
> +
> + /* configure TX descriptors */
> + for (i = 0, phys = nhi_ctxt->icm_ring_shared_mem_dma_addr;
> +  i < TBT_ICM_RING_NUM_TX_BUFS;
> +  i++, phys += TBT_ICM_RING_MAX_FRAME_SIZE) {
> + desc = _ctxt->icm_ring_shared_mem->tx_buf_desc[i];
> + desc->phys = cpu_to_le64(phys);
> + desc->attributes = cpu_to_le32(DESC_ATTR_REQ_STS);
> + }
> + /* configure RX 

RE: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi all, 

A very limited review below.

+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_GET_FEATURE = 8,

Instead /* get capabilities  SHOULD BE:  /* set capabilities .
+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_SET_FEATURE = 9,
+
..


+int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
+{
+   struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
...

You set ret=-EINVAL, but you do not use this ret as you immediately return 0 in 
the next line, which is the end of the method. Either return ret or return 
-EINVAL.
+   if (unlikely(ret)) {
+   ena_trc_err("Failed to set hash input. error: %d\n", ret);
+   ret = -EINVAL;
+   }
+
+   return 0;
+}
+



+
+/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
+ * @ena_dev: ENA communication layer struct


Instead realess_supported: SHOULD BE: readless_supported

+ * @realess_supported: readless mode (enable/disable)
+ */
+void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
+   bool readless_supported);
+



+
+/* ena_com_create_io_queue - Create io queue.
+ * @ena_dev: ENA communication layer struct

Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx

+ * ena_com_create_io_ctx - create context structure
+ *
+ * Create the submission and the completion queues.
+ *
+ * @return - 0 on success, negative value on failure.
+ */
+int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
+   struct ena_com_create_io_ctx *ctx);
+




+/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
+ * @ena_dev: ENA communication layer struct

Missing: @qid

+ */
+void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
+

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH net-next] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi all, 

A very limited review below.

+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_GET_FEATURE = 8,

Instead /* get capabilities  SHOULD BE:  /* set capabilities .
+
+   /* get capabilities of particular feature */
+   ENA_ADMIN_SET_FEATURE = 9,
+
..


+int ena_com_set_hash_ctrl(struct ena_com_dev *ena_dev)
+{
+   struct ena_com_admin_queue *admin_queue = _dev->admin_queue;
...

You set ret=-EINVAL, but you do not use this ret as you immediately return 0 in 
the next line, which is the end of the method. Either return ret or return 
-EINVAL.
+   if (unlikely(ret)) {
+   ena_trc_err("Failed to set hash input. error: %d\n", ret);
+   ret = -EINVAL;
+   }
+
+   return 0;
+}
+



+
+/* ena_com_set_mmio_read_mode - Enable/disable the mmio reg read mechanism
+ * @ena_dev: ENA communication layer struct


Instead realess_supported: SHOULD BE: readless_supported

+ * @realess_supported: readless mode (enable/disable)
+ */
+void ena_com_set_mmio_read_mode(struct ena_com_dev *ena_dev,
+   bool readless_supported);
+



+
+/* ena_com_create_io_queue - Create io queue.
+ * @ena_dev: ENA communication layer struct

Instead ena_com_create_io_ctx SHOULD BE: @ena_com_create_io_ctx

+ * ena_com_create_io_ctx - create context structure
+ *
+ * Create the submission and the completion queues.
+ *
+ * @return - 0 on success, negative value on failure.
+ */
+int ena_com_create_io_queue(struct ena_com_dev *ena_dev,
+   struct ena_com_create_io_ctx *ctx);
+




+/* ena_com_admin_destroy - Destroy IO queue with the queue id - qid.
+ * @ena_dev: ENA communication layer struct

Missing: @qid

+ */
+void ena_com_destroy_io_queue(struct ena_com_dev *ena_dev, u16 qid);
+

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi, Netanel,

+into 5 levels and assignes interrupt delay value to each level.
Should be: assigns

+The ENA device AQ and AENQ are allocated on probe and freed ontermination.
Should be: on termination.

+   /* commit previously loaded firmare */
Should be: firmware

+static int ena_com_hash_key_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   if (rss->hash_key)
+   dma_free_coherent(ena_dev->dmadev,
+ sizeof(*rss->hash_key),
+ rss->hash_key,
+ rss->hash_key_dma_addr);
+   rss->hash_key = NULL;
+   return 0;
+}

This method always returns 0.

+static int ena_com_hash_ctrl_init(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   rss->hash_ctrl = dma_alloc_coherent(ena_dev->dmadev,
+   sizeof(*rss->hash_ctrl),
+   >hash_ctrl_dma_addr,
+   GFP_KERNEL | __GFP_ZERO);
+
+   return 0;
+}
+

This method also always returns 0.


+static int ena_com_hash_ctrl_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   if (rss->hash_ctrl)
+   dma_free_coherent(ena_dev->dmadev,
+ sizeof(*rss->hash_ctrl),
+ rss->hash_ctrl,
+ rss->hash_ctrl_dma_addr);
+   rss->hash_ctrl = NULL;
+
+   return 0;
+}
+

This method also always returns 0.

+static int ena_com_indirect_table_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+   size_t tbl_size = (1 << rss->tbl_log_size) *
+   sizeof(struct ena_admin_rss_ind_table_entry);
+
+   if (rss->rss_ind_tbl)
+   dma_free_coherent(ena_dev->dmadev,
+ tbl_size,
+ rss->rss_ind_tbl,
+ rss->rss_ind_tbl_dma_addr);
+   rss->rss_ind_tbl = NULL;
+
+   if (rss->host_rss_ind_tbl)
+   devm_kfree(ena_dev->dmadev, rss->host_rss_ind_tbl);
+   rss->host_rss_ind_tbl = NULL;
+
+   return 0;
+}

This method also always returns 0.

+int ena_com_rss_destroy(struct ena_com_dev *ena_dev)
+{
+   ena_com_indirect_table_destroy(ena_dev);
+   ena_com_hash_key_destroy(ena_dev);
+   ena_com_hash_ctrl_destroy(ena_dev);
+
+   memset(_dev->rss, 0x0, sizeof(ena_dev->rss));
+
+   return 0;
+}

This method also always returns 0.

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH] net: ena: Add a driver for Amazon Elastic Network Adapters (ENA)

[Rosen, Rami]

Hi, Netanel,

+into 5 levels and assignes interrupt delay value to each level.
Should be: assigns

+The ENA device AQ and AENQ are allocated on probe and freed ontermination.
Should be: on termination.

+   /* commit previously loaded firmare */
Should be: firmware

+static int ena_com_hash_key_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   if (rss->hash_key)
+   dma_free_coherent(ena_dev->dmadev,
+ sizeof(*rss->hash_key),
+ rss->hash_key,
+ rss->hash_key_dma_addr);
+   rss->hash_key = NULL;
+   return 0;
+}

This method always returns 0.

+static int ena_com_hash_ctrl_init(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   rss->hash_ctrl = dma_alloc_coherent(ena_dev->dmadev,
+   sizeof(*rss->hash_ctrl),
+   >hash_ctrl_dma_addr,
+   GFP_KERNEL | __GFP_ZERO);
+
+   return 0;
+}
+

This method also always returns 0.


+static int ena_com_hash_ctrl_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+
+   if (rss->hash_ctrl)
+   dma_free_coherent(ena_dev->dmadev,
+ sizeof(*rss->hash_ctrl),
+ rss->hash_ctrl,
+ rss->hash_ctrl_dma_addr);
+   rss->hash_ctrl = NULL;
+
+   return 0;
+}
+

This method also always returns 0.

+static int ena_com_indirect_table_destroy(struct ena_com_dev *ena_dev)
+{
+   struct ena_rss *rss = _dev->rss;
+   size_t tbl_size = (1 << rss->tbl_log_size) *
+   sizeof(struct ena_admin_rss_ind_table_entry);
+
+   if (rss->rss_ind_tbl)
+   dma_free_coherent(ena_dev->dmadev,
+ tbl_size,
+ rss->rss_ind_tbl,
+ rss->rss_ind_tbl_dma_addr);
+   rss->rss_ind_tbl = NULL;
+
+   if (rss->host_rss_ind_tbl)
+   devm_kfree(ena_dev->dmadev, rss->host_rss_ind_tbl);
+   rss->host_rss_ind_tbl = NULL;
+
+   return 0;
+}

This method also always returns 0.

+int ena_com_rss_destroy(struct ena_com_dev *ena_dev)
+{
+   ena_com_indirect_table_destroy(ena_dev);
+   ena_com_hash_key_destroy(ena_dev);
+   ena_com_hash_ctrl_destroy(ena_dev);
+
+   memset(_dev->rss, 0x0, sizeof(ena_dev->rss));
+
+   return 0;
+}

This method also always returns 0.

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH] cgroup: Fix misspelling of CONFIG_SOCK_CGROUP_DATA in comments

[Rosen, Rami]

Hi, 

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -604,11 +604,11 @@ static inline struct cgroup *sock_cgroup_ptr(struct 
sock_cgroup_data *skcd)
 #endif
 }

In this occasion, seems that maybe something else is also missing:
Shouldn't it be hereafter : +#else /* !CONFIG_SOCK_CGROUP_DATA */ 
instead ? 
   
-#else  /* CONFIG_CGROUP_DATA */
+#else  /* CONFIG_SOCK_CGROUP_DATA */
 

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH] cgroup: Fix misspelling of CONFIG_SOCK_CGROUP_DATA in comments

[Rosen, Rami]

Hi, 

--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -604,11 +604,11 @@ static inline struct cgroup *sock_cgroup_ptr(struct 
sock_cgroup_data *skcd)
 #endif
 }

In this occasion, seems that maybe something else is also missing:
Shouldn't it be hereafter : +#else /* !CONFIG_SOCK_CGROUP_DATA */ 
instead ? 
   
-#else  /* CONFIG_CGROUP_DATA */
+#else  /* CONFIG_SOCK_CGROUP_DATA */
 

Regards,
Rami Rosen
Intel Corporation

RE: [RFC PATCH net-next 1/1] net: Support for switch port configuration

[Rosen, Rami]

Hi, all,

Regarding preferring using netlink sockets versus ethtool IOCTLs for setting 
kernel network attributes from userspace, I fully agree with Marco. The netlink 
API is much more structured and
much more geared towards this type of operation, than the IOCTL-based ethtool. 

Regards,
Rami Rosen
Software Engineer, Intel

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Varlese, Marco
Sent: Friday, December 12, 2014 11:20
To: Roopa Prabhu; Jiri Pirko
Cc: John Fastabend; net...@vger.kernel.org; step...@networkplumber.org; 
Fastabend, John R; sfel...@gmail.com; linux-kernel@vger.kernel.org
Subject: RE: [RFC PATCH net-next 1/1] net: Support for switch port configuration

> -Original Message-
> From: Roopa Prabhu [mailto:ro...@cumulusnetworks.com]
> Sent: Thursday, December 11, 2014 5:41 PM
> To: Jiri Pirko
> Cc: Varlese, Marco; John Fastabend; net...@vger.kernel.org; 
> step...@networkplumber.org; Fastabend, John R; sfel...@gmail.com; 
> linux-kernel@vger.kernel.org
> Subject: Re: [RFC PATCH net-next 1/1] net: Support for switch port 
> configuration
> 
> On 12/11/14, 8:56 AM, Jiri Pirko wrote:
> > Thu, Dec 11, 2014 at 05:37:46PM CET, ro...@cumulusnetworks.com wrote:
> >> On 12/11/14, 3:01 AM, Jiri Pirko wrote:
> >>> Thu, Dec 11, 2014 at 10:59:42AM CET, marco.varl...@intel.com wrote:
> > -Original Message-
> > From: John Fastabend [mailto:john.fastab...@gmail.com]
> > Sent: Wednesday, December 10, 2014 5:04 PM
> > To: Jiri Pirko
> > Cc: Varlese, Marco; net...@vger.kernel.org; 
> > step...@networkplumber.org; Fastabend, John R; 
> > ro...@cumulusnetworks.com; sfel...@gmail.com; linux- 
> > ker...@vger.kernel.org
> > Subject: Re: [RFC PATCH net-next 1/1] net: Support for switch 
> > port configuration
> >
> > On 12/10/2014 08:50 AM, Jiri Pirko wrote:
> >> Wed, Dec 10, 2014 at 05:23:40PM CET, marco.varl...@intel.com
> wrote:
> >>> From: Marco Varlese 
> >>>
> >>> Switch hardware offers a list of attributes that are 
> >>> configurable on a per port basis.
> >>> This patch provides a mechanism to configure switch ports by 
> >>> adding an NDO for setting specific values to specific attributes.
> >>> There will be a separate patch that extends iproute2 to call 
> >>> the new NDO.
> >> What are these attributes? Can you give some examples. I'm 
> >> asking because there is a plan to pass generic attributes to 
> >> switch ports replacing current specific 
> >> ndo_switch_port_stp_update. In this case, bridge is setting that 
> >> attribute.
> >>
> >> Is there need to set something directly from userspace or does 
> >> it make rather sense to use involved bridge/ovs/bond ? I think 
> >> that both will be needed.
> > +1
> >
> > I think for many attributes it would be best to have both. The 
> > in kernel callers and netlink userspace can use the same driver ndo_ops.
> >
> > But then we don't _require_ any specific bridge/ovs/etc module.
> > And we may have some attributes that are not specific to any 
> > existing software module. I'm guessing Marco has some examples 
> > of
> these.
> >
> > [...]
> >
> >
> > --
> > John Fastabend Intel Corporation
>  We do have a need to configure the attributes directly from 
>  user-space
> and I have identified the tool to do that in iproute2.
> 
>  An example of attributes are:
>  * enabling/disabling of learning of source addresses on a given 
>  port (you can imagine the attribute called LEARNING for example);
>  * internal loopback control (i.e. LOOPBACK) which will control 
>  how the flow of traffic behaves from the switch fabric towards an 
>  egress port;
>  * flooding for broadcast/multicast/unicast type of packets (i.e.
>  BFLOODING, MFLOODING, UFLOODING);
> 
>  Some attributes would be of the type enabled/disabled while other 
>  will
> allow specific values to allow the user to configure different 
> behaviours of that feature on that particular port on that platform.
> 
>  One thing to mention - as John stated as well - there might be 
>  some
> attributes that are not specific to any software module but rather 
> have to do with the actual hardware/platform to configure.
> 
>  I hope this clarifies some points.
> >>> It does. Makes sense. We need to expose this attr set/get for both 
> >>> in-kernel and userspace use cases.
> >>>
> >>> Please adjust you patch for this. Also, as a second patch, it 
> >>> would be great if you can convert ndo_switch_port_stp_update to 
> >>> this new
> ndo.
> >> Why are we exposing generic switch attribute get/set from userspace 
> >> ?. We already have specific attributes for learning/flooding which 
> >> can be extended further.
> > Yes, but that is for PF_BRIDGE and bridge specific 

RE: [RFC PATCH net-next 1/1] net: Support for switch port configuration

[Rosen, Rami]

Hi, all,

Regarding preferring using netlink sockets versus ethtool IOCTLs for setting 
kernel network attributes from userspace, I fully agree with Marco. The netlink 
API is much more structured and
much more geared towards this type of operation, than the IOCTL-based ethtool. 

Regards,
Rami Rosen
Software Engineer, Intel

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Varlese, Marco
Sent: Friday, December 12, 2014 11:20
To: Roopa Prabhu; Jiri Pirko
Cc: John Fastabend; net...@vger.kernel.org; step...@networkplumber.org; 
Fastabend, John R; sfel...@gmail.com; linux-kernel@vger.kernel.org
Subject: RE: [RFC PATCH net-next 1/1] net: Support for switch port configuration

 -Original Message-
 From: Roopa Prabhu [mailto:ro...@cumulusnetworks.com]
 Sent: Thursday, December 11, 2014 5:41 PM
 To: Jiri Pirko
 Cc: Varlese, Marco; John Fastabend; net...@vger.kernel.org; 
 step...@networkplumber.org; Fastabend, John R; sfel...@gmail.com; 
 linux-kernel@vger.kernel.org
 Subject: Re: [RFC PATCH net-next 1/1] net: Support for switch port 
 configuration
 
 On 12/11/14, 8:56 AM, Jiri Pirko wrote:
  Thu, Dec 11, 2014 at 05:37:46PM CET, ro...@cumulusnetworks.com wrote:
  On 12/11/14, 3:01 AM, Jiri Pirko wrote:
  Thu, Dec 11, 2014 at 10:59:42AM CET, marco.varl...@intel.com wrote:
  -Original Message-
  From: John Fastabend [mailto:john.fastab...@gmail.com]
  Sent: Wednesday, December 10, 2014 5:04 PM
  To: Jiri Pirko
  Cc: Varlese, Marco; net...@vger.kernel.org; 
  step...@networkplumber.org; Fastabend, John R; 
  ro...@cumulusnetworks.com; sfel...@gmail.com; linux- 
  ker...@vger.kernel.org
  Subject: Re: [RFC PATCH net-next 1/1] net: Support for switch 
  port configuration
 
  On 12/10/2014 08:50 AM, Jiri Pirko wrote:
  Wed, Dec 10, 2014 at 05:23:40PM CET, marco.varl...@intel.com
 wrote:
  From: Marco Varlese marco.varl...@intel.com
 
  Switch hardware offers a list of attributes that are 
  configurable on a per port basis.
  This patch provides a mechanism to configure switch ports by 
  adding an NDO for setting specific values to specific attributes.
  There will be a separate patch that extends iproute2 to call 
  the new NDO.
  What are these attributes? Can you give some examples. I'm 
  asking because there is a plan to pass generic attributes to 
  switch ports replacing current specific 
  ndo_switch_port_stp_update. In this case, bridge is setting that 
  attribute.
 
  Is there need to set something directly from userspace or does 
  it make rather sense to use involved bridge/ovs/bond ? I think 
  that both will be needed.
  +1
 
  I think for many attributes it would be best to have both. The 
  in kernel callers and netlink userspace can use the same driver ndo_ops.
 
  But then we don't _require_ any specific bridge/ovs/etc module.
  And we may have some attributes that are not specific to any 
  existing software module. I'm guessing Marco has some examples 
  of
 these.
 
  [...]
 
 
  --
  John Fastabend Intel Corporation
  We do have a need to configure the attributes directly from 
  user-space
 and I have identified the tool to do that in iproute2.
 
  An example of attributes are:
  * enabling/disabling of learning of source addresses on a given 
  port (you can imagine the attribute called LEARNING for example);
  * internal loopback control (i.e. LOOPBACK) which will control 
  how the flow of traffic behaves from the switch fabric towards an 
  egress port;
  * flooding for broadcast/multicast/unicast type of packets (i.e.
  BFLOODING, MFLOODING, UFLOODING);
 
  Some attributes would be of the type enabled/disabled while other 
  will
 allow specific values to allow the user to configure different 
 behaviours of that feature on that particular port on that platform.
 
  One thing to mention - as John stated as well - there might be 
  some
 attributes that are not specific to any software module but rather 
 have to do with the actual hardware/platform to configure.
 
  I hope this clarifies some points.
  It does. Makes sense. We need to expose this attr set/get for both 
  in-kernel and userspace use cases.
 
  Please adjust you patch for this. Also, as a second patch, it 
  would be great if you can convert ndo_switch_port_stp_update to 
  this new
 ndo.
  Why are we exposing generic switch attribute get/set from userspace 
  ?. We already have specific attributes for learning/flooding which 
  can be extended further.
  Yes, but that is for PF_BRIDGE and bridge specific attributes. There 
  might be another generic attrs, no?
 I cant think of any. And plus, the whole point of switchdev l2 
 offloads was to map these to existing bridge attributes. And we 
 already have a match for some of the attributes that marco wants.
 
 If there is a need for such attributes, i don't see why it is needed 
 for switch devices only.
 It is needed for any hw (nics etc). And, a precedence to this is to do 
 it via