Re: [PATCH] RDMA/cma: Make CM response timeout and # CM retries configurable

[Steve Wise]

On 2/22/2019 10:36 AM, Jason Gunthorpe wrote:
> On Sun, Feb 17, 2019 at 06:09:09PM +0100, Håkon Bugge wrote:
>> During certain workloads, the default CM response timeout is too
>> short, leading to excessive retries. Hence, make it configurable
>> through sysctl. While at it, also make number of CM retries
>> configurable.
>>
>> The defaults are not changed.
>>
>> Signed-off-by: Håkon Bugge 
>>  drivers/infiniband/core/cma.c | 51 ++-
>>  1 file changed, 44 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> index c43512752b8a..ce99e1cd1029 100644
>> +++ b/drivers/infiniband/core/cma.c
>> @@ -43,6 +43,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>  #include 
>>  
>>  #include 
>> @@ -68,13 +69,46 @@ MODULE_AUTHOR("Sean Hefty");
>>  MODULE_DESCRIPTION("Generic RDMA CM Agent");
>>  MODULE_LICENSE("Dual BSD/GPL");
>>  
>> -#define CMA_CM_RESPONSE_TIMEOUT 20
>>  #define CMA_QUERY_CLASSPORT_INFO_TIMEOUT 3000
>> -#define CMA_MAX_CM_RETRIES 15
>>  #define CMA_CM_MRA_SETTING (IB_CM_MRA_FLAG_DELAY | 24)
>>  #define CMA_IBOE_PACKET_LIFETIME 18
>>  #define CMA_PREFERRED_ROCE_GID_TYPE IB_GID_TYPE_ROCE_UDP_ENCAP
>>  
>> +#define CMA_DFLT_CM_RESPONSE_TIMEOUT 20
>> +static int cma_cm_response_timeout = CMA_DFLT_CM_RESPONSE_TIMEOUT;
>> +static int cma_cm_response_timeout_min = 8;
>> +static int cma_cm_response_timeout_max = 31;
>> +#undef CMA_DFLT_CM_RESPONSE_TIMEOUT
>> +
>> +#define CMA_DFLT_MAX_CM_RETRIES 15
>> +static int cma_max_cm_retries = CMA_DFLT_MAX_CM_RETRIES;
>> +static int cma_max_cm_retries_min = 1;
>> +static int cma_max_cm_retries_max = 100;
>> +#undef CMA_DFLT_MAX_CM_RETRIES
>> +
>> +static struct ctl_table_header *cma_ctl_table_hdr;
>> +static struct ctl_table cma_ctl_table[] = {
>> +{
>> +.procname   = "cma_cm_response_timeout",
>> +.data   = _cm_response_timeout,
>> +.maxlen = sizeof(cma_cm_response_timeout),
>> +.mode   = 0644,
>> +.proc_handler   = proc_dointvec_minmax,
>> +.extra1 = _cm_response_timeout_min,
>> +.extra2 = _cm_response_timeout_max,
>> +},
>> +{
>> +.procname   = "cma_max_cm_retries",
>> +.data   = _max_cm_retries,
>> +.maxlen = sizeof(cma_max_cm_retries),
>> +.mode   = 0644,
>> +.proc_handler   = proc_dointvec_minmax,
>> +.extra1 = _max_cm_retries_min,
>> +.extra2 = _max_cm_retries_max,
>> +},
>> +{ }
>> +};
> Is sysctl the right approach here? Should it be rdma tool instead?
>
> Jason

There are other rdma sysctls currently:  net.rdma_ucm.max_backlog and
net.iw_cm.default_backlog.  The core network stack seems to use sysctl
and not ip tool to set basically globals.

To use rdma tool, we'd have to have some concept of a "module" object, I
guess.  IE there's dev, link, and resource rdma tool objects currently. 
But these cma timeout settings are really not per dev, link, nor a
resource.   Maybe we have just a "core" object:  rdma core set
cma_max_cm_retries min 8 max 30.

Re: [PATCH 0/5] RDMA: reg_remote_mr

[Steve Wise]

On 1/29/2019 7:26 AM, Joel Nider wrote:
> As discussed at LPC'18, there is a need to be able to register a memory
> region (MR) on behalf of another process. One example is the case of
> post-copy container migration, in which CRIU is responsible for setting
> up the migration, but the contents of the memory are from the migrating
> process. In this case, we want all RDMA READ requests to be served by
> the address space of the migration process directly (not by CRIU). This
> patchset implements a new uverbs command which allows an application to
> register a memory region in the address space of another process.

Hey Joel,

Dumb question:

Doesn't this open a security hole by allowing any process to register
memory in any other process?

Steve.

Re: [PATCH RFC] iw_cxgb4: drop check - dead code

[Steve Wise]

On 1/23/2019 3:44 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with  | __GFP_NOFAIL  so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire 
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
> 
> As per steve's remarkes I revised this to the below and applied it to
> for-next
> 
>>From 4b2d4262ee2ea58df867de1928bf208795344432 Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe 
> Date: Sun, 20 Jan 2019 02:27:13 +0100
> Subject: [PATCH] RDMA/iw_cxgb4: Drop __GFP_NOFAIL
> 
> There is no reason for this __GFP_NOFAIL, none of the other routines in
> this file use it, and there is an error unwind here. NOFAIL should be
> reserved for special cases, not used by network drivers.
> 
> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
> Reported-by: Nicholas Mc Guire 
> Signed-off-by: Jason Gunthorpe 
> ---
>  drivers/infiniband/hw/cxgb4/qp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
> b/drivers/infiniband/hw/cxgb4/qp.c
> index 03f4c66c265946..c00a4114412694 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2597,7 +2597,7 @@ static int alloc_srq_queue(struct c4iw_srq *srq, struct 
> c4iw_dev_ucontext *uctx,
>   /* build fw_ri_res_wr */
>   wr_len = sizeof(*res_wr) + sizeof(*res);
>  
> - skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
> + skb = alloc_skb(wr_len, GFP_KERNEL);
>   if (!skb)
>   goto err_free_queue;
>   set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
> 

Thanks Jason!

Re: [PATCH RFC] iw_cxgb4: drop check - dead code

[Steve Wise]

On 1/23/2019 12:30 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with  | __GFP_NOFAIL  so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire 
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
> 
> Steve? It seems weird to have NOFAIL and then have an error unwind
> path, what is the deal here?
> 
>> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
>> b/drivers/infiniband/hw/cxgb4/qp.c
>> index 917ce5c..c2a12ba 100644
>> --- a/drivers/infiniband/hw/cxgb4/qp.c
>> +++ b/drivers/infiniband/hw/cxgb4/qp.c
>> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, 
>> struct c4iw_dev_ucontext *uctx,
>>  wr_len = sizeof(*res_wr) + sizeof(*res);
>>  
>>  skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
>> -if (!skb)
>> -goto err_free_queue;
>>  set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>>  
>>  res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
>> -- 
>> 2.1.4
>>

The other queue allocations in qp.c don't use __GFP_NOFAIL.  So either
leave it and remove the error check as per this patch, or remove the
NOFAIL and leave the check.

I suggest you remove the __GFP_NOFAIL, since I have a recollection that
using it was frowned upon.  In this case, if there is no memory
available it is reasonable to return that error to the user creating the
srq...


Steve.

Re: [PATCH RFC] iw_cxgb4: drop check - dead code

[Steve Wise]

On 1/23/2019 12:30 PM, Jason Gunthorpe wrote:
> On Sun, Jan 20, 2019 at 02:27:13AM +0100, Nicholas Mc Guire wrote:
>> The kmalloc is called with  | __GFP_NOFAIL  so there is no point in
>> checking the return value - it either returns valid storage or it would
>> hang/terminate there. But it is not possible to say if the use of
>> __GFP_NOFAIL is really needed and the check should be removed or
>> vice-versa (use of __GFP_NOFAIL should be only in exceptional
>> cases as I understand it and alloc_srq_queue() is called in quite
>> a few places)
>> In either way it would need fixing.
>>
>> Signed-off-by: Nicholas Mc Guire 
>> Fixes: 6a0b6174d35a ("rdma/cxgb4: Add support for kernel mode SRQ's")
>> ---
> 
> Steve? It seems weird to have NOFAIL and then have an error unwind
> path, what is the deal here?
> 
>> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
>> b/drivers/infiniband/hw/cxgb4/qp.c
>> index 917ce5c..c2a12ba 100644
>> --- a/drivers/infiniband/hw/cxgb4/qp.c
>> +++ b/drivers/infiniband/hw/cxgb4/qp.c
>> @@ -2597,8 +2597,6 @@ static int alloc_srq_queue(struct c4iw_srq *srq, 
>> struct c4iw_dev_ucontext *uctx,
>>  wr_len = sizeof(*res_wr) + sizeof(*res);
>>  
>>  skb = alloc_skb(wr_len, GFP_KERNEL | __GFP_NOFAIL);
>> -if (!skb)
>> -goto err_free_queue;
>>  set_wr_txq(skb, CPL_PRIORITY_CONTROL, 0);
>>  
>>  res_wr = (struct fw_ri_res_wr *)__skb_put(skb, wr_len);
>> -- 
>> 2.1.4
>>

The other queue allocations in qp.c don't use __GFP_NOFAIL.  So either
leave it and remove the error check as per this patch, or remove the
NOFAIL and leave the check.

I suggest you remove the __GFP_NOFAIL.


Steve.

Re: [PATCH 1/8] infiniband: cxgb4: no need to check return value of debugfs_create functions

[Steve Wise]

Hey Greg,

On 1/22/2019 9:17 AM, Greg Kroah-Hartman wrote:
> When calling debugfs functions, there is no need to ever check the
> return value.  The function can work or not, but the code logic should
> never do something different based on this.
> 
> Cc: Steve Wise 
> Cc: Doug Ledford 
> Cc: Jason Gunthorpe 
> Cc: linux-r...@vger.kernel.org
> Signed-off-by: Greg Kroah-Hartman 
> ---
>  drivers/infiniband/hw/cxgb4/device.c | 8 +---
>  1 file changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/device.c 
> b/drivers/infiniband/hw/cxgb4/device.c
> index c13c0ba30f63..9c10fff6dcfb 100644
> --- a/drivers/infiniband/hw/cxgb4/device.c
> +++ b/drivers/infiniband/hw/cxgb4/device.c
> @@ -720,11 +720,8 @@ static const struct file_operations ep_debugfs_fops = {
>   .read= debugfs_read,
>  };
>  
> -static int setup_debugfs(struct c4iw_dev *devp)
> +static void setup_debugfs(struct c4iw_dev *devp)
>  {
> - if (!devp->debugfs_root)
> - return -1;
> -
>   debugfs_create_file_size("qps", S_IWUSR, devp->debugfs_root,
>(void *)devp, _debugfs_fops, 4096);
>  
> @@ -740,7 +737,6 @@ static int setup_debugfs(struct c4iw_dev *devp)
>   if (c4iw_wr_log)
>   debugfs_create_file_size("wr_log", S_IWUSR, devp->debugfs_root,
>(void *)devp, _log_debugfs_fops, 
> 4096);
> - return 0;
>  }
>  
>  void c4iw_release_dev_ucontext(struct c4iw_rdev *rdev,
> @@ -1553,8 +1549,6 @@ static int __init c4iw_init_module(void)
>   return err;
>  
>   c4iw_debugfs_root = debugfs_create_dir(DRV_NAME, NULL);
> - if (!c4iw_debugfs_root)
> - pr_warn("could not create debugfs entry, continuing\n");
>  
>   reg_workq = create_singlethread_workqueue("Register_iWARP_device");
>   if (!reg_workq) {
> 

So it is not a problem to call debugfs_create_file_size() when
devp->debugfs_root is NULL?


Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb4/qp.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/12/2018 9:21 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb4/qp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
> b/drivers/infiniband/hw/cxgb4/qp.c
> index 13478f3..5a8030b 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2564,13 +2564,12 @@ static int alloc_srq_queue(struct c4iw_srq *srq, 
> struct c4iw_dev_ucontext *uctx,
>   wq->rqt_abs_idx = (wq->rqt_hwaddr - rdev->lldi.vr->rq.start) >>
>   T4_RQT_ENTRY_SHIFT;
>  
> - wq->queue = dma_alloc_coherent(>lldi.pdev->dev,
> + wq->queue = dma_zalloc_coherent(>lldi.pdev->dev,
>  wq->memsize, >dma_addr,
>   GFP_KERNEL);
>   if (!wq->queue)
>   goto err_free_rqtpool;
>  
> - memset(wq->queue, 0, wq->memsize);
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>  
>   wq->bar2_va = c4iw_bar2_addrs(rdev, wq->qid, CXGB4_BAR2_QTYPE_EGRESS,
> 

Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb4/qp.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/12/2018 9:21 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb4/qp.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/qp.c 
> b/drivers/infiniband/hw/cxgb4/qp.c
> index 13478f3..5a8030b 100644
> --- a/drivers/infiniband/hw/cxgb4/qp.c
> +++ b/drivers/infiniband/hw/cxgb4/qp.c
> @@ -2564,13 +2564,12 @@ static int alloc_srq_queue(struct c4iw_srq *srq, 
> struct c4iw_dev_ucontext *uctx,
>   wq->rqt_abs_idx = (wq->rqt_hwaddr - rdev->lldi.vr->rq.start) >>
>   T4_RQT_ENTRY_SHIFT;
>  
> - wq->queue = dma_alloc_coherent(>lldi.pdev->dev,
> + wq->queue = dma_zalloc_coherent(>lldi.pdev->dev,
>  wq->memsize, >dma_addr,
>   GFP_KERNEL);
>   if (!wq->queue)
>   goto err_free_rqtpool;
>  
> - memset(wq->queue, 0, wq->memsize);
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>  
>   wq->bar2_va = c4iw_bar2_addrs(rdev, wq->qid, CXGB4_BAR2_QTYPE_EGRESS,
> 

Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb3/cxio_hal.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/9/2018 10:50 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba..df4f7a3 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -291,13 +291,12 @@ int cxio_create_qp(struct cxio_rdev *rdev_p, u32 
> kernel_domain,
>   if (!wq->sq)
>   goto err3;
>  
> - wq->queue = dma_alloc_coherent(&(rdev_p->rnic_info.pdev->dev),
> + wq->queue = dma_zalloc_coherent(&(rdev_p->rnic_info.pdev->dev),
>depth * sizeof(union t3_wr),
>&(wq->dma_addr), GFP_KERNEL);
>   if (!wq->queue)
>   goto err4;
>  
> - memset(wq->queue, 0, depth * sizeof(union t3_wr));
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>   wq->doorbell = (void __iomem *)rdev_p->rnic_info.kdb_addr;
>   if (!kernel_domain)
> 

Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb3/cxio_hal.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/9/2018 10:50 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba..df4f7a3 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -291,13 +291,12 @@ int cxio_create_qp(struct cxio_rdev *rdev_p, u32 
> kernel_domain,
>   if (!wq->sq)
>   goto err3;
>  
> - wq->queue = dma_alloc_coherent(&(rdev_p->rnic_info.pdev->dev),
> + wq->queue = dma_zalloc_coherent(&(rdev_p->rnic_info.pdev->dev),
>depth * sizeof(union t3_wr),
>&(wq->dma_addr), GFP_KERNEL);
>   if (!wq->queue)
>   goto err4;
>  
> - memset(wq->queue, 0, depth * sizeof(union t3_wr));
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>   wq->doorbell = (void __iomem *)rdev_p->rnic_info.kdb_addr;
>   if (!kernel_domain)
> 

Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb3/cxio_hal.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/9/2018 10:50 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba..df4f7a3 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -291,13 +291,12 @@ int cxio_create_qp(struct cxio_rdev *rdev_p, u32 
> kernel_domain,
>   if (!wq->sq)
>   goto err3;
>  
> - wq->queue = dma_alloc_coherent(&(rdev_p->rnic_info.pdev->dev),
> + wq->queue = dma_zalloc_coherent(&(rdev_p->rnic_info.pdev->dev),
>depth * sizeof(union t3_wr),
>&(wq->dma_addr), GFP_KERNEL);
>   if (!wq->queue)
>   goto err4;
>  
> - memset(wq->queue, 0, depth * sizeof(union t3_wr));
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>   wq->doorbell = (void __iomem *)rdev_p->rnic_info.kdb_addr;
>   if (!kernel_domain)
> 

Acked-by: Steve Wise 

Re: [PATCH] infiniband/hw/cxgb3/cxio_hal.c: Use dma_zalloc_coherent

[Steve Wise]

On 11/9/2018 10:50 AM, Sabyasachi Gupta wrote:
> Replaced dma_alloc_coherent + memset with dma_zalloc_coherent
> 
> Signed-off-by: Sabyasachi Gupta 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba..df4f7a3 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -291,13 +291,12 @@ int cxio_create_qp(struct cxio_rdev *rdev_p, u32 
> kernel_domain,
>   if (!wq->sq)
>   goto err3;
>  
> - wq->queue = dma_alloc_coherent(&(rdev_p->rnic_info.pdev->dev),
> + wq->queue = dma_zalloc_coherent(&(rdev_p->rnic_info.pdev->dev),
>depth * sizeof(union t3_wr),
>&(wq->dma_addr), GFP_KERNEL);
>   if (!wq->queue)
>   goto err4;
>  
> - memset(wq->queue, 0, depth * sizeof(union t3_wr));
>   dma_unmap_addr_set(wq, mapping, wq->dma_addr);
>   wq->doorbell = (void __iomem *)rdev_p->rnic_info.kdb_addr;
>   if (!kernel_domain)
> 

Acked-by: Steve Wise 

Re: [PATCH] RDMA/cxgb3: Fix unintended sign extension

[Steve Wise]

On 10/25/2018 9:31 AM, Colin King wrote:
> From: Colin Ian King 
> 
> In the expression "utx_len << 28", utx_len starts as u8, but is promoted
> to a signed int, then sign-extended to u64.  If utx_len is 0xf8 or greater
> then the sign extension will set all the upper bits of utx_cmd which is
> probably not what was intended.  Cast to utx_len to u64  to avoid the sign
> extension.
> 
> Detected by CoverityScan, CID#138764 ("Unintended sign extension")
> 
> Fixes: b038ced7b370 ("RDMA/cxgb3: Add driver for Chelsio T3 RNIC")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba522ba..c5cb80ccc6d6 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -647,7 +647,7 @@ static int cxio_hal_ctrl_qp_write_mem(struct cxio_rdev 
> *rdev_p, u32 addr,
>   wqe += (sizeof(struct t3_bypass_wr) >> 3);
>   utx_cmd = (T3_UTX_MEM_WRITE << 28) | (addr + i * 3);
>   utx_cmd <<= 32;
> - utx_cmd |= (utx_len << 28) | ((utx_len << 2) + 1);
> + utx_cmd |= ((u64)utx_len << 28) | ((utx_len << 2) + 1);
>   *wqe = cpu_to_be64(utx_cmd);
>   wqe++;
>   copy_data = (u8 *) data + i * 96;
> 

Acked-by: Steve Wise 

Re: [PATCH] RDMA/cxgb3: Fix unintended sign extension

[Steve Wise]

On 10/25/2018 9:31 AM, Colin King wrote:
> From: Colin Ian King 
> 
> In the expression "utx_len << 28", utx_len starts as u8, but is promoted
> to a signed int, then sign-extended to u64.  If utx_len is 0xf8 or greater
> then the sign extension will set all the upper bits of utx_cmd which is
> probably not what was intended.  Cast to utx_len to u64  to avoid the sign
> extension.
> 
> Detected by CoverityScan, CID#138764 ("Unintended sign extension")
> 
> Fixes: b038ced7b370 ("RDMA/cxgb3: Add driver for Chelsio T3 RNIC")
> Signed-off-by: Colin Ian King 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.c 
> b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> index dcb4bba522ba..c5cb80ccc6d6 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.c
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.c
> @@ -647,7 +647,7 @@ static int cxio_hal_ctrl_qp_write_mem(struct cxio_rdev 
> *rdev_p, u32 addr,
>   wqe += (sizeof(struct t3_bypass_wr) >> 3);
>   utx_cmd = (T3_UTX_MEM_WRITE << 28) | (addr + i * 3);
>   utx_cmd <<= 32;
> - utx_cmd |= (utx_len << 28) | ((utx_len << 2) + 1);
> + utx_cmd |= ((u64)utx_len << 28) | ((utx_len << 2) + 1);
>   *wqe = cpu_to_be64(utx_cmd);
>   wqe++;
>   copy_data = (u8 *) data + i * 96;
> 

Acked-by: Steve Wise 

RE: [PATCH] iw_cxgb4: fix a missing-check bug

[Steve Wise]

> -Original Message-
> From: Wenwen Wang 
> Sent: Saturday, October 20, 2018 6:56 PM
> To: sw...@opengridcomputing.com
> Cc: Kangjie Lu ; sw...@chelsio.com; dledf...@redhat.com;
> j...@ziepe.ca; linux-r...@vger.kernel.org; open list  ker...@vger.kernel.org>; Wenwen Wang 
> Subject: Re: [PATCH] iw_cxgb4: fix a missing-check bug
> 
> On Sat, Oct 20, 2018 at 6:41 PM Steve Wise
>  wrote:
> >
> > Hey Wenwen,
> >
> > > Subject: [PATCH] iw_cxgb4: fix a missing-check bug
> > >
> > > In c4iw_flush_hw_cq, the next CQE is acquired through
> t4_next_hw_cqe(). In
> > > t4_next_hw_cqe(), the CQE, i.e., 'cq->queue[cq->cidx]', is checked to see
> > > whether it is valid through t4_valid_cqe(). If it is valid, the address of
> > > the CQE is then saved to 'hw_cqe'. Later on, the CQE is copied to the
> > local
> > > memory in create_read_req_cqe(). The problem here is that the CQE is
> > > actually in a DMA region allocated by dma_alloc_coherent() in
> create_cq().
> > > Given that the device also has the permission to access the DMA region, a
> > > malicious device controlled by an attacker can modify the CQE in the DMA
> > > region after the check in t4_next_hw_cqe() but before the copy in
> > > create_read_req_cqe(). By doing so, the attacker can supply invalid CQE,
> > > which can cause undefined behavior of the kernel and introduce
> potential
> > > security risks.
> > >
> >
> > If the dma device is malicious, couldn't it just dma some incorrect CQE but
> > still valid in the first place?  I don't think this patch actually solves
> > the issue, and it forces a copy of a 64B CQE in a critical data io path.
> 
> Thanks for your response! If the malicious dma device just dma some
> incorrect CQE, it will not be able to pass the verification in
> t4_valid_cqe().
> 

As long as the gen bit is correct, the CQE is considered valid.  You cannot 
protect against a malicious dma device.  Or at least not with the current 
driver/device contract.

Steve.

RE: [PATCH] iw_cxgb4: fix a missing-check bug

[Steve Wise]

> -Original Message-
> From: Wenwen Wang 
> Sent: Saturday, October 20, 2018 6:56 PM
> To: sw...@opengridcomputing.com
> Cc: Kangjie Lu ; sw...@chelsio.com; dledf...@redhat.com;
> j...@ziepe.ca; linux-r...@vger.kernel.org; open list  ker...@vger.kernel.org>; Wenwen Wang 
> Subject: Re: [PATCH] iw_cxgb4: fix a missing-check bug
> 
> On Sat, Oct 20, 2018 at 6:41 PM Steve Wise
>  wrote:
> >
> > Hey Wenwen,
> >
> > > Subject: [PATCH] iw_cxgb4: fix a missing-check bug
> > >
> > > In c4iw_flush_hw_cq, the next CQE is acquired through
> t4_next_hw_cqe(). In
> > > t4_next_hw_cqe(), the CQE, i.e., 'cq->queue[cq->cidx]', is checked to see
> > > whether it is valid through t4_valid_cqe(). If it is valid, the address of
> > > the CQE is then saved to 'hw_cqe'. Later on, the CQE is copied to the
> > local
> > > memory in create_read_req_cqe(). The problem here is that the CQE is
> > > actually in a DMA region allocated by dma_alloc_coherent() in
> create_cq().
> > > Given that the device also has the permission to access the DMA region, a
> > > malicious device controlled by an attacker can modify the CQE in the DMA
> > > region after the check in t4_next_hw_cqe() but before the copy in
> > > create_read_req_cqe(). By doing so, the attacker can supply invalid CQE,
> > > which can cause undefined behavior of the kernel and introduce
> potential
> > > security risks.
> > >
> >
> > If the dma device is malicious, couldn't it just dma some incorrect CQE but
> > still valid in the first place?  I don't think this patch actually solves
> > the issue, and it forces a copy of a 64B CQE in a critical data io path.
> 
> Thanks for your response! If the malicious dma device just dma some
> incorrect CQE, it will not be able to pass the verification in
> t4_valid_cqe().
> 

As long as the gen bit is correct, the CQE is considered valid.  You cannot 
protect against a malicious dma device.  Or at least not with the current 
driver/device contract.

Steve.

RE: [PATCH] iw_cxgb4: fix a missing-check bug

[Steve Wise]

Hey Wenwen,

> Subject: [PATCH] iw_cxgb4: fix a missing-check bug
> 
> In c4iw_flush_hw_cq, the next CQE is acquired through t4_next_hw_cqe(). In
> t4_next_hw_cqe(), the CQE, i.e., 'cq->queue[cq->cidx]', is checked to see
> whether it is valid through t4_valid_cqe(). If it is valid, the address of
> the CQE is then saved to 'hw_cqe'. Later on, the CQE is copied to the
local
> memory in create_read_req_cqe(). The problem here is that the CQE is
> actually in a DMA region allocated by dma_alloc_coherent() in create_cq().
> Given that the device also has the permission to access the DMA region, a
> malicious device controlled by an attacker can modify the CQE in the DMA
> region after the check in t4_next_hw_cqe() but before the copy in
> create_read_req_cqe(). By doing so, the attacker can supply invalid CQE,
> which can cause undefined behavior of the kernel and introduce potential
> security risks.
> 

If the dma device is malicious, couldn't it just dma some incorrect CQE but
still valid in the first place?  I don't think this patch actually solves
the issue, and it forces a copy of a 64B CQE in a critical data io path.  

So I must NACK this. 

Thanks,

Steve.

RE: [PATCH] iw_cxgb4: fix a missing-check bug

[Steve Wise]

Hey Wenwen,

> Subject: [PATCH] iw_cxgb4: fix a missing-check bug
> 
> In c4iw_flush_hw_cq, the next CQE is acquired through t4_next_hw_cqe(). In
> t4_next_hw_cqe(), the CQE, i.e., 'cq->queue[cq->cidx]', is checked to see
> whether it is valid through t4_valid_cqe(). If it is valid, the address of
> the CQE is then saved to 'hw_cqe'. Later on, the CQE is copied to the
local
> memory in create_read_req_cqe(). The problem here is that the CQE is
> actually in a DMA region allocated by dma_alloc_coherent() in create_cq().
> Given that the device also has the permission to access the DMA region, a
> malicious device controlled by an attacker can modify the CQE in the DMA
> region after the check in t4_next_hw_cqe() but before the copy in
> create_read_req_cqe(). By doing so, the attacker can supply invalid CQE,
> which can cause undefined behavior of the kernel and introduce potential
> security risks.
> 

If the dma device is malicious, couldn't it just dma some incorrect CQE but
still valid in the first place?  I don't think this patch actually solves
the issue, and it forces a copy of a 64B CQE in a critical data io path.  

So I must NACK this. 

Thanks,

Steve.

Re: [PATCH] iw_cxgb4: Use proper enumerated type in c4iw_bar2_addrs

[Steve Wise]

On 9/24/2018 2:29 PM, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
> 
> drivers/infiniband/hw/cxgb4/qp.c:287:8: warning: implicit conversion
> from enumeration type 'enum t4_bar2_qtype' to different enumeration type
> 'enum cxgb4_bar2_qtype' [-Wenum-conversion]
>  T4_BAR2_QTYPE_EGRESS,
>  ^~~~
> 
> c4iw_bar2_addrs expects a value from enum cxgb4_bar2_qtype so use the
> corresponding values from that type so Clang is satisfied without
> changing the meaning of the code.
> 
> T4_BAR2_QTYPE_EGRESS = CXGB4_BAR2_QTYPE_EGRESS = 0
> T4_BAR2_QTYPE_INGRESS = CXGB4_BAR2_QTYPE_INGRESS = 1
> 
> Reported-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 

Looks correct, Thanks.

Acked-by: Steve Wise 

Re: [PATCH] iw_cxgb4: Use proper enumerated type in c4iw_bar2_addrs

[Steve Wise]

On 9/24/2018 2:29 PM, Nathan Chancellor wrote:
> Clang warns when one enumerated type is implicitly converted to another.
> 
> drivers/infiniband/hw/cxgb4/qp.c:287:8: warning: implicit conversion
> from enumeration type 'enum t4_bar2_qtype' to different enumeration type
> 'enum cxgb4_bar2_qtype' [-Wenum-conversion]
>  T4_BAR2_QTYPE_EGRESS,
>  ^~~~
> 
> c4iw_bar2_addrs expects a value from enum cxgb4_bar2_qtype so use the
> corresponding values from that type so Clang is satisfied without
> changing the meaning of the code.
> 
> T4_BAR2_QTYPE_EGRESS = CXGB4_BAR2_QTYPE_EGRESS = 0
> T4_BAR2_QTYPE_INGRESS = CXGB4_BAR2_QTYPE_INGRESS = 1
> 
> Reported-by: Nick Desaulniers 
> Signed-off-by: Nathan Chancellor 

Looks correct, Thanks.

Acked-by: Steve Wise 

Re: [PATCH] RDMA/cxgb4: remove redundant null pointer check before kfree_skb

[Steve Wise]

On 9/20/2018 4:52 AM, zhong jiang wrote:
> kfree_skb has taken the null pointer into account. hence it is safe
> to remove the redundant null pointer check before kfree_skb.
> 
> Signed-off-by: zhong jiang 
> ---
>  drivers/infiniband/hw/cxgb4/cm.c | 3 +--
>  drivers/infiniband/hw/cxgb4/qp.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)

Acked-by: Steve Wise 

Re: [PATCH] RDMA/cxgb4: remove redundant null pointer check before kfree_skb

[Steve Wise]

On 9/20/2018 4:52 AM, zhong jiang wrote:
> kfree_skb has taken the null pointer into account. hence it is safe
> to remove the redundant null pointer check before kfree_skb.
> 
> Signed-off-by: zhong jiang 
> ---
>  drivers/infiniband/hw/cxgb4/cm.c | 3 +--
>  drivers/infiniband/hw/cxgb4/qp.c | 3 +--
>  2 files changed, 2 insertions(+), 4 deletions(-)

Acked-by: Steve Wise 

RE: linux-next: build failure after merge of the block tree

[Steve Wise]

> -Original Message-
> From: Christoph Hellwig 
> Sent: Thursday, July 26, 2018 12:57 PM
> To: Jens Axboe 
> Cc: Christoph Hellwig ; Stephen Rothwell
> ; Doug Ledford ; Jason
> Gunthorpe ; Linux-Next Mailing List  n...@vger.kernel.org>; Linux Kernel Mailing List  ker...@vger.kernel.org>; Steve Wise 
> Subject: Re: linux-next: build failure after merge of the block tree
> 
> On Thu, Jul 26, 2018 at 10:48:21AM -0700, Jens Axboe wrote:
> > >>  inline_page_count = num_pages(port->inline_data_size);
> > >>  inline_sge_count = max(cm_id->device->attrs.max_sge_rd,
> > >> -cm_id->device->attrs.max_sge) - 1;
> > >> +cm_id->device->attrs.max_send_sge) -
1;
> > >
> > > This should be max_recv_sge.
> >
> > Why do we even have this conflicts to begin with?
> 
> Because the nvme code added a new user of max_sges while the RDMA tree
> split it into two separate fields.  We discussed this a while ago when
> the issue came up.

See: https://www.spinics.net/lists/linux-rdma/msg66208.html 

Steve.

RE: linux-next: build failure after merge of the block tree

[Steve Wise]

> -Original Message-
> From: Christoph Hellwig 
> Sent: Thursday, July 26, 2018 12:57 PM
> To: Jens Axboe 
> Cc: Christoph Hellwig ; Stephen Rothwell
> ; Doug Ledford ; Jason
> Gunthorpe ; Linux-Next Mailing List  n...@vger.kernel.org>; Linux Kernel Mailing List  ker...@vger.kernel.org>; Steve Wise 
> Subject: Re: linux-next: build failure after merge of the block tree
> 
> On Thu, Jul 26, 2018 at 10:48:21AM -0700, Jens Axboe wrote:
> > >>  inline_page_count = num_pages(port->inline_data_size);
> > >>  inline_sge_count = max(cm_id->device->attrs.max_sge_rd,
> > >> -cm_id->device->attrs.max_sge) - 1;
> > >> +cm_id->device->attrs.max_send_sge) -
1;
> > >
> > > This should be max_recv_sge.
> >
> > Why do we even have this conflicts to begin with?
> 
> Because the nvme code added a new user of max_sges while the RDMA tree
> split it into two separate fields.  We discussed this a while ago when
> the issue came up.

See: https://www.spinics.net/lists/linux-rdma/msg66208.html 

Steve.

RE: linux-next: build failure after merge of the block tree

[Steve Wise]

Hey Stephen:

> I have applied the following merge fix patch for today:
> 
> From: Stephen Rothwell 
> Date: Thu, 26 Jul 2018 14:32:15 +1000
> Subject: [PATCH] nvme-dma: merge fix up for replacement of max_sge
> 
> Signed-off-by: Stephen Rothwell 
> ---
>  drivers/nvme/host/rdma.c   | 2 +-
>  drivers/nvme/target/rdma.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index cfa0319fcd1c..368fe5ac0c6b 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -378,7 +378,7 @@ nvme_rdma_find_get_device(struct rdma_cm_id
> *cm_id)
>   }
> 
>   ndev->num_inline_segments =
> min(NVME_RDMA_MAX_INLINE_SEGMENTS,
> - ndev->dev->attrs.max_sge - 1);
> + ndev->dev->attrs.max_send_sge - 1);
>   list_add(>entry, _list);
>  out_unlock:
>   mutex_unlock(_list_mutex);
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 86121a7a19b2..8c30ac7d8078 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -891,7 +891,7 @@ nvmet_rdma_find_get_device(struct rdma_cm_id
> *cm_id)
> 
>   inline_page_count = num_pages(port->inline_data_size);
>   inline_sge_count = max(cm_id->device->attrs.max_sge_rd,
> - cm_id->device->attrs.max_sge) - 1;
> + cm_id->device->attrs.max_send_sge) - 1;

This should actually be max_recv_sge.

Thanks!

Steve.

RE: linux-next: build failure after merge of the block tree

[Steve Wise]

Hey Stephen:

> I have applied the following merge fix patch for today:
> 
> From: Stephen Rothwell 
> Date: Thu, 26 Jul 2018 14:32:15 +1000
> Subject: [PATCH] nvme-dma: merge fix up for replacement of max_sge
> 
> Signed-off-by: Stephen Rothwell 
> ---
>  drivers/nvme/host/rdma.c   | 2 +-
>  drivers/nvme/target/rdma.c | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
> index cfa0319fcd1c..368fe5ac0c6b 100644
> --- a/drivers/nvme/host/rdma.c
> +++ b/drivers/nvme/host/rdma.c
> @@ -378,7 +378,7 @@ nvme_rdma_find_get_device(struct rdma_cm_id
> *cm_id)
>   }
> 
>   ndev->num_inline_segments =
> min(NVME_RDMA_MAX_INLINE_SEGMENTS,
> - ndev->dev->attrs.max_sge - 1);
> + ndev->dev->attrs.max_send_sge - 1);
>   list_add(>entry, _list);
>  out_unlock:
>   mutex_unlock(_list_mutex);
> diff --git a/drivers/nvme/target/rdma.c b/drivers/nvme/target/rdma.c
> index 86121a7a19b2..8c30ac7d8078 100644
> --- a/drivers/nvme/target/rdma.c
> +++ b/drivers/nvme/target/rdma.c
> @@ -891,7 +891,7 @@ nvmet_rdma_find_get_device(struct rdma_cm_id
> *cm_id)
> 
>   inline_page_count = num_pages(port->inline_data_size);
>   inline_sge_count = max(cm_id->device->attrs.max_sge_rd,
> - cm_id->device->attrs.max_sge) - 1;
> + cm_id->device->attrs.max_send_sge) - 1;

This should actually be max_recv_sge.

Thanks!

Steve.

Re: [PATCH] iw_cxgb4: add INFINIBAND_ADDR_TRANS dependency

[Steve Wise]

On 5/30/2018 5:25 PM, Jason Gunthorpe wrote:
> On Wed, May 30, 2018 at 05:10:35PM -0500, Steve Wise wrote:
>>
>> On 5/30/2018 5:04 PM, Jason Gunthorpe wrote:
>>> On Wed, May 30, 2018 at 11:58:18PM +0200, Arnd Bergmann wrote:
>>>> The newly added fill_res_ep_entry function fails to link if
>>>> CONFIG_INFINIBAND_ADDR_TRANS is not set:
>>>>
>>>> drivers/infiniband/hw/cxgb4/restrack.o: In function `fill_res_ep_entry':
>>>> restrack.c:(.text+0x3cc): undefined reference to `rdma_res_to_id'
>>>> restrack.c:(.text+0x3d0): undefined reference to `rdma_iw_cm_id'
>>>>
>>>> This adds a Kconfig dependency for the driver.
>>>>
>>>> Fixes: 116aeb887371 ("iw_cxgb4: provide detailed provider-specific CM_ID 
>>>> information")
>>>> Signed-off-by: Arnd Bergmann 
>>>>  drivers/infiniband/hw/cxgb4/Kconfig | 1 +
>>>>  1 file changed, 1 insertion(+)
>>> Oh, I think we need to solve this with maybe a header fill null stub
>>> instead..
>>>
>>> We don't want to disable drivers just because a user interface is
>>> disabled.
>>>
>> Why does CONFIG_INFINIBAND_ADDR_TRANS disable building rdma_cm.ko? That
>> is not correct.
> That seems like a reasonable thing to do..
rdma_ucm.ko is for usermode users, rdma_cm.ko is for kernel users, and
is required for iwarp drivers.  It seems rdma_cm.ko is not being
compiled if ADDR_TRANS is not set.


> But why does it break the compile? Those functions are in cma.c...
>
> Jason

Indeed, why. :)

Perhaps this line is wrong in drivers/infiniband/core/Makefile:

infiniband-$(CONFIG_INFINIBAND_ADDR_TRANS)  := rdma_cm.o

I don't understand the rules in that Makefile. 

Steve.

Re: [PATCH] iw_cxgb4: add INFINIBAND_ADDR_TRANS dependency

[Steve Wise]

On 5/30/2018 5:25 PM, Jason Gunthorpe wrote:
> On Wed, May 30, 2018 at 05:10:35PM -0500, Steve Wise wrote:
>>
>> On 5/30/2018 5:04 PM, Jason Gunthorpe wrote:
>>> On Wed, May 30, 2018 at 11:58:18PM +0200, Arnd Bergmann wrote:
>>>> The newly added fill_res_ep_entry function fails to link if
>>>> CONFIG_INFINIBAND_ADDR_TRANS is not set:
>>>>
>>>> drivers/infiniband/hw/cxgb4/restrack.o: In function `fill_res_ep_entry':
>>>> restrack.c:(.text+0x3cc): undefined reference to `rdma_res_to_id'
>>>> restrack.c:(.text+0x3d0): undefined reference to `rdma_iw_cm_id'
>>>>
>>>> This adds a Kconfig dependency for the driver.
>>>>
>>>> Fixes: 116aeb887371 ("iw_cxgb4: provide detailed provider-specific CM_ID 
>>>> information")
>>>> Signed-off-by: Arnd Bergmann 
>>>>  drivers/infiniband/hw/cxgb4/Kconfig | 1 +
>>>>  1 file changed, 1 insertion(+)
>>> Oh, I think we need to solve this with maybe a header fill null stub
>>> instead..
>>>
>>> We don't want to disable drivers just because a user interface is
>>> disabled.
>>>
>> Why does CONFIG_INFINIBAND_ADDR_TRANS disable building rdma_cm.ko? That
>> is not correct.
> That seems like a reasonable thing to do..
rdma_ucm.ko is for usermode users, rdma_cm.ko is for kernel users, and
is required for iwarp drivers.  It seems rdma_cm.ko is not being
compiled if ADDR_TRANS is not set.


> But why does it break the compile? Those functions are in cma.c...
>
> Jason

Indeed, why. :)

Perhaps this line is wrong in drivers/infiniband/core/Makefile:

infiniband-$(CONFIG_INFINIBAND_ADDR_TRANS)  := rdma_cm.o

I don't understand the rules in that Makefile. 

Steve.

Re: [PATCH] iw_cxgb4: add INFINIBAND_ADDR_TRANS dependency

[Steve Wise]

On 5/30/2018 5:04 PM, Jason Gunthorpe wrote:
> On Wed, May 30, 2018 at 11:58:18PM +0200, Arnd Bergmann wrote:
>> The newly added fill_res_ep_entry function fails to link if
>> CONFIG_INFINIBAND_ADDR_TRANS is not set:
>>
>> drivers/infiniband/hw/cxgb4/restrack.o: In function `fill_res_ep_entry':
>> restrack.c:(.text+0x3cc): undefined reference to `rdma_res_to_id'
>> restrack.c:(.text+0x3d0): undefined reference to `rdma_iw_cm_id'
>>
>> This adds a Kconfig dependency for the driver.
>>
>> Fixes: 116aeb887371 ("iw_cxgb4: provide detailed provider-specific CM_ID 
>> information")
>> Signed-off-by: Arnd Bergmann 
>> ---
>>  drivers/infiniband/hw/cxgb4/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
> Oh, I think we need to solve this with maybe a header fill null stub
> instead..
>
> We don't want to disable drivers just because a user interface is
> disabled.
>

Why does CONFIG_INFINIBAND_ADDR_TRANS disable building rdma_cm.ko?  That
is not correct.

Steve.

Re: [PATCH] iw_cxgb4: add INFINIBAND_ADDR_TRANS dependency

[Steve Wise]

On 5/30/2018 5:04 PM, Jason Gunthorpe wrote:
> On Wed, May 30, 2018 at 11:58:18PM +0200, Arnd Bergmann wrote:
>> The newly added fill_res_ep_entry function fails to link if
>> CONFIG_INFINIBAND_ADDR_TRANS is not set:
>>
>> drivers/infiniband/hw/cxgb4/restrack.o: In function `fill_res_ep_entry':
>> restrack.c:(.text+0x3cc): undefined reference to `rdma_res_to_id'
>> restrack.c:(.text+0x3d0): undefined reference to `rdma_iw_cm_id'
>>
>> This adds a Kconfig dependency for the driver.
>>
>> Fixes: 116aeb887371 ("iw_cxgb4: provide detailed provider-specific CM_ID 
>> information")
>> Signed-off-by: Arnd Bergmann 
>> ---
>>  drivers/infiniband/hw/cxgb4/Kconfig | 1 +
>>  1 file changed, 1 insertion(+)
> Oh, I think we need to solve this with maybe a header fill null stub
> instead..
>
> We don't want to disable drivers just because a user interface is
> disabled.
>

Why does CONFIG_INFINIBAND_ADDR_TRANS disable building rdma_cm.ko?  That
is not correct.

Steve.

Re: linux-next: build warning after merge of the rdma tree

[Steve Wise]

On 5/14/2018 1:09 PM, Steve Wise wrote:
>
> On 5/14/2018 1:03 PM, Jason Gunthorpe wrote:
>> On Mon, May 07, 2018 at 09:44:54AM +1000, Stephen Rothwell wrote:
>>> Hi all,
>>>
>>> After merging the rdma tree, today's linux-next build (x86_64
>>> allmodconfig) produced this warning:
>>>
>>> drivers/infiniband/hw/cxgb4/restrack.c: In function 'fill_res_qp_entry':
>>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'last_rq_idx' may be 
>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>>   ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:180:20: note: 'last_rq_idx' was 
>>> declared here
>>>   u16 first_rq_idx, last_rq_idx;
>>> ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'first_rq_idx' may 
>>> be used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>>   ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:180:6: note: 'first_rq_idx' was 
>>> declared here
>>>   u16 first_rq_idx, last_rq_idx;
>>>   ^~~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'last_sq_idx' may be 
>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (fill_swsqes(msg, , first_sq_idx, fsp, last_sq_idx, lsp))
>>>   ^
>>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'first_sq_idx' may 
>>> be used uninitialized in this function [-Wmaybe-uninitialized]
>>>
>>> Introduced by commit
>>>
>>>   056f9c7f39bf ("iw_cxgb4: dump detailed driver-specific QP information")
>>>
>> SteveW, is there a patch to fix this that I'm missing?
>>
>> Thanks,
>> Jason
> Hey Jason, 
>
> I'll send you one shortly. 
>
> Steve.
>

Posted:  https://patchwork.kernel.org/patch/10399081/

Steve.

Re: linux-next: build warning after merge of the rdma tree

[Steve Wise]

On 5/14/2018 1:09 PM, Steve Wise wrote:
>
> On 5/14/2018 1:03 PM, Jason Gunthorpe wrote:
>> On Mon, May 07, 2018 at 09:44:54AM +1000, Stephen Rothwell wrote:
>>> Hi all,
>>>
>>> After merging the rdma tree, today's linux-next build (x86_64
>>> allmodconfig) produced this warning:
>>>
>>> drivers/infiniband/hw/cxgb4/restrack.c: In function 'fill_res_qp_entry':
>>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'last_rq_idx' may be 
>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>>   ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:180:20: note: 'last_rq_idx' was 
>>> declared here
>>>   u16 first_rq_idx, last_rq_idx;
>>> ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'first_rq_idx' may 
>>> be used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>>   ^~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:180:6: note: 'first_rq_idx' was 
>>> declared here
>>>   u16 first_rq_idx, last_rq_idx;
>>>   ^~~~
>>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'last_sq_idx' may be 
>>> used uninitialized in this function [-Wmaybe-uninitialized]
>>>   if (fill_swsqes(msg, , first_sq_idx, fsp, last_sq_idx, lsp))
>>>   ^
>>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'first_sq_idx' may 
>>> be used uninitialized in this function [-Wmaybe-uninitialized]
>>>
>>> Introduced by commit
>>>
>>>   056f9c7f39bf ("iw_cxgb4: dump detailed driver-specific QP information")
>>>
>> SteveW, is there a patch to fix this that I'm missing?
>>
>> Thanks,
>> Jason
> Hey Jason, 
>
> I'll send you one shortly. 
>
> Steve.
>

Posted:  https://patchwork.kernel.org/patch/10399081/

Steve.

Re: linux-next: build warning after merge of the rdma tree

[Steve Wise]

On 5/14/2018 1:03 PM, Jason Gunthorpe wrote:
> On Mon, May 07, 2018 at 09:44:54AM +1000, Stephen Rothwell wrote:
>> Hi all,
>>
>> After merging the rdma tree, today's linux-next build (x86_64
>> allmodconfig) produced this warning:
>>
>> drivers/infiniband/hw/cxgb4/restrack.c: In function 'fill_res_qp_entry':
>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'last_rq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>   ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:180:20: note: 'last_rq_idx' was 
>> declared here
>>   u16 first_rq_idx, last_rq_idx;
>> ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'first_rq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>   ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:180:6: note: 'first_rq_idx' was 
>> declared here
>>   u16 first_rq_idx, last_rq_idx;
>>   ^~~~
>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'last_sq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (fill_swsqes(msg, , first_sq_idx, fsp, last_sq_idx, lsp))
>>   ^
>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'first_sq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>
>> Introduced by commit
>>
>>   056f9c7f39bf ("iw_cxgb4: dump detailed driver-specific QP information")
>>
> SteveW, is there a patch to fix this that I'm missing?
>
> Thanks,
> Jason

Hey Jason, 

I'll send you one shortly. 

Steve.

Re: linux-next: build warning after merge of the rdma tree

[Steve Wise]

On 5/14/2018 1:03 PM, Jason Gunthorpe wrote:
> On Mon, May 07, 2018 at 09:44:54AM +1000, Stephen Rothwell wrote:
>> Hi all,
>>
>> After merging the rdma tree, today's linux-next build (x86_64
>> allmodconfig) produced this warning:
>>
>> drivers/infiniband/hw/cxgb4/restrack.c: In function 'fill_res_qp_entry':
>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'last_rq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>   ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:180:20: note: 'last_rq_idx' was 
>> declared here
>>   u16 first_rq_idx, last_rq_idx;
>> ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:140:6: warning: 'first_rq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (rdma_nl_put_driver_u32(msg, "idx", idx))
>>   ^~~
>> drivers/infiniband/hw/cxgb4/restrack.c:180:6: note: 'first_rq_idx' was 
>> declared here
>>   u16 first_rq_idx, last_rq_idx;
>>   ^~~~
>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'last_sq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>   if (fill_swsqes(msg, , first_sq_idx, fsp, last_sq_idx, lsp))
>>   ^
>> drivers/infiniband/hw/cxgb4/restrack.c:228:6: warning: 'first_sq_idx' may be 
>> used uninitialized in this function [-Wmaybe-uninitialized]
>>
>> Introduced by commit
>>
>>   056f9c7f39bf ("iw_cxgb4: dump detailed driver-specific QP information")
>>
> SteveW, is there a patch to fix this that I'm missing?
>
> Thanks,
> Jason

Hey Jason, 

I'll send you one shortly. 

Steve.

RE: [PATCH] IB/cxgb4: use skb_put_zero()/__skb_put_zero

[Steve Wise]

> -Original Message-
> From: YueHaibing <yuehaib...@huawei.com>
> Sent: Saturday, April 28, 2018 2:31 AM
> To: sw...@chelsio.com; dledf...@redhat.com; j...@ziepe.ca;
> mo...@mellanox.com
> Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing
> <yuehaib...@huawei.com>
> Subject: [PATCH] IB/cxgb4: use skb_put_zero()/__skb_put_zero
> 
> Use the recently introduced helper to replace the pattern of
> skb_put_zero/__skb_put() && memset().
> 
> Signed-off-by: YueHaibing <yuehaib...@huawei.com>

Looks ok.

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] IB/cxgb4: use skb_put_zero()/__skb_put_zero

[Steve Wise]

> -Original Message-
> From: YueHaibing 
> Sent: Saturday, April 28, 2018 2:31 AM
> To: sw...@chelsio.com; dledf...@redhat.com; j...@ziepe.ca;
> mo...@mellanox.com
> Cc: linux-r...@vger.kernel.org; linux-kernel@vger.kernel.org; YueHaibing
> 
> Subject: [PATCH] IB/cxgb4: use skb_put_zero()/__skb_put_zero
> 
> Use the recently introduced helper to replace the pattern of
> skb_put_zero/__skb_put() && memset().
> 
> Signed-off-by: YueHaibing 

Looks ok.

Reviewed-by: Steve Wise 

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

On 3/22/2018 9:52 AM, Sinan Kaya wrote:
> On 3/22/2018 9:40 AM, Steve Wise wrote:
>> I think all these iw_cxgb4 changes should be reverted until we really have a
>> plan for multi-platform that works.  
> I know you are looking to have support for PowerPC. 
>
> Isn't this a PowerPC problem? Why penalize other architectures?
>
> Do you see anything wrong with the code itself?

I worry it breaks PPC.

> I started this thread with the PowerPC develoeprs on your request.
> "RFC on writel and writel_relaxed"
>
> They are looking into adding the relaxed API support. Support can come
> in later. Why block this change now?
>
> b...@kernel.crashing.org:
> "I've been wanting to implement the relaxed accessors for a while but
> was battling with this to try to also better support WC, and due to
> other commitments, this somewhat fell down the cracks."
>
> I have seen four different responses on this thread. Since this is an
> architecture change it will take a while to get the semantics right.
> It won't happen in the new few days.

I appreciate you doing this.

Re: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

On 3/22/2018 9:52 AM, Sinan Kaya wrote:
> On 3/22/2018 9:40 AM, Steve Wise wrote:
>> I think all these iw_cxgb4 changes should be reverted until we really have a
>> plan for multi-platform that works.  
> I know you are looking to have support for PowerPC. 
>
> Isn't this a PowerPC problem? Why penalize other architectures?
>
> Do you see anything wrong with the code itself?

I worry it breaks PPC.

> I started this thread with the PowerPC develoeprs on your request.
> "RFC on writel and writel_relaxed"
>
> They are looking into adding the relaxed API support. Support can come
> in later. Why block this change now?
>
> b...@kernel.crashing.org:
> "I've been wanting to implement the relaxed accessors for a while but
> was battling with this to try to also better support WC, and due to
> other commitments, this somewhat fell down the cracks."
>
> I have seen four different responses on this thread. Since this is an
> architecture change it will take a while to get the semantics right.
> It won't happen in the new few days.

I appreciate you doing this.

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 2018-03-22 08:24, ok...@codeaurora.org wrote:
> > On 2018-03-22 02:44, kbuild test robot wrote:
> >> Hi Sinan,
> >>
> >> Thank you for the patch! Yet something to improve:
> >>
> >> [auto build test ERROR on linus/master]
> >> [also build test ERROR on v4.16-rc6 next-20180321]
> >> [if your patch is applied to the wrong git tree, please drop us a note
> >> to help improve the system]
> >>
> >> url:
> >> https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-
> duplicate-barriers-on-weakly-ordered-archs/20180321-091659
> >> config: xtensa-allyesconfig (attached as .config)
> >> compiler: xtensa-linux-gcc (GCC) 7.2.0
> >> reproduce:
> >> wget
> >> https://raw.githubusercontent.com/intel/lkp-
> tests/master/sbin/make.cross
> >> -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> # save the attached .config to linux build tree
> >> make.cross ARCH=xtensa
> >>
> >
> > Jason,
> >
> > Can you remove the writeq change if it is too late for me to fix?
> >
> > This is an infrastructural issue on xtensa arch.
> >
> > Probably, it won't get fixed today.
> 
> AFAIS, even writeq won't compile on this arch. I started questioning
> this build test.
> 

I think all these iw_cxgb4 changes should be reverted until we really have a
plan for multi-platform that works.  

> 
> >
> > Sinan
> >
> >
> >> All errors (new ones prefixed by >>):
> >>
> >>In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
> >> from drivers/infiniband/hw/cxgb4/device.c:40:
> >>drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
>  drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration
>  of function 'writeq_relaxed'; did you mean 'writel_relaxed'?
>  [-Werror=implicit-function-declaration]
> >>   writeq_relaxed(*src, dst);
> >>   ^~
> >>   writel_relaxed
> >>cc1: some warnings being treated as errors
> >>
> >> vim +460 drivers/infiniband/hw/cxgb4/t4.h
> >>
> >>450
> >>451 /* This function copies 64 byte coalesced work request to
> >> memory
> >>452  * mapped BAR2 space. For coalesced WRs, the SGE fetches
> data
> >>453  * from the FIFO instead of from Host.
> >>454  */
> >>455 static inline void pio_copy(u64 __iomem *dst, u64 *src)
> >>456 {
> >>457 int count = 8;
> >>458
> >>459 while (count) {
> >>  > 460 writeq_relaxed(*src, dst);
> >>461 src++;
> >>462 dst++;
> >>463 count--;
> >>464 }
> >>465 }
> >>466
> >>
> >> ---
> >> 0-DAY kernel test infrastructureOpen Source Technology
> >> Center
> >> https://lists.01.org/pipermail/kbuild-all   Intel
> >> Corporation

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 2018-03-22 08:24, ok...@codeaurora.org wrote:
> > On 2018-03-22 02:44, kbuild test robot wrote:
> >> Hi Sinan,
> >>
> >> Thank you for the patch! Yet something to improve:
> >>
> >> [auto build test ERROR on linus/master]
> >> [also build test ERROR on v4.16-rc6 next-20180321]
> >> [if your patch is applied to the wrong git tree, please drop us a note
> >> to help improve the system]
> >>
> >> url:
> >> https://github.com/0day-ci/linux/commits/Sinan-Kaya/ib-Eliminate-
> duplicate-barriers-on-weakly-ordered-archs/20180321-091659
> >> config: xtensa-allyesconfig (attached as .config)
> >> compiler: xtensa-linux-gcc (GCC) 7.2.0
> >> reproduce:
> >> wget
> >> https://raw.githubusercontent.com/intel/lkp-
> tests/master/sbin/make.cross
> >> -O ~/bin/make.cross
> >> chmod +x ~/bin/make.cross
> >> # save the attached .config to linux build tree
> >> make.cross ARCH=xtensa
> >>
> >
> > Jason,
> >
> > Can you remove the writeq change if it is too late for me to fix?
> >
> > This is an infrastructural issue on xtensa arch.
> >
> > Probably, it won't get fixed today.
> 
> AFAIS, even writeq won't compile on this arch. I started questioning
> this build test.
> 

I think all these iw_cxgb4 changes should be reverted until we really have a
plan for multi-platform that works.  

> 
> >
> > Sinan
> >
> >
> >> All errors (new ones prefixed by >>):
> >>
> >>In file included from drivers/infiniband/hw/cxgb4/iw_cxgb4.h:73:0,
> >> from drivers/infiniband/hw/cxgb4/device.c:40:
> >>drivers/infiniband/hw/cxgb4/t4.h: In function 'pio_copy':
>  drivers/infiniband/hw/cxgb4/t4.h:460:3: error: implicit declaration
>  of function 'writeq_relaxed'; did you mean 'writel_relaxed'?
>  [-Werror=implicit-function-declaration]
> >>   writeq_relaxed(*src, dst);
> >>   ^~
> >>   writel_relaxed
> >>cc1: some warnings being treated as errors
> >>
> >> vim +460 drivers/infiniband/hw/cxgb4/t4.h
> >>
> >>450
> >>451 /* This function copies 64 byte coalesced work request to
> >> memory
> >>452  * mapped BAR2 space. For coalesced WRs, the SGE fetches
> data
> >>453  * from the FIFO instead of from Host.
> >>454  */
> >>455 static inline void pio_copy(u64 __iomem *dst, u64 *src)
> >>456 {
> >>457 int count = 8;
> >>458
> >>459 while (count) {
> >>  > 460 writeq_relaxed(*src, dst);
> >>461 src++;
> >>462 dst++;
> >>463 count--;
> >>464 }
> >>465 }
> >>466
> >>
> >> ---
> >> 0-DAY kernel test infrastructureOpen Source Technology
> >> Center
> >> https://lists.01.org/pipermail/kbuild-all   Intel
> >> Corporation

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> > On Mon, Mar 19, 2018 at 10:47:46PM -0400, Sinan Kaya wrote:
> > > Code includes wmb() followed by writel(). writel() already has a
barrier
> on
> > > some architectures like arm64.
> > >
> > > This ends up CPU observing two barriers back to back before executing
> > the
> > > register write.
> > >
> > > Since code already has an explicit barrier call, changing writel() to
> > > writel_relaxed().
> > >
> > > Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> > >  drivers/infiniband/hw/cxgb4/t4.h | 14 +++---
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> > b/drivers/infiniband/hw/cxgb4/t4.h
> > > index 8369c7c..6e5658a 100644
> > > +++ b/drivers/infiniband/hw/cxgb4/t4.h
> > > @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst,
> u64
> > *src)
> > >   int count = 8;
> > >
> > >   while (count) {
> > > - writeq(*src, dst);
> > > + writeq_relaxed(*src, dst);
> > >   src++;
> > >   dst++;
> > >   count--;
> >
> > This is another case where writes can be re-ordered.. IIRC dst is WC
> > BAR memory, so the NIC should tolerate re-ordering, but Steve will
> > have to ack this.
> >
> 
> Yes, this is WC BAR memory.  The goal is that pio_copy() will enable
write-
> combining this into a single 64B pci-e transaction.
> 


I'd like to see the PPC issue resolved...but

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> > On Mon, Mar 19, 2018 at 10:47:46PM -0400, Sinan Kaya wrote:
> > > Code includes wmb() followed by writel(). writel() already has a
barrier
> on
> > > some architectures like arm64.
> > >
> > > This ends up CPU observing two barriers back to back before executing
> > the
> > > register write.
> > >
> > > Since code already has an explicit barrier call, changing writel() to
> > > writel_relaxed().
> > >
> > > Signed-off-by: Sinan Kaya 
> > >  drivers/infiniband/hw/cxgb4/t4.h | 14 +++---
> > >  1 file changed, 7 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> > b/drivers/infiniband/hw/cxgb4/t4.h
> > > index 8369c7c..6e5658a 100644
> > > +++ b/drivers/infiniband/hw/cxgb4/t4.h
> > > @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst,
> u64
> > *src)
> > >   int count = 8;
> > >
> > >   while (count) {
> > > - writeq(*src, dst);
> > > + writeq_relaxed(*src, dst);
> > >   src++;
> > >   dst++;
> > >   count--;
> >
> > This is another case where writes can be re-ordered.. IIRC dst is WC
> > BAR memory, so the NIC should tolerate re-ordering, but Steve will
> > have to ack this.
> >
> 
> Yes, this is WC BAR memory.  The goal is that pio_copy() will enable
write-
> combining this into a single 64B pci-e transaction.
> 


I'd like to see the PPC issue resolved...but

Acked-by: Steve Wise 

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> On Mon, Mar 19, 2018 at 10:47:46PM -0400, Sinan Kaya wrote:
> > Code includes wmb() followed by writel(). writel() already has a barrier
on
> > some architectures like arm64.
> >
> > This ends up CPU observing two barriers back to back before executing
> the
> > register write.
> >
> > Since code already has an explicit barrier call, changing writel() to
> > writel_relaxed().
> >
> > Signed-off-by: Sinan Kaya 
> >  drivers/infiniband/hw/cxgb4/t4.h | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> b/drivers/infiniband/hw/cxgb4/t4.h
> > index 8369c7c..6e5658a 100644
> > +++ b/drivers/infiniband/hw/cxgb4/t4.h
> > @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64
> *src)
> > int count = 8;
> >
> > while (count) {
> > -   writeq(*src, dst);
> > +   writeq_relaxed(*src, dst);
> > src++;
> > dst++;
> > count--;
> 
> This is another case where writes can be re-ordered.. IIRC dst is WC
> BAR memory, so the NIC should tolerate re-ordering, but Steve will
> have to ack this.
> 

Yes, this is WC BAR memory.  The goal is that pio_copy() will enable
write-combining this into a single 64B pci-e transaction.

RE: [PATCH v4 4/6] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> On Mon, Mar 19, 2018 at 10:47:46PM -0400, Sinan Kaya wrote:
> > Code includes wmb() followed by writel(). writel() already has a barrier
on
> > some architectures like arm64.
> >
> > This ends up CPU observing two barriers back to back before executing
> the
> > register write.
> >
> > Since code already has an explicit barrier call, changing writel() to
> > writel_relaxed().
> >
> > Signed-off-by: Sinan Kaya 
> >  drivers/infiniband/hw/cxgb4/t4.h | 14 +++---
> >  1 file changed, 7 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> b/drivers/infiniband/hw/cxgb4/t4.h
> > index 8369c7c..6e5658a 100644
> > +++ b/drivers/infiniband/hw/cxgb4/t4.h
> > @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64
> *src)
> > int count = 8;
> >
> > while (count) {
> > -   writeq(*src, dst);
> > +   writeq_relaxed(*src, dst);
> > src++;
> > dst++;
> > count--;
> 
> This is another case where writes can be re-ordered.. IIRC dst is WC
> BAR memory, so the NIC should tolerate re-ordering, but Steve will
> have to ack this.
> 

Yes, this is WC BAR memory.  The goal is that pio_copy() will enable
write-combining this into a single 64B pci-e transaction.

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 3/17/2018 12:03 AM, Sinan Kaya wrote:
> > On 3/16/2018 11:40 PM, Sinan Kaya wrote:
> >> I'll change writel_relaxed() with __raw_writel() in the series like you
> suggested
> >> and also look at your other comments.
> >
> > I spoke too soon.
> >
> > Now that I realized, code needs to follow one of the following patterns
> for correctness
> >
> > 1)
> > wmb()
> > writel()/writel_relaxed()
> >
> > or
> >
> > 2)
> > wmb()
> > __raw_wrltel()
> > mmiowb()
> >
> > but definitely not
> >
> > wmb()
> > __raw_wrltel()
> >
> > Since #1 == #2, I'll stick to my current implementation of writel_relaxed()
> >
> > Changing writel() to writel_relaxed() or __raw_writel() isn't enough.
> PowerPC needs mmiowb()
> > for correctness. ARM's mmiowb() implementation is empty.
> >
> > So, there is no one size fits all solution with the current state of 
> > affairs.
> >
> >
> 
> I think I finally got what you mean.
> 
> Code seems to have
> 
> wmb()
> writel()/writeq()
> wmb()
> 
> this can be safely replaced with
> 
> wmb()
> __raw_writel()/__raw_writeq()
> wmb()
> 
> This will work on all arches. Below is the new version. Let me know if this is
> OK.
> 
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64
> *src)
> int count = 8;
> 
> while (count) {
> -   writeq(*src, dst);
> +   __raw_writeq(*src, dst);
> src++;
> dst++;
> count--;
> @@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq,
> u16 inc, union t4_wr *wqe)
>  (u64 *)wqe);
> } else {
> pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> -   writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> -  wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> +   __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> +wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> }
> 
> /* Flush user doorbell area writes. */
> wmb();
> return;
> }
> -   writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> +   __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> +   mmiowmb()
>  }
> 
>  static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
> @@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq,
> u16 inc,
>  (void *)wqe);
> } else {
> pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx);
> -   writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> -  wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> +   __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> +wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> }
> 
> /* Flush user doorbell area writes. */
> wmb();
> return;
> }
> -   writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> +   __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> +   mmiowmb();
>  }
> 
> 

Yes, this is what chelsio recommended to me.  

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 3/17/2018 12:03 AM, Sinan Kaya wrote:
> > On 3/16/2018 11:40 PM, Sinan Kaya wrote:
> >> I'll change writel_relaxed() with __raw_writel() in the series like you
> suggested
> >> and also look at your other comments.
> >
> > I spoke too soon.
> >
> > Now that I realized, code needs to follow one of the following patterns
> for correctness
> >
> > 1)
> > wmb()
> > writel()/writel_relaxed()
> >
> > or
> >
> > 2)
> > wmb()
> > __raw_wrltel()
> > mmiowb()
> >
> > but definitely not
> >
> > wmb()
> > __raw_wrltel()
> >
> > Since #1 == #2, I'll stick to my current implementation of writel_relaxed()
> >
> > Changing writel() to writel_relaxed() or __raw_writel() isn't enough.
> PowerPC needs mmiowb()
> > for correctness. ARM's mmiowb() implementation is empty.
> >
> > So, there is no one size fits all solution with the current state of 
> > affairs.
> >
> >
> 
> I think I finally got what you mean.
> 
> Code seems to have
> 
> wmb()
> writel()/writeq()
> wmb()
> 
> this can be safely replaced with
> 
> wmb()
> __raw_writel()/__raw_writeq()
> wmb()
> 
> This will work on all arches. Below is the new version. Let me know if this is
> OK.
> 
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -457,7 +457,7 @@ static inline void pio_copy(u64 __iomem *dst, u64
> *src)
> int count = 8;
> 
> while (count) {
> -   writeq(*src, dst);
> +   __raw_writeq(*src, dst);
> src++;
> dst++;
> count--;
> @@ -477,15 +477,16 @@ static inline void t4_ring_sq_db(struct t4_wq *wq,
> u16 inc, union t4_wr *wqe)
>  (u64 *)wqe);
> } else {
> pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> -   writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> -  wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> +   __raw_writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> +wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> }
> 
> /* Flush user doorbell area writes. */
> wmb();
> return;
> }
> -   writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> +   __raw_writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> +   mmiowmb()
>  }
> 
>  static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
> @@ -502,15 +503,16 @@ static inline void t4_ring_rq_db(struct t4_wq *wq,
> u16 inc,
>  (void *)wqe);
> } else {
> pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx);
> -   writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> -  wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> +   __raw_writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> +wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> }
> 
> /* Flush user doorbell area writes. */
> wmb();
> return;
> }
> -   writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> +   __raw_writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> +   mmiowmb();
>  }
> 
> 

Yes, this is what chelsio recommended to me.  

Reviewed-by: Steve Wise 

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 3/16/2018 5:05 PM, Steve Wise wrote:
> >> Code includes wmb() followed by writel(). writel() already has a barrier
> > on
> >> some architectures like arm64.
> >>
> >> This ends up CPU observing two barriers back to back before executing
> the
> >> register write.
> >>
> >> Since code already has an explicit barrier call, changing writel() to
> >> writel_relaxed().
> >>
> >> Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> >
> > NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is just
> > writeX().
> >
> > I was just looking at this with Chelsio developers, and they said the
> > writeX() should be replaced with __raw_writeX(), not writeX_relaxed(), to
> > get rid of the extra barrier for all architectures.
> 
> OK. I can do that but isn't the problem at PowerPC adaptation?
> 
> /*
>  * We don't do relaxed operations yet, at least not with this semantic
>  */
> #define readb_relaxed(addr)   readb(addr)
> #define readw_relaxed(addr)   readw(addr)
> #define readl_relaxed(addr)   readl(addr)
> #define readq_relaxed(addr)   readq(addr)
> #define writeb_relaxed(v, addr)   writeb(v, addr)
> #define writew_relaxed(v, addr)   writew(v, addr)
> #define writel_relaxed(v, addr)   writel(v, addr)
> #define writeq_relaxed(v, addr)   writeq(v, addr)
> 
> Why don't we fix the PowerPC's relaxed operators? Is that a bigger task?

I don't know the answer, but perhaps the proper fix is to correctly implement 
these for PPC?


> 
> >From API perspective both __raw_writeX() and writeX_relaxed() are
> correct.
> It is just PowerPC doesn't seem the follow the definition yet.

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On 3/16/2018 5:05 PM, Steve Wise wrote:
> >> Code includes wmb() followed by writel(). writel() already has a barrier
> > on
> >> some architectures like arm64.
> >>
> >> This ends up CPU observing two barriers back to back before executing
> the
> >> register write.
> >>
> >> Since code already has an explicit barrier call, changing writel() to
> >> writel_relaxed().
> >>
> >> Signed-off-by: Sinan Kaya 
> >
> > NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is just
> > writeX().
> >
> > I was just looking at this with Chelsio developers, and they said the
> > writeX() should be replaced with __raw_writeX(), not writeX_relaxed(), to
> > get rid of the extra barrier for all architectures.
> 
> OK. I can do that but isn't the problem at PowerPC adaptation?
> 
> /*
>  * We don't do relaxed operations yet, at least not with this semantic
>  */
> #define readb_relaxed(addr)   readb(addr)
> #define readw_relaxed(addr)   readw(addr)
> #define readl_relaxed(addr)   readl(addr)
> #define readq_relaxed(addr)   readq(addr)
> #define writeb_relaxed(v, addr)   writeb(v, addr)
> #define writew_relaxed(v, addr)   writew(v, addr)
> #define writel_relaxed(v, addr)   writel(v, addr)
> #define writeq_relaxed(v, addr)   writeq(v, addr)
> 
> Why don't we fix the PowerPC's relaxed operators? Is that a bigger task?

I don't know the answer, but perhaps the proper fix is to correctly implement 
these for PPC?


> 
> >From API perspective both __raw_writeX() and writeX_relaxed() are
> correct.
> It is just PowerPC doesn't seem the follow the definition yet.

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On Fri, Mar 16, 2018 at 04:05:10PM -0500, Steve Wise wrote:
> > > Code includes wmb() followed by writel(). writel() already has a
barrier
> > on
> > > some architectures like arm64.
> > >
> > > This ends up CPU observing two barriers back to back before executing
> the
> > > register write.
> > >
> > > Since code already has an explicit barrier call, changing writel() to
> > > writel_relaxed().
> > >
> > > Signed-off-by: Sinan Kaya <ok...@codeaurora.org>
> >
> > NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is
just
> > writeX().
> 
> ?? Why is changing writex() to writeX() a NAK then?

Because I want it correct for PPC as well.

> 
> > I was just looking at this with Chelsio developers, and they said the
> > writeX() should be replaced with __raw_writeX(), not writeX_relaxed(),
to
> > get rid of the extra barrier for all architectures.
> 
> That doesn't seem semanticaly sane.
> 
> __raw_writeX() should not appear in driver code, IMHO. Only the arch
> code can know what the exact semantics of that accessor are..
> 
> If ppc can't use writel_relaxed to optimize then we probably need yet
> another io accessor semantic defined :(


Anybody understand why the PPC implementation of writeX_relaxed() isn't
relaxed?


Steve.

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> 
> On Fri, Mar 16, 2018 at 04:05:10PM -0500, Steve Wise wrote:
> > > Code includes wmb() followed by writel(). writel() already has a
barrier
> > on
> > > some architectures like arm64.
> > >
> > > This ends up CPU observing two barriers back to back before executing
> the
> > > register write.
> > >
> > > Since code already has an explicit barrier call, changing writel() to
> > > writel_relaxed().
> > >
> > > Signed-off-by: Sinan Kaya 
> >
> > NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is
just
> > writeX().
> 
> ?? Why is changing writex() to writeX() a NAK then?

Because I want it correct for PPC as well.

> 
> > I was just looking at this with Chelsio developers, and they said the
> > writeX() should be replaced with __raw_writeX(), not writeX_relaxed(),
to
> > get rid of the extra barrier for all architectures.
> 
> That doesn't seem semanticaly sane.
> 
> __raw_writeX() should not appear in driver code, IMHO. Only the arch
> code can know what the exact semantics of that accessor are..
> 
> If ppc can't use writel_relaxed to optimize then we probably need yet
> another io accessor semantic defined :(


Anybody understand why the PPC implementation of writeX_relaxed() isn't
relaxed?


Steve.

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> Code includes wmb() followed by writel(). writel() already has a barrier
on
> some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing the
> register write.
> 
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
> 
> Signed-off-by: Sinan Kaya 

NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is just
writeX().  

I was just looking at this with Chelsio developers, and they said the
writeX() should be replaced with __raw_writeX(), not writeX_relaxed(), to
get rid of the extra barrier for all architectures.

Also, t4.h:pio_copy() needs to  use __raw_writeq() to enable the write
combining fastpath for ARM and PowerPC.  The code as it stands doesn't
achieve any write combining on PowerPC at least.  

And the writel()s at the end of the ring functions (the non bar2 udb path)
needs a mmiowb() afterwards if you're going to use __raw_writeX() there.
However that path is only used for very old hardware (T4), so I wouldn't
worry about them. 

Steve.


> ---
>  drivers/infiniband/hw/cxgb4/t4.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> b/drivers/infiniband/hw/cxgb4/t4.h
> index 8369c7c..7a48c9e 100644
> --- a/drivers/infiniband/hw/cxgb4/t4.h
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -477,15 +477,15 @@ static inline void t4_ring_sq_db(struct t4_wq *wq,
> u16 inc, union t4_wr *wqe)
>(u64 *)wqe);
>   } else {
>   pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> -wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> + writel_relaxed(PIDX_T5_V(inc) | QID_V(wq-
> >sq.bar2_qid),
> +wq->sq.bar2_va +
> SGE_UDB_KDOORBELL);
>   }
> 
>   /* Flush user doorbell area writes. */
>   wmb();
>   return;
>   }
> - writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> + writel_relaxed(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
>  }
> 
>  static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
> @@ -502,15 +502,15 @@ static inline void t4_ring_rq_db(struct t4_wq *wq,
> u16 inc,
>(void *)wqe);
>   } else {
>   pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx);
> - writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> -wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> + writel_relaxed(PIDX_T5_V(inc) | QID_V(wq-
> >rq.bar2_qid),
> +wq->rq.bar2_va +
> SGE_UDB_KDOORBELL);
>   }
> 
>   /* Flush user doorbell area writes. */
>   wmb();
>   return;
>   }
> - writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> + writel_relaxed(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
>  }
> 
>  static inline int t4_wq_in_error(struct t4_wq *wq)
> --
> 2.7.4

RE: [PATCH v3 18/18] infiniband: cxgb4: Eliminate duplicate barriers on weakly-ordered archs

[Steve Wise]

> Code includes wmb() followed by writel(). writel() already has a barrier
on
> some architectures like arm64.
> 
> This ends up CPU observing two barriers back to back before executing the
> register write.
> 
> Since code already has an explicit barrier call, changing writel() to
> writel_relaxed().
> 
> Signed-off-by: Sinan Kaya 

NAK - This isn't correct for PowerPC.  For PowerPC, writeX_relaxed() is just
writeX().  

I was just looking at this with Chelsio developers, and they said the
writeX() should be replaced with __raw_writeX(), not writeX_relaxed(), to
get rid of the extra barrier for all architectures.

Also, t4.h:pio_copy() needs to  use __raw_writeq() to enable the write
combining fastpath for ARM and PowerPC.  The code as it stands doesn't
achieve any write combining on PowerPC at least.  

And the writel()s at the end of the ring functions (the non bar2 udb path)
needs a mmiowb() afterwards if you're going to use __raw_writeX() there.
However that path is only used for very old hardware (T4), so I wouldn't
worry about them. 

Steve.


> ---
>  drivers/infiniband/hw/cxgb4/t4.h | 12 ++--
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/cxgb4/t4.h
> b/drivers/infiniband/hw/cxgb4/t4.h
> index 8369c7c..7a48c9e 100644
> --- a/drivers/infiniband/hw/cxgb4/t4.h
> +++ b/drivers/infiniband/hw/cxgb4/t4.h
> @@ -477,15 +477,15 @@ static inline void t4_ring_sq_db(struct t4_wq *wq,
> u16 inc, union t4_wr *wqe)
>(u64 *)wqe);
>   } else {
>   pr_debug("DB wq->sq.pidx = %d\n", wq->sq.pidx);
> - writel(PIDX_T5_V(inc) | QID_V(wq->sq.bar2_qid),
> -wq->sq.bar2_va + SGE_UDB_KDOORBELL);
> + writel_relaxed(PIDX_T5_V(inc) | QID_V(wq-
> >sq.bar2_qid),
> +wq->sq.bar2_va +
> SGE_UDB_KDOORBELL);
>   }
> 
>   /* Flush user doorbell area writes. */
>   wmb();
>   return;
>   }
> - writel(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
> + writel_relaxed(QID_V(wq->sq.qid) | PIDX_V(inc), wq->db);
>  }
> 
>  static inline void t4_ring_rq_db(struct t4_wq *wq, u16 inc,
> @@ -502,15 +502,15 @@ static inline void t4_ring_rq_db(struct t4_wq *wq,
> u16 inc,
>(void *)wqe);
>   } else {
>   pr_debug("DB wq->rq.pidx = %d\n", wq->rq.pidx);
> - writel(PIDX_T5_V(inc) | QID_V(wq->rq.bar2_qid),
> -wq->rq.bar2_va + SGE_UDB_KDOORBELL);
> + writel_relaxed(PIDX_T5_V(inc) | QID_V(wq-
> >rq.bar2_qid),
> +wq->rq.bar2_va +
> SGE_UDB_KDOORBELL);
>   }
> 
>   /* Flush user doorbell area writes. */
>   wmb();
>   return;
>   }
> - writel(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
> + writel_relaxed(QID_V(wq->rq.qid) | PIDX_V(inc), wq->db);
>  }
> 
>  static inline int t4_wq_in_error(struct t4_wq *wq)
> --
> 2.7.4

RE: [PATCH] IB/cxgb3: remove cxio_dbg.c

[Steve Wise]

> 
> cxio_dbg.c is uncompiled since commit 2b540355cd2f ("RDMA/cxgb3:
> cleanups")
> 10 years after, we could remove it.
> 
> Signed-off-by: Corentin Labbe <cla...@baylibre.com>

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] IB/cxgb3: remove cxio_dbg.c

[Steve Wise]

> 
> cxio_dbg.c is uncompiled since commit 2b540355cd2f ("RDMA/cxgb3:
> cleanups")
> 10 years after, we could remove it.
> 
> Signed-off-by: Corentin Labbe 

Acked-by: Steve Wise 

RE: [PATCH] infiniband: cxgb4: use ktime_get for timestamps

[Steve Wise]

> Subject: Re: [PATCH] infiniband: cxgb4: use ktime_get for timestamps
> 
> On Mon, Nov 27, 2017 at 12:44:53PM +0100, Arnd Bergmann wrote:
> > The debugfs file prints the difference between host timestamps as a
> > seconds/nanoseconds tuple, along with a 64-bit nanoseconds hardware
> > timestamp. The host time is read using getnstimeofday() which is
> > deprecated because of the y2038 overflow, and it suffers from time jumps
> > during settimeofday() and leap seconds.
> >
> > Converting to ktime_get_ts64() would solve those two, but I'm going
> > a little further here by changing to ktime_get() and printing 64-bit
> > nanoseconds on both host and hw timestamps.  This simplifies the code
> > further and makes the output easier to understand.
> >
> > The format of the debugfs file obviously changes here, but this should
> > only be read by humans and not scripts, so I assume it's fine.
> >
> > Signed-off-by: Arnd Bergmann <a...@arndb.de>
> >  drivers/infiniband/hw/cxgb4/device.c   | 34
+++---
> >  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  4 ++--
> >  drivers/infiniband/hw/cxgb4/qp.c   |  6 +++---
> >  drivers/infiniband/hw/cxgb4/t4.h   |  4 ++--
> >  4 files changed, 22 insertions(+), 26 deletions(-)
> 
> Steve? This changes the format of the debugfs files, so please ack it
> if it is Ok..
> 

I apologize for missing this patch initially.  I think this is fine.

Acked-by: Steve Wise <sw...@opengridcomputing.com>



---
This email has been checked for viruses by AVG.
http://www.avg.com

RE: [PATCH] infiniband: cxgb4: use ktime_get for timestamps

[Steve Wise]

> Subject: Re: [PATCH] infiniband: cxgb4: use ktime_get for timestamps
> 
> On Mon, Nov 27, 2017 at 12:44:53PM +0100, Arnd Bergmann wrote:
> > The debugfs file prints the difference between host timestamps as a
> > seconds/nanoseconds tuple, along with a 64-bit nanoseconds hardware
> > timestamp. The host time is read using getnstimeofday() which is
> > deprecated because of the y2038 overflow, and it suffers from time jumps
> > during settimeofday() and leap seconds.
> >
> > Converting to ktime_get_ts64() would solve those two, but I'm going
> > a little further here by changing to ktime_get() and printing 64-bit
> > nanoseconds on both host and hw timestamps.  This simplifies the code
> > further and makes the output easier to understand.
> >
> > The format of the debugfs file obviously changes here, but this should
> > only be read by humans and not scripts, so I assume it's fine.
> >
> > Signed-off-by: Arnd Bergmann 
> >  drivers/infiniband/hw/cxgb4/device.c   | 34
+++---
> >  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  4 ++--
> >  drivers/infiniband/hw/cxgb4/qp.c   |  6 +++---
> >  drivers/infiniband/hw/cxgb4/t4.h   |  4 ++--
> >  4 files changed, 22 insertions(+), 26 deletions(-)
> 
> Steve? This changes the format of the debugfs files, so please ack it
> if it is Ok..
> 

I apologize for missing this patch initially.  I think this is fine.

Acked-by: Steve Wise 



---
This email has been checked for viruses by AVG.
http://www.avg.com

RE: [PATCH] iw_cxgb4: make pointer reg_workq static

[Steve Wise]

> 
> From: Colin Ian King <colin.k...@canonical.com>
> 
> The pointer reg_workq is local to the source and does not need to be
> in global scope, so make it static.
> 
> Cleans up sparse warning:
> drivers/infiniband/hw/cxgb4/device.c:69:25: warning: symbol 'reg_workq'
> was not declared. Should it be static?
> 
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Looks correct.  This fixes a recent commit.

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>
Fixes: 1c8f1da5d851 ("iw_cxgb4: Fix possible circular dependency locking 
warning")

Thanks,

Steve.

RE: [PATCH] iw_cxgb4: make pointer reg_workq static

[Steve Wise]

> 
> From: Colin Ian King 
> 
> The pointer reg_workq is local to the source and does not need to be
> in global scope, so make it static.
> 
> Cleans up sparse warning:
> drivers/infiniband/hw/cxgb4/device.c:69:25: warning: symbol 'reg_workq'
> was not declared. Should it be static?
> 
> Signed-off-by: Colin Ian King 

Looks correct.  This fixes a recent commit.

Reviewed-by: Steve Wise 
Fixes: 1c8f1da5d851 ("iw_cxgb4: Fix possible circular dependency locking 
warning")

Thanks,

Steve.

RE: [PATCH] RDMA/cxgb3: Convert timers to use timer_setup()

[Steve Wise]

> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. Also removes an unused timer.
> 
> Cc: Steve Wise <sw...@chelsio.com>
> Cc: Doug Ledford <dledf...@redhat.com>
> Cc: Sean Hefty <sean.he...@intel.com>
> Cc: Hal Rosenstock <hal.rosenst...@gmail.com>
> Cc: linux-r...@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] RDMA/cxgb4: Convert timers to use timer_setup()

[Steve Wise]

> 
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. Also removes an unused timer and
> drops a redundant initialization.
> 
> Cc: Steve Wise <sw...@chelsio.com>
> Cc: Doug Ledford <dledf...@redhat.com>
> Cc: Sean Hefty <sean.he...@intel.com>
> Cc: Hal Rosenstock <hal.rosenst...@gmail.com>
> Cc: linux-r...@vger.kernel.org
> Signed-off-by: Kees Cook <keesc...@chromium.org>

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] RDMA/cxgb3: Convert timers to use timer_setup()

[Steve Wise]

> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. Also removes an unused timer.
> 
> Cc: Steve Wise 
> Cc: Doug Ledford 
> Cc: Sean Hefty 
> Cc: Hal Rosenstock 
> Cc: linux-r...@vger.kernel.org
> Signed-off-by: Kees Cook 

Acked-by: Steve Wise 

RE: [PATCH] RDMA/cxgb4: Convert timers to use timer_setup()

[Steve Wise]

> 
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly. Also removes an unused timer and
> drops a redundant initialization.
> 
> Cc: Steve Wise 
> Cc: Doug Ledford 
> Cc: Sean Hefty 
> Cc: Hal Rosenstock 
> Cc: linux-r...@vger.kernel.org
> Signed-off-by: Kees Cook 

Acked-by: Steve Wise 

RE: [PATCH] RDMA/cxgb3: remove redundant first assignement of sqp

[Steve Wise]

> 
> From: Colin Ian King <colin.k...@canonical.com>
> 
> sqp is being initialised when it is being declared and then updated
> a little later on making the first initialization redundant. Clean
> this up by initializing ptr and sqp at their declaration.
> 
> Cleans up warning: "warning: Value stored to 'sqp' during its
> initialization is never read"
> 
> Fixes: a58e58fafdff ("RDMA/cxgb3: Wrap the software send queue pointer as
> needed on flush")
> Signed-off-by: Colin Ian King <colin.k...@canonical.com>

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] RDMA/cxgb3: remove redundant first assignement of sqp

[Steve Wise]

> 
> From: Colin Ian King 
> 
> sqp is being initialised when it is being declared and then updated
> a little later on making the first initialization redundant. Clean
> this up by initializing ptr and sqp at their declaration.
> 
> Cleans up warning: "warning: Value stored to 'sqp' during its
> initialization is never read"
> 
> Fixes: a58e58fafdff ("RDMA/cxgb3: Wrap the software send queue pointer as
> needed on flush")
> Signed-off-by: Colin Ian King 

Acked-by: Steve Wise 

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> I have put the patch v3 in the following location:
> https://github.com/longlimsft/linux-next/tree/patch_v3
> 
> I will be sending it out soon. Please give it a try.
> 

Hey Long, how do I request a CIFS RDMA mount from the Linux client?  Is
there a mount.cifs option?  If so, where can I get the mount.cifs code that
enables this?  Or is there some other way? 

Thanks,

Steve.

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> I have put the patch v3 in the following location:
> https://github.com/longlimsft/linux-next/tree/patch_v3
> 
> I will be sending it out soon. Please give it a try.
> 

Hey Long, how do I request a CIFS RDMA mount from the Linux client?  Is
there a mount.cifs option?  If so, where can I get the mount.cifs code that
enables this?  Or is there some other way? 

Thanks,

Steve.

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> >
> > Hey Long,
> >
> > What testing have you done with this on the various rdma transports?  Does
> > it work over IB, RoCE, and iWARP providers?
> 
> Hi Steve,
> 
> Currently all the tests have been done over Infiniband. We haven't tested on
RoCE
> or iWARP, but planned to do it in the following weeks.
> 
> Long

Ok, good.

Is this series available on github or somewhere so we can clone it and review it
as it is applied to the kernel src?

Thanks,

Steve.

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> >
> > Hey Long,
> >
> > What testing have you done with this on the various rdma transports?  Does
> > it work over IB, RoCE, and iWARP providers?
> 
> Hi Steve,
> 
> Currently all the tests have been done over Infiniband. We haven't tested on
RoCE
> or iWARP, but planned to do it in the following weeks.
> 
> Long

Ok, good.

Is this series available on github or somewhere so we can clone it and review it
as it is applied to the kernel src?

Thanks,

Steve.

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> 
> From: Long Li 
> 
> Starting with SMB2 dialect 3.0, Microsoft introduced SMBDirect transport
protocol
> for transferring upper layer (SMB2) payload over RDMA via Infiniband, RoCE or
> iWARP. The prococol is published in [MS-SMBD] (https://msdn.microsoft.com/en-
> us/library/hh536346.aspx).
> 
> The patch v2 added RDMA read/write via memory registration, and addressed
> feedbacks on v1.
> 

Hey Long,

What testing have you done with this on the various rdma transports?  Does it
work over IB, RoCE, and iWARP providers?

Thanks,

Steve.

RE: [Patch v2 00/19] CIFS: Implement SMBDirect

[Steve Wise]

> 
> From: Long Li 
> 
> Starting with SMB2 dialect 3.0, Microsoft introduced SMBDirect transport
protocol
> for transferring upper layer (SMB2) payload over RDMA via Infiniband, RoCE or
> iWARP. The prococol is published in [MS-SMBD] (https://msdn.microsoft.com/en-
> us/library/hh536346.aspx).
> 
> The patch v2 added RDMA read/write via memory registration, and addressed
> feedbacks on v1.
> 

Hey Long,

What testing have you done with this on the various rdma transports?  Does it
work over IB, RoCE, and iWARP providers?

Thanks,

Steve.

RE: [PATCH 1/2] RDMA/uverbs: Fix the check for port number

[Steve Wise]

> The port number is only valid if IB_QP_PORT is set in the mask.
> So only check port number if it is valid to prevent modify_qp from
> failing due to an invalid port number.
> 
> Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> Signed-off-by: Mustafa Ismail <mustafa.ism...@intel.com>

Looks good.

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 2/2] RDMA/core: Initialize port_num in qp_attr

[Steve Wise]

> Initialize the port_num for iWARP in rdma_init_qp_attr.
> 
> Signed-off-by: Mustafa Ismail <mustafa.ism...@intel.com>

Looks fine.

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 1/2] RDMA/uverbs: Fix the check for port number

[Steve Wise]

> The port number is only valid if IB_QP_PORT is set in the mask.
> So only check port number if it is valid to prevent modify_qp from
> failing due to an invalid port number.
> 
> Fixes: 5ecce4c9b17b("Check port number supplied by user verbs cmds")
> Signed-off-by: Mustafa Ismail 

Looks good.

Reviewed-by: Steve Wise 

RE: [PATCH 2/2] RDMA/core: Initialize port_num in qp_attr

[Steve Wise]

> Initialize the port_num for iWARP in rdma_init_qp_attr.
> 
> Signed-off-by: Mustafa Ismail 

Looks fine.

Reviewed-by: Steve Wise 

RE: [PATCH] cxgb4: Remove some dead code

[Steve Wise]

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH] cxgb4: Remove some dead code

[Steve Wise]

Acked-by: Steve Wise 

RE: [RFC 4/8] p2pmem: Add debugfs "stats" file

[Steve Wise]

> 
> > +   p2pmem_debugfs_root = debugfs_create_dir("p2pmem", NULL);
> > +   if (!p2pmem_debugfs_root)
> > +   pr_info("could not create debugfs entry, continuing\n");
> > +
> 
> Why continue? I think it'd be better to just fail it.
> 

Because not having debugfs support isn't fatal to using p2pmem.  So I
believe it is better to continue.  But this is trivial, IMO, so either was
is ok with me.

> Besides, this can be safely squashed into patch 1.

Yes.

RE: [RFC 4/8] p2pmem: Add debugfs "stats" file

[Steve Wise]

> 
> > +   p2pmem_debugfs_root = debugfs_create_dir("p2pmem", NULL);
> > +   if (!p2pmem_debugfs_root)
> > +   pr_info("could not create debugfs entry, continuing\n");
> > +
> 
> Why continue? I think it'd be better to just fail it.
> 

Because not having debugfs support isn't fatal to using p2pmem.  So I
believe it is better to continue.  But this is trivial, IMO, so either was
is ok with me.

> Besides, this can be safely squashed into patch 1.

Yes.

RE: [RFC 2/8] cxgb4: setup pcie memory window 4 and create p2pmem region

[Steve Wise]

> 
> 
> > +static void setup_memwin_p2pmem(struct adapter *adap)
> > +{
> > +   unsigned int mem_base = t4_read_reg(adap,
> CIM_EXTMEM2_BASE_ADDR_A);
> > +   unsigned int mem_size = t4_read_reg(adap,
> CIM_EXTMEM2_ADDR_SIZE_A);
> > +
> > +   if (!use_p2pmem)
> > +   return;
> 
> This is weird, why even call this if !use_p2pmem?
> 

The use_p2pmem was added after the original change.  I'll update as you
suggest.

> > +static int init_p2pmem(struct adapter *adapter)
> > +{
> > +   unsigned int mem_size = t4_read_reg(adapter,
> CIM_EXTMEM2_ADDR_SIZE_A);
> > +   struct p2pmem_dev *p;
> > +   int rc;
> > +   struct resource res;
> > +
> > +   if (!mem_size || !use_p2pmem)
> > +   return 0;
> 
> Again, weird...

Yup.

RE: [RFC 2/8] cxgb4: setup pcie memory window 4 and create p2pmem region

[Steve Wise]

> 
> 
> > +static void setup_memwin_p2pmem(struct adapter *adap)
> > +{
> > +   unsigned int mem_base = t4_read_reg(adap,
> CIM_EXTMEM2_BASE_ADDR_A);
> > +   unsigned int mem_size = t4_read_reg(adap,
> CIM_EXTMEM2_ADDR_SIZE_A);
> > +
> > +   if (!use_p2pmem)
> > +   return;
> 
> This is weird, why even call this if !use_p2pmem?
> 

The use_p2pmem was added after the original change.  I'll update as you
suggest.

> > +static int init_p2pmem(struct adapter *adapter)
> > +{
> > +   unsigned int mem_size = t4_read_reg(adapter,
> CIM_EXTMEM2_ADDR_SIZE_A);
> > +   struct p2pmem_dev *p;
> > +   int rc;
> > +   struct resource res;
> > +
> > +   if (!mem_size || !use_p2pmem)
> > +   return 0;
> 
> Again, weird...

Yup.

RE: [PATCH 4/4] cxgb4: Convert PDBG to pr_debug

[Steve Wise]

> ker...@vger.kernel.org
> Subject: [PATCH 4/4] cxgb4: Convert PDBG to pr_debug
> 
> Use a more typical logging style.
> 
> Miscellanea:
> 
> o Obsolete the c4iw_debug module parameter
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/infiniband/hw/cxgb4/cm.c   | 303
+
>  drivers/infiniband/hw/cxgb4/cq.c   |  71 
>  drivers/infiniband/hw/cxgb4/device.c   |  58 +++
>  drivers/infiniband/hw/cxgb4/ev.c   |  29 ++--
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  44 +++--
>  drivers/infiniband/hw/cxgb4/mem.c  |  28 +--
>  drivers/infiniband/hw/cxgb4/provider.c |  38 ++---
>  drivers/infiniband/hw/cxgb4/qp.c   |  91 +-
>  drivers/infiniband/hw/cxgb4/resource.c |  46 ++---
>  drivers/infiniband/hw/cxgb4/t4.h   |  22 +--
>  10 files changed, 368 insertions(+), 362 deletions(-)


Looks good (and thanks).

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 4/4] cxgb4: Convert PDBG to pr_debug

[Steve Wise]

> ker...@vger.kernel.org
> Subject: [PATCH 4/4] cxgb4: Convert PDBG to pr_debug
> 
> Use a more typical logging style.
> 
> Miscellanea:
> 
> o Obsolete the c4iw_debug module parameter
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/infiniband/hw/cxgb4/cm.c   | 303
+
>  drivers/infiniband/hw/cxgb4/cq.c   |  71 
>  drivers/infiniband/hw/cxgb4/device.c   |  58 +++
>  drivers/infiniband/hw/cxgb4/ev.c   |  29 ++--
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  44 +++--
>  drivers/infiniband/hw/cxgb4/mem.c  |  28 +--
>  drivers/infiniband/hw/cxgb4/provider.c |  38 ++---
>  drivers/infiniband/hw/cxgb4/qp.c   |  91 +-
>  drivers/infiniband/hw/cxgb4/resource.c |  46 ++---
>  drivers/infiniband/hw/cxgb4/t4.h   |  22 +--
>  10 files changed, 368 insertions(+), 362 deletions(-)


Looks good (and thanks).

Reviewed-by: Steve Wise 

RE: [PATCH 3/4] cxgb4: Use more common logging style

[Steve Wise]

> 
> Convert printks to pr_
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/infiniband/hw/cxgb4/cm.c   | 84
+++---
>  drivers/infiniband/hw/cxgb4/cq.c   |  8 ++--
>  drivers/infiniband/hw/cxgb4/device.c   | 83 ++---
>  drivers/infiniband/hw/cxgb4/ev.c   | 10 ++--
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  6 +++
>  drivers/infiniband/hw/cxgb4/mem.c  |  6 +--
>  drivers/infiniband/hw/cxgb4/provider.c |  4 +-
>  drivers/infiniband/hw/cxgb4/qp.c   |  5 +-
>  drivers/infiniband/hw/cxgb4/resource.c | 18 +++-
>  drivers/infiniband/hw/cxgb4/t4.h   |  2 +-
>  10 files changed, 97 insertions(+), 129 deletions(-)
>

Looks good (and thanks).

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 3/4] cxgb4: Use more common logging style

[Steve Wise]

> 
> Convert printks to pr_
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/infiniband/hw/cxgb4/cm.c   | 84
+++---
>  drivers/infiniband/hw/cxgb4/cq.c   |  8 ++--
>  drivers/infiniband/hw/cxgb4/device.c   | 83 ++---
>  drivers/infiniband/hw/cxgb4/ev.c   | 10 ++--
>  drivers/infiniband/hw/cxgb4/iw_cxgb4.h |  6 +++
>  drivers/infiniband/hw/cxgb4/mem.c  |  6 +--
>  drivers/infiniband/hw/cxgb4/provider.c |  4 +-
>  drivers/infiniband/hw/cxgb4/qp.c   |  5 +-
>  drivers/infiniband/hw/cxgb4/resource.c | 18 +++-
>  drivers/infiniband/hw/cxgb4/t4.h   |  2 +-
>  10 files changed, 97 insertions(+), 129 deletions(-)
>

Looks good (and thanks).

Reviewed-by: Steve Wise 

RE: [PATCH 1/4] cxgb3: Use more common logging style

[Steve Wise]

> Subject: [PATCH 1/4] cxgb3: Use more common logging style
> 
> Convert printks to pr_
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c  | 27 ++--
>  drivers/infiniband/hw/cxgb3/cxio_hal.h  |  9 +++-
>  drivers/infiniband/hw/cxgb3/cxio_resource.c |  5 ++-
>  drivers/infiniband/hw/cxgb3/iwch.c  | 13 +++---
>  drivers/infiniband/hw/cxgb3/iwch_cm.c   | 66
-
>  drivers/infiniband/hw/cxgb3/iwch_cq.c   |  7 ++-
>  drivers/infiniband/hw/cxgb3/iwch_ev.c   | 11 +++--
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 ++---
>  drivers/infiniband/hw/cxgb3/iwch_qp.c   |  7 ++-
>  9 files changed, 70 insertions(+), 86 deletions(-)



> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.h
> b/drivers/infiniband/hw/cxgb3/cxio_hal.h
> index 78fbe9ffe7f0..115c0e3a5df5 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.h
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.h
> @@ -196,8 +196,13 @@ int cxio_poll_cq(struct t3_wq *wq, struct t3_cq *cq,
struct
> t3_cqe *cqe,
>u8 *cqe_flushed, u64 *cookie, u32 *credit);
>  int iwch_cxgb3_ofld_send(struct t3cdev *tdev, struct sk_buff *skb);
> 
> -#define MOD "iw_cxgb3: "
> -#define PDBG(fmt, args...) pr_debug(MOD fmt, ## args)
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#endif

Is the ifdef/undef needed?  I see other modules just define pr_fmt() regardless.

Otherwise, looks good (and thanks).

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 1/4] cxgb3: Use more common logging style

[Steve Wise]

> Subject: [PATCH 1/4] cxgb3: Use more common logging style
> 
> Convert printks to pr_
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c  | 27 ++--
>  drivers/infiniband/hw/cxgb3/cxio_hal.h  |  9 +++-
>  drivers/infiniband/hw/cxgb3/cxio_resource.c |  5 ++-
>  drivers/infiniband/hw/cxgb3/iwch.c  | 13 +++---
>  drivers/infiniband/hw/cxgb3/iwch_cm.c   | 66
-
>  drivers/infiniband/hw/cxgb3/iwch_cq.c   |  7 ++-
>  drivers/infiniband/hw/cxgb3/iwch_ev.c   | 11 +++--
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 11 ++---
>  drivers/infiniband/hw/cxgb3/iwch_qp.c   |  7 ++-
>  9 files changed, 70 insertions(+), 86 deletions(-)



> diff --git a/drivers/infiniband/hw/cxgb3/cxio_hal.h
> b/drivers/infiniband/hw/cxgb3/cxio_hal.h
> index 78fbe9ffe7f0..115c0e3a5df5 100644
> --- a/drivers/infiniband/hw/cxgb3/cxio_hal.h
> +++ b/drivers/infiniband/hw/cxgb3/cxio_hal.h
> @@ -196,8 +196,13 @@ int cxio_poll_cq(struct t3_wq *wq, struct t3_cq *cq,
struct
> t3_cqe *cqe,
>u8 *cqe_flushed, u64 *cookie, u32 *credit);
>  int iwch_cxgb3_ofld_send(struct t3cdev *tdev, struct sk_buff *skb);
> 
> -#define MOD "iw_cxgb3: "
> -#define PDBG(fmt, args...) pr_debug(MOD fmt, ## args)
> +#ifdef pr_fmt
> +#undef pr_fmt
> +#endif

Is the ifdef/undef needed?  I see other modules just define pr_fmt() regardless.

Otherwise, looks good (and thanks).

Reviewed-by: Steve Wise 

RE: [PATCH 2/4] cxgb3: Convert PDBG to pr_debug

[Steve Wise]

> Subject: [PATCH 2/4] cxgb3: Convert PDBG to pr_debug
> 
> Using the normal mechanism, not an indirected one, is clearer.
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches <j...@perches.com>
> ---
>  drivers/infiniband/hw/cxgb3/cxio_dbg.c  |  35 ++---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c  | 174 
>  drivers/infiniband/hw/cxgb3/cxio_hal.h  |   2 -
>  drivers/infiniband/hw/cxgb3/cxio_resource.c |  20 +--
>  drivers/infiniband/hw/cxgb3/iwch.c  |   6 +-
>  drivers/infiniband/hw/cxgb3/iwch_cm.c   | 203
++--
>  drivers/infiniband/hw/cxgb3/iwch_cm.h   |  18 +--
>  drivers/infiniband/hw/cxgb3/iwch_cq.c   |  14 +-
>  drivers/infiniband/hw/cxgb3/iwch_ev.c   |  15 +-
>  drivers/infiniband/hw/cxgb3/iwch_mem.c  |   2 +-
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 101 +++---
>  drivers/infiniband/hw/cxgb3/iwch_provider.h |   9 +-
>  drivers/infiniband/hw/cxgb3/iwch_qp.c   |  60 
>  13 files changed, 329 insertions(+), 330 deletions(-)
> 

Looks good (and thanks).

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 2/4] cxgb3: Convert PDBG to pr_debug

[Steve Wise]

> Subject: [PATCH 2/4] cxgb3: Convert PDBG to pr_debug
> 
> Using the normal mechanism, not an indirected one, is clearer.
> 
> Miscellanea:
> 
> o Coalesce formats
> o Realign arguments
> 
> Signed-off-by: Joe Perches 
> ---
>  drivers/infiniband/hw/cxgb3/cxio_dbg.c  |  35 ++---
>  drivers/infiniband/hw/cxgb3/cxio_hal.c  | 174 
>  drivers/infiniband/hw/cxgb3/cxio_hal.h  |   2 -
>  drivers/infiniband/hw/cxgb3/cxio_resource.c |  20 +--
>  drivers/infiniband/hw/cxgb3/iwch.c  |   6 +-
>  drivers/infiniband/hw/cxgb3/iwch_cm.c   | 203
++--
>  drivers/infiniband/hw/cxgb3/iwch_cm.h   |  18 +--
>  drivers/infiniband/hw/cxgb3/iwch_cq.c   |  14 +-
>  drivers/infiniband/hw/cxgb3/iwch_ev.c   |  15 +-
>  drivers/infiniband/hw/cxgb3/iwch_mem.c  |   2 +-
>  drivers/infiniband/hw/cxgb3/iwch_provider.c | 101 +++---
>  drivers/infiniband/hw/cxgb3/iwch_provider.h |   9 +-
>  drivers/infiniband/hw/cxgb3/iwch_qp.c   |  60 
>  13 files changed, 329 insertions(+), 330 deletions(-)
> 

Looks good (and thanks).

Reviewed-by: Steve Wise 

RE: [PATCH] IB/cxgb3: header should be defining CXGB3_ABI_USER_H

[Steve Wise]

Acked-by: Steve Wise <sw...@chelsio.com>

RE: [PATCH] IB/cxgb3: header should be defining CXGB3_ABI_USER_H

[Steve Wise]

Acked-by: Steve Wise 

RE: [PATCH 1/1] IB/cxgb3: fix misspelling in header guard

[Steve Wise]

> 
> Use CXGB3_... instead of CXBG3_...
> 
> Signed-off-by: Nicolas Iooss <nicolas.iooss_li...@m4x.org>

Acked-by: Steve Wise <sw...@chelsio.com>

RE: [PATCH 1/1] IB/cxgb3: fix misspelling in header guard

[Steve Wise]

> 
> Use CXGB3_... instead of CXBG3_...
> 
> Signed-off-by: Nicolas Iooss 

Acked-by: Steve Wise 

RE: [PATCH 1/1] infiniband: hw: cxgb4: set errno on failure

[Steve Wise]

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 1/1] infiniband: hw: cxgb4: set errno on failure

[Steve Wise]

Acked-by: Steve Wise 

RE: [PATCH v3] IB/cxgb4: Mark symbols static for _free_qp

[Steve Wise]

> Subject: [PATCH v3] IB/cxgb4: Mark symbols static for _free_qp
> 
> We get 1 warning when build kernel with W=1:
> drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for
> '_free_qp' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared
> and don't need a declaration, but can be made static.
> so this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie <baoyou@linaro.org>
> Reviewed-by: Yuval Shaia <yuval.sh...@oracle.com>
> Reviewed-by: Leon Romanovsky <leo...@mellanox.com>

Ignore my comment on v2...

Acked-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH v3] IB/cxgb4: Mark symbols static for _free_qp

[Steve Wise]

> Subject: [PATCH v3] IB/cxgb4: Mark symbols static for _free_qp
> 
> We get 1 warning when build kernel with W=1:
> drivers/infiniband/hw/cxgb4/qp.c:686:6: warning: no previous prototype for
> '_free_qp' [-Wmissing-prototypes]
> 
> In fact, this function is only used in the file in which it is declared
> and don't need a declaration, but can be made static.
> so this patch marks it 'static'.
> 
> Signed-off-by: Baoyou Xie 
> Reviewed-by: Yuval Shaia 
> Reviewed-by: Leon Romanovsky 

Ignore my comment on v2...

Acked-by: Steve Wise 

RE: [PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Steve Wise]

> Subject: [PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where
possible
>

Is "fix:infiniband:hw:cxgb4:qp:" some standard way patches are being submitted
these days?  Usually it would be the module name, something like:

iw_cxgb4: make _free_qp() static

Steve

RE: [PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where possible

[Steve Wise]

> Subject: [PATCH v2] fix:infiniband:hw:cxgb4:qp:mark symbols static where
possible
>

Is "fix:infiniband:hw:cxgb4:qp:" some standard way patches are being submitted
these days?  Usually it would be the module name, something like:

iw_cxgb4: make _free_qp() static

Steve

RE: [PATCH 05/22] IB/cma: Remove deprecated create_singlethread_workqueue

[Steve Wise]

> alloc_ordered_workqueue() with WQ_MEM_RECLAIM set, replaces
> deprecated create_singlethread_workqueue(). This is the identity
> conversion.
> 
> The workqueue "cma_wq" queues work item cma_work_handler. It has been
> identity converted.
> 
> WQ_MEM_RECLAIM has been set to ensure forward progress under
> memory pressure.
> 
> Signed-off-by: Bhaktipriya Shridhar <bhaktipriy...@gmail.com>


Looks fine.

Reviewed-by: Steve Wise <sw...@opengridcomputing.com>

RE: [PATCH 05/22] IB/cma: Remove deprecated create_singlethread_workqueue

[Steve Wise]

> alloc_ordered_workqueue() with WQ_MEM_RECLAIM set, replaces
> deprecated create_singlethread_workqueue(). This is the identity
> conversion.
> 
> The workqueue "cma_wq" queues work item cma_work_handler. It has been
> identity converted.
> 
> WQ_MEM_RECLAIM has been set to ensure forward progress under
> memory pressure.
> 
> Signed-off-by: Bhaktipriya Shridhar 


Looks fine.

Reviewed-by: Steve Wise