Re: [PATCH] net, swap: Remove a warning and clarify why sk_mem_reclaim is required when deactivating swap

[Leon Romanovsky]

On Tue, Jun 9, 2015 at 9:40 PM, Jeff Layton jlay...@poochiereds.net wrote:
 From: Mel Gorman mgor...@suse.de

 Jeff Layton reported the following;

  [   74.232485] [ cut here ]
  [   74.233354] WARNING: CPU: 2 PID: 754 at net/core/sock.c:364 
 sk_clear_memalloc+0x51/0x80()
  [   74.234790] Modules linked in: cts rpcsec_gss_krb5 nfsv4 dns_resolver nfs 
 fscache xfs libcrc32c snd_hda_codec_generic snd_hda_intel snd_hda_controller 
 snd_hda_codec snd_hda_core snd_hwdep snd_seq snd_seq_device nfsd snd_pcm 
 snd_timer snd e1000 ppdev parport_pc joydev parport pvpanic soundcore floppy 
 serio_raw i2c_piix4 pcspkr nfs_acl lockd virtio_balloon acpi_cpufreq 
 auth_rpcgss grace sunrpc qxl drm_kms_helper ttm drm virtio_console virtio_blk 
 virtio_pci ata_generic virtio_ring pata_acpi virtio
  [   74.243599] CPU: 2 PID: 754 Comm: swapoff Not tainted 4.1.0-rc6+ #5
  [   74.244635] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
  [   74.245546]   79e69e31 8800d066bde8 
 8179263d
  [   74.246786]    8800d066be28 
 8109e6fa
  [   74.248175]   880118d48000 8800d58f5c08 
 880036e380a8
  [   74.249483] Call Trace:
  [   74.249872]  [8179263d] dump_stack+0x45/0x57
  [   74.250703]  [8109e6fa] warn_slowpath_common+0x8a/0xc0
  [   74.251655]  [8109e82a] warn_slowpath_null+0x1a/0x20
  [   74.252585]  [81661241] sk_clear_memalloc+0x51/0x80
  [   74.253519]  [a0116c72] xs_disable_swap+0x42/0x80 [sunrpc]
  [   74.254537]  [a01109de] rpc_clnt_swap_deactivate+0x7e/0xc0 
 [sunrpc]
  [   74.255610]  [a03e4fd7] nfs_swap_deactivate+0x27/0x30 [nfs]
  [   74.256582]  [811e99d4] destroy_swap_extents+0x74/0x80
  [   74.257496]  [811ecb52] SyS_swapoff+0x222/0x5c0
  [   74.258318]  [81023f27] ? syscall_trace_leave+0xc7/0x140
  [   74.259253]  [81798dae] system_call_fastpath+0x12/0x71
  [   74.260158] ---[ end trace 2530722966429f10 ]---

 The warning in question was unnecessary but with Jeff's series the rules
 are also clearer.  This patch removes the warning and updates the comment
 to explain why sk_mem_reclaim() may still be called.

 Signed-off-by: Mel Gorman mgor...@suse.de
 Acked-by: Jeff Layton jeff.lay...@primarydata.com
 ---
  net/core/sock.c | 12 +---
  1 file changed, 5 insertions(+), 7 deletions(-)

 diff --git a/net/core/sock.c b/net/core/sock.c
 index 292f42228bfb..2bb4c56370e5 100644
 --- a/net/core/sock.c
 +++ b/net/core/sock.c
 @@ -354,14 +354,12 @@ void sk_clear_memalloc(struct sock *sk)

 /*
  * SOCK_MEMALLOC is allowed to ignore rmem limits to ensure forward
 -* progress of swapping. However, if SOCK_MEMALLOC is cleared while
 -* it has rmem allocations there is a risk that the user of the
 -* socket cannot make forward progress due to exceeding the rmem
 -* limits. By rights, sk_clear_memalloc() should only be called
 -* on sockets being torn down but warn and reset the accounting if
 -* that assumption breaks.
 +* progress of swapping. SOCK_MEMALLOC may be cleared while
 +* it has rmem allocations due to the last swapfile being deactivated
 +* but there is a risk that the socket is unusable due to exceeding
 +* the rmem limits. Reclaim the reserves and obey rmem limits again.
  */
 -   if (WARN_ON(sk-sk_forward_alloc))
 +   if (sk-sk_forward_alloc)
You don't really need this IF. if sk-sk_forward_alloc is equal to
zero, it will be less than SK_MEM_QUANTUM.
http://lxr.free-electrons.com/source/include/net/sock.h#L1405

1405 static inline void sk_mem_reclaim(struct sock *sk)
1406 {
1407 if (!sk_has_account(sk))
1408 return;
1409 if (sk-sk_forward_alloc = SK_MEM_QUANTUM)
1410 __sk_mem_reclaim(sk);
1411 }


 sk_mem_reclaim(sk);
  }
  EXPORT_SYMBOL_GPL(sk_clear_memalloc);
 --
 2.4.2

 --
 To unsubscribe, send a message with 'unsubscribe linux-mm' in
 the body to majord...@kvack.org.  For more info on Linux MM,
 see: http://www.linux-mm.org/ .
 Don't email: a href=mailto:d...@kvack.org; em...@kvack.org /a



-- 
Leon Romanovsky | Independent Linux Consultant
www.leon.nu | l...@leon.nu
--
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 v9 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Wed, Jun 01, 2016 at 11:37:50PM +0800, Lijun Ou wrote:
> This patch mainly added icm support for RoCE. It initializes icm
> which managers the relative memory blocks for RoCE. The data
> structures of RoCE will be located in it. For example, CQ table,
> QP table and MTPT table so on.

I wonder if you have the same needs for ICM as it is in mlx4 device.
Do you have firmware?

Thanks


signature.asc
Description: Digital signature

Re: [PATCH v9 05/22] IB/hns: Add initial profile resource

[Leon Romanovsky]

On Wed, Jun 01, 2016 at 11:37:47PM +0800, Lijun Ou wrote:
> This patch mainly configured some profile resoure. For example,
> vendor_id, hardware version, and some data structure sizes so on.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---

<...>

> +#define HNS_ROCE_V1_NUM_COMP_EQE 0x8000
> +#define  HNS_ROCE_V1_NUM_ASYNC_EQE   0x400

Something wrong with indentation.


signature.asc
Description: Digital signature

Re: [PATCH v9 09/22] IB/hns: Add hca support

[Leon Romanovsky]

On Wed, Jun 01, 2016 at 11:37:51PM +0800, Lijun Ou wrote:
> This patch mainly setup hca for RoCE. It will do a series of
> initial works, as follows:
> 1. init uar table, allocate uar resource
> 2. init pd table
> 3. init cq table
> 4. init mr table
> 5. init qp table
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---

<...>

> +
> + ret = hns_roce_bitmap_init(_table->bitmap, hr_dev->caps.num_cqs,
> +hr_dev->caps.num_cqs - 1,
> +hr_dev->caps.reserved_cqs, 0);
> + if (ret)
> + return ret;
> +
> + return 0;

You can return directly ret, instead last 4 lines.

> +}
> +


signature.asc
Description: Digital signature

Re: [PATCH v8 10/22] IB/hns: Add process flow to init RoCE engine

[Leon Romanovsky]

On Wed, May 25, 2016 at 11:05:13PM +0800, Lijun Ou wrote:
> This patch mainly initialized the RoCE engine. It is absolutely
> necessary to run RoCE. It mainly includes that configure DMAE
> user, initialize doorbell and raq operations, enable port.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---
>  drivers/infiniband/hw/hns/hns_roce_common.h | 107 +++
>  drivers/infiniband/hw/hns/hns_roce_device.h |  15 +
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c  | 477 
> 
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.h  |  68 +++-
>  drivers/infiniband/hw/hns/hns_roce_main.c   |  20 ++
>  5 files changed, 686 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h 
> b/drivers/infiniband/hw/hns/hns_roce_common.h
> index 5998778..73c6220 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_common.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
> @@ -53,6 +53,93 @@
>  #define roce_set_bit(origin, shift, val) \
>   roce_set_field((origin), (1ul << (shift)), (shift), (val))
>  
> +#define ROCEE_GLB_CFG_ROCEE_DB_SQ_MODE_S 3
> +#define ROCEE_GLB_CFG_ROCEE_DB_OTH_MODE_S 4
> +
> +#define ROCEE_GLB_CFG_SQ_EXT_DB_MODE_S 5
> +
> +#define ROCEE_GLB_CFG_OTH_EXT_DB_MODE_S 6
> +
> +#define ROCEE_GLB_CFG_ROCEE_PORT_ST_S 10
> +#define ROCEE_GLB_CFG_ROCEE_PORT_ST_M  \
> + (((1UL << 6) - 1) << ROCEE_GLB_CFG_ROCEE_PORT_ST_S)
> +
> +#define ROCEE_GLB_CFG_TRP_RAQ_DROP_EN_S 16
> +
> +#define ROCEE_DMAE_USER_CFG1_ROCEE_STREAM_ID_TB_CFG_S 0
> +#define ROCEE_DMAE_USER_CFG1_ROCEE_STREAM_ID_TB_CFG_M  \
> + (((1UL << 24) - 1) << ROCEE_DMAE_USER_CFG1_ROCEE_STREAM_ID_TB_CFG_S)
> +
> +#define ROCEE_DMAE_USER_CFG1_ROCEE_CACHE_TB_CFG_S 24
> +#define ROCEE_DMAE_USER_CFG1_ROCEE_CACHE_TB_CFG_M  \
> + (((1UL << 4) - 1) << ROCEE_DMAE_USER_CFG1_ROCEE_CACHE_TB_CFG_S)
> +
> +#define ROCEE_DMAE_USER_CFG2_ROCEE_STREAM_ID_PKT_CFG_S 0
> +#define ROCEE_DMAE_USER_CFG2_ROCEE_STREAM_ID_PKT_CFG_M   \
> + (((1UL << 24) - 1) << ROCEE_DMAE_USER_CFG2_ROCEE_STREAM_ID_PKT_CFG_S)
> +
> +#define ROCEE_DMAE_USER_CFG2_ROCEE_CACHE_PKT_CFG_S 24
> +#define ROCEE_DMAE_USER_CFG2_ROCEE_CACHE_PKT_CFG_M   \
> + (((1UL << 4) - 1) << ROCEE_DMAE_USER_CFG2_ROCEE_CACHE_PKT_CFG_S)
> +
> +#define ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_S 0
> +#define ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_M   \
> + (((1UL << 16) - 1) << ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_S)
> +
> +#define ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_EMPTY_S 16
> +#define ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_EMPTY_M   \
> + (((1UL << 16) - 1) << ROCEE_DB_SQ_WL_ROCEE_DB_SQ_WL_EMPTY_S)
> +
> +#define ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_S 0
> +#define ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_M   \
> + (((1UL << 16) - 1) << ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_S)
> +
> +#define ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_EMPTY_S 16
> +#define ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_EMPTY_M   \
> + (((1UL << 16) - 1) << ROCEE_DB_OTHERS_WL_ROCEE_DB_OTH_WL_EMPTY_S)
> +
> +#define ROCEE_RAQ_WL_ROCEE_RAQ_WL_S 0
> +#define ROCEE_RAQ_WL_ROCEE_RAQ_WL_M   \
> + (((1UL << 8) - 1) << ROCEE_RAQ_WL_ROCEE_RAQ_WL_S)
> +
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_POL_TIME_INTERVAL_S 0
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_POL_TIME_INTERVAL_M   \
> + (((1UL << 15) - 1) << \
> + ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_POL_TIME_INTERVAL_S)
> +
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_RAQ_TIMEOUT_CHK_CFG_S 16
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_RAQ_TIMEOUT_CHK_CFG_M   \
> + (((1UL << 4) - 1) << \
> + ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_RAQ_TIMEOUT_CHK_CFG_S)
> +
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_RAQ_TIMEOUT_CHK_EN_S 20
> +
> +#define ROCEE_WRMS_POL_TIME_INTERVAL_WRMS_EXT_RAQ_MODE 21
> +
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_SHIFT_S 0
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_SHIFT_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_SHIFT_S)
> +
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_BA_H_S 5
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_BA_H_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_DB_SQ_H_EXT_DB_SQ_BA_H_S)
> +
> +#define ROCEE_EXT_DB_OTH_H_EXT_DB_OTH_SHIFT_S 0
> +#define ROCEE_EXT_DB_OTH_H_EXT_DB_OTH_SHIFT_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_DB_OTH_H_EXT_DB_OTH_SHIFT_S)
> +
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_OTH_BA_H_S 5
> +#define ROCEE_EXT_DB_SQ_H_EXT_DB_OTH_BA_H_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_DB_SQ_H_EXT_DB_OTH_BA_H_S)
> +
> +#define ROCEE_EXT_RAQ_H_EXT_RAQ_SHIFT_S 0
> +#define ROCEE_EXT_RAQ_H_EXT_RAQ_SHIFT_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_RAQ_H_EXT_RAQ_SHIFT_S)
> +
> +#define ROCEE_EXT_RAQ_H_EXT_RAQ_BA_H_S 8
> +#define ROCEE_EXT_RAQ_H_EXT_RAQ_BA_H_M   \
> + (((1UL << 5) - 1) << ROCEE_EXT_RAQ_H_EXT_RAQ_BA_H_S)
> +
>  #define ROCEE_BT_CMD_H_ROCEE_BT_CMD_IN_MDF_S 0
>  #define ROCEE_BT_CMD_H_ROCEE_BT_CMD_IN_MDF_M   \
>   (((1UL << 19) - 1) << ROCEE_BT_CMD_H_ROCEE_BT_CMD_IN_MDF_S)
> @@ -120,6 +207,26 

Re: [GIT PULL] Please Pull Mellanox Shared Code

[Leon Romanovsky]

On Mon, Jun 13, 2016 at 03:03:01PM +0300, Leon Romanovsky wrote:
> Hi Doug and David,
> 
> This is Mellanox mlx5_core shared code for both net-next and RDMA
> trees for 4.8 kernel cycle.
> 
> This mlx5_ifc update introduces following ConnectX-4 features.
> 
> * netdev part:
>  - MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation support
>  - QoS rate limiting
>  - SQ context rate limiting
>  - Auto negotiation fields in PTYS register
>  - Source SQN field in flow table entry match structure
>  - DCBX parameters
> 
> * RDMA part:
>  - New XRQ opcodes, commands and capabilities layout
>  - Extend Q counters definition to support IB.
> 
> Thank you,
>   Saeed and Leon.
> 
> The following changes since commit af8c34ce6ae32addda3788d54a7e340cad22516b:
> 
>   Linux 4.7-rc2 (2016-06-05 14:31:26 -0700)
> 
> are available in the git repository at:
> 
>   g...@gitolite.kernel.org:pub/scm/linux/kernel/git/leon/linux-rdma 
> tags/shared
> 
> for you to fetch changes up to 7486216b3a0bd26375b17b2cc168a311106cea70:
> 
>   {net,IB}/mlx5: mlx5_ifc updates (2016-06-10 13:29:14 +0300)
> 
> 
> Saeed Mahameed (1):
>   {net,IB}/mlx5: mlx5_ifc updates
> 
>  include/linux/mlx5/mlx5_ifc.h | 275 
> --
>  1 file changed, 263 insertions(+), 12 deletions(-)

From 7486216b3a0bd26375b17b2cc168a311106cea70 Mon Sep 17 00:00:00 2001
From: Saeed Mahameed <sae...@mellanox.com>
Date: Thu, 9 Jun 2016 15:11:34 +0300
Subject: [PATCH] {net,IB}/mlx5: mlx5_ifc updates

Introducing mlx5_ifc updates for upcoming ConnectX-4 features.

Needed bits and hardware structures for mlx5e netdev:
- MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation
  support
- QoS rate limiting
- SQ context rate limiting
- Auto negotiation fields in PTYS register
- Source SQN field in flow table entry match structure
- DCBX parameters

Needed bits and hardware structures for IB:
- New XRQ opcodes, commands and capabilities layout
    - Extend q counters definition to support IB.

Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
Signed-off-by: Leon Romanovsky <leo...@mellanox.com>
Signed-off-by: Leon Romanovsky <l...@kernel.org>
---
 include/linux/mlx5/mlx5_ifc.h | 275 --
 1 file changed, 263 insertions(+), 12 deletions(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 9a05cd7..209add9 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -123,6 +123,10 @@ enum {
MLX5_CMD_OP_DRAIN_DCT = 0x712,
MLX5_CMD_OP_QUERY_DCT = 0x713,
MLX5_CMD_OP_ARM_DCT_FOR_KEY_VIOLATION = 0x714,
+   MLX5_CMD_OP_CREATE_XRQ= 0x717,
+   MLX5_CMD_OP_DESTROY_XRQ   = 0x718,
+   MLX5_CMD_OP_QUERY_XRQ = 0x719,
+   MLX5_CMD_OP_ARM_XRQ   = 0x71a,
MLX5_CMD_OP_QUERY_VPORT_STATE = 0x750,
MLX5_CMD_OP_MODIFY_VPORT_STATE= 0x751,
MLX5_CMD_OP_QUERY_ESW_VPORT_CONTEXT   = 0x752,
@@ -139,6 +143,8 @@ enum {
MLX5_CMD_OP_ALLOC_Q_COUNTER   = 0x771,
MLX5_CMD_OP_DEALLOC_Q_COUNTER = 0x772,
MLX5_CMD_OP_QUERY_Q_COUNTER   = 0x773,
+   MLX5_CMD_OP_SET_RATE_LIMIT= 0x780,
+   MLX5_CMD_OP_QUERY_RATE_LIMIT  = 0x781,
MLX5_CMD_OP_ALLOC_PD  = 0x800,
MLX5_CMD_OP_DEALLOC_PD= 0x801,
MLX5_CMD_OP_ALLOC_UAR = 0x802,
@@ -361,7 +367,8 @@ struct mlx5_ifc_fte_match_set_lyr_2_4_bits {
 };
 
 struct mlx5_ifc_fte_match_set_misc_bits {
-   u8 reserved_at_0[0x20];
+   u8 reserved_at_0[0x8];
+   u8 source_sqn[0x18];
 
u8 reserved_at_20[0x10];
u8 source_port[0x10];
@@ -505,6 +512,17 @@ struct mlx5_ifc_e_switch_cap_bits {
u8 reserved_at_20[0x7e0];
 };
 
+struct mlx5_ifc_qos_cap_bits {
+   u8 packet_pacing[0x1];
+   u8 reserved_0[0x1f];
+   u8 reserved_1[0x20];
+   u8 packet_pacing_max_rate[0x20];
+   u8 packet_pacing_min_rate[0x20];
+   u8 reserved_2[0x10];
+   u8 packet_pacing_rate_table_size[0x10];
+   u8 reserved_3[0x760];
+};
+
 struct mlx5_ifc_per_protocol_networking_offload_caps_bits {
u8 csum_cap[0x1];
u8 vlan_cap[0x1];
@@ -744,7 +762,8 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 
u8 out_of_seq_cnt[0x1];
u8 vport_counters[0x1];
-   u8 reserved_at_182[0x4];
+   u8 retransmission_q_co

[for-next 0/1] [pull request] Mellanox ConnectX-4 Shared Code

[Leon Romanovsky]

Hi Doug and Dave,

This is Mellanox mlx5_core shared code for both net-next and RDMA
trees for 4.8 kernel cycle.

This mlx5_ifc update introduces following ConnectX-4 features.

* netdev part:
 - MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation support
 - QoS rate limiting
 - SQ context rate limiting
 - Auto negotiation fields in PTYS register
 - Source SQN field in flow table entry match structure
 - DCBX parameters

* RDMA part:
 - New XRQ opcodes, commands and capabilities layout
 - Extend Q counters definition to support IB.

Thank you,
  Saeed and Leon.

The following changes since commit af8c34ce6ae32addda3788d54a7e340cad22516b:

  Linux 4.7-rc2 (2016-06-05 14:31:26 -0700)

are available in the git repository at:

  g...@gitolite.kernel.org:pub/scm/linux/kernel/git/leon/linux-rdma tags/shared

for you to fetch changes up to 7486216b3a0bd26375b17b2cc168a311106cea70:

  {net,IB}/mlx5: mlx5_ifc updates (2016-06-10 13:29:14 +0300)

Saeed Mahameed (1):
  {net,IB}/mlx5: mlx5_ifc updates

 Makefile  |   2 +-
 include/linux/mlx5/mlx5_ifc.h | 275 --
 2 files changed, 264 insertions(+), 13 deletions(-)

-- 
2.1.4

Re: [GIT PULL] Please Pull Mellanox Shared Code

[Leon Romanovsky]

On Tue, Jun 14, 2016 at 02:12:49AM -0400, David Miller wrote:
> 
> A bare pull request does not work.
> 
> You must post the actual patches so that they can be reviewed
> properly.

Sorry, It was first time attempt to get the sense for the proper procedure.

I will send the patch as a reply to this pull request and my hope that it will
be fine with you.

Thanks.


signature.asc
Description: Digital signature

[PATCH for-next 1/1] {net,IB}/mlx5: mlx5_ifc updates

[Leon Romanovsky]

From: Saeed Mahameed <sae...@mellanox.com>

Introducing mlx5_ifc updates for upcoming ConnectX-4 features.

Needed bits and hardware structures for mlx5e netdev:
- MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation
  support
- QoS rate limiting
- SQ context rate limiting
- Auto negotiation fields in PTYS register
- Source SQN field in flow table entry match structure
- DCBX parameters

Needed bits and hardware structures for IB:
- New XRQ opcodes, commands and capabilities layout
- Extend q counters definition to support IB.

Signed-off-by: Saeed Mahameed <sae...@mellanox.com>
Signed-off-by: Leon Romanovsky <leo...@mellanox.com>
Signed-off-by: Leon Romanovsky <l...@kernel.org>
---
 include/linux/mlx5/mlx5_ifc.h | 275 --
 1 file changed, 263 insertions(+), 12 deletions(-)

diff --git a/include/linux/mlx5/mlx5_ifc.h b/include/linux/mlx5/mlx5_ifc.h
index 9a05cd7..209add9 100644
--- a/include/linux/mlx5/mlx5_ifc.h
+++ b/include/linux/mlx5/mlx5_ifc.h
@@ -123,6 +123,10 @@ enum {
MLX5_CMD_OP_DRAIN_DCT = 0x712,
MLX5_CMD_OP_QUERY_DCT = 0x713,
MLX5_CMD_OP_ARM_DCT_FOR_KEY_VIOLATION = 0x714,
+   MLX5_CMD_OP_CREATE_XRQ= 0x717,
+   MLX5_CMD_OP_DESTROY_XRQ   = 0x718,
+   MLX5_CMD_OP_QUERY_XRQ = 0x719,
+   MLX5_CMD_OP_ARM_XRQ   = 0x71a,
MLX5_CMD_OP_QUERY_VPORT_STATE = 0x750,
MLX5_CMD_OP_MODIFY_VPORT_STATE= 0x751,
MLX5_CMD_OP_QUERY_ESW_VPORT_CONTEXT   = 0x752,
@@ -139,6 +143,8 @@ enum {
MLX5_CMD_OP_ALLOC_Q_COUNTER   = 0x771,
MLX5_CMD_OP_DEALLOC_Q_COUNTER = 0x772,
MLX5_CMD_OP_QUERY_Q_COUNTER   = 0x773,
+   MLX5_CMD_OP_SET_RATE_LIMIT= 0x780,
+   MLX5_CMD_OP_QUERY_RATE_LIMIT  = 0x781,
MLX5_CMD_OP_ALLOC_PD  = 0x800,
MLX5_CMD_OP_DEALLOC_PD= 0x801,
MLX5_CMD_OP_ALLOC_UAR = 0x802,
@@ -361,7 +367,8 @@ struct mlx5_ifc_fte_match_set_lyr_2_4_bits {
 };
 
 struct mlx5_ifc_fte_match_set_misc_bits {
-   u8 reserved_at_0[0x20];
+   u8 reserved_at_0[0x8];
+   u8 source_sqn[0x18];
 
u8 reserved_at_20[0x10];
u8 source_port[0x10];
@@ -505,6 +512,17 @@ struct mlx5_ifc_e_switch_cap_bits {
u8 reserved_at_20[0x7e0];
 };
 
+struct mlx5_ifc_qos_cap_bits {
+   u8 packet_pacing[0x1];
+   u8 reserved_0[0x1f];
+   u8 reserved_1[0x20];
+   u8 packet_pacing_max_rate[0x20];
+   u8 packet_pacing_min_rate[0x20];
+   u8 reserved_2[0x10];
+   u8 packet_pacing_rate_table_size[0x10];
+   u8 reserved_3[0x760];
+};
+
 struct mlx5_ifc_per_protocol_networking_offload_caps_bits {
u8 csum_cap[0x1];
u8 vlan_cap[0x1];
@@ -744,7 +762,8 @@ struct mlx5_ifc_cmd_hca_cap_bits {
 
u8 out_of_seq_cnt[0x1];
u8 vport_counters[0x1];
-   u8 reserved_at_182[0x4];
+   u8 retransmission_q_counters[0x1];
+   u8 reserved_at_183[0x3];
u8 max_qp_cnt[0xa];
u8 pkey_table_size[0x10];
 
@@ -771,7 +790,9 @@ struct mlx5_ifc_cmd_hca_cap_bits {
u8 log_max_msg[0x5];
u8 reserved_at_1c8[0x4];
u8 max_tc[0x4];
-   u8 reserved_at_1d0[0x6];
+   u8 reserved_at_1d0[0x1];
+   u8 dcbx[0x1];
+   u8 reserved_at_1d2[0x4];
u8 rol_s[0x1];
u8 rol_g[0x1];
u8 reserved_at_1d8[0x1];
@@ -803,7 +824,7 @@ struct mlx5_ifc_cmd_hca_cap_bits {
u8 tph[0x1];
u8 rf[0x1];
u8 dct[0x1];
-   u8 reserved_at_21b[0x1];
+   u8 qos[0x1];
u8 eth_net_offloads[0x1];
u8 roce[0x1];
u8 atomic[0x1];
@@ -929,7 +950,15 @@ struct mlx5_ifc_cmd_hca_cap_bits {
u8 cqe_compression_timeout[0x10];
u8 cqe_compression_max_num[0x10];
 
-   u8 reserved_at_5e0[0x220];
+   u8 reserved_at_5e0[0x10];
+   u8 tag_matching[0x1];
+   u8 rndv_offload_rc[0x1];
+   u8 rndv_offload_dc[0x1];
+   u8 log_tag_matching_list_sz[0x5];
+   u8 reserved_at_5e8[0x3];
+   u8 log_max_xrq[0x5];
+
+   u8 reserved_at_5f0[0x200];
 };
 
 enum mlx5_flow_destination_type {
@@ -1967,7 +1996,7 @@ struct mlx5_ifc_qpc_bits {
 
u8 reserved_at_560[0x5];
u8 rq_type[0x3];
-   u8 srqn_rmpn[0x18];
+   u8 srqn_rmpn_

[for-next 0/1] [pull request] Mellanox ConnectX-4 Shared Code

[Leon Romanovsky]

Hi Doug and Dave,

This is Mellanox mlx5_core shared code for both net-next and RDMA
trees for 4.8 kernel cycle.

This mlx5_ifc update introduces following ConnectX-4 features.

* netdev part:
 - MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation support
 - QoS rate limiting
 - SQ context rate limiting
 - Auto negotiation fields in PTYS register
 - Source SQN field in flow table entry match structure
 - DCBX parameters

* RDMA part:
 - New XRQ opcodes, commands and capabilities layout
 - Extend Q counters definition to support IB.

Thank you,
  Saeed and Leon.

The following changes since commit af8c34ce6ae32addda3788d54a7e340cad22516b:

  Linux 4.7-rc2 (2016-06-05 14:31:26 -0700)

are available in the git repository at:

  g...@gitolite.kernel.org:pub/scm/linux/kernel/git/leon/linux-rdma tags/shared

for you to fetch changes up to 7486216b3a0bd26375b17b2cc168a311106cea70:

  {net,IB}/mlx5: mlx5_ifc updates (2016-06-10 13:29:14 +0300)

Saeed Mahameed (1):
  {net,IB}/mlx5: mlx5_ifc updates

 Makefile  |   2 +-
 include/linux/mlx5/mlx5_ifc.h | 275 --
 2 files changed, 264 insertions(+), 13 deletions(-)

-- 
2.1.4

Re: [PATCH v9 11/22] IB/hns: Add IB device registration

[Leon Romanovsky]

On Sun, Jun 12, 2016 at 05:41:06PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/9 14:26, Leon Romanovsky wrote:
> >On Wed, Jun 01, 2016 at 11:37:53PM +0800, Lijun Ou wrote:
> >>This patch registered IB device when loaded, and unregistered
> >>IB device when removed.
> >>
> >>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>---
> >>  drivers/infiniband/hw/hns/hns_roce_main.c | 46 
> >> +++
> >>  1 file changed, 46 insertions(+)
> >>
> >>diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c 
> >>b/drivers/infiniband/hw/hns/hns_roce_main.c
> >>index 7fb0d34..f179a7f 100644
> >>--- a/drivers/infiniband/hw/hns/hns_roce_main.c
> >>+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> >>@@ -62,6 +62,41 @@
> >>  #include "hns_roce_device.h"
> >>  #include "hns_roce_icm.h"
> >>+void hns_roce_unregister_device(struct hns_roce_dev *hr_dev)
> >You are not calling to this function in this patch.
> >
> >>+{
> >>+   ib_unregister_device(_dev->ib_dev);
> >>+}
> >>+
> >>+int hns_roce_register_device(struct hns_roce_dev *hr_dev)
> >This function should be static.
> >
> >>+{
> >>+   int ret;
> >>+   struct hns_roce_ib_iboe *iboe = NULL;
> >>+   struct ib_device *ib_dev = NULL;
> >>+   struct device *dev = _dev->pdev->dev;
> >>+
> >>+   iboe = _dev->iboe;
> >>+
> >>+   ib_dev = _dev->ib_dev;
> >>+   strlcpy(ib_dev->name, "hisi_%d", IB_DEVICE_NAME_MAX);
> >>+
> >>+   ib_dev->owner   = THIS_MODULE;
> >>+   ib_dev->node_type   = RDMA_NODE_IB_CA;
> >>+   ib_dev->dma_device  = dev;
> >>+
> >>+   ib_dev->phys_port_cnt   = hr_dev->caps.num_ports;
> >>+   ib_dev->local_dma_lkey  = hr_dev->caps.reserved_lkey;
> >>+   ib_dev->num_comp_vectors= hr_dev->caps.num_comp_vectors;
> >>+   ib_dev->uverbs_abi_ver  = 1;
> >>+
> >>+   ret = ib_register_device(ib_dev, NULL);
> >>+   if (ret) {
> >>+   dev_err(dev, "ib_register_device failed!\n");
> >>+   return ret;
> >>+   }
> >>+
> >>+   return 0;
> >>+}
> >>+
> >>  int hns_roce_get_cfg(struct hns_roce_dev *hr_dev)
> >>  {
> >>int i;
> >>@@ -363,6 +398,17 @@ static int hns_roce_probe(struct platform_device *pdev)
> >>goto error_failed_engine_init;
> >>}
> >>+   ret = hns_roce_register_device(hr_dev);
> >>+   if (ret) {
> >>+   dev_err(dev, "register_device failed!\n");
> >According to the current code, you will print this error together with
> >error line in hns_roce_register_device for the same failure.
> >
> >"ib_register_device failed!"
> >"register_device failed!"
> Hi, leon
> In this patch [PATCH v9 11/22], there is only one error branch in
> funtion named hns_roce_register_device.
> In the following patch [PATCH v9 13/22], we add more operation, there
> are more
> than two error branch in this function as below.

Yes, and in all these error flows you already printed debug messages, your
"register_device failed" print is useless.



signature.asc
Description: Digital signature

Re: [PATCH 1/3] infiniband: rxe: avoid 64-bit division

[Leon Romanovsky]

On Mon, Jun 13, 2016 at 02:54:53PM +0200, Arnd Bergmann wrote:
> The rxe driver fails to build on 32-bit because of a 64-bit division:
> 
> In function `rxe_qp_from_attr':
> :(.text+0x53158): undefined reference to `__aeabi_uldivmod'
> 
> We can easily avoid this division by converting the nanosecond value
> into jiffies directly rather than converting to microseconds first.
> 
> Signed-off-by: Arnd Bergmann 

Thanks Arnd,
All three patches are applied on topic/rxe now.


signature.asc
Description: Digital signature

Re: [GIT PULL] Please Pull Mellanox Shared Code

[Leon Romanovsky]

On Tue, Jun 14, 2016 at 01:16:50PM -0400, Doug Ledford wrote:
> On 6/14/2016 3:02 AM, David Miller wrote:
> > 
> > Please, just stop right now.
> > 
> > We're not asking for the a huge patch covering the entire pull
> > request.
> > 
> > We're asking for you to post a patch series, one per commit, and as
> > numbered, fresh, new mailing list patch postings.  As if you were
> > submitting these changes for the very first time to the list.
> > 
> > Please ask a colleague for how to do this properly before you spam the
> > list again with more unnecessary and improperly formatted submissions.
> 
> Dave,
> 
> He's doing what Linus requested from Mellanox.  They were instructed to
> identify all of the coming changes in the next release where patches
> sent through your tree and patches sent through my tree would have
> conflicts.  They were then instructed to make git commits that merged
> those changes and have both you and I pull them (please note "pull", not
> git am patches) so that we have the exact same commit hashes in our
> trees and git will do the right thing when your tree and my tree are
> merged in the next merge window.  As it turns out the conflicts are
> mainly in this firmware offset definition file.  They just put all of
> the needed changes in there in one commit.  I doubt they have individual
> patches here.  This was a custom made patch who's sole purpose is to
> combine all the otherwise possibly conflicting changes in one place.
> I'm sure they could split it out into different commits, but they
> wouldn't be full commits, just the portion that applies to this firmware
> file.

Thanks Doug,

In addition to your accurate description. We don't have separate commits
for this file (mlx5_ifc.h) for many reasons: auto generated file, one
logical change (enable FW features), no helpful for bisect e.t.c.

My goal was to simplify the maintainers' work by sending minimal
possible pull request with easy for review information.

I also resent the pull request [1] as a cover letter together with patch itself 
[2].

[1] 
http://lkml.kernel.org/r/1465900500-1578-1-git-send-email-leonro+()+mellanox+!+com
[2] 
http://lkml.kernel.org/r/1465900500-1578-2-git-send-email-leonro+()+mellanox+!+com

Thanks.

> 
> --
> Doug Ledford 
>   GPG Key ID: 0E572FDD
> 





signature.asc
Description: Digital signature

[GIT PULL] Please Pull Mellanox Shared Code

[Leon Romanovsky]

Hi Doug and David,

This is Mellanox mlx5_core shared code for both net-next and RDMA
trees for 4.8 kernel cycle.

This mlx5_ifc update introduces following ConnectX-4 features.

* netdev part:
 - MLX5_CQ_PERIOD_NUM_MODES for adaptive moderation support
 - QoS rate limiting
 - SQ context rate limiting
 - Auto negotiation fields in PTYS register
 - Source SQN field in flow table entry match structure
 - DCBX parameters

* RDMA part:
 - New XRQ opcodes, commands and capabilities layout
 - Extend Q counters definition to support IB.

Thank you,
  Saeed and Leon.

The following changes since commit af8c34ce6ae32addda3788d54a7e340cad22516b:

  Linux 4.7-rc2 (2016-06-05 14:31:26 -0700)

are available in the git repository at:

  g...@gitolite.kernel.org:pub/scm/linux/kernel/git/leon/linux-rdma tags/shared

for you to fetch changes up to 7486216b3a0bd26375b17b2cc168a311106cea70:

  {net,IB}/mlx5: mlx5_ifc updates (2016-06-10 13:29:14 +0300)


Saeed Mahameed (1):
  {net,IB}/mlx5: mlx5_ifc updates

 include/linux/mlx5/mlx5_ifc.h | 275 --
 1 file changed, 263 insertions(+), 12 deletions(-)


signature.asc
Description: Digital signature

Re: [PATCH v9 11/22] IB/hns: Add IB device registration

[Leon Romanovsky]

On Wed, Jun 01, 2016 at 11:37:53PM +0800, Lijun Ou wrote:
> This patch registered IB device when loaded, and unregistered
> IB device when removed.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---
>  drivers/infiniband/hw/hns/hns_roce_main.c | 46 
> +++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c 
> b/drivers/infiniband/hw/hns/hns_roce_main.c
> index 7fb0d34..f179a7f 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_main.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> @@ -62,6 +62,41 @@
>  #include "hns_roce_device.h"
>  #include "hns_roce_icm.h"
>  
> +void hns_roce_unregister_device(struct hns_roce_dev *hr_dev)

You are not calling to this function in this patch.

> +{
> + ib_unregister_device(_dev->ib_dev);
> +}
> +
> +int hns_roce_register_device(struct hns_roce_dev *hr_dev)

This function should be static.

> +{
> + int ret;
> + struct hns_roce_ib_iboe *iboe = NULL;
> + struct ib_device *ib_dev = NULL;
> + struct device *dev = _dev->pdev->dev;
> +
> + iboe = _dev->iboe;
> +
> + ib_dev = _dev->ib_dev;
> + strlcpy(ib_dev->name, "hisi_%d", IB_DEVICE_NAME_MAX);
> +
> + ib_dev->owner   = THIS_MODULE;
> + ib_dev->node_type   = RDMA_NODE_IB_CA;
> + ib_dev->dma_device  = dev;
> +
> + ib_dev->phys_port_cnt   = hr_dev->caps.num_ports;
> + ib_dev->local_dma_lkey  = hr_dev->caps.reserved_lkey;
> + ib_dev->num_comp_vectors= hr_dev->caps.num_comp_vectors;
> + ib_dev->uverbs_abi_ver  = 1;
> +
> + ret = ib_register_device(ib_dev, NULL);
> + if (ret) {
> + dev_err(dev, "ib_register_device failed!\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
>  int hns_roce_get_cfg(struct hns_roce_dev *hr_dev)
>  {
>   int i;
> @@ -363,6 +398,17 @@ static int hns_roce_probe(struct platform_device *pdev)
>   goto error_failed_engine_init;
>   }
>  
> + ret = hns_roce_register_device(hr_dev);
> + if (ret) {
> + dev_err(dev, "register_device failed!\n");

According to the current code, you will print this error together with
error line in hns_roce_register_device for the same failure.

"ib_register_device failed!"
"register_device failed!"

> + goto error_failed_register_device;
> + }
> +
> + return 0;
> +
> +error_failed_register_device:
> + hns_roce_engine_exit(hr_dev);
> +
>  error_failed_engine_init:
>   hns_roce_cleanup_bitmap(hr_dev);
>  
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH v9 03/22] IB/hns: Add initial main frame driver and get cfg info

[Leon Romanovsky]

On Wed, Jun 01, 2016 at 11:37:45PM +0800, Lijun Ou wrote:
> This patch mainly added the initial bare main driver. It
> could get the relative configure information of net node.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |  72 ++
>  drivers/infiniband/hw/hns/hns_roce_main.c   | 197 
> 
>  2 files changed, 269 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_device.h
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_main.c
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h 
> b/drivers/infiniband/hw/hns/hns_roce_device.h
> new file mode 100644
> index 000..f9de8e4
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _HNS_ROCE_DEVICE_H
> +#define _HNS_ROCE_DEVICE_H
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DRV_NAME "hns_roce"
> +
> +#define HNS_ROCE_MAX_IRQ_NUM 34
> +#define HNS_ROCE_MAX_PORTS   6
> +
> +struct hns_roce_ib_iboe {
> + struct net_device  *netdevs[HNS_ROCE_MAX_PORTS];
> + u8  phy_port[HNS_ROCE_MAX_PORTS];
> +};
> +
> +struct hns_roce_caps {
> + u8  num_ports;
> +};
> +
> +struct hns_roce_dev {
> + struct ib_deviceib_dev;
> + struct platform_device  *pdev;
> + struct hns_roce_ib_iboe iboe;
> +
> + int irq[HNS_ROCE_MAX_IRQ_NUM];
> + u8 __iomem  *reg_base;
> + struct hns_roce_capscaps;
> +
> + int cmd_mod;
> + int loop_idc;
> +};
> +
> +#endif /* _HNS_ROCE_DEVICE_H */
> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c 
> b/drivers/infiniband/hw/hns/hns_roce_main.c
> new file mode 100644
> index 000..21c5e8e
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> @@ -0,0 +1,197 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + * Copyright (c) 2007, 2008 Mellanox Technologies. All rights reserved.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR 

Re: [PATCH v10 06/22] IB/hns: Add initial cmd operation

[Leon Romanovsky]

On Tue, Jun 21, 2016 at 06:50:51PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/20 21:33, Leon Romanovsky wrote:
> >On Thu, Jun 16, 2016 at 10:35:14PM +0800, Lijun Ou wrote:
> >>This patch added the operation for cmd, and added some functions
> >>for initializing eq table and selecting cmd mode.
> >>
> >>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>---
> >>PATCH v9/v8/v7/v6:
> >>- No change over the PATCH v5
> >>
> >>PATCH v5:
> >>- The initial patch which was redesigned based on the second patch
> >>   in PATCH v4
> >>---
> ><...>
> >
> >>+#define CMD_MAX_NUM32
> >>+
> >>+int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
> >>+{
> >>+   struct device *dev = _dev->pdev->dev;
> >>+
> >>+   mutex_init(_dev->cmd.hcr_mutex);
> >>+   sema_init(_dev->cmd.poll_sem, 1);
> >>+   hr_dev->cmd.use_events = 0;
> >>+   hr_dev->cmd.toggle = 1;
> >>+   hr_dev->cmd.max_cmds = CMD_MAX_NUM;
> ><...>
> >
> >>+   for (hr_cmd->token_mask = 1; hr_cmd->token_mask < hr_cmd->max_cmds;
> >>+hr_cmd->token_mask <<= 1)
> >>+   ;
> >>+   --hr_cmd->token_mask;
> >It doesn't look that you dynamically change max_cmds supported.
> >Why do you need to calculate token_mask dynamically?
> Hi, Leon
> 
> 1. The four lines above are in the function named
> hns_roce_cmd_use_events.
>  and now this function is only called once in hns_roce_probe.
> 2. In hns_roce_cmd_use_events,
> we use these 4 lines to achieve the value of hr_cmd->token_mask
> according to hr_cmd->max_cmds dynamically,
> then we only define one marco for hr_cmd->max_cmds as below:
> 
>   #define CMD_MAX_NUM 32
> 
>And it looks more flexible.

It is called over engineering.
I would recommend to you to remove it.

We don't need over complicated code which is executed
once with need to maintain with zero benefit.

The other places need such simplification too.

> 
> Regards
> Wei Hu
> 
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH v10 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Tue, Jun 21, 2016 at 12:37:39PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/20 21:04, Leon Romanovsky wrote:
> >On Mon, Jun 20, 2016 at 05:48:15PM +0800, Wei Hu (Xavier) wrote:
> >>
> >>On 2016/6/20 17:27, Leon Romanovsky wrote:
> >>>On Mon, Jun 20, 2016 at 03:49:24PM +0800, Wei Hu (Xavier) wrote:
> >>>>On 2016/6/20 14:06, Leon Romanovsky wrote:
> >>>>>On Mon, Jun 20, 2016 at 12:37:40PM +0800, Wei Hu (Xavier) wrote:
> >>>>>>On 2016/6/17 17:58, Leon Romanovsky wrote:
> >>>>>>>On Thu, Jun 16, 2016 at 10:35:16PM +0800, Lijun Ou wrote:
> >>>>>>>>This patch mainly added icm support for RoCE. It initializes icm
> >>>>>>>>which managers the relative memory blocks for RoCE. The data
> >>>>>>>>structures of RoCE will be located in it. For example, CQ table,
> >>>>>>>>QP table and MTPT table so on.
> >>>>>>>>
> >>>>>>>>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>>>>>>>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>>>>>>>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>>>>>>>---
> >>>>>>><...>
> >>>>>>>
> >>>>>>>>+
> >>>>>Another question which you didn't answer [1].
> >>>>>
> >>>>>"I wonder if you have the same needs for ICM as it is in mlx4 device.
> >>>>>Do you have firmware?"
> >>>>>
> >>>>>[1] http://marc.info/?l=linux-rdma=146545553104913=2
> >>>>Hi, Leon
> >>>> Now we haven't firmware.
> >>>> But hardware still need memory for QPC\CQC\MTPT\mtt etc.
> >>>ICM stands for InfiniHost (Interconnect) Context Memory is a specific
> >>>memory place to share between host <-> FW and host <-> HW if HW is
> >>>aware of specific structures.
> >>>
> >>>I assume that in your case, it is enough to allocate memory region and
> >>>supply it to HW. Am I right?
> >>For Our hardware,
> >>1. ICM has a memory management method, It's very good for QPC\CQC\MTPT\mtt
> >>etc. we need it.
> >You need special HW to leverage its. AFAIK it is Mellanox specific.
> For our hardware, we use ICM to memory management, the memory shared with
> host and HW.
> QPC\CQC\MTPT\mtt has specific memory requirement.
> QPC\CQC\MTPT need continuous memory. we use ICM to management the block of
> memory. It's very good！

I wasn't convinced why do you need to copy whole ICM logic which is
specific to Mellanox. Your requirements can be implemented by standard CMA
and/or DMA.

> >>2. The meomry for QPC\CQC\MTPT\mtt only used for RoCE hardware and driver,
> >>we don't want use MR.
> >I didn't mean Infiniband MR, but memory region returned from standard
> >allocation functions (kmalloc, ...).
> >
> >>3. Now we haven't firmware, maybe we need it next version.
> >You are always invited to add support once it will be needed, no need to
> >add it in advance.
> >
> >Thanks
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH v10 06/22] IB/hns: Add initial cmd operation

[Leon Romanovsky]

On Tue, Jun 21, 2016 at 09:01:57PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/21 19:28, Leon Romanovsky wrote:
> >On Tue, Jun 21, 2016 at 06:50:51PM +0800, Wei Hu (Xavier) wrote:
> >>
> >>On 2016/6/20 21:33, Leon Romanovsky wrote:
> >>>On Thu, Jun 16, 2016 at 10:35:14PM +0800, Lijun Ou wrote:
> >>>>This patch added the operation for cmd, and added some functions
> >>>>for initializing eq table and selecting cmd mode.
> >>>>
> >>>>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>>>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>>>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>>>---
> >>>>PATCH v9/v8/v7/v6:
> >>>>- No change over the PATCH v5
> >>>>
> >>>>PATCH v5:
> >>>>- The initial patch which was redesigned based on the second patch
> >>>>   in PATCH v4
> >>>>---
> >>><...>
> >>>
> >>>>+#define CMD_MAX_NUM  32
> >>>>+
> >>>>+int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
> >>>>+{
> >>>>+ struct device *dev = _dev->pdev->dev;
> >>>>+
> >>>>+ mutex_init(_dev->cmd.hcr_mutex);
> >>>>+ sema_init(_dev->cmd.poll_sem, 1);
> >>>>+ hr_dev->cmd.use_events = 0;
> >>>>+ hr_dev->cmd.toggle = 1;
> >>>>+ hr_dev->cmd.max_cmds = CMD_MAX_NUM;
> >>><...>
> >>>
> >>>>+ for (hr_cmd->token_mask = 1; hr_cmd->token_mask < hr_cmd->max_cmds;
> >>>>+  hr_cmd->token_mask <<= 1)
> >>>>+ ;
> >>>>+ --hr_cmd->token_mask;
> >>>It doesn't look that you dynamically change max_cmds supported.
> >>>Why do you need to calculate token_mask dynamically?
> >>Hi, Leon
> >>
> >> 1. The four lines above are in the function named
> >>hns_roce_cmd_use_events.
> >>  and now this function is only called once in hns_roce_probe.
> >> 2. In hns_roce_cmd_use_events,
> >> we use these 4 lines to achieve the value of hr_cmd->token_mask
> >>according to hr_cmd->max_cmds dynamically,
> >> then we only define one marco for hr_cmd->max_cmds as below:
> >>
> >>#define CMD_MAX_NUM 32
> >>
> >>And it looks more flexible.
> >It is called over engineering.
> >I would recommend to you to remove it.
> >
> >We don't need over complicated code which is executed
> >once with need to maintain with zero benefit.
> >
> >The other places need such simplification too.
> Hi, Leon
> 
> We will modify this place as below:
> In hns_roce_hw_v1.c(for hip06 soc) file:
> 
> void hns_roce_v1_profile(struct hns_roce_dev *hr_dev)
> {
> 
> caps->max_cmds = 32;
> 
> }
> 
> In hns_roce_cmd.c file:
> 
> int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
> {
>
>hr_dev->cmd.max_cmds = hr_dev->caps->max_cmds;
>   
>   }
> 
>Can you give more suggestions?

I would be happy to do it if I had enough time to review this code.

General suggestion will be to ask yourself, if value is going to be
changed during the runtime. In case the answer is no, there is no room
to additional logic which translate constant to different value which
will be other constant.

You should do it across all the patchset.

So, in this specific case, the proposed change is not enough, you are
not solving an issue, but hiding it.

Thanks

> 
> 
> Regards
> Wei Hu
> >>Regards
> >>Wei Hu
> >>
> >>
> >>
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH v10 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:16PM +0800, Lijun Ou wrote:
> This patch mainly added icm support for RoCE. It initializes icm
> which managers the relative memory blocks for RoCE. The data
> structures of RoCE will be located in it. For example, CQ table,
> QP table and MTPT table so on.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---

<...>

> +
> +static int hns_roce_alloc_icm_pages(struct scatterlist *mem, int order,
> + gfp_t gfp_mask)
> +{
> + struct page *page;
> +
> + page = alloc_pages(gfp_mask, order);
> + if (!page)
> + return -ENOMEM;
> +
> + sg_set_page(mem, page, PAGE_SIZE << order, 0);
> +
> + return 0;
> +}
> +
> +static int hns_roce_alloc_icm_coherent(struct device *dev,
> +struct scatterlist *mem, int order,
> +gfp_t gfp_mask)
> +{
> + void *buf = dma_alloc_coherent(dev, PAGE_SIZE << order,
> +_dma_address(mem), gfp_mask);
> + if (!buf)
> + return -ENOMEM;
> +
> + sg_set_buf(mem, buf, PAGE_SIZE << order);
> + WARN_ON(mem->offset);
> + sg_dma_len(mem) = PAGE_SIZE << order;
> + return 0;
> +}
> +

<...>

> +
> +static void hns_roce_free_icm_pages(struct hns_roce_dev *hr_dev,
> + struct hns_roce_icm_chunk *chunk)
> +{
> + int i;
> +
> + if (chunk->nsg > 0)
> + dma_unmap_sg(_dev->pdev->dev, chunk->mem, chunk->npages,
> +  DMA_BIDIRECTIONAL);
> +
> + for (i = 0; i < chunk->npages; ++i)
> + __free_pages(sg_page(>mem[i]),
> +  get_order(chunk->mem[i].length));

You used alloc_pages for this allocation, so why are you using
__free_pages instead of free_pages?




signature.asc
Description: Digital signature

Re: [PATCH v10 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Mon, Jun 20, 2016 at 12:37:40PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/17 17:58, Leon Romanovsky wrote:
> >On Thu, Jun 16, 2016 at 10:35:16PM +0800, Lijun Ou wrote:
> >>This patch mainly added icm support for RoCE. It initializes icm
> >>which managers the relative memory blocks for RoCE. The data
> >>structures of RoCE will be located in it. For example, CQ table,
> >>QP table and MTPT table so on.
> >>
> >>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>---
> ><...>
> >
> >>+
> >>+static int hns_roce_alloc_icm_pages(struct scatterlist *mem, int order,
> >>+   gfp_t gfp_mask)
> >>+{
> >>+   struct page *page;
> >>+
> >>+   page = alloc_pages(gfp_mask, order);
> >>+   if (!page)
> >>+   return -ENOMEM;
> >>+
> >>+   sg_set_page(mem, page, PAGE_SIZE << order, 0);
> >>+
> >>+   return 0;
> >>+}
> >>+
> >>+static int hns_roce_alloc_icm_coherent(struct device *dev,
> >>+  struct scatterlist *mem, int order,
> >>+  gfp_t gfp_mask)
> >>+{
> >>+   void *buf = dma_alloc_coherent(dev, PAGE_SIZE << order,
> >>+  _dma_address(mem), gfp_mask);
> >>+   if (!buf)
> >>+   return -ENOMEM;
> >>+
> >>+   sg_set_buf(mem, buf, PAGE_SIZE << order);
> >>+   WARN_ON(mem->offset);
> >>+   sg_dma_len(mem) = PAGE_SIZE << order;
> >>+   return 0;
> >>+}
> >>+
> ><...>
> >
> >>+
> >>+static void hns_roce_free_icm_pages(struct hns_roce_dev *hr_dev,
> >>+   struct hns_roce_icm_chunk *chunk)
> >>+{
> >>+   int i;
> >>+
> >>+   if (chunk->nsg > 0)
> >>+   dma_unmap_sg(_dev->pdev->dev, chunk->mem, chunk->npages,
> >>+DMA_BIDIRECTIONAL);
> >>+
> >>+   for (i = 0; i < chunk->npages; ++i)
> >>+   __free_pages(sg_page(>mem[i]),
> >>+get_order(chunk->mem[i].length));
> >You used alloc_pages for this allocation, so why are you using
> >__free_pages instead of free_pages?
> Hi, Leon
> The function prototype of these functions as below：
> static inline struct page * alloc_pages(gfp_t gfp_mask, unsigned int
> order);
> void free_pages(unsigned long addr, unsigned int order);
> void __free_pages(struct page *page, unsigned int order);
> 
> The type of the first parameter of free_pages is same with the type of
> return value of alloc_pages.
> Maybe it is better to call __free_pages to release memory that allocated
> by calling alloc_pages.

OK, I see.

Another question which you didn't answer [1].

"I wonder if you have the same needs for ICM as it is in mlx4 device.
Do you have firmware?"

[1] http://marc.info/?l=linux-rdma=146545553104913=2

> 
> Regards
> Wei Hu
> 
> >
> 


signature.asc
Description: Digital signature

Re: [PATCH] mellanox: mlx5: Use logging functions to reduce text ~10k/5%

[Leon Romanovsky]

On Wed, Jun 22, 2016 at 11:23:59AM -0700, Joe Perches wrote:
> The logging macros create a bit of duplicated code/text.
> 
> Use specialized functions to reduce the duplication.
> 
> (defconfig/x86-64)
> $ size drivers/net/ethernet/mellanox/mlx5/core/built-in.o*
>    text      data bss dec hex filename
>  178634      2059  16  180709   2c1e5 
> drivers/net/ethernet/mellanox/mlx5/core/built-in.o.new
>  188679      2059  16  190754   2e922 
> drivers/net/ethernet/mellanox/mlx5/core/built-in.o.old
> 
> The output changes now do not include line #,
> but do include the function offset.
> 
> Signed-off-by: Joe Perches <j...@perches.com>

As far as I see all these functions are used in error paths, so no
implication on performance is expected.

And I'm fine with function offsets.

Saeed,
What do you think?

Reviewed-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

Re: [PATCH] mellanox: mlx5: Use logging functions to reduce text ~10k/5%

[Leon Romanovsky]

On Thu, Jun 23, 2016 at 08:27:01AM +0300, Leon Romanovsky wrote:
> On Wed, Jun 22, 2016 at 11:23:59AM -0700, Joe Perches wrote:
> > The logging macros create a bit of duplicated code/text.
> > 
> > Use specialized functions to reduce the duplication.
> > 
> > (defconfig/x86-64)
> > $ size drivers/net/ethernet/mellanox/mlx5/core/built-in.o*
> >    text    data bss dec hex filename
> >  178634    2059  16  180709   2c1e5 
> > drivers/net/ethernet/mellanox/mlx5/core/built-in.o.new
> >  188679    2059  16  190754   2e922 
> > drivers/net/ethernet/mellanox/mlx5/core/built-in.o.old
> > 
> > The output changes now do not include line #,
> > but do include the function offset.
> > 
> > Signed-off-by: Joe Perches <j...@perches.com>
> 
> As far as I see all these functions are used in error paths, so no
> implication on performance is expected.
> 
> And I'm fine with function offsets.
> 
> Saeed,
> What do you think?
> 
> Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

I continued to play with this patch and it doesn't pass checkpatch.
It looks like corrupted file.

➜  linux-rdma git:(master) ./scripts/checkpatch.pl
~/Downloads/mellanox-mlx5-Use-logging-functions-to-reduce-text-10k-5.patch
WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per 
line)
#21: 
 178634    2059  16  180709   2c1e5
drivers/net/ethernet/mellanox/mlx5/core/built-in.o.new

ERROR: patch seems to be corrupt (line wrapped?)
#46: FILE: drivers/net/ethernet/mellanox/mlx5/core/main.c:1556:

CHECK: Alignment should match open parenthesis
#78: FILE: drivers/net/ethernet/mellanox/mlx5/core/main.c:1586:
+   dev_warn(>pdev->dev, "%s:%pS:(pid %d): %pV",
+    dev->priv.name, __builtin_return_address(0), current->pid,

ERROR: space required before that '&' (ctx:VxV)
#79: FILE: drivers/net/ethernet/mellanox/mlx5/core/main.c:1587:
+    );
  ^
total: 2 errors, 1 warnings, 1 checks, 58 lines  checked


signature.asc
Description: Digital signature

Re: [PATCH v10 06/22] IB/hns: Add initial cmd operation

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:14PM +0800, Lijun Ou wrote:
> This patch added the operation for cmd, and added some functions
> for initializing eq table and selecting cmd mode.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---
> PATCH v9/v8/v7/v6:
> - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---

<...>

> +#define CMD_MAX_NUM  32
> +
> +int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
> +{
> + struct device *dev = _dev->pdev->dev;
> +
> + mutex_init(_dev->cmd.hcr_mutex);
> + sema_init(_dev->cmd.poll_sem, 1);
> + hr_dev->cmd.use_events = 0;
> + hr_dev->cmd.toggle = 1;
> + hr_dev->cmd.max_cmds = CMD_MAX_NUM;

<...>

> + for (hr_cmd->token_mask = 1; hr_cmd->token_mask < hr_cmd->max_cmds;
> +  hr_cmd->token_mask <<= 1)
> + ;
> + --hr_cmd->token_mask;

It doesn't look that you dynamically change max_cmds supported.
Why do you need to calculate token_mask dynamically?


signature.asc
Description: Digital signature

Re: [PATCH v10 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Mon, Jun 20, 2016 at 05:48:15PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/20 17:27, Leon Romanovsky wrote:
> >On Mon, Jun 20, 2016 at 03:49:24PM +0800, Wei Hu (Xavier) wrote:
> >>
> >>On 2016/6/20 14:06, Leon Romanovsky wrote:
> >>>On Mon, Jun 20, 2016 at 12:37:40PM +0800, Wei Hu (Xavier) wrote:
> >>>>On 2016/6/17 17:58, Leon Romanovsky wrote:
> >>>>>On Thu, Jun 16, 2016 at 10:35:16PM +0800, Lijun Ou wrote:
> >>>>>>This patch mainly added icm support for RoCE. It initializes icm
> >>>>>>which managers the relative memory blocks for RoCE. The data
> >>>>>>structures of RoCE will be located in it. For example, CQ table,
> >>>>>>QP table and MTPT table so on.
> >>>>>>
> >>>>>>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>>>>>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>>>>>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>>>>>---
> >>>>><...>
> >>>>>
> >>>>>>+
> >>>Another question which you didn't answer [1].
> >>>
> >>>"I wonder if you have the same needs for ICM as it is in mlx4 device.
> >>>Do you have firmware?"
> >>>
> >>>[1] http://marc.info/?l=linux-rdma=146545553104913=2
> >>Hi, Leon
> >> Now we haven't firmware.
> >> But hardware still need memory for QPC\CQC\MTPT\mtt etc.
> >ICM stands for InfiniHost (Interconnect) Context Memory is a specific
> >memory place to share between host <-> FW and host <-> HW if HW is
> >aware of specific structures.
> >
> >I assume that in your case, it is enough to allocate memory region and
> >supply it to HW. Am I right?
> For Our hardware,
> 1. ICM has a memory management method, It's very good for QPC\CQC\MTPT\mtt
> etc. we need it.

You need special HW to leverage its. AFAIK it is Mellanox specific.

> 2. The meomry for QPC\CQC\MTPT\mtt only used for RoCE hardware and driver,
> we don't want use MR.

I didn't mean Infiniband MR, but memory region returned from standard
allocation functions (kmalloc, ...).

> 3. Now we haven't firmware, maybe we need it next version.

You are always invited to add support once it will be needed, no need to
add it in advance.

Thanks


signature.asc
Description: Digital signature

Re: [PATCH v10 08/22] IB/hns: Add icm support

[Leon Romanovsky]

On Mon, Jun 20, 2016 at 03:49:24PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/20 14:06, Leon Romanovsky wrote:
> >On Mon, Jun 20, 2016 at 12:37:40PM +0800, Wei Hu (Xavier) wrote:
> >>
> >>On 2016/6/17 17:58, Leon Romanovsky wrote:
> >>>On Thu, Jun 16, 2016 at 10:35:16PM +0800, Lijun Ou wrote:
> >>>>This patch mainly added icm support for RoCE. It initializes icm
> >>>>which managers the relative memory blocks for RoCE. The data
> >>>>structures of RoCE will be located in it. For example, CQ table,
> >>>>QP table and MTPT table so on.
> >>>>
> >>>>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>>>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>>>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>>>---
> >>><...>
> >>>
> >>>>+
> >Another question which you didn't answer [1].
> >
> >"I wonder if you have the same needs for ICM as it is in mlx4 device.
> >Do you have firmware?"
> >
> >[1] http://marc.info/?l=linux-rdma=146545553104913=2
> Hi, Leon
> Now we haven't firmware.
> But hardware still need memory for QPC\CQC\MTPT\mtt etc.

ICM stands for InfiniHost (Interconnect) Context Memory is a specific
memory place to share between host <-> FW and host <-> HW if HW is
aware of specific structures.

I assume that in your case, it is enough to allocate memory region and
supply it to HW. Am I right?

> 
> Thanks
> Wei Hu
> >>Regards
> >>Wei Hu
> >>
> 
> 


signature.asc
Description: Digital signature

Re: [PATCH v10 01/22] net: hns: Add reset function support for RoCE driver

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:09PM +0800, Lijun Ou wrote:
> It added reset function for RoCE driver. RoCE is a feature of hns.
> In hip06 SoC, in RoCE reset process, it's needed to configure dsaf
> channel reset, port and sl map info. Reset function of RoCE is
> located in dsaf module, we only call it in RoCE driver when needed.
>
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> Signed-off-by: Sheng Li <lisheng...@huawei.com>
> ---
> PATCH v9/v8/v7:
> - No change over PATCH v6
>
> PATCH v6:
> This fixes the comments given by Leon Romanovsky over the PATCH v5:
>   Link: https://lkml.org/lkml/2016/5/3/733
>
> PATCH v5/v4/v3:
> - No change over PATCH v2
>
> PATCH v2:
> This fixes the comments given by Leon Romanovsky over the PATCH v1:
>   Link: https://lkml.org/lkml/2016/3/12/46
>
> PATCH v1:
> - The initial patch
> ---
> ---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 84 
> ++
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 30 
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c | 36 ++
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  | 14 +++-
>  4 files changed, 163 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> index 1c2ddb2..0c4a87c 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> @@ -14,6 +14,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2685,6 +2686,89 @@ static struct platform_driver g_dsaf_driver = {
>
>  module_platform_driver(g_dsaf_driver);
>
> +/**
> + * hns_dsaf_roce_reset - reset dsaf and roce
> + * @dsaf_fwnode: Pointer to framework node for the dasf
> + * @enable: false - request reset , true - drop reset
> + * retuen 0 - success , negative -fail
> + */
> +int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable)
> +{
> + struct dsaf_device *dsaf_dev;
> + struct platform_device *pdev;
> + unsigned int mp;
> + unsigned int sl;
> + unsigned int credit;
> + int i;
> + const u32 port_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE_NUM] = {
> + {DSAF_ROCE_PORT_0, DSAF_ROCE_PORT_0, DSAF_ROCE_PORT_0},
> + {DSAF_ROCE_PORT_1, DSAF_ROCE_PORT_0, DSAF_ROCE_PORT_0},
> + {DSAF_ROCE_PORT_2, DSAF_ROCE_PORT_1, DSAF_ROCE_PORT_0},
> + {DSAF_ROCE_PORT_3, DSAF_ROCE_PORT_1, DSAF_ROCE_PORT_0},
> + {DSAF_ROCE_PORT_4, DSAF_ROCE_PORT_2, DSAF_ROCE_PORT_1},
> + {DSAF_ROCE_PORT_4, DSAF_ROCE_PORT_2, DSAF_ROCE_PORT_1},
> + {DSAF_ROCE_PORT_5, DSAF_ROCE_PORT_3, DSAF_ROCE_PORT_1},
> + {DSAF_ROCE_PORT_5, DSAF_ROCE_PORT_3, DSAF_ROCE_PORT_1},
> + };
> + const u32 sl_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE_NUM] = {
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_0, DSAF_ROCE_SL_0},
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_1, DSAF_ROCE_SL_1},
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_0, DSAF_ROCE_SL_2},
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_1, DSAF_ROCE_SL_3},
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_0, DSAF_ROCE_SL_0},
> + {DSAF_ROCE_SL_1, DSAF_ROCE_SL_1, DSAF_ROCE_SL_1},
> + {DSAF_ROCE_SL_0, DSAF_ROCE_SL_0, DSAF_ROCE_SL_2},
> + {DSAF_ROCE_SL_1, DSAF_ROCE_SL_1, DSAF_ROCE_SL_3},
> + };
> +
> + if (!is_of_node(dsaf_fwnode)) {
> + pr_err("hisi_dsaf: Only support DT node!\n");
> + return -EINVAL;
> + }
> + pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
> + dsaf_dev = dev_get_drvdata(>dev);
> + if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
> + dev_err(dsaf_dev->dev, "%s v1 chip do not support roce!\n",

chip don't support roce -> chip doesn't support RoCE

> + dsaf_dev->ae_dev.name);
> + return -ENODEV;
> + }
> +
> + if (!enable) {
> + /* Reset rocee-channels in dsaf and rocee */
> + hns_dsaf_srst_chns(dsaf_dev, DSAF_CHNS_MASK, false);
> + hns_dsaf_roce_srst(dsaf_dev, false);
> + } else {
> + /* Configure dsaf tx roce correspond to port map and sl map */
> + mp = dsaf_read_dev(dsaf_dev, DSAF_ROCE_PORT_MAP_REG);
> + for (i = 0; i < DSAF_ROCE_CREDIT_CHN; i++)
> + dsaf_set_field(mp, 7 << i * 3, i * 3,
> +   port_map[i][DSAF_ROCE_6PORT_MODE]);
> + dsaf_set_field(mp, 3 << i * 3, i * 3, 0);
> + dsaf_write_dev(dsaf_dev, DSAF_ROCE_PORT_MAP_REG, mp);
> +
> + sl = dsaf_read_dev(dsaf_dev, DSAF_ROCE_SL_MAP_REG);
> + for (i = 0; i < DSAF_ROCE_CREDIT_CHN; i++)
> + dsaf_set_field(sl, 3 << i * 2, i * 2,
> +   sl_map[i][DSAF_ROCE_6PORT_MODE]);
> + dsaf_write_dev(dsaf_dev, DSAF_ROCE_SL_MAP_REG, sl);
> +
> + /* De-reset roce

Re: [PATCH v10 03/22] IB/hns: Add initial main frame driver and get cfg info

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:11PM +0800, Lijun Ou wrote:
> This patch mainly added the initial bare main driver. It
> could get the relative configure information of net node.
> 
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> ---
> PATCH v9:
> This fixes comments given by Leon Romanovsky over the PATCH v8:
>   Link: https://lkml.org/lkml/2016/6/9/56
> 
> PATCH v8/v7/v6:
> - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |  73 ++
>  drivers/infiniband/hw/hns/hns_roce_main.c   | 200 
> 
>  2 files changed, 273 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_device.h
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_main.c
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h 
> b/drivers/infiniband/hw/hns/hns_roce_device.h
> new file mode 100644
> index 000..946b470
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -0,0 +1,73 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _HNS_ROCE_DEVICE_H
> +#define _HNS_ROCE_DEVICE_H
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define DRV_NAME "hns_roce"
> +
> +#define HNS_ROCE_MAX_IRQ_NUM 34
> +#define HNS_ROCE_MAX_PORTS   6
> +
> +struct hns_roce_ib_iboe {
> + struct net_device  *netdevs[HNS_ROCE_MAX_PORTS];
> + u8  phy_port[HNS_ROCE_MAX_PORTS];
> +};
> +
> +struct hns_roce_caps {
> + u8  num_ports;
> +};
> +
> +struct hns_roce_dev {
> + struct ib_deviceib_dev;
> + struct platform_device  *pdev;
> + const char  *irq_names;
> + struct hns_roce_ib_iboe iboe;
> +
> + int irq[HNS_ROCE_MAX_IRQ_NUM];
> + u8 __iomem  *reg_base;
> + struct hns_roce_capscaps;
> +
> + int cmd_mod;
> + int loop_idc;
> +};
> +
> +#endif /* _HNS_ROCE_DEVICE_H */
> diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c 
> b/drivers/infiniband/hw/hns/hns_roce_main.c
> new file mode 100644
> index 000..8924ce3
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> @@ -0,0 +1,200 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + * Copyright (c) 2007, 2008 Mellanox Technologies. All rights reserved.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions 

Re: [PATCH v10 05/22] IB/hns: Add initial profile resource

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:13PM +0800, Lijun Ou wrote:
> This patch added the operation for cmd, and added some functions
> for initializing eq table and selecting cmd mode.
> 
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> ---
> PATCH v9:
> This fixes the comments given by Leon Romanovsky over the PATCH v8:
>   Link: https://lkml.org/lkml/2016/6/9/65
> 
> PATCH v8/v7/v6:
> - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---
> ---
>  drivers/infiniband/hw/hns/hns_roce_common.h | 49 +++
>  drivers/infiniband/hw/hns/hns_roce_device.h | 55 -
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c  | 75 
> +
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.h  | 36 ++
>  drivers/infiniband/hw/hns/hns_roce_main.c   |  7 +++
>  5 files changed, 221 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_common.h
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h 
> b/drivers/infiniband/hw/hns/hns_roce_common.h
> new file mode 100644
> index 000..4cc4761
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#ifndef _HNS_ROCE_COMMON_H
> +#define _HNS_ROCE_COMMON_H
> +
> +#define roce_read(dev, reg)  readl((dev)->reg_base + (reg))
> +
> +/*ROCEE_REG DEFINITION/
> +#define ROCEE_VENDOR_ID_REG  0x0
> +#define ROCEE_VENDOR_PART_ID_REG 0x4
> +
> +#define ROCEE_HW_VERSION_REG 0x8
> +
> +#define ROCEE_SYS_IMAGE_GUID_L_REG   0xC
> +#define ROCEE_SYS_IMAGE_GUID_H_REG   0x10
> +
> +#define ROCEE_ACK_DELAY_REG  0x14
> +
> +#endif /* _HNS_ROCE_COMMON_H */
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h 
> b/drivers/infiniband/hw/hns/hns_roce_device.h
> index b857c76..e01ea34 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -45,6 +45,12 @@
>  #define DRV_NAME "hns_roce"
>  
>  #define HNS_ROCE_MAX_IRQ_NUM 34
> +
> +#define HNS_ROCE_COMP_VEC_NUM32
> +
> +#define HNS_ROCE_AEQE_VEC_NUM1
> +#define HNS_ROCE_AEQE_OF_VEC_NUM 1
> +
>  #define HNS_ROCE_MAX_PORTS   6
>  
>  struct hns_roce_ib_iboe {
> @@ -53,11 +59,52 @@ struct hns_roce_ib_iboe {
>  };
>  
>  struct hns_roce_caps {
> - u8  num_ports;
> + u64 fw_ver;
> + u8  num_ports;
> + int gid_table_len[HNS_ROCE_MAX_PORTS];
> + int pkey_table_len[HNS_ROCE_MAX_PORTS];
> + int local_ca_ack_delay;
> + int num_uars;
> + u32 phy_num_uars;
> + u32 max_sq_sg;  /* 2 */
> + u32 max_sq_inline;  /* 32 */
> + u32 max_rq_sg; 

Re: [PATCH v10 09/22] IB/hns: Add hca support

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:17PM +0800, Lijun Ou wrote:
> This patch mainly setup hca for RoCE. It will do a series of
> initial works, as follows:
> 1. init uar table, allocate uar resource
> 2. init pd table
> 3. init cq table
> 4. init mr table
> 5. init qp table
> 
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> ---
> PATCH v9:
> This fixes the comments given by Leon Romanovsky over the PATCH v8
>   Link: https://lkml.org/lkml/2016/6/9/67
> 
> PATCH v8/v7/v6:
>   - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---
> ---
>  drivers/infiniband/hw/hns/hns_roce_alloc.c  | 128 +
>  drivers/infiniband/hw/hns/hns_roce_cq.c |  17 +++
>  drivers/infiniband/hw/hns/hns_roce_device.h |  69 +
>  drivers/infiniband/hw/hns/hns_roce_icm.c|  88 
>  drivers/infiniband/hw/hns/hns_roce_icm.h|   7 +
>  drivers/infiniband/hw/hns/hns_roce_main.c   |  79 +++
>  drivers/infiniband/hw/hns/hns_roce_mr.c | 210 
> 
>  drivers/infiniband/hw/hns/hns_roce_pd.c |  88 
>  drivers/infiniband/hw/hns/hns_roce_qp.c |  30 
>  9 files changed, 716 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_alloc.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_mr.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_pd.c
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_alloc.c 
> b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> new file mode 100644
> index 000..d2932c1
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> @@ -0,0 +1,128 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + * Copyright (c) 2007, 2008 Mellanox Technologies. All rights reserved.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "hns_roce_device.h"
> +
> +int hns_roce_bitmap_alloc(struct hns_roce_bitmap *bitmap, unsigned long *obj)
> +{
> + int ret = 0;
> +
> + spin_lock(>lock);
> + *obj = find_next_zero_bit(bitmap->table, bitmap->max, bitmap->last);
> + if (*obj >= bitmap->max) {
> + bitmap->top = (bitmap->top + bitmap->max + bitmap->reserved_top)
> +& bitmap->mask;
> + *obj = find_first_zero_bit(bitmap->table, bitmap->max);
> + }
> +
> + if (*obj < bitmap->max) {
> + set_bit(*obj, bitmap->table);
> + bitmap->last = (*obj + 1);
> + if (bitmap->last == bitmap->max)
> + bitmap->last = 0;
> + *obj |= bitmap->top;
> + } else {
> + ret = -1;
> + }
> +
> + spin_unlock(>lock);
> +
> + return ret;
> +}
> +
> +void hns_roce_bitmap_free(struct hns_roce_bitmap *bitmap, unsigned long obj)
> +{
> + hns_roce_bitmap_free_range(bitmap, obj, 1);
> +}
> +
> +void hns_roce_bitmap_free_range(struct

Re: [PATCH v10 04/22] IB/hns: Add RoCE engine reset function

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:12PM +0800, Lijun Ou wrote:
> This patch mainly added reset flow of RoCE engine in RoCE
> driver. It is necessary when RoCE is loaded and removed.
> 
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> ---
> PATCH v9/v8:
> - No change over the PATCH v7
> 
> PATCH v7:
> This fixes the comments given by Leon Romanovsky over the PATCH v6:
>   Link: https://lkml.org/lkml/2016/5/3/733
> 
> PATCH v6:
> - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---
> ---
>  drivers/infiniband/hw/hns/hns_roce_device.h |  7 +++
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.c  | 72 
> +
>  drivers/infiniband/hw/hns/hns_roce_hw_v1.h  | 40 
>  drivers/infiniband/hw/hns/hns_roce_main.c   | 17 ++-
>  4 files changed, 135 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_hw_v1.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_hw_v1.h
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h 
> b/drivers/infiniband/hw/hns/hns_roce_device.h
> index 946b470..b857c76 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_device.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_device.h
> @@ -56,6 +56,10 @@ struct hns_roce_caps {
>   u8  num_ports;
>  };
>  
> +struct hns_roce_hw {
> + int (*reset)(struct hns_roce_dev *hr_dev, bool enable);
> +};
> +
>  struct hns_roce_dev {
>   struct ib_deviceib_dev;
>   struct platform_device  *pdev;
> @@ -68,6 +72,9 @@ struct hns_roce_dev {
>  
>   int cmd_mod;
>   int loop_idc;
> + struct hns_roce_hw  *hw;
>  };
>  
> +extern struct hns_roce_hw hns_roce_hw_v1;
> +
>  #endif /* _HNS_ROCE_DEVICE_H */
> diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c 
> b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> new file mode 100644
> index 000..198be3b
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
> @@ -0,0 +1,72 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + *
> + * This software is available to you under a choice of one of two
> + * licenses.  You may choose to be licensed under the terms of the GNU
> + * General Public License (GPL) Version 2, available from the file
> + * COPYING in the main directory of this source tree, or the
> + * OpenIB.org BSD license below:
> + *
> + * Redistribution and use in source and binary forms, with or
> + * without modification, are permitted provided that the following
> + * conditions are met:
> + *
> + *  - Redistributions of source code must retain the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer.
> + *
> + *  - Redistributions in binary form must reproduce the above
> + *copyright notice, this list of conditions and the following
> + *disclaimer in the documentation and/or other materials
> + *provided with the distribution.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> + * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> + * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
> + * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS
> + * BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
> + * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
> + * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
> + * SOFTWARE.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "hns_roce_device.h"
> +#include "hns_roce_hw_v1.h"
> +
> +/**
> + * hns_roce_v1_reset - reset roce
> + * @hr_dev: roce device struct pointer
> + * @enable: true -- drop reset, false -- reset
> + * return 0 - success , negative --fail
> + */
> +int hns_roce_v1_reset(struct hns_roce_dev *hr_dev, bool enable)
> +{
> + struct device_node *dsaf_node;
> + struct device *dev = _dev->pdev->dev;
> + struct device_node *np = dev->of_node;
> + int ret;
> +
> + dsaf_node = of_parse_phandle(np, "dsaf-handle", 0);
> +
> + if (!enable) {
> + ret = hns_dsaf_roce_reset(_node->fwnode, false);
> + } else {
> + ret = hns_dsaf_roce_reset(_node->fwnode, false);

Move this line out of if-else and leave "if (enable)" part only.

> +

Re: [PATCH v10 07/22] IB/hns: Add event queue support

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:15PM +0800, Lijun Ou wrote:
> This patch added event queue support for RoCE driver. It is used
> for RoCE interrupt. RoCE includes 32 synchronous event irqs, 1
> asynchronous event irq and 1 common overflow irq.
> 
> Signed-off-by: Wei Hu 
> Signed-off-by: Nenglong Zhao 
> Signed-off-by: Lijun Ou 
> ---
> PATCH v9/v8:
> - No change over the PATCH v7
> 
> PATCH v7:
> This fixes the comments given by Doug Ledford over the PATCH v6:
>   Link: https://lkml.org/lkml/2016/5/13/510
> 
> PATCH v6:
> - No change over the PATCH v5
> 
> PATCH v5:
> - The initial patch which was redesigned based on the second patch
>   in PATCH v4
> ---
> ---
>  drivers/infiniband/hw/hns/hns_roce_cmd.c|  22 +
>  drivers/infiniband/hw/hns/hns_roce_common.h |  70 +++
>  drivers/infiniband/hw/hns/hns_roce_cq.c |  77 +++
>  drivers/infiniband/hw/hns/hns_roce_device.h | 135 +
>  drivers/infiniband/hw/hns/hns_roce_eq.c | 750 
> 
>  drivers/infiniband/hw/hns/hns_roce_eq.h | 130 +
>  drivers/infiniband/hw/hns/hns_roce_main.c   |  24 +
>  drivers/infiniband/hw/hns/hns_roce_qp.c |  63 +++
>  8 files changed, 1271 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_cq.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_eq.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_eq.h
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_qp.c
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_cmd.c 
> b/drivers/infiniband/hw/hns/hns_roce_cmd.c
> index 64e84fe..67b3137 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_cmd.c
> +++ b/drivers/infiniband/hw/hns/hns_roce_cmd.c
> @@ -45,6 +45,28 @@
>  
>  #define CMD_MAX_NUM  32
>  
> +static int hns_roce_status_to_errno(u8 orig_status)
> +{
> + if (orig_status == HNS_ROCE_CMD_SUCCESS)
> + return 0;
> + else
> + return -EIO;
> +}

1. Can orig_status be different from SUCCESS? You defined one enum only.
2. return (orig_status == HNS_ROCE_CMD_SUCCESS)?0:(-EIO);

> +
> +void hns_roce_cmd_event(struct hns_roce_dev *hr_dev, u16 token, u8 status,
> + u64 out_param)
> +{
> + struct hns_roce_cmd_context
> + *context = _dev->cmd.context[token & hr_dev->cmd.token_mask];
> +
> + if (token != context->token)
> + return;
> +
> + context->result = hns_roce_status_to_errno(status);
> + context->out_param = out_param;
> + complete(>done);
> +}
> +
>  int hns_roce_cmd_init(struct hns_roce_dev *hr_dev)
>  {
>   struct device *dev = _dev->pdev->dev;
> diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h 
> b/drivers/infiniband/hw/hns/hns_roce_common.h
> index 595cda9..4805852 100644
> --- a/drivers/infiniband/hw/hns/hns_roce_common.h
> +++ b/drivers/infiniband/hw/hns/hns_roce_common.h
> @@ -33,7 +33,56 @@
>  #ifndef _HNS_ROCE_COMMON_H
>  #define _HNS_ROCE_COMMON_H
>  
> +#define roce_write(dev, reg, val)writel((val), (dev)->reg_base + (reg))
>  #define roce_read(dev, reg)  readl((dev)->reg_base + (reg))
> +#define roce_raw_write(value, addr) \
> + __raw_writel((__force u32)cpu_to_le32(value), (addr))
> +
> +#define roce_get_field(origin, mask, shift) \
> + (((origin) & (mask)) >> (shift))
> +
> +#define roce_get_bit(origin, shift) \
> + roce_get_field((origin), (1ul << (shift)), (shift))
> +
> +#define roce_set_field(origin, mask, shift, val) \
> + do { \
> + (origin) &= (~(mask)); \
> + (origin) |= (((u32)(val) << (shift)) & (mask)); \
> + } while (0)
> +
> +#define roce_set_bit(origin, shift, val) \
> + roce_set_field((origin), (1ul << (shift)), (shift), (val))
> +
> +#define ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_STATE_S 0
> +#define ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_STATE_M   \
> + (((1UL << 2) - 1) << ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_STATE_S)
> +
> +#define ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_AEQE_SHIFT_S 8
> +#define ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_AEQE_SHIFT_M   \
> + (((1UL << 4) - 1) << ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQC_AEQE_SHIFT_S)
> +
> +#define ROCEE_CAEP_AEQC_AEQE_SHIFT_CAEP_AEQ_ALM_OVF_INT_ST_S 17
> +
> +#define ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQ_BT_H_S 0
> +#define ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQ_BT_H_M   \
> + (((1UL << 5) - 1) << ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQ_BT_H_S)
> +
> +#define ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQE_CUR_IDX_S 16
> +#define ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQE_CUR_IDX_M   \
> + (((1UL << 16) - 1) << ROCEE_CAEP_AEQE_CUR_IDX_CAEP_AEQE_CUR_IDX_S)
> +
> +#define ROCEE_CAEP_AEQE_CONS_IDX_CAEP_AEQE_CONS_IDX_S 0
> +#define ROCEE_CAEP_AEQE_CONS_IDX_CAEP_AEQE_CONS_IDX_M   \
> + (((1UL << 16) - 1) << ROCEE_CAEP_AEQE_CONS_IDX_CAEP_AEQE_CONS_IDX_S)
> +
> +#define ROCEE_CAEP_CEQC_SHIFT_CAEP_CEQ_ALM_OVF_INT_ST_S 16
> +#define ROCEE_CAEP_CE_IRQ_MASK_CAEP_CEQ_ALM_OVF_MASK_S 1
> +#define 

Re: [PATCH v10 00/22] Add HiSilicon RoCE driver

[Leon Romanovsky]

On Thu, Jun 16, 2016 at 10:35:08PM +0800, Lijun Ou wrote:
> The HiSilicon Network Substem is a long term evolution IP which is
> supposed to be used in HiSilicon ICT SoCs. HNS (HiSilicon Network
> Sybsystem) also has a hardware support of performing RDMA with
> RoCEE.
> The driver for HiSilicon RoCEE(RoCE Engine) is a platform driver and
> will support mulitple versions of SOCs in future. This version of driver
> is meant to support Hip06 SoC(which confirms to RoCEv1 hardware
> specifications).
> 
> Changes v9 -> v10:

I stopped my review on patch 9.


signature.asc
Description: Digital signature

Re: [PATCH 1/2] mlx5: only register devlink when ethernet is available

[Leon Romanovsky]

On Fri, Jun 17, 2016 at 05:02:33PM +0200, Arnd Bergmann wrote:
> On Friday, June 17, 2016 5:50:14 PM CEST Saeed Mahameed wrote:
> > On Wed, Jun 15, 2016 at 11:50 PM, Arnd Bergmann  wrote:
> > > On Wednesday, June 15, 2016 7:04:54 PM CEST Saeed Mahameed wrote:
> > > Ok, I see. It would be nice if the process had a way to avoid build 
> > > regressions
> > > in linux-next, in particular if you already have a fix by the time a patch
> > > that introduces a problem gets added.
> > >
> > 
> > The reason we added this tree is to get 0-day testing but currently it
> > makes some unwanted noise
> > so we will remove it until we figure it out.
> 
> I think you can simply ask Fengguang Wu to add your git tree to the list
> of trees he pulls from for the 0-day test bot.

It is not 0-day only, but linux-next too. It works flawlessly for RDMA
topics and Doug receives cleaned and fully tested patches. Sadly enough,
it didn't work well for mlx5 net part.

Till further notice, I removed mlx5 net part (submission queue) from my
tree and from linux-next.


signature.asc
Description: Digital signature

Re: [PATCH v10 04/22] IB/hns: Add RoCE engine reset function

[Leon Romanovsky]

On Tue, Jun 28, 2016 at 02:31:41PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/27 16:31, oulijun wrote:
> >Hi, Leon
> >在 2016/6/27 16:01, Leon Romanovsky 写道:
> >>On Sat, Jun 25, 2016 at 06:25:37PM +0800, Wei Hu (Xavier) wrote:
> >>>
> >>>On 2016/6/24 22:59, Leon Romanovsky wrote:
> >>>>On Thu, Jun 16, 2016 at 10:35:12PM +0800, Lijun Ou wrote:
> >>>>>This patch mainly added reset flow of RoCE engine in RoCE
> >>>>>driver. It is necessary when RoCE is loaded and removed.
> >>>>>
> >>>>>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>>>>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>>>>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>>>>---
> >>...
> >>
> >>>>>+
> >>>>>+#define SLEEP_TIME_INTERVAL 20
> >>>>>+
> >>>>>+extern int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool 
> >>>>>enable);
> >>>>Why did you add this extern?
> >>>>You already exported this function.
> >>>>drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:EXPORT_SYMBOL(hns_dsaf_roce_reset);
> >>>Hi, Leon
> >>>
> >>> The function named hns_dsaf_roce_reset is defined in 
> >>> hns_dsaf_main.c
> >>> It exists in hns_dsaf.ko(ethernet driver)
> >>>
> >>> RoCE driver will call this function.
> >>>
> >>> Your suggestion is that delete "extern" as below:
> >>> In /drivers/infiniband/hw/hns/hns_roce_hw_v1.h:
> >>>
> >>>   int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool
> >>>enable);
> >>>
> >>>Right? or other soultion?
> >>You placed it in header file.
> >>Please move it to your hns_roce_hw_v1.c file.
> >>
> >  You suggest to do as follows, right?
> >  in hns_roce_hw_v1.c
> >int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable);
> >
> >  and delete the keyword extern
> >
> >  Bcause reserve the extern in hns_roce_hw_v1.c, the checkpatch is not pass.
> Hi, Leon & Doug Ledford
> 
> If we move it to hns_roce_hw_v1.c file as below:
> int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool
> enable);
> The result of checkpatch is warning.
> 
> We prepare to add a head file for this function as below:
> In the directory of include\linux,  mkdir hns.
> add hns_driver.h in include\linux\hns.
> In the file of hns_driver.h, the declaration:
>int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode,
> bool enable);
> What do you think about?
> 
>

Please avoid creating new directories/files under include/linux,
especially for one function only.


signature.asc
Description: Digital signature

Re: [PATCH v10 03/22] IB/hns: Add initial main frame driver and get cfg info

[Leon Romanovsky]

On Sat, Jun 25, 2016 at 06:29:31PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/24 19:48, Leon Romanovsky wrote:
> >On Thu, Jun 16, 2016 at 10:35:11PM +0800, Lijun Ou wrote:
> >>This patch mainly added the initial bare main driver. It
> >>could get the relative configure information of net node.
> >>
> >>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>---

...

> >>+   return -ENOMEM;
> >>+
> >>+   for (i = 0; i < HNS_ROCE_MAX_PORTS; i++) {
> >>+   net_node = of_parse_phandle(np, "eth-handle", i);
> >>+   if (net_node) {
> >>+   pdev = of_find_device_by_node(net_node);
> >>+   netdev = platform_get_drvdata(pdev);
> >>+   phy_port = (u8)i;
> >>+   if (netdev) {
> >>+   hr_dev->iboe.netdevs[port_cnt] = netdev;
> >>+   hr_dev->iboe.phy_port[port_cnt] = phy_port;
> >>+   } else {
> >>+   return -ENODEV;
> >>+   }
> >>+   port_cnt++;
> >>+   }
> >>+   }
> >Do you want to check port_cnt value, before continue?
> Hi, Leon
> Maybe we need not to check port_cnt value.
> port_cnt can be ensured smaller than HNS_ROCE_MAX_PORTS.

You can in theory to get port_cnt == 0, in such case it is wise to
return error.



signature.asc
Description: Digital signature

Re: [PATCH 2/3] net: hns: add Hisilicon RoCE support

[Leon Romanovsky]

On Mon, Mar 14, 2016 at 09:12:28AM +0800, Yankejian (Hackim Yim) wrote:
> 
> 
> On 2016/3/12 18:43, Leon Romanovsky wrote:
> > On Fri, Mar 11, 2016 at 06:37:10PM +0800, Lijun Ou wrote:
> >> It added hns_dsaf_roce_reset routine for roce driver.
> >> RoCE is a feature of hns.
> >> In hip06 SOC, in roce reset process, it's needed to configure
> >> dsaf channel reset,port and sl map info.
> >>
> >> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >> Signed-off-by: Wei Hu(Xavier) <xavier.hu...@huawei.com>
> >> Signed-off-by: Lisheng <lisheng...@huawei.com>
> >> ---
> >>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 84 
> >> ++
> >>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 14 
> >>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c | 62 +---
> >>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  | 13 
> >>  4 files changed, 163 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
> >> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> >> index 38fc5be..a0f0d4f 100644
> >> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> >> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> >> @@ -12,6 +12,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>  #include 
> >>  #include 
> >>  #include 
> >> @@ -2593,6 +2594,89 @@ static struct platform_driver g_dsaf_driver = {
> >>  
> >>  module_platform_driver(g_dsaf_driver);
> >>  
> >> +/**
> >> + * hns_dsaf_roce_reset - reset dsaf and roce
> >> + * @dsaf_fwnode: Pointer to framework node for the dasf
> >> + * @val: 0 - request reset , 1 - drop reset
> >> + * retuen 0 - success , negative --fail
> >> + */
> >> +int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, u32 val)
> >> +{
> >> +  struct dsaf_device *dsaf_dev;
> >> +  struct platform_device *pdev;
> >> +  unsigned int mp;
> >> +  unsigned int sl;
> >> +  unsigned int credit;
> >> +  int i;
> >> +  const u32 port_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> >> +  {0, 0, 0},
> >> +  {1, 0, 0},
> >> +  {2, 1, 0},
> >> +  {3, 1, 0},
> >> +  {4, 2, 1},
> >> +  {4, 2, 1},
> >> +  {5, 3, 1},
> >> +  {5, 3, 1},
> >> +  };
> >> +  const u32 sl_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> >> +  {0, 0, 0},
> >> +  {0, 1, 1},
> >> +  {0, 0, 2},
> >> +  {0, 1, 3},
> >> +  {0, 0, 0},
> >> +  {1, 1, 1},
> >> +  {0, 0, 2},
> >> +  {1, 1, 3},
> >> +  };
> > Do you have a plan to send a version with enums/defines for this
> > numbers? Especially for _CHAN_MODE.
> >
> > .
> 
> Hi leon,
> 
> it seems the enums is added in hns_dsaf_main.h, as belows:
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h 
> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h
> index 5fea226..c917b9a 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h
> @@ -40,6 +40,16 @@ struct hns_mac_cb;
>  #define DSAF_DUMP_REGS_NUM 504
>  #define DSAF_STATIC_NUM 28
>  
> +#define DSAF_ROCE_CREDIT_CHN 8
> +#define DSAF_ROCE_CHAN_MODE 3
> +
> +enum dsaf_roce_port_port_mode {
> + DSAF_ROCE_6PORT_MODE,
> + DSAF_ROCE_4PORT_MODE,
> + DSAF_ROCE_2PORT_MODE,
> + DSAF_ROCE_CHAN_MODE_NUM
> +};
> +

These defines are used as an index entry into si_map and port_map arrays
and seems as not related to the actual data.

I suggest you to take this code back to drawing table, redesign it,
clean unused functions and defines and resubmit it.

Thanks.

> 
> MBR, Kejian
> 
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/5] mlx4: add missing braces in verify_qp_parameters

[Leon Romanovsky]

On Mon, Mar 14, 2016 at 03:18:34PM +0100, Arnd Bergmann wrote:
> The implementation of QP paravirtualization back in linux-3.7 included
> some code that looks very dubious, and gcc-6 has grown smart enough
> to warn about it:
> 
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c: In function 
> 'verify_qp_parameters':
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3154:5: error: 
> statement is indented as if it were guarded by... 
> [-Werror=misleading-indentation]
>  if (optpar & MLX4_QP_OPTPAR_ALT_ADDR_PATH) {
>  ^~
> drivers/net/ethernet/mellanox/mlx4/resource_tracker.c:3144:4: note: ...this 
> 'if' clause, but it is not
> if (slave != mlx4_master_func_num(dev))
> 
> From looking at the context, I'm reasonably sure that the indentation
> is correct but that it should have contained curly braces from the
> start, as the update_gid() function in the same patch correctly does.
> 
> Signed-off-by: Arnd Bergmann <a...@arndb.de>
> Fixes: 54679e148287 ("mlx4: Implement QP paravirtualization and maintain 
> phys_pkey_cache for smp_snoop")

Thanks, looks good.
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

> ---
>  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c 
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> index 25ce1b030a00..cd9b2b28df88 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> @@ -3141,7 +3141,7 @@ static int verify_qp_parameters(struct mlx4_dev *dev,
>   case QP_TRANS_RTS2RTS:
>   case QP_TRANS_SQD2SQD:
>   case QP_TRANS_SQD2RTS:
> - if (slave != mlx4_master_func_num(dev))
> + if (slave != mlx4_master_func_num(dev)) {
>   if (optpar & MLX4_QP_OPTPAR_PRIMARY_ADDR_PATH) {
>   port = (qp_ctx->pri_path.sched_queue >> 
> 6 & 1) + 1;
>   if (dev->caps.port_mask[port] != 
> MLX4_PORT_TYPE_IB)
> @@ -3160,6 +3160,7 @@ static int verify_qp_parameters(struct mlx4_dev *dev,
>   if (qp_ctx->alt_path.mgid_index >= 
> num_gids)
>   return -EINVAL;
>   }
> + }
>   break;
>   default:
>   break;
> -- 
> 2.7.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] infiniband: IB/hns: add Hisilicon RoCE support

[Leon Romanovsky]

On Fri, Mar 11, 2016 at 06:37:09PM +0800, Lijun Ou wrote:
> The driver for Hisilicon RoCE is a platform driver.
> The driver will support mulitple versions of hardware. Currently only "v1"
> for hip06 SOC is supported.
> The driver includes two parts: common driver and hardware-specific
> operations. hns_roce_v1_hw.c and hns_roce_v1_hw.h are files for
> hardware-specific operations only for v1 engine, and other files(.c and .h)
> for common algorithm and common hardware operations
> 
> Signed-off-by: Lijun Ou 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: Znlong 
> ---
>  drivers/infiniband/Kconfig |2 +-
>  drivers/infiniband/hw/Makefile |1 +
>  drivers/infiniband/hw/hisilicon/hns/Kconfig|   10 +
>  drivers/infiniband/hw/hisilicon/hns/Makefile   |9 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c  |  114 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_alloc.c   |  253 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.c |  354 +++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.h |  163 ++
>  .../infiniband/hw/hisilicon/hns/hns_roce_common.h  |  704 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cq.c  |  454 +++
>  .../infiniband/hw/hisilicon/hns/hns_roce_device.h  |  840 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.c  |  798 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.h  |  138 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.c |  608 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.h |  121 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_main.c| 1124 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_mr.c  |  637 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_pd.c  |  129 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_qp.c  |  890 ++
>  .../infiniband/hw/hisilicon/hns/hns_roce_user.h|   31 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c   | 2992 
> 
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h   | 1068 +++
>  22 files changed, 11439 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/Kconfig
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/Makefile
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_alloc.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_common.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cq.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_device.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_main.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_mr.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_pd.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_qp.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_user.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 8a8440c..c7a24bb 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -73,7 +73,7 @@ source "drivers/infiniband/hw/mlx5/Kconfig"
>  source "drivers/infiniband/hw/nes/Kconfig"
>  source "drivers/infiniband/hw/ocrdma/Kconfig"
>  source "drivers/infiniband/hw/usnic/Kconfig"
> -

No need to remove this line. It separates HW from ULPs.

> +source "drivers/infiniband/hw/hisilicon/hns/Kconfig"
>  source "drivers/infiniband/ulp/ipoib/Kconfig"
>  
>  source "drivers/infiniband/ulp/srp/Kconfig"



> +int hns_roce_bitmap_alloc_range(
> + struct hns_roce_bitmap *bitmap,
> + int cnt, int align, u32 *obj)

You have indentation issues.

> +{
> + int i;
> + int ret = 0;
> +
> + if (likely(cnt == 1 && align == 1))
> + return hns_roce_bitmap_alloc(bitmap, obj);
> +
> + spin_lock(>lock);
> +
> + *obj = bitmap_find_next_zero_area(bitmap->table, bitmap->max,
> +   bitmap->last, cnt, align - 1);
> + if (*obj >= bitmap->max) {
> + bitmap->top = (bitmap->top + bitmap->max + bitmap->reserved_top)
> +& bitmap->mask;
> + *obj = bitmap_find_next_zero_area(bitmap->table, bitmap->max, 0,
> 

Re: [PATCH 2/3] net: hns: add Hisilicon RoCE support

[Leon Romanovsky]

On Fri, Mar 11, 2016 at 06:37:10PM +0800, Lijun Ou wrote:
> It added hns_dsaf_roce_reset routine for roce driver.
> RoCE is a feature of hns.
> In hip06 SOC, in roce reset process, it's needed to configure
> dsaf channel reset,port and sl map info.
> 
> Signed-off-by: Lijun Ou 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: Lisheng 
> ---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 84 
> ++
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h | 14 
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c | 62 +---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  | 13 
>  4 files changed, 163 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> index 38fc5be..a0f0d4f 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2593,6 +2594,89 @@ static struct platform_driver g_dsaf_driver = {
>  
>  module_platform_driver(g_dsaf_driver);
>  
> +/**
> + * hns_dsaf_roce_reset - reset dsaf and roce
> + * @dsaf_fwnode: Pointer to framework node for the dasf
> + * @val: 0 - request reset , 1 - drop reset
> + * retuen 0 - success , negative --fail
> + */
> +int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, u32 val)
> +{
> + struct dsaf_device *dsaf_dev;
> + struct platform_device *pdev;
> + unsigned int mp;
> + unsigned int sl;
> + unsigned int credit;
> + int i;
> + const u32 port_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> + {0, 0, 0},
> + {1, 0, 0},
> + {2, 1, 0},
> + {3, 1, 0},
> + {4, 2, 1},
> + {4, 2, 1},
> + {5, 3, 1},
> + {5, 3, 1},
> + };
> + const u32 sl_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> + {0, 0, 0},
> + {0, 1, 1},
> + {0, 0, 2},
> + {0, 1, 3},
> + {0, 0, 0},
> + {1, 1, 1},
> + {0, 0, 2},
> + {1, 1, 3},
> + };

Do you have a plan to send a version with enums/defines for this
numbers? Especially for _CHAN_MODE.

Re: [PATCH 0/3] infiniband: IB/hns: Hisilicon RoCE support

[Leon Romanovsky]

On Fri, Mar 11, 2016 at 06:37:08PM +0800, Lijun Ou wrote:

1) It is redundant to write "infiniband" and "IB" in one title to mention
relevant subsystem, since it is the same.

Please take a look on the other submissions here on the list and use
similar construction.

2) Please use version number in the titles [PATCH vXXX]

> The Hisilicon Network Substem(hns) is a long term evolution IP which is
> supposed to be used in Hisilicon ICT SoC. RoCE is a feature of hns.
> The driver for Hisilicon RoCE engine is a platform driver.
> The driver will support mulitple versions of hns. Currently only "v1"
> for hip06 SOC is supported.
> 
> 
> Changes v1 -> v2:
> 1. adjust the formats of roce driver code by the experts reviewing
> 2. modify the bindings file with roce dts. add the attribute named 
> interrput-names.
> 3. modify the way of defining port mode in hns_dsaf_main.c
> 4. move the Kconfig file into the hns directory and send it with roce
> driver code file together.
> 
> 
> Lijun Ou (3):
>   infiniband: IB/hns: add Hisilicon RoCE support
>   net: hns: add Hisilicon RoCE support
>   infiniband: IB/hns: add Hisilicon RoCE support with bindings
> 

Re: [RESEND PATCH V4 2/3] IB/hns: Add HiSilicon RoCE driver support

[Leon Romanovsky]

On Tue, Apr 05, 2016 at 03:32:53PM +0800, oulijun wrote:
> >>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c   | 2832 
> >> 
> >>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h   |  985 +++
> >   ^^
> > Do you support v1 of RoCE or v1 of your HW?
> > 
> Here, v1 stands for hw, that is, we support v1 of our hw.

So you should write hns_roce_hw_v1 and not hns_roce_v1_hw

> >>  23 files changed, 10429 insertions(+)
> > 
> > Please appreciate the effort needed to review such large patch and
> > invest time and effort to divide this to number of small easy review 
> > patches.
> > 
> Surely, i have pay attention to the patch, but i consider that it is not 
> better to
> split the patch into small patch. because it will the base function of RoCE.
> For your advice, i will make further efforts to taking a discussion how 
> to reslove the question.

Faisal Latif's submission [1] can help you to get inspiration.

Also please DON'T submit your patches till you get rid of unrelated
functions/macros/defines and DMA operations for register access.

[1] https://lwn.net/Articles/668721/

> 
> thanks
> Lijun Ou
> > .
> > 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH for-next 2/2] net/mlx5: Update mlx5_ifc hardware features

[Leon Romanovsky]

On Tue, Apr 12, 2016 at 12:37:34AM +0300, Or Gerlitz wrote:
> On Tue, Apr 12, 2016 at 12:24 AM, Saeed Mahameed
>  wrote:
> > On Tue, Apr 12, 2016 at 12:17 AM, Or Gerlitz  wrote:
> 
> >> feature --> features
> 
> > Correct, will fix.
> 
> >>> * Add vport to steering commands for SRIOV ACL support
> >>> * Add mlcr, pcmr and mcia registers for dump module EEPROM
> >>> * Add support for FCS, baeacon led and disable_link bits to hca caps
> >>> * Add CQE period mode bit in  CQ context for CQE based CQ
> >>>   moderation support
> >>> * Add umr SQ bit for fragmented memory registration
> >>> * Add needed bits and caps for Striding RQ support
> 
> >> AFAIK, all the above are features will go through net-next, what made
> >> you anticipate conflicts with linux-rdma?
> 
> > FCS bit is needed also for rdma, so we took the liberty of updating
> > all the needed HW structs, bits, caps, etc ..
> > at once for all mlx5 features planned for 4.7 regardless of rdma/net 
> > conflicts.
> 
> The cover letter states that this series deals with shared code.
> 
> I guess you might also could extend it a bit to deal also with code
> that you suspect could lead to conflicts, but I don't see why it
> evolved to that extent.

Or,
All these micro-optimizations on this shared file can potentially lead
to undesired merge conflicts. Subsystem maintainers and Linus don't need to
deal with these conflicts at all.

It won't help to anyone to split this commit to more than one patch.


signature.asc
Description: Digital signature

Re: [PATCH for-next 2/2] net/mlx5: Update mlx5_ifc hardware features

[Leon Romanovsky]

On Tue, Apr 12, 2016 at 08:36:21AM +0300, Or Gerlitz wrote:
> Conflicts happens @ all times, life.
> 

...

> 
> I understand your desire to get it down to zero, but it's not gonna
> work, pick another target.

Maybe you are right and the time will show, but now we (Saeed, Matan and me)
are trying hard to achieve this goal.

> 
> For example, the networking community has a fairly large rc activity
> (I would say 10-20x
> vs rdma), so when Dave does his "merge-rebases" for net-next over net
> and linus tree
> (4-5 times in a release), he has to this way or another solve
> conflicts, yes! ditto for
> Linus during merge windows and to some extent in rc times too.

I don't see any harm in our desire to decrease work overhead from these
busy people.

> 
> > It won't help to anyone to split this commit to more than one patch.
> 
> The commit change-log should make it clear what this is about, and it doesn't.
> If you believe in something, state that clear, be precise.

I agree.

> 
> As Saeed admitted the shared code in the commit spans maybe 2% of it.
> 
> The 1st commit deals with a field which is not used in the driver,
> this is a cleanup
> that you can do in rc (net) patch (remove the field all together) and
> overall, w.o seeing

I don't agree with your point that cleanup should go to RC.

> the down-stream patches that depend on the newly introduced fields,
> how do you know there aren't such (unused) bits in the 2nd commit?

No, I don't know in advance, but the truth is that it doesn't bother
anyone, because we are exposing our internal HW to kernel clients and
doing it with minimal impact on the maintainers.


signature.asc
Description: Digital signature

Re: [PATCH v3 2/3] IB/hns: Add HiSilicon RoCE driver support

[Leon Romanovsky]

On Sat, Mar 19, 2016 at 06:50:57PM +0800, Lijun Ou wrote:
> The driver for HiSilicon RoCE is a platform driver.
> The driver will support multiple versions of hardware. Currently only "v1"
> for hip06 SoC is supported.
> The driver includes two parts: common driver and hardware-specific
> operations. hns_roce_v1_hw.c and hns_roce_v1_hw.h are files for
> hardware-specific operations only for v1 engine, and other files(.c and .h)
> for common algorithm and common hardware operations.
> 
> Signed-off-by: Lijun Ou 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: Znlong 
> ---
>  MAINTAINERS|8 +
>  drivers/infiniband/Kconfig |1 +
>  drivers/infiniband/hw/Makefile |1 +
>  drivers/infiniband/hw/hisilicon/hns/Kconfig|   10 +
>  drivers/infiniband/hw/hisilicon/hns/Makefile   |9 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c  |  110 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_alloc.c   |  239 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.c |  338 +++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.h |   80 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_common.h  |  308 +++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cq.c  |  436 +++
>  .../infiniband/hw/hisilicon/hns/hns_roce_device.h  |  794 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.c  |  758 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.h  |  133 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.c |  578 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.h |  112 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_main.c| 1097 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_mr.c  |  605 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_pd.c  |  124 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_qp.c  |  841 ++
>  .../infiniband/hw/hisilicon/hns/hns_roce_user.h|   31 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c   | 2835 
> 
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h   |  986 +++
>  23 files changed, 10434 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/Kconfig
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/Makefile
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_alloc.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_common.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_cq.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_device.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_main.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_mr.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_pd.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_qp.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_user.h
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c
>  create mode 100644 drivers/infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2933d90..0c7fac5 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -9878,6 +9878,14 @@ W: http://www.emulex.com
>  S:   Supported
>  F:   drivers/infiniband/hw/ocrdma/
>  
> +HISILICON ROCE DRIVER
> +M:   Wei Hu(Xavier) 
> +M:   Lijun Ou 
> +L:   linux-r...@vger.kernel.org
> +S:   Maintained
> +F:   drivers/infiniband/hw/hisilicon/
> +F:   Documentation/devicetree/bindings/infiniband/hisilicon-hns-roce.txt
> +
>  SFC NETWORK DRIVER
>  M:   Solarflare linux maintainers 
>  M:   Shradha Shah 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 8a8440c..02eca75 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -73,6 +73,7 @@ source "drivers/infiniband/hw/mlx5/Kconfig"
>  source "drivers/infiniband/hw/nes/Kconfig"
>  source "drivers/infiniband/hw/ocrdma/Kconfig"
>  source "drivers/infiniband/hw/usnic/Kconfig"
> +source "drivers/infiniband/hw/hisilicon/hns/Kconfig"
>  
>  source "drivers/infiniband/ulp/ipoib/Kconfig"
>  
> diff --git a/drivers/infiniband/hw/Makefile b/drivers/infiniband/hw/Makefile
> index aded2a5..ddbbf715 100644
> --- a/drivers/infiniband/hw/Makefile
> +++ b/drivers/infiniband/hw/Makefile
> 

Re: [PATCH 1/3] infiniband: IB/hns: add Hisilicon RoCE support

[Leon Romanovsky]

On Wed, Mar 16, 2016 at 11:36:38AM +0100, Jiri Pirko wrote:
> >so, I continue to have it.
> 
> I will continue to bash on your odd codingstyle. Please fix it!

Jiri,

Checkpatch errors is an easiest issue with this patch.

It is full of functions without use, unconnected macros and
if you replace "hsi" to name of other well known driver, you will get
same code :).

They need to redesign the whole driver before resubmission.

Thanks.

Re: call attention to review

[Leon Romanovsky]

On Thu, Mar 24, 2016 at 01:50:30PM +0800, oulijun wrote:
> Hi,
>I am Lijun Ou. I have sent the PATCH v4 of HiSilicon RoCE driver at March 
> 22, 2016.
> if you are convenient, please help to review. Welcome to give your reviewing.

Hi Lijun,
Please read whole document which describes how to submit patches [1]
and especially section 9, but please don't limit yourself to that
section only. The whole document is relevant to your patches.

[1] https://www.kernel.org/doc/Documentation/SubmittingPatches

> 
> thanks
> Lijun Ou
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/4] infiniband: hns: Hisilicon RoCE support

[Leon Romanovsky]

On Fri, Mar 04, 2016 at 04:41:13PM +0800, Wei Hu(Xavier) wrote:
> The Hisilicon Network Substem(hns) is a long term evolution IP which is
> supposed to be used in Hisilicon ICT SoC. RoCE is a feature of hns.
> The driver for Hisilicon RoCE engine is a platform driver.
> The driver will support mulitple versions of hns. Currently only "v1"
> for hip06 SOC is supported.
> 
> Wei Hu(Xavier) (4):
>   net: hns: add Hisilicon RoCE support(the dependent routine)
>   infiniband: hns: add Hisilicon RoCE support(binding)
>   infiniband: hns: add Hisilicon RoCE support(driver code)
>   infiniband: hns: add Hisilicon RoCE support(Kconfig)

Please use commonly used in this subsystem titles:
"infiniband: hns: add" -> "IB/hns: Add"

> 
>  .../bindings/infiniband/hisilicon-hns-roce.txt |   68 +

...^^^... it looks strange.

Thanks.

Re: [PATCH 4/4] infiniband: hns: add Hisilicon RoCE support(Kconfig)

[Leon Romanovsky]

On Fri, Mar 04, 2016 at 04:41:17PM +0800, Wei Hu(Xavier) wrote:
> This submit add Kconfig.

It needs be part of previous patch.

> 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: oulijun 

^^^.. -- Is this first name or second name?

> ---
>  drivers/infiniband/Kconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 8a8440c..c7a24bb 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -73,7 +73,7 @@ source "drivers/infiniband/hw/mlx5/Kconfig"
>  source "drivers/infiniband/hw/nes/Kconfig"
>  source "drivers/infiniband/hw/ocrdma/Kconfig"
>  source "drivers/infiniband/hw/usnic/Kconfig"
> -
> +source "drivers/infiniband/hw/hisilicon/hns/Kconfig"
>  source "drivers/infiniband/ulp/ipoib/Kconfig"
>  
>  source "drivers/infiniband/ulp/srp/Kconfig"
> -- 
> 1.9.1
> 

Re: [PATCH 1/4] net: hns: add Hisilicon RoCE support(the dependent routine)

[Leon Romanovsky]

Please rewrite your title to be without (...).

On Fri, Mar 04, 2016 at 04:41:14PM +0800, Wei Hu(Xavier) wrote:
> It added hns_dsaf_roce_reset routine for roce driver.
> RoCE is a feature of hns.
> In hip06 SOC, in roce reset process, it's needed to configure
> dsaf channel reset,port and sl map info.
> 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: Lisheng 
> Signed-off-by: oulijun 
> ---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c | 82 
> ++
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.h |  7 ++
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_misc.c | 62 +---
>  drivers/net/ethernet/hisilicon/hns/hns_dsaf_reg.h  | 14 
>  4 files changed, 155 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c 
> b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> index 9439f04..41ba948 100644
> --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c
> @@ -12,6 +12,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -2556,6 +2557,87 @@ static struct platform_driver g_dsaf_driver = {
>  
>  module_platform_driver(g_dsaf_driver);
>  
> +/**
> + * hns_dsaf_roce_reset - reset dsaf and roce
> + * @dsaf_fwnode: Pointer to framework node for the dasf
> + * @val: 0 - request reset , 1 - drop reset
> + * retuen 0 - success , negative --fail
> + */
> +int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, u32 val)
> +{
> + struct dsaf_device *dsaf_dev;
> + struct platform_device *pdev;
> + unsigned int mp;
> + unsigned int sl;
> + unsigned int credit;
> + int i;
> + const u32 port_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> + {0, 0, 0},
> + {1, 0, 0},
> + {2, 1, 0},
> + {3, 1, 0},
> + {4, 2, 1},
> + {4, 2, 1},
> + {5, 3, 1},
> + {5, 3, 1},
> + };
> + const u32 sl_map[DSAF_ROCE_CREDIT_CHN][DSAF_ROCE_CHAN_MODE] = {
> + {0, 0, 0},
> + {0, 1, 1},
> + {0, 0, 2},
> + {0, 1, 3},
> + {0, 0, 0},
> + {1, 1, 1},
> + {0, 0, 2},
> + {1, 1, 3},
> + };

Please prefer enums/defines instead of hard coded values.
it is applicable to whole submitted code.

Re: [PULL REQUEST] Please pull rdma.git

[Leon Romanovsky]

On Thu, Mar 31, 2016 at 10:18:47PM -0400, David Miller wrote:
> From: Leon Romanovsky <l...@leon.nu>
> Date: Fri, 1 Apr 2016 02:38:28 +0300
> 
> > On Wed, Mar 23, 2016 at 09:37:54AM -0400, Doug Ledford wrote:
> >> On 03/23/2016 06:57 AM, Leon Romanovsky wrote:
> >> > On Sat, Mar 19, 2016 at 02:37:08PM -0700, Linus Torvalds wrote:
> >> >> So the *best* situation would be:
> >> >>
> >> >>  - your two groups talk it over, and figure out what the common commits 
> >> >> are
> >> >>
> >> >>  - you put those common commits as a "base" branch in git
> >> >>
> >> >>  - you ask the two upper-level maintainers to both pull that base branch
> >> >>
> >> >>  - you then make sure that you send the later patches (whether as
> >> >> emailed patches or as pull requests) based on top of that base branch.
> >> > 
> >> > Hi David and Doug,
> >> > 
> >> > Are you OK with the approach suggested by Linus?
> >> > We are eager to know it, so we will adopt it as soon
> >> > as possible in our development flow.
> >> > 
> >> > The original thread [1].
> >> > 
> >> > [1] http://thread.gmane.org/gmane.linux.drivers.rdma/34907
> >> > 
> >> > Thanks.
> >> > 
> >> 
> >> I'm fine with it.  Since I happen to use topic branches to build my
> >> for-next from anyway, I might need to be the one that Dave pulls from
> >> versus the other way around.
> > 
> > Resending to linux-netdev.
> > 
> > David,
> > Can you please express your opinion about Linus's suggestion to
> > eliminate merge conflicts in Mellanox related products?
> 
> Sure, sounds fine.

Thank you, I appreciate a lot Doug's and your openness and
willingness to help us eliminate the future merge obstacles.

Re: [PULL REQUEST] Please pull rdma.git

[Leon Romanovsky]

On Wed, Mar 23, 2016 at 09:37:54AM -0400, Doug Ledford wrote:
> On 03/23/2016 06:57 AM, Leon Romanovsky wrote:
> > On Sat, Mar 19, 2016 at 02:37:08PM -0700, Linus Torvalds wrote:
> >> So the *best* situation would be:
> >>
> >>  - your two groups talk it over, and figure out what the common commits are
> >>
> >>  - you put those common commits as a "base" branch in git
> >>
> >>  - you ask the two upper-level maintainers to both pull that base branch
> >>
> >>  - you then make sure that you send the later patches (whether as
> >> emailed patches or as pull requests) based on top of that base branch.
> > 
> > Hi David and Doug,
> > 
> > Are you OK with the approach suggested by Linus?
> > We are eager to know it, so we will adopt it as soon
> > as possible in our development flow.
> > 
> > The original thread [1].
> > 
> > [1] http://thread.gmane.org/gmane.linux.drivers.rdma/34907
> > 
> > Thanks.
> > 
> 
> I'm fine with it.  Since I happen to use topic branches to build my
> for-next from anyway, I might need to be the one that Dave pulls from
> versus the other way around.

Resending to linux-netdev.

David,
Can you please express your opinion about Linus's suggestion to
eliminate merge conflicts in Mellanox related products?

Thanks

> 
> -- 
> Doug Ledford <dledf...@redhat.com>
>   GPG KeyID: 0E572FDD
> 
> 

Re: [RESEND PATCH V4 2/3] IB/hns: Add HiSilicon RoCE driver support

[Leon Romanovsky]

On Fri, Apr 01, 2016 at 05:21:31PM +0800, Lijun Ou wrote:
> The driver for HiSilicon RoCE is a platform driver.
> The driver will support multiple versions of hardware. Currently only "v1"
> for hip06 SoC is supported.
> The driver includes two parts: common driver and hardware-specific
> operations. hns_roce_v1_hw.c and hns_roce_v1_hw.h are files for
> hardware-specific operations only for v1 engine, and other files(.c and .h)
> for common algorithm and common hardware operations.
> 
> Signed-off-by: Lijun Ou 
> Signed-off-by: Wei Hu(Xavier) 
> Signed-off-by: Znlong 
> ---
>  MAINTAINERS|8 +
>  drivers/infiniband/Kconfig |1 +
>  drivers/infiniband/hw/Makefile |1 +
>  drivers/infiniband/hw/hisilicon/hns/Kconfig|   10 +
>  drivers/infiniband/hw/hisilicon/hns/Makefile   |9 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c  |  110 +

We are not adding name of company (hisilicon) for infiniband HW drivers
drivers/infiniband/hw/hisilicon/hns/hns_roce_ah.c
--->
drivers/infiniband/hw/hns/hns_roce_ah.c


>  .../infiniband/hw/hisilicon/hns/hns_roce_alloc.c   |  239 ++
 ^^
Please fix you paths.

>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.c |  338 +++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cmd.h |   80 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_common.h  |  308 +++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_cq.c  |  436 +++
>  .../infiniband/hw/hisilicon/hns/hns_roce_device.h  |  794 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.c  |  758 ++
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_eq.h  |  132 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.c |  578 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_icm.h |  112 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_main.c| 1097 
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_mr.c  |  605 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_pd.c  |  124 +
>  drivers/infiniband/hw/hisilicon/hns/hns_roce_qp.c  |  841 ++
>  .../infiniband/hw/hisilicon/hns/hns_roce_user.h|   31 +
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.c   | 2832 
> 
>  .../infiniband/hw/hisilicon/hns/hns_roce_v1_hw.h   |  985 +++
  ^^
Do you support v1 of RoCE or v1 of your HW?

>  23 files changed, 10429 insertions(+)

Please appreciate the effort needed to review such large patch and
invest time and effort to divide this to number of small easy review patches.

Re: [PATCH] RDS: sync congestion map updating

[Leon Romanovsky]

On Fri, Apr 01, 2016 at 12:47:24PM -0700, santosh shilimkar wrote:
> (cc-ing netdev)
> On 3/30/2016 7:59 PM, Wengang Wang wrote:
> >
> >
> >在 2016年03月31日 09:51, Wengang Wang 写道:
> >>
> >>
> >>在 2016年03月31日 01:16, santosh shilimkar 写道:
> >>>Hi Wengang,
> >>>
> >>>On 3/30/2016 9:19 AM, Leon Romanovsky wrote:
> >>>>On Wed, Mar 30, 2016 at 05:08:22PM +0800, Wengang Wang wrote:
> >>>>>Problem is found that some among a lot of parallel RDS
> >>>>>communications hang.
> >>>>>In my test ten or so among 33 communications hang. The send
> >>>>>requests got
> >>>>>-ENOBUF error meaning the peer socket (port) is congested. But
> >>>>>meanwhile,
> >>>>>peer socket (port) is not congested.
> >>>>>
> >>>>>The congestion map updating can happen in two paths: one is in
> >>>>>rds_recvmsg path
> >>>>>and the other is when it receives packets from the hardware. There
> >>>>>is no
> >>>>>synchronization when updating the congestion map. So a bit
> >>>>>operation (clearing)
> >>>>>in the rds_recvmsg path can be skipped by another bit operation
> >>>>>(setting) in
> >>>>>hardware packet receving path.
> >>>>>
> >
> >To be more detailed.  Here, the two paths (user calls recvmsg and
> >hardware receives data) are for different rds socks. thus the
> >rds_sock->rs_recv_lock is not helpful to sync the updating on congestion
> >map.
> >
> For archive purpose, let me try to conclude the thread. I synced
> with Wengang offlist and came up with below fix. I was under
> impression that __set_bit_le() was atmoic version. After fixing
> it like patch(end of the email), the bug gets addressed.
> 
> I will probably send this as fix for stable as well.
> 
> 
> From 5614b61f6fdcd6ae0c04e50b97efd13201762294 Mon Sep 17 00:00:00 2001
> From: Santosh Shilimkar <santosh.shilim...@oracle.com>
> Date: Wed, 30 Mar 2016 23:26:47 -0700
> Subject: [PATCH] RDS: Fix the atomicity for congestion map update
> 
> Two different threads with different rds sockets may be in
> rds_recv_rcvbuf_delta() via receive path. If their ports
> both map to the same word in the congestion map, then
> using non-atomic ops to update it could cause the map to
> be incorrect. Lets use atomics to avoid such an issue.
> 
> Full credit to Wengang <wen.gang.w...@oracle.com> for
> finding the issue, analysing it and also pointing out
> to offending code with spin lock based fix.

I'm glad that you solved the issue without spinlocks.
Out of curiosity, I see that this patch is needed to be sent
to Dave and applied by him. Is it right?

➜  linus-tree git:(master) ./scripts/get_maintainer.pl -f net/rds/cong.c
Santosh Shilimkar <santosh.shilim...@oracle.com> (supporter:RDS -
RELIABLE DATAGRAM SOCKETS)
"David S. Miller" <da...@davemloft.net> (maintainer:NETWORKING
[GENERAL])
netdev@vger.kernel.org (open list:RDS - RELIABLE DATAGRAM SOCKETS)
linux-r...@vger.kernel.org (open list:RDS - RELIABLE DATAGRAM SOCKETS)
rds-de...@oss.oracle.com (moderated list:RDS - RELIABLE DATAGRAM
SOCKETS)
linux-ker...@vger.kernel.org (open list)

> 
> Signed-off-by: Wengang Wang <wen.gang.w...@oracle.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilim...@oracle.com>

Reviewed-by: Leon Romanovsky <l...@leon.nu>

Re: [PATCH net] net/mlx4: Avoid wrong virtual mappings

[Leon Romanovsky]

On Mon, Apr 25, 2016 at 04:34:47PM +0300, Haggai Abramovsky wrote:
> -int mlx4_buf_alloc(struct mlx4_dev *dev, int size, int max_direct,
> -struct mlx4_buf *buf, gfp_t gfp)
> +static int mlx4_buf_direct_alloc(struct mlx4_dev *dev, int size,
> +  struct mlx4_buf *buf, gfp_t gfp)
>  {
> - dma_addr_t t;
> + dma_addr_t t;
>  

You have wrong indentation in whole this function.

> - if (size <= max_direct) {
>   buf->nbufs= 1;
>   buf->npages   = 1;
>   buf->page_shift   = get_order(size) + PAGE_SHIFT;
> - buf->direct.buf   = dma_alloc_coherent(>persist->pdev->dev,
> -size, , gfp);
> + buf->direct.buf   =
> + dma_zalloc_coherent(>persist->pdev->dev,
> + size, , gfp);
>   if (!buf->direct.buf)
>   return -ENOMEM;


signature.asc
Description: Digital signature

[no subject]

[Leon Romanovsky]

, Doug Ledford 
Bcc: 
Subject: Re: [PATCH net] RDMA/nes: don't leak skb if carrier down
Reply-To: l...@kernel.org
In-Reply-To: <1461529139-28582-1-git-send-email...@strlen.de>

On Sun, Apr 24, 2016 at 10:18:59PM +0200, Florian Westphal wrote:
> Alternatively one could free the skb, OTOH I don't think this test is
> useful so just remove it.
> 
> Cc: 
> Signed-off-by: Florian Westphal 

I don't know why did you decide to send it to netdev and not to relevant
maintainers, but the relevant mailing list is linux-rdma and Faisal
needs to Review/Acknowledge it.

➜  linux-rdma git:(master) ./scripts/get_maintainer.pl -f
drivers/infiniband/hw/nes/nes_nic.c
Faisal Latif  (supporter:NETEFFECT IWARP RNIC
DRIVER (IW_NES))
Doug Ledford  (supporter:INFINIBAND SUBSYSTEM)
Sean Hefty  (supporter:INFINIBAND SUBSYSTEM)
Hal Rosenstock  (supporter:INFINIBAND
SUBSYSTEM)
linux-r...@vger.kernel.org (open list:NETEFFECT IWARP RNIC DRIVER
(IW_NES))
linux-ker...@vger.kernel.org (open list)


> ---
>  Noticed this while working on the TX_LOCKED removal.
> 
> diff --git a/drivers/infiniband/hw/nes/nes_nic.c 
> b/drivers/infiniband/hw/nes/nes_nic.c
> index 3ea9e05..9291453 100644
> --- a/drivers/infiniband/hw/nes/nes_nic.c
> +++ b/drivers/infiniband/hw/nes/nes_nic.c
> @@ -500,9 +500,6 @@ static int nes_netdev_start_xmit(struct sk_buff *skb, 
> struct net_device *netdev)
>*  skb_shinfo(skb)->nr_frags, skb_is_gso(skb));
>*/
>  
> - if (!netif_carrier_ok(netdev))
> - return NETDEV_TX_OK;
> -
>   if (netif_queue_stopped(netdev))
>   return NETDEV_TX_BUSY;
>  
> -- 
> 2.7.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH v5 09/21] IB/hns: Add hca support

[Leon Romanovsky]

On Tue, Apr 26, 2016 at 02:34:44PM +0800, oulijun wrote:
> On 2016/4/24 15:54, Leon Romanovsky wrote:
> > On Sat, Apr 23, 2016 at 06:26:47PM +0800, Lijun Ou wrote:
> >> This patch mainly setup hca for RoCE. it will do a series of
> >> initial works as follows:
> >>   1. init uar table, allocate uar resource
> >>   2. init pd table
> >>   3. init cq table
> >>   4. init mr table
> >>   5. init qp table
> >>
> >> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >> Signed-off-by: Wei Hu(Xavier) <xavier.hu...@huawei.com>
> >> ---
> >>  drivers/infiniband/hw/hns/hns_roce_alloc.c  | 104 
> >>  drivers/infiniband/hw/hns/hns_roce_cq.c |  25 
> >>  drivers/infiniband/hw/hns/hns_roce_device.h |  69 ++
> >>  drivers/infiniband/hw/hns/hns_roce_eq.c |   1 -
> >>  drivers/infiniband/hw/hns/hns_roce_icm.c|  88 +
> >>  drivers/infiniband/hw/hns/hns_roce_icm.h|   9 ++
> >>  drivers/infiniband/hw/hns/hns_roce_main.c   |  79 
> >>  drivers/infiniband/hw/hns/hns_roce_mr.c | 187 
> >> 
> >>  drivers/infiniband/hw/hns/hns_roce_pd.c |  65 ++
> >>  drivers/infiniband/hw/hns/hns_roce_qp.c |  30 +
> >>  10 files changed, 656 insertions(+), 1 deletion(-)
> >>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_alloc.c
> >>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_mr.c
> >>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_pd.c
> >>
> >> diff --git a/drivers/infiniband/hw/hns/hns_roce_alloc.c 
> >> b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> >> new file mode 100644
> >> index 000..0c76f1b
> >> --- /dev/null
> >> +++ b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> >> @@ -0,0 +1,104 @@
> >> +/*
> >> + * Copyright (c) 2016 Hisilicon Limited.
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License as published by
> >> + * the Free Software Foundation; either version 2 of the License, or
> >> + * (at your option) any later version.
> >> + */
> >> +
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include 
> >> +#include "hns_roce_device.h"
> >> +
> >> +int hns_roce_bitmap_alloc(struct hns_roce_bitmap *bitmap, u32 *obj)
> >> +{
> >> +  int ret = 0;
> >> +
> >> +  spin_lock(>lock);
> >> +  *obj = find_next_zero_bit(bitmap->table, bitmap->max, bitmap->last);
> >> +  if (*obj >= bitmap->max) {
> >> +  bitmap->top = (bitmap->top + bitmap->max + bitmap->reserved_top)
> >> + & bitmap->mask;
> >> +  *obj = find_first_zero_bit(bitmap->table, bitmap->max);
> > 
> > find_first_zero_bit function returns "unsigned long" which may or may
> > not be equal to u32 on some architectures.
> > 
> Hi Leon,
> I appreciate your keen eye. this code is meant for ARM64bit therefore 
> should run corretly for 64-bit AARCH64.
> I will consider changing it as part of good partice and better portability "
> I will give a primary plan to modified it.
> for example:
> *obj = (u32)find_next_zero_bit(bitmap->table, bitmap->max, bitmap->last);
> Beause the max size of bitmap->table is u32 in current version.
> 
> int hns_roce_bitmap_init(struct hns_roce_bitmap *bitmap, u32 num, u32 mask,
>u32 reserved_bot, u32 reserved_top)
> {
>   u32 i;
> 
>   if (num != roundup_pow_of_two(num))
>   return -EINVAL;
> 
>   bitmap->last = 0;
>   bitmap->top = 0;
>   bitmap->max = num - reserved_top;
>   bitmap->mask = mask;
>   bitmap->reserved_top = reserved_top;
>   spin_lock_init(>lock);
>   bitmap->table = kcalloc(BITS_TO_LONGS(bitmap->max), sizeof(long),
>   GFP_KERNEL);
> 
> Is this plan ok?

No,
You are submitting new driver, please do it properly (without casting)
from the beginning.

> 
> Thanks
> Lijun Ou
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH v5 09/21] IB/hns: Add hca support

[Leon Romanovsky]

On Sat, Apr 23, 2016 at 06:26:47PM +0800, Lijun Ou wrote:
> This patch mainly setup hca for RoCE. it will do a series of
> initial works as follows:
>   1. init uar table, allocate uar resource
>   2. init pd table
>   3. init cq table
>   4. init mr table
>   5. init qp table
> 
> Signed-off-by: Lijun Ou 
> Signed-off-by: Wei Hu(Xavier) 
> ---
>  drivers/infiniband/hw/hns/hns_roce_alloc.c  | 104 
>  drivers/infiniband/hw/hns/hns_roce_cq.c |  25 
>  drivers/infiniband/hw/hns/hns_roce_device.h |  69 ++
>  drivers/infiniband/hw/hns/hns_roce_eq.c |   1 -
>  drivers/infiniband/hw/hns/hns_roce_icm.c|  88 +
>  drivers/infiniband/hw/hns/hns_roce_icm.h|   9 ++
>  drivers/infiniband/hw/hns/hns_roce_main.c   |  79 
>  drivers/infiniband/hw/hns/hns_roce_mr.c | 187 
> 
>  drivers/infiniband/hw/hns/hns_roce_pd.c |  65 ++
>  drivers/infiniband/hw/hns/hns_roce_qp.c |  30 +
>  10 files changed, 656 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_alloc.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_mr.c
>  create mode 100644 drivers/infiniband/hw/hns/hns_roce_pd.c
> 
> diff --git a/drivers/infiniband/hw/hns/hns_roce_alloc.c 
> b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> new file mode 100644
> index 000..0c76f1b
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/hns_roce_alloc.c
> @@ -0,0 +1,104 @@
> +/*
> + * Copyright (c) 2016 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "hns_roce_device.h"
> +
> +int hns_roce_bitmap_alloc(struct hns_roce_bitmap *bitmap, u32 *obj)
> +{
> + int ret = 0;
> +
> + spin_lock(>lock);
> + *obj = find_next_zero_bit(bitmap->table, bitmap->max, bitmap->last);
> + if (*obj >= bitmap->max) {
> + bitmap->top = (bitmap->top + bitmap->max + bitmap->reserved_top)
> +& bitmap->mask;
> + *obj = find_first_zero_bit(bitmap->table, bitmap->max);

find_first_zero_bit function returns "unsigned long" which may or may
not be equal to u32 on some architectures.


signature.asc
Description: Digital signature

Re: [PATCH v5 00/21] Add HiSilicon RoCE driver

[Leon Romanovsky]

On Sat, Apr 23, 2016 at 06:26:38PM +0800, Lijun Ou wrote:
> The HiSilicon Network Substem is a long term evolution IP which is
> supposed to be used in HiSilicon ICT SoCs. HNS (HiSilicon Network
> Sybsystem) also has a hardware support of performing RDMA with
> RoCEE.
> The driver for HiSilicon RoCEE(RoCE Engine) is a platform driver and will
> support mulitple versions of SOCs in future. This version of driver is 
> meant to support Hip06 SoC (which confirms to RoCEEv1 hardware 
> specifications).
>  
> Changes v5 -> v4:

v4 -> v5

> 1. redesign the patchset for RoCE modules in order to split the huge
> patch into small patches.

Thanks,
I may admit that I didn't look on the patches yet, however two things
already spotted my attention:
1. Please fix Signed-off-by signatures to include First name, Last name
and email address.
2. Please run spelling checker on all your patches.


signature.asc
Description: Digital signature

Re: [PATCH v6 00/21] Add HiSilicon RoCE driver

[Leon Romanovsky]

On Thu, Apr 28, 2016 at 08:09:35PM +0800, Lijun Ou wrote:
> The HiSilicon Network Substem is a long term evolution IP which is
> supposed to be used in HiSilicon ICT SoCs. HNS (HiSilicon Network
> Sybsystem) also has a hardware support of performing RDMA with
> RoCEE.
> The driver for HiSilicon RoCEE(RoCE Engine) is a platform driver and
> will support mulitple versions of SOCs in future. This version of driver
> is meant to support Hip06 SoC(which confirms to RoCEEv1 hardware
> specifications).

Please read Dave's comment [1], it is valuable for your code as
well.

* Please use 'bool' and "true/false"

[1] http://marc.info/?l=linux-rdma=146229367301442=2

Thanks


signature.asc
Description: Digital signature

Re: [PATCH] i40e: constify i40e_client_ops structure

[Leon Romanovsky]

On Sun, May 01, 2016 at 02:07:23PM +0200, Julia Lawall wrote:
> The i40e_client_ops structure is never modified, so declare it as const.
> 
> Done with the help of Coccinelle.
> 
> Signed-off-by: Julia Lawall <julia.law...@lip6.fr>
> 

Thanks Julia,

Reviewed-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

Re: [PATCH] Add support for configuring Infiniband GUIDs

[Leon Romanovsky]

On Fri, May 06, 2016 at 10:43:25AM -0500, Eli Cohen wrote:
> Add two NLA's that allow configuration of Infiniband node or port GUIDs
> by referencing the IPoIB net device set over then physical function. The
> format to be used is as follows:
> 
> ip link set dev ib0 vf 0 node_guid 00:02:c9:03:00:21:6e:70
> ip link set dev ib0 vf 0 port_guid 00:02:c9:03:00:21:6e:78
> 
> Issue: 702759
> Change-Id: I5ffb54d6de7bfa8650bf5818f484279914991d6e

These two lines are not needed to be part of commit message.


signature.asc
Description: Digital signature

Re: [PATCH v2] net/mlx5_core/health: Remove deprecated create_singlethread_workqueue

[Leon Romanovsky]

On Tue, Jul 26, 2016 at 10:38:24PM +0530, Bhaktipriya Shridhar wrote:
> The workqueue health->wq was used as per device private health thread.
> This was done to perform delayed work.
> 
> The workqueue has a single workitem(>work) and
> hence doesn't require ordering. It is involved in handling the health of
> the device and is not being used on a memory reclaim path.
> Hence, the singlethreaded workqueue has been replaced with the use of
> system_wq.
> 
> Work item has been flushed in mlx5_health_cleanup() to ensure that
> there are no pending tasks while disconnecting the driver.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriy...@gmail.com>
> ---
>  Changes in v2:
>   -Updated commit description as per the feedback received.
> 
>  drivers/net/ethernet/mellanox/mlx5/core/health.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)

Thanks,
Acked-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

On Thu, Jul 21, 2016 at 12:42:42PM -0500, Steve Wise wrote:
> > 
> > On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > >
> > > > > > You are the one user of this new inline function.
> > > > > > Why don't you directly call to netlink_unicast() in your 
> > > > > > ibnl_unicast()
> > > > > > without messing with widely visible header file?
> > > > >
> > > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is
> > > > > to make a blocking version available to others as well as maintain
> > > > > consistency of existing code.
> > > > >
> > > >
> > > > In such way, please provide patch series which will convert all other
> > > > users to this new call.
> > > >
> > > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, 
> > > > audit_nlk_portid, 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->portid, 
> > > > 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, 
> > > > reply->portid, 0);
> > > > kernel/audit.c: return netlink_unicast(audit_sock, skb, 
> > > > audit_nlk_portid, 0);
> > > > samples/connector/cn_test.c:netlink_unicast(nls, skb, 0, 0);
> > >
> > > These usages of netlink_unicast() with blocking are not the same as the 
> > > new
> > > nlmsg_unicast_block() function.
> > 
> > Really?
> > Did you look in the code?
> > Let's take first function from that grep output
> > 
> > 414 err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > 415 if (err < 0) {
> > ... do something ...
> > 437 } else
> > ... do something else ...
> > 
> > which fits nicely with your proposal.
> >
> 
> The key is to ensure that places calling a blocking service are never called 
> in a non-blocking context.   Leon, do you know if the new sites are always 
> safe to block?  
> 
> In general, I think blocking due to sockbuf flow control vs dropping or 
> retrying is a good thing for all the users in the rdam core, assuming they 
> are safe to block.

Steve,
Sorry for my slow response,

I afraid that you was misled by the author of the proposed patch who did
two logical changes in one patch. One is move from non-blocking mode to
blocking mode which is fine enough after justification was added. And
the second change is introduction of general inline function in common
header file (include/net/netlink.h) with one caller only.

This second change is in question and I'm not feeling comfortable by
half done work.

> 
>  
> > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff 
> > *skb, u32
> > portid)
> > +{
> > +   int err;
> > +
> > +   err = netlink_unicast(sk, skb, portid, 0);
> > +   if (err > 0)
> > +   err = 0;
> > +
> > +   return err;
> > +}
> > 
> > 
> > > You can't drop in nlmsg_unicast_block() in
> > > place of netlink_unicast() in these places. I'm not going to introduce 
> > > code
> > > which modifies old behavior.
> > 
> > Again, you aren't changing any behaviour.
> 
> Potential block/sleep is a change.  But if we can conclude that these 
> additional sites are safe to block, then probably its ok to just go ahead and 
> use the blocking service everywhere.

These potential sites has the same blocking call now netlink_unicast(... , ... 
, ... , 0),
the difference and question if they can handle normalized return value from new 
nlmsg_unicast_block
function. I'm convinced that the answer is yes.


signature.asc
Description: Digital signature

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

On Fri, Jul 22, 2016 at 10:26:01AM -0500, Shiraz Saleem wrote:
> On Thu, Jul 21, 2016 at 08:29:42PM +0300, Leon Romanovsky wrote:
> > On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > > > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > > > 
> > > > > > You are the one user of this new inline function.
> > > > > > Why don't you directly call to netlink_unicast() in your 
> > > > > > ibnl_unicast()
> > > > > > without messing with widely visible header file?
> > > > > 
> > > > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > > > to make a blocking version available to others as well as maintain 
> > > > > consistency of existing code.
> > > > > 
> > > > 
> > > > In such way, please provide patch series which will convert all other
> > > > users to this new call.
> > > > 
> > > > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > > > kernel/audit.c: err = netlink_unicast(audit_sock, skb, 
> > > > audit_nlk_portid, 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->portid, 
> > > > 0);
> > > > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, 
> > > > reply->portid, 0);
> > > > kernel/audit.c: return netlink_unicast(audit_sock, skb, 
> > > > audit_nlk_portid, 0);
> > > > samples/connector/cn_test.c:netlink_unicast(nls, skb, 0, 0);
> > > 
> > > These usages of netlink_unicast() with blocking are not the same as the 
> > > new
> > > nlmsg_unicast_block() function. 
> > 
> > Really?
> > Did you look in the code?
> > Let's take first function from that grep output
> > 
> > 414 err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > 415 if (err < 0) {
> > ... do something ...
> > 437 } else
> > ... do something else ...
> > 
> > which fits nicely with your proposal.
> > 
> > +static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff 
> > *skb, u32 portid)
> > +{
> > +   int err;
> > +
> > +   err = netlink_unicast(sk, skb, portid, 0);
> > +   if (err > 0)
> > +   err = 0;
> > +
> > +   return err;
> > +}
> > 
> > 
> > > You can't drop in nlmsg_unicast_block() in 
> > > place of netlink_unicast() in these places. I'm not going to introduce 
> > > code 
> > > which modifies old behavior.
> > 
> > Again, you aren't changing any behaviour.
> > Anyway we are not adding general function to common include file just
> > because one caller wants it.
> > 
> 
> We assumed the nlmsg_ API in linux/include/net/netlink.h is there for a 
> purpose. 
> That purpose is to normalize the return code. That API is used in places 
> where 
> the return code needs to be normalized, and when normalization is not needed, 
> then the direct calls are used. 
> 
> Now since the nlm_ API in netlink.h is missing a blocking version of the 
> nlmsg_unicast function, it would seem reasonable to add it there.
> 
> Changing all the direct calls as you suggest would at the very least be 
> less efficient since it would normalize return codes when not needed. 

One if with one assignment in non data path.
Please look at the code.

> 
> However, if there is a strict rule against adding an API unless you 
> immediately 
> have at least 2 callers, then I guess, we will make the direct call. The 
> amount 
> of code added will be the same, except that the next person who wants a 
> normalized 
> return code will have to duplicate the same code.

Yes, we are not adding to general header file code which has not
multiple callers.

> 
> Changing other code to be less efficient so that we can meet the 2 caller 
> criteria 
> doesn't seem reasonable.

I'm sorry to hear that you didn't look at the code.

> 
> 


signature.asc
Description: Digital signature

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

[Leon Romanovsky]

On Tue, Jul 19, 2016 at 10:49:57AM -0700, Joe Perches wrote:
> On Tue, 2016-07-19 at 20:26 +0300, Leon Romanovsky wrote:
> > On Tue, Jul 19, 2016 at 02:09:25PM +0300, Netanel Belgazal wrote:
> > > This is the debugging message interface.
> > > https://www.kernel.org/doc/Documentation/networking/netif-msg.txt
> > This document was updated last time in 2006 and I doubt that it is
> > relevant in 2016. You have dynamic debug prints infrastructure for it,
> > use it.
> 
> I think this is uninformed.
> 
> netif_ works well, is compatible with dynamic debug,
> and is commonly used with new networking drivers.
> 

I have a very strong feeling that it is not "used in new drivers" by was
influenced (copied) from "old drivers". The same goes for real life usage of
module version which was introduced in this patch.


signature.asc
Description: Digital signature

Re: [PATCH] net/mlx5_core/pagealloc: Remove deprecated create_singlethread_workqueue

[Leon Romanovsky]

On Thu, Jul 28, 2016 at 01:49:49PM +0530, Bhaktipriya Shridhar wrote:
> A dedicated workqueue has been used since the work items are being used
> on a memory reclaim path. WQ_MEM_RECLAIM has been set to guarantee forward
> progress under memory pressure.
> 
> The workqueue has a single work item. Hence, alloc_workqueue() is used
> instead of alloc_ordered_workqueue() since ordering is unnecessary when
> there's only one work item.
> 
> Explicit concurrency limit is unnecessary here since there are only a
> fixed number of work items.
> 
> Signed-off-by: Bhaktipriya Shridhar 
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)

Hi Bhaktipriya,

First of all, I would like to thank you for your work and invite you to
continue, but can you please submit ONE patch SERIES which changes all
similar places?

BTW,
Did you test this patch? Did you notice the memory reclaim path nature
of this work?

Thanks

> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c 
> b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> index 905..7c85262 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/pagealloc.c
> @@ -552,7 +552,8 @@ void mlx5_pagealloc_cleanup(struct mlx5_core_dev *dev)
> 
>  int mlx5_pagealloc_start(struct mlx5_core_dev *dev)
>  {
> - dev->priv.pg_wq = create_singlethread_workqueue("mlx5_page_allocator");
> + dev->priv.pg_wq = alloc_workqueue("mlx5_page_allocator",
> +   WQ_MEM_RECLAIM, 0);
>   if (!dev->priv.pg_wq)
>   return -ENOMEM;
> 
> --
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH] net/mlx5_core/pagealloc: Remove deprecated create_singlethread_workqueue

[Leon Romanovsky]

On Fri, Jul 29, 2016 at 08:22:37AM -0400, Tejun Heo wrote:
> Hello,
> 
> On Thu, Jul 28, 2016 at 12:37:35PM +0300, Leon Romanovsky wrote:
> > Did you test this patch? Did you notice the memory reclaim path nature
> > of this work?
> 
> The conversion uses WQ_MEM_RECLAIM, which is standard for all
> workqueues which can stall packet processing if stalled.  The
> requirement comes from nfs or block devices over network.

The title stays "remove deprecated create_singlethread_workqueue" and if
we put aside the word "deprecated", the code moves from ordered
workqueue to unordered workqueue and changes max_active count which
isn't expressed in commit message.

For reclaim paths, I want to be extra caution and want to see
justification for doing that along with understanding if it is tested or
not.

Right now, I'm feeling that I'm participating in soapie where one sends
patch for every line, waits and sends the same patch for another file.
It is worth to send one patch set and let us to test it all in once.

Thanks.

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


signature.asc
Description: Digital signature

Re: [patch v2] net/mlx5: missing error code in esw_create_offloads_fdb_table()

[Leon Romanovsky]

On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote:
> We accidentally return success when we had intended to return an error
> code.
> 
> Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads 
> mode')
> Signed-off-by: Dan Carpenter 
> ---
> v2: return -ENOTSUPP instead --EINVAL

I'm a little bit confused. Why did you prefer ENOTSUPP over EOPNOTSUPP?

Thanks.


signature.asc
Description: Digital signature

Re: [PATCH v11 00/22] Add HiSilicon RoCE driver

[Leon Romanovsky]

On Thu, Jul 14, 2016 at 11:43:59AM +0800, oulijun wrote:
> 在 2016/7/2 17:39, Lijun Ou 写道:
> > 
> Hi, Doug & Sean Hefty & Hal Rosenstock
> "Hello, I understand that maintainer is dealing with lots of patches not just 
> mine. Also, I could not see any further review comments from the community.
>  I also understand that I should not resend the patch-set again unless I am 
> sure my patch-set is lost.
>  I was just wondering what should I do in the current circumstance where my 
> PATCH" has not activity.
>  I am not sure if this has been accepted or how much I need to wait to resend 
> it (if ever). Please guide, I am new to open-source and learning from people 
> like you. Thanks a lot :)

You was asked numerous times to clean the mess in your TO/CC fields.
Most of the people have nothing to do with your submission.
Understanding who is the RDMA maintainer will help you a lot (hint: it
is one of three in your opening sentence). Another request from you which
you successfully ignored, was to stop reply with whole email,
but reply with relevant part only.

Ignoring community rules is a good way to be ignored back.

BTW, you don't need to resend patches, please follow after patchwork status
https://patchwork.kernel.org/project/linux-rdma/list/?submitter=157841=1

> 
> Thanks
> Lijun Ou
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH v11 21/22] IB/hns: Kconfig and Makefile for RoCE module

[Leon Romanovsky]

On Sat, Jul 02, 2016 at 05:39:23PM +0800, Lijun Ou wrote:
> This patch added Kconfig and Makefile for building RoCE module.
> 
> Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> Signed-off-by: Lijun Ou <ouli...@huawei.com>
> ---
> PATCH v11:
> hns_roce_icm.o -> hns_roce_hem.o
> 
> PATCH v10/v9/v8/v7/v6/v5:
> - No change over the PATCH v4
> 
> PATCH v4:
> This fixes the comments given by Christoph Hellwig over the PATCH v3:
>   Link: https://lkml.org/lkml/2016/3/22/609
> 
> PATCH V3:
> This fixes the comments given by Leon Romanovsky over the PATCH v2:
>   Link: https://lkml.org/lkml/2016/3/20/5
> 
> PATCH v2:
> This fixes the comments given by Leon Romanovsky over the PATCH v1:
>   Link: https://lkml.org/lkml/2016/3/6/94
> Fixes the error tested by kbuild test robot over the PATCH v1:
>   Link: https://lkml.org/lkml/2016/3/4/343
> 
> PATCH v1:
> - The initial patch
> ---
> ---
>  drivers/infiniband/Kconfig |  1 +
>  drivers/infiniband/hw/Makefile |  1 +
>  drivers/infiniband/hw/hns/Kconfig  | 10 ++
>  drivers/infiniband/hw/hns/Makefile |  8 
>  4 files changed, 20 insertions(+)
>  create mode 100644 drivers/infiniband/hw/hns/Kconfig
>  create mode 100644 drivers/infiniband/hw/hns/Makefile
> 
> diff --git a/drivers/infiniband/Kconfig b/drivers/infiniband/Kconfig
> index 2137adf..767f92b 100644
> --- a/drivers/infiniband/Kconfig
> +++ b/drivers/infiniband/Kconfig
> @@ -74,6 +74,7 @@ source "drivers/infiniband/hw/mlx5/Kconfig"
>  source "drivers/infiniband/hw/nes/Kconfig"
>  source "drivers/infiniband/hw/ocrdma/Kconfig"
>  source "drivers/infiniband/hw/usnic/Kconfig"
> +source "drivers/infiniband/hw/hns/Kconfig"
>  
>  source "drivers/infiniband/ulp/ipoib/Kconfig"
>  
> diff --git a/drivers/infiniband/hw/Makefile b/drivers/infiniband/hw/Makefile
> index c0c7cf8..2ad851d 100644
> --- a/drivers/infiniband/hw/Makefile
> +++ b/drivers/infiniband/hw/Makefile
> @@ -9,3 +9,4 @@ obj-$(CONFIG_INFINIBAND_NES)  += nes/
>  obj-$(CONFIG_INFINIBAND_OCRDMA)  += ocrdma/
>  obj-$(CONFIG_INFINIBAND_USNIC)   += usnic/
>  obj-$(CONFIG_INFINIBAND_HFI1)+= hfi1/
> +obj-$(CONFIG_INFINIBAND_HISILICON_HNS)   += hns/

--^^^--
There is no need in HISILICON word here.

> diff --git a/drivers/infiniband/hw/hns/Kconfig 
> b/drivers/infiniband/hw/hns/Kconfig
> new file mode 100644
> index 000..c47c168
> --- /dev/null
> +++ b/drivers/infiniband/hw/hns/Kconfig
> @@ -0,0 +1,10 @@
> +config INFINIBAND_HISILICON_HNS
> + tristate "Hisilicon Hns ROCE Driver"

And you are still inconsistent with the names
Hisilicon/HiSilicon/hisilicon/HISILICON/e.t.c., ROCE/roce/RoCE/e.t.c.


signature.asc
Description: Digital signature

Re: [patch v2] net/mlx5: missing error code in esw_create_offloads_fdb_table()

[Leon Romanovsky]

On Wed, Jul 13, 2016 at 11:54:54AM -0600, Jason Gunthorpe wrote:
> On Wed, Jul 13, 2016 at 02:48:44PM +0300, Dan Carpenter wrote:
> > We accidentally return success when we had intended to return an error
> > code.
> > 
> > Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov 
> > offloads mode')
> > Signed-off-by: Dan Carpenter 
> > v2: return -ENOTSUPP instead --EINVAL
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c 
> > b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > index 1842dfb..7d982cf 100644
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/eswitch_offloads.c
> > @@ -183,6 +183,7 @@ static int esw_create_offloads_fdb_table(struct 
> > mlx5_eswitch *esw, int nvports)
> >  
> > root_ns = mlx5_get_flow_namespace(dev, MLX5_FLOW_NAMESPACE_FDB);
> > if (!root_ns) {
> > +   err = -ENOTSUPP;
> 
> Did you mean ENOTSUP?
> 
> I thought ENOTSUPP was not to be used outside NFS, and isn't properly
> exported to userspace..
> 
> $ find /usr/include -name "*errno*" | xargs grep 524

I asked similar question [1] with different return value in reply
to this patch.

[1] http://marc.info/?l=linux-netdev=146843171508230=2

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


signature.asc
Description: Digital signature

Re: [PATCH] net/mlx5_core/health: Remove deprecated create_singlethread_workqueue

[Leon Romanovsky]

On Sat, Jul 16, 2016 at 01:29:20PM +0530, Bhaktipriya Shridhar wrote:
> The workqueue health->wq was used as per device private health thread.
> This was done so that system error handling could be processed
> concurrently.

Not exactly, AFAIK it was intended to perform delayed work and not
relevant to concurrency.

> The workqueue has a single workitem(>work) and
> hence doesn't require ordering. It is involved in handling the health of
> the deviceand is not being used on a memory reclaim path.
> Hence, the singlethreaded workqueue has been replaced with the use of
> system_wq.

Yes

> 
> System workqueues have been able to handle high level of concurrency
> for a long time now and hence it's not required to have a singlethreaded
> workqueue just to gain concurrency. Unlike a dedicated per-cpu workqueue
> created with create_singlethread_workqueue(), system_wq allows multiple
> work items to overlap executions even on the same CPU; however, a
> per-cpu workqueue doesn't have any CPU locality or global ordering
> guarantee unless the target CPU is explicitly specified and thus the
> increase of local concurrency shouldn't make any difference.

Not relevant.

> 
> Work item has been flushed in mlx5_health_cleanup() to ensure that
> there are no pending tasks while disconnecting the driver.
> 
> Signed-off-by: Bhaktipriya Shridhar 
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/health.c | 7 ++-
>  1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/health.c 
> b/drivers/net/ethernet/mellanox/mlx5/core/health.c
> index 42d16b9..9acbccf 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/health.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/health.c
> @@ -267,7 +267,7 @@ static void poll_health(unsigned long data)
>   if (in_fatal(dev) && !health->sick) {
>   health->sick = true;
>   print_health_info(dev);
> - queue_work(health->wq, >work);
> + schedule_work(>work);
>   }
>  }
> 
> @@ -296,7 +296,7 @@ void mlx5_health_cleanup(struct mlx5_core_dev *dev)
>  {
>   struct mlx5_core_health *health = >priv.health;
> 
> - destroy_workqueue(health->wq);
> + flush_work(>work);
>  }
> 
>  int mlx5_health_init(struct mlx5_core_dev *dev)
> @@ -311,10 +311,7 @@ int mlx5_health_init(struct mlx5_core_dev *dev)
> 
>   strcpy(name, "mlx5_health");
>   strcat(name, dev_name(>pdev->dev));
> - health->wq = create_singlethread_workqueue(name);
>   kfree(name);

You need to remove "name" initialization/usage too.
It is not needed.

> - if (!health->wq)
> - return -ENOMEM;
> 
>   INIT_WORK(>work, health_care);
> 
> --
> 2.1.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: Digital signature

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

On Mon, Jul 18, 2016 at 02:23:30PM -0500, Shiraz Saleem wrote:
> From: Mustafa Ismail 
> 
> During connection establishment with a large number of connections,
> it is possible that the connection requests might fail. Adding flow
> control prevents this failure. Change ibnl unicast to use netlink
> messaging with blocking to enable flow control.

You are the one user of this new inline function.
Why don't you directly call to netlink_unicast() in your ibnl_unicast()
without messing with widely visible header file?

Thanks


signature.asc
Description: Digital signature

Re: [PATCH -next] net/mlx5: Use PTR_ERR_OR_ZERO() to simplify the code

[Leon Romanovsky]

On Tue, Jul 19, 2016 at 11:35:46AM +, Wei Yongjun wrote:
> From: Wei Yongjun <yongjun_...@trendmicro.com.cn>
> 
> Use PTR_ERR_OR_ZERO rather than if(IS_ERR(...)) + PTR_ERR.
> 
> Generated by: scripts/coccinelle/api/ptr_ret.cocci
> 
> Signed-off-by: Wei Yongjun <yongjun_...@trendmicro.com.cn>

Thanks,
Acked-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > 
> > You are the one user of this new inline function.
> > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > without messing with widely visible header file?
> 
> Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> to make a blocking version available to others as well as maintain 
> consistency of existing code.
> 

In such way, please provide patch series which will convert all other
users to this new call.

➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
samples/connector/cn_test.c:netlink_unicast(nls, skb, 0, 0);

Thanks


signature.asc
Description: Digital signature

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

[Leon Romanovsky]

On Tue, Jul 19, 2016 at 02:09:25PM +0300, Netanel Belgazal wrote:
> 
> 
> On 07/15/2016 08:00 AM, Leon Romanovsky wrote:
> > On Thu, Jul 14, 2016 at 09:46:14AM +0300, Netanel Belgazal wrote:
> >> This is a driver for the ENA family of networking devices.
> >>
> >> Signed-off-by: Netanel Belgazal <neta...@annapurnalabs.com>
> >> ---
> >>
> >> Notes:
> > ...
> >
> >> - Increase driver version to 1.0.2
> > ...
> >
> >> +static void ena_get_drvinfo(struct net_device *dev,
> >> +  struct ethtool_drvinfo *info)
> >> +{
> >> +  struct ena_adapter *adapter = netdev_priv(dev);
> >> +
> >> +  strlcpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
> >> +  strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
> > Does module version give anything valuable in real life usage?
> > Do you plan to bump version after every patch?
> >
> > Hint, NO.
> 
> I think it is appropriate to expose driver version to ethtool, and itis 
> appropriate to be able to version a driver in upstream (mainly for debug 
> purpose)
> I don't think there is upstream agreementthat no driver should be allowed to 
> maintain a versionnumber.

You didn't answer on my questions, so I suppose that this version
interface will be forgotten and won't be relevant after first major
rework.

You have kernel version to know which driver you are running, mixing
different versions of driver with other kernels are seeing as
not-supported by the community.

> 
> >> +  strlcpy(info->bus_info, pci_name(adapter->pdev),
> >> +  sizeof(info->bus_info));
> >> +}
> >> +
> >> +
> > ...
> >
> >> +
> >> +static char version[] =
> >> +  DEVICE_NAME " v"
> >> +  DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> >> +
> >> +MODULE_AUTHOR("Amazon.com, Inc. or its affiliates");
> >> +MODULE_DESCRIPTION(DEVICE_NAME);
> >> +MODULE_LICENSE("GPL");
> >> +MODULE_VERSION(DRV_MODULE_VERSION);
> >> +
> >> +/* Time in jiffies before concluding the transmitter is hung. */
> >> +#define TX_TIMEOUT  (5 * HZ)
> >> +
> >> +#define ENA_NAPI_BUDGET 64
> >> +
> >> +#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | 
> >> NETIF_MSG_IFUP | \
> >> +  NETIF_MSG_TX_DONE | NETIF_MSG_TX_ERR | NETIF_MSG_RX_ERR)
> >> +static int debug = -1;
> >> +module_param(debug, int, 0);
> >> +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> > What is it?
> 
> This is the debugging message interface.
> https://www.kernel.org/doc/Documentation/networking/netif-msg.txt

This document was updated last time in 2006 and I doubt that it is
relevant in 2016. You have dynamic debug prints infrastructure for it,
use it.


signature.asc
Description: Digital signature

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

[Leon Romanovsky]

On Fri, Jul 15, 2016 at 08:17:59AM -0700, Benjamin Poirier wrote:
> On 2016/07/15 08:00, Leon Romanovsky wrote:
> > On Thu, Jul 14, 2016 at 09:46:14AM +0300, Netanel Belgazal wrote:
> > > This is a driver for the ENA family of networking devices.
> > > 
> > > Signed-off-by: Netanel Belgazal <neta...@annapurnalabs.com>
> > > ---
> > > 
> > > Notes:
> > 
> > ...
> > 
> > > - Increase driver version to 1.0.2
> > 
> > ...
> > 
> > > +static void ena_get_drvinfo(struct net_device *dev,
> > > + struct ethtool_drvinfo *info)
> > > +{
> > > + struct ena_adapter *adapter = netdev_priv(dev);
> > > +
> > > + strlcpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
> > > + strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));
> > 
> > Does module version give anything valuable in real life usage?
> > Do you plan to bump version after every patch?
> > 
> > Hint, NO.
> > 
> [...]
> > > +
> > > +#define DRV_MODULE_VER_MAJOR 1
> > > +#define DRV_MODULE_VER_MINOR 0
> > > +#define DRV_MODULE_VER_SUBMINOR 1
> > > +
> > > +#define DRV_MODULE_NAME  "ena"
> > > +#ifndef DRV_MODULE_VERSION
> > > +#define DRV_MODULE_VERSION \
> > > + __stringify(DRV_MODULE_VER_MAJOR) "."   \
> > > + __stringify(DRV_MODULE_VER_MINOR) "."   \
> > > + __stringify(DRV_MODULE_VER_SUBMINOR)
> > > +#endif
> > > +#define DRV_MODULE_RELDATE  "22-JUNE-2016"
> > 
> > Please remove it, driver version is useless in real life kernel usage.
> > 
> 
> The release date might be a bit overkill but the driver version is
> useful in the context of distribution kernels where users sometimes mix
> and match newer drivers (ex: the intel sf.net drivers) with older
> kernels. When a bug is reported, a quick look at the module version can
> help indicate the provenance of the driver.

We already discussed it in a number of occasions, for example this is
response of Greg Kroah-Hartman to similar attempt to bump driver version
[1].

And as I said before, mostly this driver will change without any
reflection in driver version.

[1] http://www.spinics.net/lists/linux-rdma/msg29855.html


signature.asc
Description: Digital signature

Re: [patch] net/mlx5: missing error code in esw_create_offloads_fdb_table()

[Leon Romanovsky]

On Wed, Jul 13, 2016 at 02:40:26PM +0300, Or Gerlitz wrote:
> On 7/13/2016 2:19 PM, Matan Barak wrote:
> >I'm not sure EINVAL is the right error here though.
> >Maybe -ENOTSUPP is a bit more appropriate here.
> 
> I agree, Dan, can you please change to be along Matan's suggestion?

Or,
Dan already did it before Matan's response and we have very vivid discussion 
about it [1].

[1] http://marc.info/?t=14684106302=1=2


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


signature.asc
Description: Digital signature

Re: [patch] net/mlx5: missing error code in esw_create_offloads_fdb_table()

[Leon Romanovsky]

On Wed, Jul 13, 2016 at 01:08:25PM +0300, Dan Carpenter wrote:
> We accidentally return success when we had intended to return an error
> code.
> 
> Fixes: 69697b6e2086 ('net/mlx5: E-Switch, Add support for the sriov offloads 
> mode')
> Signed-off-by: Dan Carpenter <dan.carpen...@oracle.com>

Thanks Dan,
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

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

[Leon Romanovsky]

On Thu, Jul 14, 2016 at 09:46:14AM +0300, Netanel Belgazal wrote:
> This is a driver for the ENA family of networking devices.
> 
> Signed-off-by: Netanel Belgazal 
> ---
> 
> Notes:

...

> - Increase driver version to 1.0.2

...

> +static void ena_get_drvinfo(struct net_device *dev,
> + struct ethtool_drvinfo *info)
> +{
> + struct ena_adapter *adapter = netdev_priv(dev);
> +
> + strlcpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
> + strlcpy(info->version, DRV_MODULE_VERSION, sizeof(info->version));

Does module version give anything valuable in real life usage?
Do you plan to bump version after every patch?

Hint, NO.

> + strlcpy(info->bus_info, pci_name(adapter->pdev),
> + sizeof(info->bus_info));
> +}
> +
> +

...

> +
> +static char version[] =
> + DEVICE_NAME " v"
> + DRV_MODULE_VERSION " (" DRV_MODULE_RELDATE ")\n";
> +
> +MODULE_AUTHOR("Amazon.com, Inc. or its affiliates");
> +MODULE_DESCRIPTION(DEVICE_NAME);
> +MODULE_LICENSE("GPL");
> +MODULE_VERSION(DRV_MODULE_VERSION);
> +
> +/* Time in jiffies before concluding the transmitter is hung. */
> +#define TX_TIMEOUT  (5 * HZ)
> +
> +#define ENA_NAPI_BUDGET 64
> +
> +#define DEFAULT_MSG_ENABLE (NETIF_MSG_DRV | NETIF_MSG_PROBE | NETIF_MSG_IFUP 
> | \
> + NETIF_MSG_TX_DONE | NETIF_MSG_TX_ERR | NETIF_MSG_RX_ERR)
> +static int debug = -1;
> +module_param(debug, int, 0);
> +MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");

What is it?

> +
> +static int push_mode;
> +module_param(push_mode, int, 0);
> +MODULE_PARM_DESC(push_mode, "Descriptor / header push mode 
> (0=automatic,1=disable,3=enable)\n"
> +  "\t\t\t  0 - Automatically choose according to device 
> capability (default)\n"
> +  "\t\t\t  1 - Don't push anything to device memory\n"
> +  "\t\t\t  3 - Push descriptors and header buffer to device 
> memory");
> +
> +static int enable_wd = 1;
> +module_param(enable_wd, int, 0);
> +MODULE_PARM_DESC(enable_wd, "Enable keepalive watchdog 
> (0=disable,1=enable,default=1)");
> +
> +static int enable_missing_tx_detection = 1;
> +module_param(enable_missing_tx_detection, int, 0);
> +MODULE_PARM_DESC(enable_missing_tx_detection, "Enable missing Tx 
> completions. (default=1)");
> +
> +static int numa_node_override_array[NR_CPUS] = {[0 ... (NR_CPUS - 1)] = 
> NUMA_NO_NODE };
> +module_param_array(numa_node_override_array, int, NULL, 0);
> +MODULE_PARM_DESC(numa_node_override_array, "Numa node override map\n");
> +
> +static int numa_node_override;
> +module_param(numa_node_override, int, 0);
> +MODULE_PARM_DESC(numa_node_override, "Enable/Disable numa node override 
> (0=disable)\n");

As fas as I remember, new drivers are not supposed to add module
parameters.

...

> +
> +#define DRV_MODULE_VER_MAJOR 1
> +#define DRV_MODULE_VER_MINOR 0
> +#define DRV_MODULE_VER_SUBMINOR 1
> +
> +#define DRV_MODULE_NAME  "ena"
> +#ifndef DRV_MODULE_VERSION
> +#define DRV_MODULE_VERSION \
> + __stringify(DRV_MODULE_VER_MAJOR) "."   \
> + __stringify(DRV_MODULE_VER_MINOR) "."   \
> + __stringify(DRV_MODULE_VER_SUBMINOR)
> +#endif
> +#define DRV_MODULE_RELDATE  "22-JUNE-2016"

Please remove it, driver version is useless in real life kernel usage.



signature.asc
Description: Digital signature

Re: [PATCH v10 04/22] IB/hns: Add RoCE engine reset function

[Leon Romanovsky]

On Sat, Jun 25, 2016 at 06:25:37PM +0800, Wei Hu (Xavier) wrote:
> 
> 
> On 2016/6/24 22:59, Leon Romanovsky wrote:
> >On Thu, Jun 16, 2016 at 10:35:12PM +0800, Lijun Ou wrote:
> >>This patch mainly added reset flow of RoCE engine in RoCE
> >>driver. It is necessary when RoCE is loaded and removed.
> >>
> >>Signed-off-by: Wei Hu <xavier.hu...@huawei.com>
> >>Signed-off-by: Nenglong Zhao <zhaonengl...@hisilicon.com>
> >>Signed-off-by: Lijun Ou <ouli...@huawei.com>
> >>---

...

> >>+
> >>+#define SLEEP_TIME_INTERVAL20
> >>+
> >>+extern int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool 
> >>enable);
> >Why did you add this extern?
> >You already exported this function.
> >drivers/net/ethernet/hisilicon/hns/hns_dsaf_main.c:EXPORT_SYMBOL(hns_dsaf_roce_reset);
> Hi, Leon
> 
> The function named hns_dsaf_roce_reset is defined in hns_dsaf_main.c
> It exists in hns_dsaf.ko(ethernet driver)
> 
> RoCE driver will call this function.
> 
> Your suggestion is that delete "extern" as below:
> In /drivers/infiniband/hw/hns/hns_roce_hw_v1.h:
> 
>   int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool
> enable);
> 
> Right? or other soultion?

You placed it in header file.
Please move it to your hns_roce_hw_v1.c file.

> 
> 
> Regards
> Wei Hu
> >>+
> >>+#endif
> >>diff --git a/drivers/infiniband/hw/hns/hns_roce_main.c 
> >>b/drivers/infiniband/hw/hns/hns_roce_main.c
> >>index 8924ce3..d5ccce2 100644
> >>--- a/drivers/infiniband/hw/hns/hns_roce_main.c
> >>+++ b/drivers/infiniband/hw/hns/hns_roce_main.c
> >>@@ -71,7 +71,9 @@ static int hns_roce_get_cfg(struct hns_roce_dev *hr_dev)
> >>struct platform_device *pdev = NULL;
> >>struct resource *res;
> >>-   if (!of_device_is_compatible(np, "hisilicon,hns-roce-v1")) {
> >>+   if (of_device_is_compatible(np, "hisilicon,hns-roce-v1")) {
> >>+   hr_dev->hw = _roce_hw_v1;
> >>+   } else {
> >>dev_err(dev, "device no compatible!\n");
> >>return -EINVAL;
> >>}
> >>@@ -118,6 +120,11 @@ static int hns_roce_get_cfg(struct hns_roce_dev 
> >>*hr_dev)
> >>return 0;
> >>  }
> >>+static int hns_roce_engine_reset(struct hns_roce_dev *hr_dev, bool enable)
> >>+{
> >>+   return hr_dev->hw->reset(hr_dev, enable);
> >>+}
> >>+
> >>  /**
> >>  * hns_roce_probe - RoCE driver entrance
> >>  * @pdev: pointer to platform device
> >>@@ -156,6 +163,12 @@ static int hns_roce_probe(struct platform_device *pdev)
> >>goto error_failed_get_cfg;
> >>}
> >>+   ret = hns_roce_engine_reset(hr_dev, true);
> >Do you have better solution to sense device without doing full reset of
> >your hardware?
> Hi, Leon
> 
> In this place, we need reset RoCEE engine to ensure that RoCE engine can
> work correctly.
> Hip06 Soc only support full reset RoCEE engine.
> 
> Regards
> Wei Hu
> 
> >
> >>+   if (ret) {
> >>+   dev_err(dev, "Reset roce engine failed!\n");
> >>+   goto error_failed_get_cfg;
> >>+   }
> >>+
> >>  error_failed_get_cfg:
> >>ib_dealloc_device(_dev->ib_dev);
> >>@@ -170,6 +183,8 @@ static int hns_roce_remove(struct platform_device *pdev)
> >>  {
> >>struct hns_roce_dev *hr_dev = platform_get_drvdata(pdev);
> >>+   (void)hns_roce_engine_reset(hr_dev, false);
> >Any reason to do explicit casting?
> Hi, Leon
> 
> /**
>  * hns_roce_engine_reset - reset roce
>  * @hr_dev: roce device struct pointer
>  * @enable: true -- drop reset, false -- reset
>  * return 0 - success , negative --fail
>  */
> static int hns_roce_engine_reset(struct hns_roce_dev *hr_dev, bool enable);
> 
> hns_roce_engine_reset->hns_roce_v1_reset->hns_dsaf_roce_reset
> 
> The err branch of hns_roce_engine_reset as below：
> int hns_dsaf_roce_reset(struct fwnode_handle *dsaf_fwnode, bool enable)
> {
> <...>
> if (!is_of_node(dsaf_fwnode)) {
> pr_err("hisi_dsaf: Only support DT node!\n");
> return -EINVAL;
> }
> 
> pdev = of_find_device_by_node(to_of_node(dsaf_fwnode));
> dsaf_dev = dev_get_drvdata(>dev);
> if (AE_IS_VER1(dsaf_dev->dsaf_ver)) {
> dev_err(dsaf_dev->dev, "%s v1 c

Re: [PATCH] net/mlx5_core/pagealloc: Remove deprecated create_singlethread_workqueue

[Leon Romanovsky]

On Mon, Aug 01, 2016 at 11:11:19AM -0400, Tejun Heo wrote:
> > Right now, I'm feeling that I'm participating in soapie where one sends
> > patch for every line, waits and sends the same patch for another file.
> > It is worth to send one patch set and let us to test it all in once.
> 
> Yeah, I guess it can be a bit annoying.  Bhaktipriya, can you please
> group patches for meallnox?

Please do.
Thanks.

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


signature.asc
Description: Digital signature

Re: [PATCH V2] Add flow control to the portmapper

[Leon Romanovsky]

On Wed, Jul 20, 2016 at 09:47:50PM -0500, Shiraz Saleem wrote:
> On Tue, Jul 19, 2016 at 08:32:53PM +0300, Leon Romanovsky wrote:
> > On Tue, Jul 19, 2016 at 09:50:24AM -0500, Shiraz Saleem wrote:
> > > On Tue, Jul 19, 2016 at 08:40:06AM +0300, Leon Romanovsky wrote:
> > > > 
> > > > You are the one user of this new inline function.
> > > > Why don't you directly call to netlink_unicast() in your ibnl_unicast()
> > > > without messing with widely visible header file?
> > > 
> > > Since there is a non-blocking version of nlmsg_unicast(), the idea is 
> > > to make a blocking version available to others as well as maintain 
> > > consistency of existing code.
> > > 
> > 
> > In such way, please provide patch series which will convert all other
> > users to this new call.
> > 
> > ➜  linux-rdma git:(master) grep -rI netlink_unicast * | grep -I 0
> > kernel/audit.c: err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
> > kernel/audit.c: netlink_unicast(aunet->nlsk, skb, dest->portid, 0);
> > kernel/audit.c: netlink_unicast(aunet->nlsk , reply->skb, reply->portid, 0);
> > kernel/audit.c: return netlink_unicast(audit_sock, skb, audit_nlk_portid, 
> > 0);
> > samples/connector/cn_test.c:netlink_unicast(nls, skb, 0, 0);
> 
> These usages of netlink_unicast() with blocking are not the same as the new
> nlmsg_unicast_block() function. 

Really?
Did you look in the code?
Let's take first function from that grep output

414 err = netlink_unicast(audit_sock, skb, audit_nlk_portid, 0);
415 if (err < 0) {
... do something ...
437 } else
... do something else ...

which fits nicely with your proposal.

+static inline int nlmsg_unicast_block(struct sock *sk, struct sk_buff *skb, 
u32 portid)
+{
+   int err;
+
+   err = netlink_unicast(sk, skb, portid, 0);
+   if (err > 0)
+   err = 0;
+
+   return err;
+}


> You can't drop in nlmsg_unicast_block() in 
> place of netlink_unicast() in these places. I'm not going to introduce code 
> which modifies old behavior.

Again, you aren't changing any behaviour.
Anyway we are not adding general function to common include file just
because one caller wants it.

> 


signature.asc
Description: Digital signature

Re: [PATCH v11 00/22] Add HiSilicon RoCE driver

[Leon Romanovsky]

On Sat, Jul 02, 2016 at 05:39:02PM +0800, Lijun Ou wrote:

This v11
>  28 files changed, 10626 insertions(+), 1 deletion(-)

First version
>  27 files changed, 11670 insertions(+), 11 deletions(-)

1K LOC less, we are moving in right direction.


signature.asc
Description: Digital signature

Re: [PATCH] net/mlx4: Fix some indent inconsistancy

[Leon Romanovsky]

On Sat, Jul 02, 2016 at 02:31:05PM +0200, Christophe JAILLET wrote:
> Silent a few smatch warnings about indentation.
> This include the removal of a 'return' statement in 'resource_tracker.c'.
> This 'return' will still be performed when breaking out of the
> corresponding 'switch' block.
> 
> Signed-off-by: Christophe JAILLET <christophe.jail...@wanadoo.fr>

Thanks
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: Digital signature

Re: [PATCH v10 07/22] IB/hns: Add event queue support

[Leon Romanovsky]

On Wed, Jun 29, 2016 at 04:53:39PM +0800, oulijun wrote:

> >> +
> >> +  for (i = 0; i < npages; ++i)
> >> +  if (eq->buf_list[i].buf)
> > 
> > Is it possible situation to have eq->buf_list[i].buf == NULL at the
> > middle of iteration?
> > 
> We have analysized it according to your reivews. We think that the 
> eq->buf_list[i].buf
> is not NULL at the middle of iteration because only the eq->buf_list[i].buf 
> allocated
> when the hns_roce_free_eq be called. As result, the if (eq->buf_list[i].buf) 
> condition
> should be deleted, right?

Yes, please.


signature.asc
Description: Digital signature

Re: [RFC v3 00/11] HFI Virtual Network Interface Controller (VNIC)

[Leon Romanovsky]

On Tue, Feb 07, 2017 at 12:22:59PM -0800, Vishwanathapura, Niranjana wrote:
> ChangeLog:
> =
> v2 => v3:
> a) Introduce and adopt generic RDMA netdev interface including,
>  - having bottom hfi1 driver directly interfacing with netstack.
>  - optimizing interface between hfi_vnic and hfi1 driver.
> b) Remove bitfield usage.
> c) Move hfi_vnic driver to drivers/infiniband/ulp folder.

I didn't read patches yet, and prefer to ask it in advance. Does this new ULP 
work with all
drivers/infiniband/hw/* devices as it is expected from ULP?

Thanks


signature.asc
Description: PGP signature

Re: [RFC v3 00/11] HFI Virtual Network Interface Controller (VNIC)

[Leon Romanovsky]

On Tue, Feb 07, 2017 at 01:43:03PM -0800, Vishwanathapura, Niranjana wrote:
> On Tue, Feb 07, 2017 at 01:00:05PM -0800, Hefty, Sean wrote:
> > > I didn't read patches yet, and prefer to ask it in advance. Does this
> > > new ULP work with all
> > > drivers/infiniband/hw/* devices as it is expected from ULP?
> >
> > Like the way ipoib or srp work with all hw devices?  What is the real point 
> > of this question?
>
> Leon,
> It was already discussed in below threads.
>
> https://www.spinics.net/lists/linux-rdma/msg44128.html
> https://www.spinics.net/lists/linux-rdma/msg44131.html
> https://www.spinics.net/lists/linux-rdma/msg44155.html

Yes, but you still didn't answer on my question.
From the first link:
--
  If that is your position then this should be a straight up IB ULP that
  works with any IB hardware.

Yes, see my comments in point #3 of my previous email...
--

Can I grab these patches and run on one of 14 drivers available in
drivers/inifiniband/hw/* ?

>
> Niranjana
>


signature.asc
Description: PGP signature

Re: [RFC v3 00/11] HFI Virtual Network Interface Controller (VNIC)

[Leon Romanovsky]

On Tue, Feb 07, 2017 at 09:00:05PM +, Hefty, Sean wrote:
> > I didn't read patches yet, and prefer to ask it in advance. Does this
> > new ULP work with all
> > drivers/infiniband/hw/* devices as it is expected from ULP?
>
> Like the way ipoib or srp work with all hw devices?  What is the real point 
> of this question?

Sorry, but I don't understand your response. Both IPoIB and SRP were 
standardized
and implemented years before hfi was brought into the RDMA stack, so on
time of introduction they clearly supported all the devices.

Does this VNIC interface have standard? Where can I see HFI wire
protocol to implement HFI VNIC support in our HW?

Thanks.


signature.asc
Description: PGP signature

Re: [RFC v3 02/11] IB/hfi-vnic: Virtual Network Interface Controller (VNIC) interface

[Leon Romanovsky]

On Tue, Feb 07, 2017 at 02:19:01PM -0700, Jason Gunthorpe wrote:
> On Tue, Feb 07, 2017 at 12:23:01PM -0800, Vishwanathapura, Niranjana wrote:
> > Add rdma netdev interface to ib device structure allowing rdma netdev
> > devices to be allocated by ib clients.
> > Define HFI VNIC interface between hardware independent VNIC
> > functionality and the hardware dependent VNIC functionality.
>
> This commit message could be a bit clearer.
>
> The alloc_rdma_netdev multiplexer is inteded as a new general
> interface and this adds a protocol definition for ethernet VNIC on
> OPA.
>
> The hope is that ipoib can follow the same example and use the same
> alloc_rdma_netdev entry point. Hopefully Mellanox will look at this
> patch as I have talked to them in the past about doing this...

Jason,

We looked on it and it is useless for us, mainly because of the fact
that most  of the work is done in our net part of the driver.

So, as it looks for now, this ULP exercise will be for HFI only.

Thanks


signature.asc
Description: PGP signature

Re: [PATCH] net/mlx4: use rb_entry()

[Leon Romanovsky]

On Sun, Jan 22, 2017 at 10:42:25PM +0800, Geliang Tang wrote:
> On Sun, Jan 22, 2017 at 09:48:39AM +0200, Leon Romanovsky wrote:
> > On Fri, Jan 20, 2017 at 10:36:57PM +0800, Geliang Tang wrote:
> > > To make the code clearer, use rb_entry() instead of container_of() to
> > > deal with rbtree.
> > >
> > > Signed-off-by: Geliang Tang <geliangt...@gmail.com>
> > > ---
> > >  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 
> > >  1 file changed, 4 insertions(+), 4 deletions(-)
> >
> > I don't understand completely the rationale behind this conversion.
> > rb_entry == container_of, why do we need another name for it?
> >
>
> There are several *_entry macros which are defined in kernel data
> structures, like list_entry, hlist_entry, rb_entry, etc. Each of them is
> just another name for container_of. We use different *_entry so that we
> could identify the specific type of data structure that we are dealing
> with.

Your proposed patch doesn't support the importance of such knowledge for
rb_entry. The list_entry case is totally different, because you perform
operation on it.

Anyway, It doesn't matter.
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>


signature.asc
Description: PGP signature

Re: [PATCH for-next V2 00/11] Mellanox mlx5 core and ODP updates 2017-01-01

[Leon Romanovsky]

On Tue, Jan 24, 2017 at 02:39:59PM -0500, Doug Ledford wrote:
> On Tue, 2017-01-03 at 09:23 +0200, Leon Romanovsky wrote:
> > On Tue, Jan 03, 2017 at 01:30:16AM +0200, Saeed Mahameed wrote:
> > >
> > > On Mon, Jan 2, 2017 at 10:53 PM, David Miller <da...@davemloft.net>
> > > wrote:
> > > >
> > > > From: Saeed Mahameed <sae...@mellanox.com>
> > > > Date: Mon,  2 Jan 2017 11:37:37 +0200
> > > >
> > > > >
> > > > > The following eleven patches mainly come from Artemy Kovalyov
> > > > > who expanded mlx5 on-demand-paging (ODP) support. In addition
> > > > > there are three cleanup patches which don't change any
> > > > > functionality,
> > > > > but are needed to align codebase prior accepting other patches.
> > > >
> > > > Series applied to net-next, thanks.
> > >
> > > Whoops,
> > >
> > > This series was meant as a pull request, you can blame it on me I
> > > kinda messed up the V2 title.
> > > Doug will have to pull same patches later, will this produce a
> > > conflict on merge window ?
> >
> > Yes, but it can be easily avoided.
> >
> > Doug,
> >
> > We have another pull request to send and we will base its code on the
> > Dave's tree instead of Linus's rc tag. In such way, you will have the
> > same commits as Dave and won't have merge failures.
> >
> > Please don't apply manually this specific patchset.
> >
> > Sorry for the inconvenience.
>
> Hi Leon and Saeed,
>
> As I understand it, Dave took this series, but I have not.  You were
> going to base another pull request on top of this so I could get both.

We did it, ODP was our first pull request and 4K UAR was second one.
http://marc.info/?l=linux-rdma=148398855416090=2
Dave pulled this tag:
https://git.kernel.org/cgit/linux/kernel/git/mellanox/linux.git/tag/?h=mlx5-4kuar-for-4.11

Since 4K UAR tag was based on ODP, by pulling mlx5-4kuar-for-4.11 tag you
will get ODP too.

>  However, I believe that was the attempt to reorg the driver pull
> request, which David NAKed.  

It was our third pull request
http://marc.info/?l=linux-rdma=148424983101796=2
https://git.kernel.org/cgit/linux/kernel/git/mellanox/linux.git/tag/?h=mlx5-dir-layout-reorg

> That means I have not picked this up.  Are
> all of the other for-next patches you posted against the RDMA list
> going to go in cleanly without this series, or should I expect
> conflicts between Dave's tree and my own?

All patches sent by me are based on mlx5-4kuar-for-4.11 tag which
exists in Dave's tree and everything will apply cleanly on your
k.o/for-4.11 branch and won't have merge conflicts between your tree and
Dave's net-next tree.

For you convenience, my submission queue can be found at:
https://git.kernel.org/cgit/linux/kernel/git/leon/linux-rdma.git/log/?h=rdma-next

Thanks

>
> --
> Doug Ledford <dledf...@redhat.com>
>     GPG KeyID: B826A3330E572FDD
>    
> Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD




signature.asc
Description: PGP signature

Re: [PATCH for bnxt_re V4 15/21] RDMA/bnxt_re: Support post_recv

[Leon Romanovsky]

On Wed, Dec 21, 2016 at 03:42:04AM -0800, Selvin Xavier wrote:
> Enables the fastpath verb ib_post_recv.
>
> v3: Fixes sparse warnings
>
> Signed-off-by: Eddie Wai 
> Signed-off-by: Devesh Sharma 
> Signed-off-by: Somnath Kotur 
> Signed-off-by: Sriharsha Basavapatna 
> Signed-off-by: Selvin Xavier 
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 123 
> +++
>  drivers/infiniband/hw/bnxt_re/ib_verbs.h |   2 +
>  drivers/infiniband/hw/bnxt_re/main.c |   2 +
>  drivers/infiniband/hw/bnxt_re/qplib_fp.c | 100 +
>  drivers/infiniband/hw/bnxt_re/qplib_fp.h |   8 ++
>  5 files changed, 235 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c 
> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index e659490..7476994c 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -1637,6 +1637,51 @@ static int bnxt_re_build_qp1_send_v2(struct bnxt_re_qp 
> *qp,
>   return rc;
>  }
>
> +/* For the MAD layer, it only provides the recv SGE the size of
> + * ib_grh + MAD datagram.  No Ethernet headers, Ethertype, BTH, DETH,
> + * nor RoCE iCRC.  The Cu+ solution must provide buffer for the entire
> + * receive packet (334 bytes) with no VLAN and then copy the GRH
> + * and the MAD datagram out to the provided SGE.
> + */
> +static int bnxt_re_build_qp1_shadow_qp_recv(struct bnxt_re_qp *qp,
> + struct ib_recv_wr *wr,
> + struct bnxt_qplib_swqe *wqe,
> + int payload_size)
> +{
> + struct bnxt_qplib_sge ref, sge;
> + u32 rq_prod_index;
> + struct bnxt_re_sqp_entries *sqp_entry;
> +
> + rq_prod_index = bnxt_qplib_get_rq_prod_index(>qplib_qp);
> +
> + if (bnxt_qplib_get_qp1_rq_buf(>qplib_qp, )) {
> + /* Create 1 SGE to receive the entire
> +  * ethernet packet
> +  */
> + /* Save the reference from ULP */
> + ref.addr = wqe->sg_list[0].addr;
> + ref.lkey = wqe->sg_list[0].lkey;
> + ref.size = wqe->sg_list[0].size;
> +
> + sqp_entry = >rdev->sqp_tbl[rq_prod_index];
> +
> + /* SGE 1 */
> + wqe->sg_list[0].addr = sge.addr;
> + wqe->sg_list[0].lkey = sge.lkey;
> + wqe->sg_list[0].size = BNXT_QPLIB_MAX_QP1_RQ_HDR_SIZE_V2;
> + sge.size -= wqe->sg_list[0].size;
> +
> + sqp_entry->sge.addr = ref.addr;
> + sqp_entry->sge.lkey = ref.lkey;
> + sqp_entry->sge.size = ref.size;
> + /* Store the wrid for reporting completion */
> + sqp_entry->wrid = wqe->wr_id;
> + /* change the wqe->wrid to table index */
> + wqe->wr_id = rq_prod_index;
> + }
> + return 0;
> +}
> +
>  static int is_ud_qp(struct bnxt_re_qp *qp)
>  {
>   return qp->qplib_qp.type == CMDQ_CREATE_QP_TYPE_UD;
> @@ -1983,6 +2028,84 @@ int bnxt_re_post_send(struct ib_qp *ib_qp, struct 
> ib_send_wr *wr,
>   return rc;
>  }
>
> +static int bnxt_re_post_recv_shadow_qp(struct bnxt_re_dev *rdev,
> +struct bnxt_re_qp *qp,
> +struct ib_recv_wr *wr)
> +{
> + struct bnxt_qplib_swqe wqe;
> + int rc = 0, payload_sz = 0;
> +
> + memset(, 0, sizeof(wqe));
> + while (wr) {
> + /* House keeping */
> + memset(, 0, sizeof(wqe));
> +
> + /* Common */
> + wqe.num_sge = wr->num_sge;
> + if (wr->num_sge > qp->qplib_qp.rq.max_sge) {
> + dev_err(rdev_to_dev(rdev),
> + "Limit exceeded for Receive SGEs");
> + rc = -EINVAL;
> + goto bad;

"goto bad" directly to other place in while() to call the "break".
It will be more convenient to call "break" here.


> + }
> + payload_sz = bnxt_re_build_sgl(wr->sg_list, wqe.sg_list,
> +wr->num_sge);
> + wqe.wr_id = wr->wr_id;
> + wqe.type = BNXT_QPLIB_SWQE_TYPE_RECV;
> +
> + if (!rc)

If we are here, we will always have rc == 0.

> + rc = bnxt_qplib_post_recv(>qplib_qp, );
> +bad:
> + if (rc)
> + break;
> +
> + wr = wr->next;
> + }
> + bnxt_qplib_post_recv_db(>qplib_qp);
> + return rc;
> +}


signature.asc
Description: PGP signature

Re: [PATCH for bnxt_re V4 20/21] RDMA/bnxt_re: Add QP event handling

[Leon Romanovsky]

On Wed, Dec 21, 2016 at 03:42:09AM -0800, Selvin Xavier wrote:
> Implements callback handler for processing Async events related to a QP.
> This patch also implements the control path command completion handling.
>
> v3: Removes unwanted braces
>
> Signed-off-by: Eddie Wai 
> Signed-off-by: Devesh Sharma 
> Signed-off-by: Somnath Kotur 
> Signed-off-by: Sriharsha Basavapatna 
> Signed-off-by: Selvin Xavier 
> ---
>  drivers/infiniband/hw/bnxt_re/qplib_rcfw.c | 47 
> ++
>  1 file changed, 47 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c 
> b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> index 9144b5a..a000397 100644
> --- a/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> +++ b/drivers/infiniband/hw/bnxt_re/qplib_rcfw.c
> @@ -257,6 +257,44 @@ static int bnxt_qplib_process_func_event(struct 
> bnxt_qplib_rcfw *rcfw,
>   return 0;
>  }
>
> +static int bnxt_qplib_process_qp_event(struct bnxt_qplib_rcfw *rcfw,
> +struct creq_qp_event *qp_event)
> +{
> + struct bnxt_qplib_crsq *crsq = >crsq;
> + struct bnxt_qplib_hwq *cmdq = >cmdq;
> + struct bnxt_qplib_crsqe *crsqe;
> + u16 cbit, cookie, blocked = 0;
> + unsigned long flags;
> + u32 sw_cons;
> +
> + switch (qp_event->event) {
> + case CREQ_QP_EVENT_EVENT_QP_ERROR_NOTIFICATION:
> + break;

it looks like if( ... ) return 0;

> + default:
> + /* Command Response */
> + spin_lock_irqsave(>lock, flags);
> + sw_cons = HWQ_CMP(crsq->cons, crsq);
> + crsqe = >crsq[sw_cons];
> + crsq->cons++;
> + memcpy(>qp_event, qp_event, sizeof(crsqe->qp_event));
> +
> + cookie = le16_to_cpu(crsqe->qp_event.cookie);
> + blocked = cookie & RCFW_CMD_IS_BLOCKING;
> + cookie &= RCFW_MAX_COOKIE_VALUE;
> + cbit = cookie % RCFW_MAX_OUTSTANDING_CMD;
> + if (!test_and_clear_bit(cbit, rcfw->cmdq_bitmap))
> + dev_warn(>pdev->dev,
> +  "QPLIB: CMD bit %d was not requested", cbit);
> +
> + cmdq->cons += crsqe->req_size;
> + spin_unlock_irqrestore(>lock, flags);
> + if (!blocked)
> + wake_up(>waitq);
> + break;
> + }
> + return 0;
> +}
> +
>  /* SP - CREQ Completion handlers */
>  static void bnxt_qplib_service_creq(unsigned long data)
>  {
> @@ -280,6 +318,15 @@ static void bnxt_qplib_service_creq(unsigned long data)
>   type = creqe->type & CREQ_BASE_TYPE_MASK;
>   switch (type) {
>   case CREQ_BASE_TYPE_QP_EVENT:
> + if (!bnxt_qplib_process_qp_event
> + (rcfw, (struct creq_qp_event *)creqe))
> + rcfw->creq_qp_event_processed++;
> + else {
> + dev_warn(>pdev->dev, "QPLIB: crsqe with");
> + dev_warn(>pdev->dev,
> +  "QPLIB: type = 0x%x not handled",
> +  type);
> + }
>   break;
>   case CREQ_BASE_TYPE_FUNC_EVENT:
>   if (!bnxt_qplib_process_func_event
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH for bnxt_re V4 17/21] RDMA/bnxt_re: Handling dispatching of events to IB stack

[Leon Romanovsky]

On Wed, Dec 21, 2016 at 03:42:06AM -0800, Selvin Xavier wrote:
> This patch implements events dispatching to the IB stack
> based on NETDEV events received.
>
> v2: Removed cleanup of the resources during driver unload since
> we are calling unregister_netdevice_notifier first in the exit.
>
> v3: Fixes cocci warnings and some sparse warnings
>
> Signed-off-by: Eddie Wai 
> Signed-off-by: Devesh Sharma 
> Signed-off-by: Somnath Kotur 
> Signed-off-by: Sriharsha Basavapatna 
> Signed-off-by: Selvin Xavier 
> ---
>  drivers/infiniband/hw/bnxt_re/main.c | 65 
> 
>  1 file changed, 65 insertions(+)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/main.c 
> b/drivers/infiniband/hw/bnxt_re/main.c
> index ab0b35a..bd13414 100644
> --- a/drivers/infiniband/hw/bnxt_re/main.c
> +++ b/drivers/infiniband/hw/bnxt_re/main.c
> @@ -729,6 +729,60 @@ static int bnxt_re_alloc_res(struct bnxt_re_dev *rdev)
>   return rc;
>  }
>
> +static void bnxt_re_dispatch_event(struct ib_device *ibdev, struct ib_qp *qp,
> +u8 port_num, enum ib_event_type event)
> +{
> + struct ib_event ib_event;
> +
> + ib_event.device = ibdev;
> + if (qp)
> + ib_event.element.qp = qp;
> + else
> + ib_event.element.port_num = port_num;
> + ib_event.event = event;
> + ib_dispatch_event(_event);
> +}
> +
> +static bool bnxt_re_is_qp1_or_shadow_qp(struct bnxt_re_dev *rdev,
> + struct bnxt_re_qp *qp)
> +{
> + return (qp->ib_qp.qp_type == IB_QPT_GSI) || (qp == rdev->qp1_sqp);
> +}
> +
> +static void bnxt_re_dev_stop(struct bnxt_re_dev *rdev, bool qp_wait)
> +{
> + int mask = IB_QP_STATE, qp_count, count = 1;
> + struct ib_qp_attr qp_attr;
> + struct bnxt_re_qp *qp;
> +
> + qp_attr.qp_state = IB_QPS_ERR;
> + mutex_lock(>qp_lock);
> + list_for_each_entry(qp, >qp_list, list) {
> + /* Modify the state of all QPs except QP1/Shadow QP */
> + if (!bnxt_re_is_qp1_or_shadow_qp(rdev, qp)) {
> + if (qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_RESET &&
> + qp->qplib_qp.state !=
> + CMDQ_MODIFY_QP_NEW_STATE_ERR) {
> + bnxt_re_dispatch_event(>ibdev, >ib_qp,
> +1, IB_EVENT_QP_FATAL);
> + bnxt_re_modify_qp(>ib_qp, _attr, mask,
> +   NULL);
> + }
> + }
> + }
> +
> + mutex_unlock(>qp_lock);
> + if (qp_wait) {

All callers to this function in this patch set qp_wait to be false.
Do you have in following patches qp_wait == true?
I'm curious because of your msleep below.

> + /* Give the application some time to clean up */
> + do {
> + qp_count = atomic_read(>qp_count);
> + msleep(100);
> + } while ((qp_count != atomic_read(>qp_count)) &&
> +   count--);
> + }
> +}
> +
>  static void bnxt_re_ib_unreg(struct bnxt_re_dev *rdev, bool lock_wait)
>  {
>   int i, rc;
> @@ -888,6 +942,9 @@ static int bnxt_re_ib_reg(struct bnxt_re_dev *rdev)
>   }
>   }
>   set_bit(BNXT_RE_FLAG_IBDEV_REGISTERED, >flags);
> + bnxt_re_dispatch_event(>ibdev, NULL, 1, IB_EVENT_PORT_ACTIVE);
> + bnxt_re_dispatch_event(>ibdev, NULL, 1, IB_EVENT_GID_CHANGE);
> +
>   return 0;
>  free_sctx:
>   bnxt_re_net_stats_ctx_free(rdev, rdev->qplib_ctx.stats.fw_id, true);
> @@ -967,10 +1024,18 @@ static void bnxt_re_task(struct work_struct *work)
>   "Failed to register with IB: %#x", rc);
>   break;
>   case NETDEV_UP:
> + bnxt_re_dispatch_event(>ibdev, NULL, 1,
> +IB_EVENT_PORT_ACTIVE);
>   break;
>   case NETDEV_DOWN:
> + bnxt_re_dev_stop(rdev, false);
>   break;
>   case NETDEV_CHANGE:
> + if (!netif_carrier_ok(rdev->netdev))
> + bnxt_re_dev_stop(rdev, false);
> + else if (netif_carrier_ok(rdev->netdev))
> + bnxt_re_dispatch_event(>ibdev, NULL, 1,
> +IB_EVENT_PORT_ACTIVE);
>   break;
>   default:
>   break;
> --
> 2.5.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH 5/9] treewide: use kv[mz]alloc* rather than opencoded variants

[Leon Romanovsky]

On Mon, Jan 30, 2017 at 10:49:36AM +0100, Michal Hocko wrote:
> From: Michal Hocko <mho...@suse.com>
>
> There are many code paths opencoding kvmalloc. Let's use the helper
> instead. The main difference to kvmalloc is that those users are usually
> not considering all the aspects of the memory allocator. E.g. allocation
> requests <= 32kB (with 4kB pages) are basically never failing and invoke
> OOM killer to satisfy the allocation. This sounds too disruptive for
> something that has a reasonable fallback - the vmalloc. On the other
> hand those requests might fallback to vmalloc even when the memory
> allocator would succeed after several more reclaim/compaction attempts
> previously. There is no guarantee something like that happens though.
>
> This patch converts many of those places to kv[mz]alloc* helpers because
> they are more conservative.
>
> Changes since v1
> - add kvmalloc_array - this might silently fix some overflow issues
>   because most users simply didn't check the overflow for the vmalloc
>   fallback.
>
> Cc: Martin Schwidefsky <schwidef...@de.ibm.com>
> Cc: Heiko Carstens <heiko.carst...@de.ibm.com>
> Cc: Herbert Xu <herb...@gondor.apana.org.au>
> Cc: Anton Vorontsov <an...@enomsg.org>
> Cc: Colin Cross <ccr...@android.com>
> Cc: Kees Cook <keesc...@chromium.org>
> Cc: Tony Luck <tony.l...@intel.com>
> Cc: "Rafael J. Wysocki" <r...@rjwysocki.net>
> Cc: Ben Skeggs <bske...@redhat.com>
> Cc: Kent Overstreet <kent.overstr...@gmail.com>
> Cc: Santosh Raspatur <sant...@chelsio.com>
> Cc: Hariprasad S <haripra...@chelsio.com>
> Cc: Yishai Hadas <yish...@mellanox.com>
> Cc: Oleg Drokin <oleg.dro...@intel.com>
> Cc: "Yan, Zheng" <z...@redhat.com>
> Cc: Alexander Viro <v...@zeniv.linux.org.uk>
> Cc: Alexei Starovoitov <a...@kernel.org>
> Cc: Eric Dumazet <eric.duma...@gmail.com>
> Cc: netdev@vger.kernel.org
> Acked-by: Andreas Dilger <andreas.dil...@intel.com> # Lustre
> Reviewed-by: Boris Ostrovsky <boris.ostrov...@oracle.com> # Xen bits
> Acked-by: Christian Borntraeger <borntrae...@de.ibm.com> # KVM/s390
> Acked-by: Dan Williams <dan.j.willi...@intel.com> # nvdim
> Acked-by: David Sterba <dste...@suse.com> # btrfs
> Acked-by: Ilya Dryomov <idryo...@gmail.com> # Ceph
> Acked-by: Tariq Toukan <tar...@mellanox.com> # mlx4
> Signed-off-by: Michal Hocko <mho...@suse.com>

Acked-by: Leon Romanovsky <leo...@mellanox.com> # mlx5


signature.asc
Description: PGP signature

Re: [PATCH] net/mlx4: use rb_entry()

[Leon Romanovsky]

On Fri, Jan 20, 2017 at 10:36:57PM +0800, Geliang Tang wrote:
> To make the code clearer, use rb_entry() instead of container_of() to
> deal with rbtree.
>
> Signed-off-by: Geliang Tang 
> ---
>  drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)

I don't understand completely the rationale behind this conversion.
rb_entry == container_of, why do we need another name for it?

>
> diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c 
> b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> index 1822382..6da6e01 100644
> --- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> +++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
> @@ -236,8 +236,8 @@ static void *res_tracker_lookup(struct rb_root *root, u64 
> res_id)
>   struct rb_node *node = root->rb_node;
>
>   while (node) {
> - struct res_common *res = container_of(node, struct res_common,
> -   node);
> + struct res_common *res = rb_entry(node, struct res_common,
> +   node);
>
>   if (res_id < res->res_id)
>   node = node->rb_left;
> @@ -255,8 +255,8 @@ static int res_tracker_insert(struct rb_root *root, 
> struct res_common *res)
>
>   /* Figure out where to put new node */
>   while (*new) {
> - struct res_common *this = container_of(*new, struct res_common,
> -node);
> + struct res_common *this = rb_entry(*new, struct res_common,
> +node);
>
>   parent = *new;
>   if (res->res_id < this->res_id)
> --
> 2.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


signature.asc
Description: PGP signature

Re: [PATCH V5 for-next 21/21] RDMA/bnxt_re: Add bnxt_re driver build support

[Leon Romanovsky]

On Fri, Feb 10, 2017 at 03:19:53AM -0800, Selvin Xavier wrote:
> Makefile and Kconfig changes for enabling bnxt_re compilation
>
> v3: Adds list of MAINTAINERS of bnxt_re driver. Removes bnxt_re_debugfs.c
> from Makefile as this file is no longer present
>
> Signed-off-by: Devesh Sharma <devesh.sha...@broadcom.com>
> Signed-off-by: Somnath Kotur <somnath.ko...@broadcom.com>
> Signed-off-by: Sriharsha Basavapatna <sriharsha.basavapa...@broadcom.com>
> Signed-off-by: Selvin Xavier <selvin.xav...@broadcom.com>
> ---
>  MAINTAINERS| 11 +++
>  drivers/infiniband/Kconfig |  2 ++
>  drivers/infiniband/hw/Makefile |  1 +
>  drivers/infiniband/hw/bnxt_re/Kconfig  |  9 +
>  drivers/infiniband/hw/bnxt_re/Makefile |  6 ++
>  5 files changed, 29 insertions(+)
>  create mode 100644 drivers/infiniband/hw/bnxt_re/Kconfig
>  create mode 100644 drivers/infiniband/hw/bnxt_re/Makefile
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 5f0420a..468d2e8 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2808,6 +2808,17 @@ L: linux-arm-ker...@lists.infradead.org (moderated 
> for non-subscribers)
>  S:   Maintained
>  F:   arch/arm64/boot/dts/broadcom/vulcan*
>
> +BROADCOM NETXTREME-E ROCE DRIVER
> +M:   Selvin Xavier <selvin.xav...@broadcom.com>
> +M:   Devesh Sharma <devesh.sha...@broadcom.com>
> +M:   Somnath Kotur <somnath.ko...@broadcom.com>
> +M:   Sriharsha Basavapatna <sriharsha.basavapa...@broadcom.com>
> +L:   linux-r...@vger.kernel.org
> +W:   http://www.broadcom.com
> +S:   Supported
> +F:   drivers/infiniband/hw/bnxt_re/
> +F:   include/uapi/rdma/bnxt_re-abi.h
> +

Please take a look on the note which I gave to Faisal a long time ago [1].
Our experience shows that the more people you add here, the less chances
to be CCed on the relevant patches.

It is definitely up to you.

Thanks,
Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

[1] https://www.spinics.net/lists/linux-rdma/msg41457.html


signature.asc
Description: PGP signature

Re: [PATCH V5 for-next 16/21] RDMA/bnxt_re: Support poll_cq verb

[Leon Romanovsky]

On Fri, Feb 10, 2017 at 03:19:48AM -0800, Selvin Xavier wrote:
> Enables the fastpath ib_poll_cq verb.
>
> v2: Fixed sparse warnings
> v3: Fixes endianness related warnings reported by sparse. Also, fixes
> smatch and checkpatch warnings
> v5: Uses ETH_P_IBOE macro for RoCE ethertype
>
> Signed-off-by: Eddie Wai 
> Signed-off-by: Devesh Sharma 
> Signed-off-by: Somnath Kotur 
> Signed-off-by: Sriharsha Basavapatna 
> Signed-off-by: Selvin Xavier 
> ---
>  drivers/infiniband/hw/bnxt_re/ib_verbs.c | 522 
>  drivers/infiniband/hw/bnxt_re/ib_verbs.h |   1 +
>  drivers/infiniband/hw/bnxt_re/main.c |  22 +-
>  drivers/infiniband/hw/bnxt_re/qplib_fp.c | 560 
> ++-
>  drivers/infiniband/hw/bnxt_re/qplib_fp.h |   7 +-
>  5 files changed, 1107 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/infiniband/hw/bnxt_re/ib_verbs.c 
> b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> index 54d85bc..33af2e3 100644
> --- a/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> +++ b/drivers/infiniband/hw/bnxt_re/ib_verbs.c
> @@ -2230,6 +2230,528 @@ struct ib_cq *bnxt_re_create_cq(struct ib_device 
> *ibdev,
>   return ERR_PTR(rc);
>  }
>
> +static u8 __req_to_ib_wc_status(u8 qstatus)
> +{
> + switch (qstatus) {
> + case CQ_REQ_STATUS_OK:
> + return IB_WC_SUCCESS;
> + case CQ_REQ_STATUS_BAD_RESPONSE_ERR:
> + return IB_WC_BAD_RESP_ERR;
> + case CQ_REQ_STATUS_LOCAL_LENGTH_ERR:
> + return IB_WC_LOC_LEN_ERR;
> + case CQ_REQ_STATUS_LOCAL_QP_OPERATION_ERR:
> + return IB_WC_LOC_QP_OP_ERR;
> + case CQ_REQ_STATUS_LOCAL_PROTECTION_ERR:
> + return IB_WC_LOC_PROT_ERR;
> + case CQ_REQ_STATUS_MEMORY_MGT_OPERATION_ERR:
> + return IB_WC_GENERAL_ERR;
> + case CQ_REQ_STATUS_REMOTE_INVALID_REQUEST_ERR:
> + return IB_WC_REM_INV_REQ_ERR;
> + case CQ_REQ_STATUS_REMOTE_ACCESS_ERR:
> + return IB_WC_REM_ACCESS_ERR;
> + case CQ_REQ_STATUS_REMOTE_OPERATION_ERR:
> + return IB_WC_REM_OP_ERR;
> + case CQ_REQ_STATUS_RNR_NAK_RETRY_CNT_ERR:
> + return IB_WC_RNR_RETRY_EXC_ERR;
> + case CQ_REQ_STATUS_TRANSPORT_RETRY_CNT_ERR:
> + return IB_WC_RETRY_EXC_ERR;
> + case CQ_REQ_STATUS_WORK_REQUEST_FLUSHED_ERR:
> + return IB_WC_WR_FLUSH_ERR;
> + default:
> + return IB_WC_GENERAL_ERR;
> + }
> + return 0;
> +}
> +
> +static u8 __rawqp1_to_ib_wc_status(u8 qstatus)
> +{
> + switch (qstatus) {
> + case CQ_RES_RAWETH_QP1_STATUS_OK:
> + return IB_WC_SUCCESS;
> + case CQ_RES_RAWETH_QP1_STATUS_LOCAL_ACCESS_ERROR:
> + return IB_WC_LOC_ACCESS_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_HW_LOCAL_LENGTH_ERR:
> + return IB_WC_LOC_LEN_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_LOCAL_PROTECTION_ERR:
> + return IB_WC_LOC_PROT_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_LOCAL_QP_OPERATION_ERR:
> + return IB_WC_LOC_QP_OP_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_MEMORY_MGT_OPERATION_ERR:
> + return IB_WC_GENERAL_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_WORK_REQUEST_FLUSHED_ERR:
> + return IB_WC_WR_FLUSH_ERR;
> + case CQ_RES_RAWETH_QP1_STATUS_HW_FLUSH_ERR:
> + return IB_WC_WR_FLUSH_ERR;
> + default:
> + return IB_WC_GENERAL_ERR;
> + }
> +}
> +
> +static u8 __rc_to_ib_wc_status(u8 qstatus)
> +{
> + switch (qstatus) {
> + case CQ_RES_RC_STATUS_OK:
> + return IB_WC_SUCCESS;
> + case CQ_RES_RC_STATUS_LOCAL_ACCESS_ERROR:
> + return IB_WC_LOC_ACCESS_ERR;
> + case CQ_RES_RC_STATUS_LOCAL_LENGTH_ERR:
> + return IB_WC_LOC_LEN_ERR;
> + case CQ_RES_RC_STATUS_LOCAL_PROTECTION_ERR:
> + return IB_WC_LOC_PROT_ERR;
> + case CQ_RES_RC_STATUS_LOCAL_QP_OPERATION_ERR:
> + return IB_WC_LOC_QP_OP_ERR;
> + case CQ_RES_RC_STATUS_MEMORY_MGT_OPERATION_ERR:
> + return IB_WC_GENERAL_ERR;
> + case CQ_RES_RC_STATUS_REMOTE_INVALID_REQUEST_ERR:
> + return IB_WC_REM_INV_REQ_ERR;
> + case CQ_RES_RC_STATUS_WORK_REQUEST_FLUSHED_ERR:
> + return IB_WC_WR_FLUSH_ERR;
> + case CQ_RES_RC_STATUS_HW_FLUSH_ERR:
> + return IB_WC_WR_FLUSH_ERR;
> + default:
> + return IB_WC_GENERAL_ERR;
> + }
> +}
> +

Why don't you use these defines directly?

Thanks


signature.asc
Description: PGP signature