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 ; netdev@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: [PATCH net-next] genetlink: remove ops_list from genetlink header.

[Rosen, Rami]

Hi, Sergei,

Thanks! I will send V2 with 12 digits for the commit.

Regards,
Rami Rosen


-Original Message-
From: Sergei Shtylyov [mailto:sergei.shtyl...@cogentembedded.com] 
Sent: Sunday, June 04, 2017 20:18
To: Rosen, Rami <rami.ro...@intel.com>; da...@davemloft.net
Cc: netdev@vger.kernel.org; Berg, Johannes <johannes.b...@intel.com>
Subject: Re: [PATCH net-next] genetlink: remove ops_list from genetlink header.

Hello!

On 6/4/2017 10:07 AM, Rami Rosen wrote:

> commit d91824 ("genetlink: register family ops as array") removed the

At least 12 digits required.

> ops_list member from both genl_family and genl_ops; while the 
> documentation of genl_family was updated accordingly by this patch, 
> ops_list remained in the documentation of the genl_ops object.
> This patch fixes it by removing ops_list from genl_ops documentation.
>
> Signed-off-by: Rami Rosen <rami.ro...@intel.com>
[...]

MNR, Sergei

RE: [PATCH net-next 1/8] net: ipv4: refactor __ip_route_output_key_hash

[Rosen, Rami]

Hi, Rupa /David Ahern,

First, thanks for this patch set!

Second, it seems to me that something might be incorrect here.

You have these additions in this patch  (1/8):
...
+struct rtable *ip_route_output_key_hash_rcu(struct net *net, struct flowi4 
*flp,
+   const struct sk_buff *skb,
+   struct fib_result *res);
...
+struct rtable *ip_route_output_key_hash(struct net *net, struct flowi4 *fl4,
+   const struct sk_buff *skb)
+{
+   struct fib_result res;
+   struct rtable *rth;
+
+   res.tclassid= 0;
+   res.fi  = NULL;
+   res.table   = NULL;
+
+   rcu_read_lock();
+   rth = ip_route_output_key_hash_rcu(net, fl4, , mp_hash);
rcu_read_unlock();
+
return rth;
 }
-EXPORT_SYMBOL_GPL(__ip_route_output_key_hash);
+EXPORT_SYMBOL_GPL(ip_route_output_key_hash);
 

So the third parameter to ip_route_output_key_hash_rcu() should be skb*, and 
the fourth parameter should be fib_result *. However, you do not pass the skb 
parameter 
when calling ip_route_output_key_hash_rcu() in 
ip_route_output_key_hash()  (in fact you don't use it at all),  and you pass 
mp_hash as the fourth parameter.

Regards,
Rami Rosen
Intel Corporation

RE: iproute2: using .maxattr = IFLA_VLAN_MAX??

[Rosen, Rami]

Hi, Murali,

> This appears to be a cut-n-paste bug as the source is based on 
> ip/iplink_vlan.c and should be fixed to IFLA_HSR_MAX. 

You are right.
The "HSR section" indeed defines IFLA_HSR_MAX in include/linux/if_link.h of 
iproute2 as the max attributes number.

And in the top of ip/iplink_hsr.c you will find this comment, which enhances 
your assumption about the origin of this mistake:

/*
...
 Based on iplink_vlan.c by Patrick McHardy 

RE: [RFC PATCH net-next 4/5] bridge: vlan lwt and dst_metadata netlink support

[Rosen, Rami]

Hi, Roopa,

Two minor comments:

The parameter br is not used in the br_add_vlan_tunnel_info() method, it should 
be removed:

+static int br_add_vlan_tunnel_info(struct net_bridge *br,
+  struct net_bridge_port *p, int cmd,
+  u16 vid, u32 tun_id)
+{
+   int err;
+
+   switch (cmd) {
+   case RTM_SETLINK:
+   if (p) {
+   /* if the MASTER flag is set this will act on the global
+* per-VLAN entry as well
+*/
+   err = nbp_vlan_tunnel_info_add(p, vid, tun_id);
+   if (err)
+   break;
+   } else {
+   return -EINVAL;
+   }
+
+   break;
+
+   case RTM_DELLINK:
+   if (p)
+   nbp_vlan_tunnel_info_delete(p, vid);
+   else
+   return -EINVAL;
+   break;
+   }
+
+   return 0;
+}
+

The parameter br is used inside br_process_vlan_tunnel_info() only in the two 
Cases, when br_add_vlan_tunnel_info() is invoked. Since we saw earlier that it 
should be removed from br_add_vlan_tunnel_info(), it should also be removed 
from br_process_vlan_tunnel_info() as it is not needed anymore:

+static int br_process_vlan_tunnel_info(struct net_bridge *br,
+  struct net_bridge_port *p, int cmd,
+  struct vtunnel_info *tinfo_curr,
+  struct vtunnel_info *tinfo_last) {
+   int t, v;
+   int err;
+
+   if (tinfo_curr->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN) {
+   if (tinfo_last->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN)
+   return -EINVAL;
+   memcpy(tinfo_last, tinfo_curr, sizeof(struct vtunnel_info));
+   } else if (tinfo_curr->flags & BRIDGE_VLAN_INFO_RANGE_END) {
+   if (!(tinfo_last->flags & BRIDGE_VLAN_INFO_RANGE_BEGIN))
+   return -EINVAL;
+   if ((tinfo_curr->vid - tinfo_last->vid) !=
+   (tinfo_curr->tunid - tinfo_last->tunid))
+   return -EINVAL;
+   /* XXX: tun id and vlan id attrs must be same
+*/
+   t = tinfo_last->tunid;
+   for (v = tinfo_last->vid; v <= tinfo_curr->vid; v++) {
+   err = br_add_vlan_tunnel_info(br, p, cmd,
+ v, t);
+   if (err)
+   return err;
+   t++;
+   }
+   memset(tinfo_last, 0, sizeof(struct vtunnel_info));
+   memset(tinfo_curr, 0, sizeof(struct vtunnel_info));
+   } else {
+   err = br_add_vlan_tunnel_info(br, p, cmd,
+ tinfo_curr->vid,
+ tinfo_curr->tunid);
+   if (err)
+   return err;
+   }
+
+   return 0;
+}
+

Regards,
Rami Rosen

RE: [PATCH iproute2] Revert "man pages: add man page for skbmod action"

[Rosen, Rami]

Acked-by: Rami Rosen 

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Jiri Benc
Sent: Wednesday, January 18, 2017 14:37
To: netdev@vger.kernel.org
Cc: Lucas Bates ; Jamal Hadi Salim ; 
Stephen Hemminger 
Subject: [PATCH iproute2] Revert "man pages: add man page for skbmod action"

This reverts commit a40995d1c79e5a1b8711f6cd26eca9807fc4dd50.

The patch is missing the actual tc-skbmod.8 file which causes 'make install' to 
fail:

install -m 0755 -d /tmp/ip/usr/share/man/man8 install -m 0644 ip-address.8 
ip-link.8 ip-route.8 ip.8 arpd.8 lnstat.8
routel.8 rtacct.8 rtmon.8 rtpr.8 ss.8 tc.8 tc-bfifo.8 tc-bpf.8 tc-cbq.8
tc-cbq-details.8 tc-choke.8 tc-codel.8 tc-fq.8 tc-drr.8 tc-ematch.8
tc-fq_codel.8 tc-hfsc.8 tc-htb.8 tc-pie.8 tc-mqprio.8 tc-netem.8 tc-pfifo.8
tc-pfifo_fast.8 tc-prio.8 tc-red.8 tc-sfb.8 tc-sfq.8 tc-stab.8 tc-tbf.8
bridge.8 rtstat.8 ctstat.8 nstat.8 routef.8 ip-addrlabel.8 ip-fou.8 ip-gue.8
ip-l2tp.8 ip-macsec.8 ip-maddress.8 ip-monitor.8 ip-mroute.8 ip-neighbour.8
ip-netns.8 ip-ntable.8 ip-rule.8 ip-tunnel.8 ip-xfrm.8 ip-tcp_metrics.8
ip-netconf.8 ip-token.8 tipc.8 tipc-bearer.8 tipc-link.8 tipc-media.8
tipc-nametable.8 tipc-node.8 tipc-socket.8 tc-basic.8 tc-cgroup.8 tc-flow.8
tc-flower.8 tc-fw.8 tc-route.8 tc-tcindex.8 tc-u32.8 tc-matchall.8
tc-connmark.8 tc-csum.8 tc-mirred.8 tc-nat.8 tc-pedit.8 tc-police.8
tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8  tc-ife.8 tc-skbmod.8
tc-tunnel_key.8 devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8
devlink-sb.8 /tmp/ip/usr/share/man/man8
install: cannot stat ‘tc-skbmod.8’: No such file or directory
make[2]: *** [install] Error 1
make[1]: *** [install] Error 2

Signed-off-by: Jiri Benc 
---
 man/man8/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/man/man8/Makefile b/man/man8/Makefile index 
d4cb01ac3f13..77d347ca98fc 100644
--- a/man/man8/Makefile
+++ b/man/man8/Makefile
@@ -16,7 +16,7 @@ MAN8PAGES = $(TARGETS) ip.8 arpd.8 lnstat.8 routel.8 rtacct.8 
rtmon.8 rtpr.8 ss.
tc-basic.8 tc-cgroup.8 tc-flow.8 tc-flower.8 tc-fw.8 tc-route.8 \
tc-tcindex.8 tc-u32.8 tc-matchall.8 \
tc-connmark.8 tc-csum.8 tc-mirred.8 tc-nat.8 tc-pedit.8 tc-police.8 \
-   tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8  tc-ife.8 tc-skbmod.8 \
+   tc-simple.8 tc-skbedit.8 tc-vlan.8 tc-xt.8  tc-ife.8 \
tc-tunnel_key.8 \
devlink.8 devlink-dev.8 devlink-monitor.8 devlink-port.8 devlink-sb.8
 
--
1.8.3.1

RE: [net-next 15/21] ixgbe: Configure advertised speeds correctly for KR/KX backplane

[Rosen, Rami]

Hi,

Hi,

On this occasion, also the next can be fixed:

> From: Don Skidmore 
>
> This patch ensures that the advertised link speeds are configured for 
> X553 KR/KX backplane.  Without this patch the link remains at 1G when 
> resuming form low power after being downshifted by LPLU.

Should be "from" instead "from", I would assume.

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH iproute2 2/2] tc/act_tunnel: Introduce ip tunnel action

[Rosen, Rami]

Hi, Amir,

Following are three minor comments:

Seems that TCA_TUNNEL_KEY_PAD used anywhere:
 
+   TCA_TUNNEL_KEY_PAD,
+   __TCA_TUNNEL_KEY_MAX,
+};


Should be "and destination IP 11.11.0.2" instead of  "and destination IP 
11.11.0.1":

+Tunnel ID (for example VNI in VXLAN tunnel) .TP .B src_ip Outer header 
+source IP address (IPv4 or IPv6) .TP .B dst_ip Outer header destination 
+IP address (IPv4 or IPv6) .RE .SH EXAMPLES The following example 
+encapsulates incoming ICMP packets on eth0 into a vxlan tunnel by 
+setting metadata to VNI 11, source IP 11.11.0.1 and destination IP
+11.11.0.1 by forwarding the skb with the metadata to device vxlan0, 
+which will prepare the VXLAN headers:
+
+.RS
+.EX
+#tc qdisc add dev eth0 handle : ingress #tc filter add dev eth0 
+protocol ip parent : \\
+  flower \\
+ip_proto icmp \\
+  action tunnel_key set \\
+src_ip 11.11.0.1 \\
+dst_ip 11.11.0.2 \\
+id 11 \\


Typo: should be "ip tunnel" instead of "ip tunel":

+ * m_tunnel_key.c  ip tunel manipulation module
+ *
+ *  This program is free software; you can redistribute it and/or

Keep on the good work!

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 net-next V5 4/4] net/sched: Introduce act_tunnel_key

[Rosen, Rami]

Hi, Hadar,

>For example, the following flower filter will forward all ICMP packets 
>destined to 11.11.11.2 >through the shared vxlan device 'vxlan0'. Before 
>redirecting, a metadata for the vxlan tunnel >is created using the tunnel_key 
>action and it's arguments:

Shouldn't it be "tc filter add dev ..."?

>$ filter add dev net0 protocol ip parent : \
>flower \
>  ip_proto 1 \
>  dst_ip 11.11.11.2 \
>action tunnel_key set \
>  src_ip 11.11.0.1 \
>  dst_ip 11.11.0.2 \
>  id 11 \
>action mirred egress redirect dev vxlan0

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 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-next RFC] rtnetlink: add new RTM_GETSTATS to dump link stats

[Rosen, Rami]

Hi,

+   if (!dev)
+   return -ENODEV;
+
+   nskb = nlmsg_new(if_nlmsg_stats_size(dev), GFP_KERNEL);
+   if (!nskb)
+   return -ENOBUFS;
+
+   err = rtnl_fill_statsinfo(nskb, dev, RTM_NEWSTATS,
+ NETLINK_CB(skb).portid, nlh->nlmsg_seq, 0, 0);
+   if (err < 0) {

It should be here:  -EMSGSIZE implies BUG in if_nlmsg_stats_size  (instead of 
if_nlmsg_size)

+   /* -EMSGSIZE implies BUG in if_nlmsg_size */
+   WARN_ON(err == -EMSGSIZE);
+   kfree_skb(nskb);
+   } else {
+   err = rtnl_unicast(nskb, net, NETLINK_CB(skb).portid);
+   }


Other than that, it seems ok, thanks for this patch!

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH iproute2] bridge: add support for dynamic fdb entries

[Rosen, Rami]

+1

Rami Rosen
Intel Corporation

RE: [patch net-next RFC 0/6] Introduce devlink interface and first drivers to use it

[Rosen, Rami]

Hi,

>Btw, if you add this tool into iproute2 (which would be preferred), >then
>probably dl should be ok (and easier retrievable in that relation).

+1.

This tool, which uses netlink messages, seems a natural candidate for iproute2.
 
And also apart from "ip", we have also another short command in iproute2, 
namely "tc", which is yet another justification for using the short form with 
"dl".

Regards,
Rami Rosen
Intel Corporation

RE: [PATCH iproute2] bridge: support for static fdb entries

[Rosen, Rami]

+1
'temp' seems indeed not to be intuitive enough as 'static' in this context.

Regards,
Rami Rosen
Intel Corporation


-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org] On 
Behalf Of Roopa Prabhu
Sent: Wednesday, January 27, 2016 7:10 PM
To: step...@networkplumber.org
Cc: netdev@vger.kernel.org; w...@cumulusnetworks.com; 
sco...@cumulusnetworks.com; niko...@cumulusnetworks.com; 
makita.toshi...@lab.ntt.co.jp
Subject: [PATCH iproute2] bridge: support for static fdb entries

From: Roopa Prabhu 

There is no intuitive option to add static fdb entries today.
'temp' seems to have a side effect of adding
'static' fdb entries. But the name and intent
of 'temp' does not say anything about it being static.

example:
bridge fdb add operates as follows:

$bridge fdb add 00:01:02:03:04:05 dev eth0 master
$bridge fdb add 00:01:02:03:04:06 dev eth0 master temp
$bridge fdb add 00:01:02:03:04:07 dev eth0 master local

$bridge fdb show
00:01:02:03:04:05 dev eth0 permanent
00:01:02:03:04:06 dev eth0 static
00:01:02:03:04:07 dev eth0 permanent
00:01:02:03:04:08 dev eth0 <<== dynamic, ageable learned mac

This patch adds a new bridge fdb type 'static' which
makes sure NUD_NOARP and NUD_REACHABLE is set for static
entries. This effectively is nothing but what 'temp'
does today. But the name 'temp' is misleading.

After the patch:
$bridge fdb add 00:01:02:03:04:06 dev eth0 master static

$bridge fdb show
00:01:02:03:04:06 dev eth0 static

'temp' could ideally be a dynamic mac that can age (ie just
NUD_REACHABLE). But, 'temp' sets 'NUD_NOARP' and 'NUD_REACHABLE'.
Too late to change 'temp' now. But, we are thinking of introduing a
'dynamic' keyword after this patch that only sets NUD_REACHABLE.

Signed-off-by: Wilson Kok 
Signed-off-by: Roopa Prabhu 
---
Will submit another patch to document bridge fdb options
once we agree on the behaviour and this patch is accepted.

 bridge/fdb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/bridge/fdb.c b/bridge/fdb.c
index 4d10925..9bc6b94 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -33,7 +33,7 @@ static void usage(void)
 {
fprintf(stderr, "Usage: bridge fdb { add | append | del | replace } 
ADDR dev DEV\n"
"  [ self ] [ master ] [ use ] [ router ]\n"
-   "  [ local | temp ] [ dst IPADDR ] [ vlan 
VID ]\n"
+   "  [ local | temp | static ] [ dst IPADDR ] 
[ vlan VID ]\n"
"  [ port PORT] [ vni VNI ] [ via DEV ]\n");
fprintf(stderr, "   bridge fdb [ show [ br BRDEV ] [ brport DEV ] 
]\n");
exit(-1);
@@ -301,7 +301,8 @@ static int fdb_modify(int cmd, int flags, int argc, char 
**argv)
} else if (matches(*argv, "local") == 0||
   matches(*argv, "permanent") == 0) {
req.ndm.ndm_state |= NUD_PERMANENT;
-   } else if (matches(*argv, "temp") == 0) {
+   } else if (matches(*argv, "temp") == 0 ||
+  matches(*argv, "static") == 0) {
req.ndm.ndm_state |= NUD_REACHABLE;
} else if (matches(*argv, "vlan") == 0) {
if (vid >= 0)
-- 
1.9.1

RE: HW communication debugging interface - ideas?

[Rosen, Rami]

Hi, Jiri,

>In our case, we put message into skb and  ...

Just to be sure we are on the same page:

By "In our case" - I assume you are referring to the mlxsw Ethernet switch 
driver, right ? 

>we put message into skb and push that out as an ordinary packet. >HW then > 
>sends us reply in a packet, similar to other rx-ed packets.

Are you referring here to messages of the EMAD protocol ?

Rami Rosen
Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch net-next RFC 0/6] switchdev: introduce tranction enfra and for pre-commit split

[Rosen, Rami]

Hi,

>introduce tranction enfra and for pre-commit split

Typo:
Instead "tranction enfra" should be "transaction infrastructure".

Regards,
Rami Rosen
Intel Corporation
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: list of all network namespaces

[Rosen, Rami]

Hi,

>Presumably you could copy what "ip netns" does, which appears to be to look in 
>/var/run/netns .  At least that is what an strace of that >command suggests.

This is true, but keep in mind that the output of "ip netns", as well as 
listing the contents of /var/run/netns, reflects only network namespaces
which were created with the "ip netns" command. The "ip netns" userspace 
implementation consists of code which enables this,
by creating /var/run/netns, bind mounting it, etc.

Network namespaces which were created by other ways (like userspace applications
using the clone() system call) will *not* be reflected by neither of them.

Regards,
Rami Rosen
Intel Corporation


N�r��yb�X��ǧv�^�)޺{.n�+���z�^�)w*jg����ݢj/���z�ޖ��2�ޙ&�)ߡ�a�����G���h��j:+v���w��٥

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Rosen, Rami]

Hi, Prem, 

I recall that there was a patch proposed to prevent ageing of fdb entried by 
bridge in kernel, when the fdb entry was added due to notification by switch 
device.  Please see:
http://www.spinics.net/lists/netdev/msg314770.html

Somehow the patch is not visible in the net-next pull.

It seems that this patch was reverted:
http://lists.openwall.net/netdev/2015/02/05/129
https://lkml.org/lkml/2015/2/11/12

Regards,
Rami Rosen
Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] bridge: Enable configuration of ageing interval for bridges and switch devices.

[Rosen, Rami]

Hi,

First, I agree about the need to propagate the ageing interval to switchdev 
devices, so that hardware based aging can be setup correctly.

Second, in this occasion, I want to mention the need to 
turn off bridge ageing in the kernel as part of using switchdev devices. This 
is mentioned in
https://kernel.googlesource.com/pub/scm/linux/kernel/git/davem/net-next/+/master/Documentation/networking/switchdev.txt:

...
XXX: how to turn off ageing in kernel on a per-port basis or
otherwise prevent the kernel from ageing out the FDB entry?
...

One can think of the option of using value 0 of the ageing interval as an 
indication to turn off bridge ageing in the kernel, and any other value bigger 
than MIN_AGEING_INTERVAL_SECS to turn on bridge ageing.

As another option for a *per-port* boolean flag for enabling/disabling ageing, 
one can think of adding an IFLA_BRPORT_AGEING bool flag (and BR_AGEING) for 
IFLA_PROTINFO.

Regards,
Rami Rosen
Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [patch net-next 4/4] mlxsw: Introduce Mellanox SwitchX-2 ASIC support

[Rosen, Rami]

Hi, Jiri,

Keep on the good work!

The .func  member of the mlxsw_rx_listener object has this prototype:
void (*func)(struct sk_buff *skb, u8 local_port, u16 trap_id, void *priv);

Is the trap_id  parameter needed ?
In the three use cases of .func, which are either 
mlxsw_emad_rx_listener_func(), mlxsw_core_event_listener_func(), or 
mlxsw_sx_rx_listener_func(), this parameter is not used at all.


Regards,
Rami Rosen
Intel Corporation

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html