[PATCH 0/3] srp/srpt - some useful patches

[PATCH 0/3] srp/srpt - some useful patches

[Max Gurtovoy]

No bug fixes, but few improvements that might help.

Max Gurtovoy (3):
  IB/srp: allocate ib_qp_attr on the stack
  IB/srpt: allocate ib_qp_attr on the stack
  IB/srpt: fix compilation warning

 drivers/infiniband/ulp/srp/ib_srp.c   |   51 ++++++++++++--------------------
 drivers/infiniband/ulp/srpt/ib_srpt.c |   20 +++++--------
 2 files changed, 27 insertions(+), 44 deletions(-)

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

[PATCH 1/3] IB/srp: allocate ib_qp_attr on the stack

[Max Gurtovoy]

No reason to allocate it dynamically.

Tested-by: Israel Rukshin <israelr-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c |   51 +++++++++++++----------------------
 1 files changed, 19 insertions(+), 32 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index d980fb4..8a003eb 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -263,33 +263,28 @@ static void srp_qp_event(struct ib_event *event, void *context)
 static int srp_init_qp(struct srp_target_port *target,
 		       struct ib_qp *qp)
 {
-	struct ib_qp_attr *attr;
+	struct ib_qp_attr attr;
 	int ret;
 
-	attr = kmalloc(sizeof *attr, GFP_KERNEL);
-	if (!attr)
-		return -ENOMEM;
-
 	ret = ib_find_cached_pkey(target->srp_host->srp_dev->dev,
 				  target->srp_host->port,
 				  be16_to_cpu(target->pkey),
-				  &attr->pkey_index);
+				  &attr.pkey_index);
 	if (ret)
 		goto out;
 
-	attr->qp_state        = IB_QPS_INIT;
-	attr->qp_access_flags = (IB_ACCESS_REMOTE_READ |
+	attr.qp_state        = IB_QPS_INIT;
+	attr.qp_access_flags = (IB_ACCESS_REMOTE_READ |
 				    IB_ACCESS_REMOTE_WRITE);
-	attr->port_num        = target->srp_host->port;
+	attr.port_num        = target->srp_host->port;
 
-	ret = ib_modify_qp(qp, attr,
+	ret = ib_modify_qp(qp, &attr,
 			   IB_QP_STATE		|
 			   IB_QP_PKEY_INDEX	|
 			   IB_QP_ACCESS_FLAGS	|
 			   IB_QP_PORT);
 
 out:
-	kfree(attr);
 	return ret;
 }
 
@@ -2280,7 +2275,7 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 			       struct srp_rdma_ch *ch)
 {
 	struct srp_target_port *target = ch->target;
-	struct ib_qp_attr *qp_attr = NULL;
+	struct ib_qp_attr qp_attr;
 	int attr_mask = 0;
 	int ret;
 	int i;
@@ -2312,44 +2307,36 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 			goto error;
 	}
 
-	ret = -ENOMEM;
-	qp_attr = kmalloc(sizeof *qp_attr, GFP_KERNEL);
-	if (!qp_attr)
-		goto error;
-
-	qp_attr->qp_state = IB_QPS_RTR;
-	ret = ib_cm_init_qp_attr(cm_id, qp_attr, &attr_mask);
+	qp_attr.qp_state = IB_QPS_RTR;
+	ret = ib_cm_init_qp_attr(cm_id, &qp_attr, &attr_mask);
 	if (ret)
-		goto error_free;
+		goto error;
 
-	ret = ib_modify_qp(ch->qp, qp_attr, attr_mask);
+	ret = ib_modify_qp(ch->qp, &qp_attr, attr_mask);
 	if (ret)
-		goto error_free;
+		goto error;
 
 	for (i = 0; i < target->queue_size; i++) {
 		struct srp_iu *iu = ch->rx_ring[i];
 
 		ret = srp_post_recv(ch, iu);
 		if (ret)
-			goto error_free;
+			goto error;
 	}
 
-	qp_attr->qp_state = IB_QPS_RTS;
-	ret = ib_cm_init_qp_attr(cm_id, qp_attr, &attr_mask);
+	qp_attr.qp_state = IB_QPS_RTS;
+	ret = ib_cm_init_qp_attr(cm_id, &qp_attr, &attr_mask);
 	if (ret)
-		goto error_free;
+		goto error;
 
-	target->rq_tmo_jiffies = srp_compute_rq_tmo(qp_attr, attr_mask);
+	target->rq_tmo_jiffies = srp_compute_rq_tmo(&qp_attr, attr_mask);
 
-	ret = ib_modify_qp(ch->qp, qp_attr, attr_mask);
+	ret = ib_modify_qp(ch->qp, &qp_attr, attr_mask);
 	if (ret)
-		goto error_free;
+		goto error;
 
 	ret = ib_send_cm_rtu(cm_id, NULL, 0);
 
-error_free:
-	kfree(qp_attr);
-
 error:
 	ch->status = ret;
 }
-- 
1.7.1

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

[PATCH 2/3] IB/srpt: allocate ib_qp_attr on the stack

[Max Gurtovoy]

No reason to allocate it dynamically.

Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0b1f69e..aa18d74 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
  */
 static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
 {
-	struct ib_qp_attr *attr;
+	struct ib_qp_attr attr;
 	int ret;
 
-	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-	if (!attr)
-		return -ENOMEM;
-
-	attr->qp_state = IB_QPS_INIT;
-	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
+	memset(&attr, 0, sizeof(attr));
+	attr.qp_state = IB_QPS_INIT;
+	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
 	    IB_ACCESS_REMOTE_WRITE;
-	attr->port_num = ch->sport->port;
-	attr->pkey_index = 0;
+	attr.port_num = ch->sport->port;
+	attr.pkey_index = 0;
 
-	ret = ib_modify_qp(qp, attr,
+	ret = ib_modify_qp(qp, &attr,
 			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
 			   IB_QP_PKEY_INDEX);
 
-	kfree(attr);
 	return ret;
 }
 
-- 
1.7.1

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

[PATCH 3/3] IB/srpt: fix compilation warning

[Max Gurtovoy]

warning: 'prev_nents' may be used uninitialized in this function

Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index aa18d74..b36f9a5 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -809,7 +809,7 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 	enum dma_data_direction dir = target_reverse_dma_direction(&ioctx->cmd);
 	struct srpt_rdma_ch *ch = ioctx->ch;
 	struct scatterlist *prev = NULL;
-	unsigned prev_nents;
+	unsigned prev_nents = 0;
 	int ret, i;
 
 	if (nbufs == 1) {
-- 
1.7.1

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

Re: [PATCH 2/3] IB/srpt: allocate ib_qp_attr on the stack

[Yuval Shaia]

One minor comment in line but w/ or w/o it:

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.
> 
> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 0b1f69e..aa18d74 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>   */
>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>  {
> -	struct ib_qp_attr *attr;
> +	struct ib_qp_attr attr;
>  	int ret;
>  
> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
> -	attr->qp_state = IB_QPS_INIT;
> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> +	memset(&attr, 0, sizeof(attr));
> +	attr.qp_state = IB_QPS_INIT;
> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>  	    IB_ACCESS_REMOTE_WRITE;
> -	attr->port_num = ch->sport->port;
> -	attr->pkey_index = 0;
> +	attr.port_num = ch->sport->port;
> +	attr.pkey_index = 0;

Correct me if i'm wrong but this looks like redundant since we zeroed the
entire attr object few lines ago.

>  
> -	ret = ib_modify_qp(qp, attr,
> +	ret = ib_modify_qp(qp, &attr,
>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>  			   IB_QP_PKEY_INDEX);
>  
> -	kfree(attr);
>  	return ret;
>  }
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] IB/srp: allocate ib_qp_attr on the stack

[Yuval Shaia]

On Wed, Dec 28, 2016 at 01:00:11PM +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.
> 
> Tested-by: Israel Rukshin <israelr-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srp/ib_srp.c |   51 +++++++++++++----------------------
>  1 files changed, 19 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index d980fb4..8a003eb 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -263,33 +263,28 @@ static void srp_qp_event(struct ib_event *event, void *context)
>  static int srp_init_qp(struct srp_target_port *target,
>  		       struct ib_qp *qp)
>  {
> -	struct ib_qp_attr *attr;
> +	struct ib_qp_attr attr;
>  	int ret;
>  
> -	attr = kmalloc(sizeof *attr, GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
>  	ret = ib_find_cached_pkey(target->srp_host->srp_dev->dev,
>  				  target->srp_host->port,
>  				  be16_to_cpu(target->pkey),
> -				  &attr->pkey_index);
> +				  &attr.pkey_index);
>  	if (ret)
>  		goto out;
>  
> -	attr->qp_state        = IB_QPS_INIT;
> -	attr->qp_access_flags = (IB_ACCESS_REMOTE_READ |
> +	attr.qp_state        = IB_QPS_INIT;

Since kzmalloc was not used this patch do the job it intend to do.
Do you think, while-we-are-there that it will be safer to use memset before
using the object?

> +	attr.qp_access_flags = (IB_ACCESS_REMOTE_READ |
>  				    IB_ACCESS_REMOTE_WRITE);
> -	attr->port_num        = target->srp_host->port;
> +	attr.port_num        = target->srp_host->port;
>  
> -	ret = ib_modify_qp(qp, attr,
> +	ret = ib_modify_qp(qp, &attr,
>  			   IB_QP_STATE		|
>  			   IB_QP_PKEY_INDEX	|
>  			   IB_QP_ACCESS_FLAGS	|
>  			   IB_QP_PORT);
>  
>  out:
> -	kfree(attr);
>  	return ret;
>  }
>  
> @@ -2280,7 +2275,7 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
>  			       struct srp_rdma_ch *ch)
>  {
>  	struct srp_target_port *target = ch->target;
> -	struct ib_qp_attr *qp_attr = NULL;
> +	struct ib_qp_attr qp_attr;
>  	int attr_mask = 0;
>  	int ret;
>  	int i;
> @@ -2312,44 +2307,36 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
>  			goto error;
>  	}
>  
> -	ret = -ENOMEM;
> -	qp_attr = kmalloc(sizeof *qp_attr, GFP_KERNEL);
> -	if (!qp_attr)
> -		goto error;
> -
> -	qp_attr->qp_state = IB_QPS_RTR;
> -	ret = ib_cm_init_qp_attr(cm_id, qp_attr, &attr_mask);
> +	qp_attr.qp_state = IB_QPS_RTR;
> +	ret = ib_cm_init_qp_attr(cm_id, &qp_attr, &attr_mask);
>  	if (ret)
> -		goto error_free;
> +		goto error;
>  
> -	ret = ib_modify_qp(ch->qp, qp_attr, attr_mask);
> +	ret = ib_modify_qp(ch->qp, &qp_attr, attr_mask);
>  	if (ret)
> -		goto error_free;
> +		goto error;
>  
>  	for (i = 0; i < target->queue_size; i++) {
>  		struct srp_iu *iu = ch->rx_ring[i];
>  
>  		ret = srp_post_recv(ch, iu);
>  		if (ret)
> -			goto error_free;
> +			goto error;
>  	}
>  
> -	qp_attr->qp_state = IB_QPS_RTS;
> -	ret = ib_cm_init_qp_attr(cm_id, qp_attr, &attr_mask);
> +	qp_attr.qp_state = IB_QPS_RTS;
> +	ret = ib_cm_init_qp_attr(cm_id, &qp_attr, &attr_mask);
>  	if (ret)
> -		goto error_free;
> +		goto error;
>  
> -	target->rq_tmo_jiffies = srp_compute_rq_tmo(qp_attr, attr_mask);
> +	target->rq_tmo_jiffies = srp_compute_rq_tmo(&qp_attr, attr_mask);
>  
> -	ret = ib_modify_qp(ch->qp, qp_attr, attr_mask);
> +	ret = ib_modify_qp(ch->qp, &qp_attr, attr_mask);
>  	if (ret)
> -		goto error_free;
> +		goto error;
>  
>  	ret = ib_send_cm_rtu(cm_id, NULL, 0);
>  
> -error_free:
> -	kfree(qp_attr);
> -
>  error:
>  	ch->status = ret;
>  }
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] IB/srpt: fix compilation warning

[Yuval Shaia]

Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>

On Wed, Dec 28, 2016 at 01:00:13PM +0200, Max Gurtovoy wrote:
> warning: 'prev_nents' may be used uninitialized in this function
> 
> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index aa18d74..b36f9a5 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -809,7 +809,7 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
>  	enum dma_data_direction dir = target_reverse_dma_direction(&ioctx->cmd);
>  	struct srpt_rdma_ch *ch = ioctx->ch;
>  	struct scatterlist *prev = NULL;
> -	unsigned prev_nents;
> +	unsigned prev_nents = 0;
>  	int ret, i;
>  
>  	if (nbufs == 1) {
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] IB/srp: allocate ib_qp_attr on the stack

[Max Gurtovoy]

>
> Since kzmalloc was not used this patch do the job it intend to do.
> Do you think, while-we-are-there that it will be safer to use memset before
> using the object?
>

You're safe because of the flags you pass to ib_modify_qp, but we can 
add it.
Bart, what do you think ?

>> +	attr.qp_access_flags = (IB_ACCESS_REMOTE_READ |
>>  				    IB_ACCESS_REMOTE_WRITE);
>> -	attr->port_num        = target->srp_host->port;
>> +	attr.port_num        = target->srp_host->port;
>>
>> -	ret = ib_modify_qp(qp, attr,
>> +	ret = ib_modify_qp(qp, &attr,
>>  			   IB_QP_STATE		|
>>  			   IB_QP_PKEY_INDEX	|
>>  			   IB_QP_ACCESS_FLAGS	|
>>  			   IB_QP_PORT);
>>
>>  out:
>> -	kfree(attr);
>>  	return ret;
>>  }
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] IB/srpt: allocate ib_qp_attr on the stack

[Max Gurtovoy]

On 12/28/2016 1:06 PM, Yuval Shaia wrote:
> One minor comment in line but w/ or w/o it:
>
> Reviewed-by: Yuval Shaia <yuval.shaia-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>
> On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
>> No reason to allocate it dynamically.
>>
>> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
>> ---
>>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>>  1 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index 0b1f69e..aa18d74 100644
>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>>   */
>>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>>  {
>> -	struct ib_qp_attr *attr;
>> +	struct ib_qp_attr attr;
>>  	int ret;
>>
>> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
>> -	if (!attr)
>> -		return -ENOMEM;
>> -
>> -	attr->qp_state = IB_QPS_INIT;
>> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>> +	memset(&attr, 0, sizeof(attr));
>> +	attr.qp_state = IB_QPS_INIT;
>> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>>  	    IB_ACCESS_REMOTE_WRITE;
>> -	attr->port_num = ch->sport->port;
>> -	attr->pkey_index = 0;
>> +	attr.port_num = ch->sport->port;
>> +	attr.pkey_index = 0;
>
> Correct me if i'm wrong but this looks like redundant since we zeroed the
> entire attr object few lines ago.
>

Yes this is redundant but I think it's implies the flags you'll pass 
ib_modify_qp. don't realy mind,

Bart, your call :)

>>
>> -	ret = ib_modify_qp(qp, attr,
>> +	ret = ib_modify_qp(qp, &attr,
>>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>>  			   IB_QP_PKEY_INDEX);
>>
>> -	kfree(attr);
>>  	return ret;
>>  }
>>
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] IB/srp: allocate ib_qp_attr on the stack

[Bart Van Assche]

On Wed, 2016-12-28 at 13:00 +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.

The patch description is not correct. The reason struct ib_qp_attr is
allocated on the stack is to keep stack usage low, something that is
important in kernel code.

Bart.

Re: [PATCH 2/3] IB/srpt: allocate ib_qp_attr on the stack

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 331 bytes --]

On Wed, 2016-12-28 at 13:00 +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.

Same comment here: struct ib_qp_attr is allocated on the stack
to keep stack usage low.

Bart.
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 3/3] IB/srpt: fix compilation warning

[Bart Van Assche]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 574 bytes --]

On Wed, 2016-12-28 at 13:00 +0200, Max Gurtovoy wrote:
> warning: 'prev_nents' may be used uninitialized in this function

What compiler and which version of that compiler reported that 'prev_nents'
may be used uninitialized? That warning indicates a compiler bug. Recent
versions of gcc do not report that warning. Patches that suppress warnings
that are reported by bugs in old compiler versions should not be sent upstream.

Bart.N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±­ÙšŠ{ayº\x1dʇڙë,j\a­¢f£¢·hš‹»öì\x17/oSc¾™Ú³9˜uÀ¦æå‰È&jw¨®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿïêäz¹Þ–Šàþf£¢·hšˆ§~ˆmš

Re: [PATCH 3/3] IB/srpt: fix compilation warning

[Max Gurtovoy]

On 12/28/2016 1:41 PM, Bart Van Assche wrote:
> On Wed, 2016-12-28 at 13:00 +0200, Max Gurtovoy wrote:
>> warning: 'prev_nents' may be used uninitialized in this function
>
> What compiler and which version of that compiler reported that 'prev_nents'
> may be used uninitialized? That warning indicates a compiler bug. Recent
> versions of gcc do not report that warning. Patches that suppress warnings
> that are reported by bugs in old compiler versions should not be sent upstream.
>
> Bart.
>

gcc version 4.4.7 20120313 (Red Hat 4.4.7-3) (GCC)
I'll remove this patch in V2.

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

Re: [PATCH 2/3] IB/srpt: allocate ib_qp_attr on the stack

[Leon Romanovsky]

[-- Attachment #1: Type: text/plain, Size: 1805 bytes --]

On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.
>
> Signed-off-by: Max Gurtovoy <maxg-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 0b1f69e..aa18d74 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>   */
>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>  {
> -	struct ib_qp_attr *attr;
> +	struct ib_qp_attr attr;
>  	int ret;
>
> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
> -	attr->qp_state = IB_QPS_INIT;
> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> +	memset(&attr, 0, sizeof(attr));

This line will not needed if you initialize to zero this struct attr at the beginning of function.


> +	attr.qp_state = IB_QPS_INIT;
> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>  	    IB_ACCESS_REMOTE_WRITE;
> -	attr->port_num = ch->sport->port;
> -	attr->pkey_index = 0;
> +	attr.port_num = ch->sport->port;
> +	attr.pkey_index = 0;
>
> -	ret = ib_modify_qp(qp, attr,
> +	ret = ib_modify_qp(qp, &attr,
>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>  			   IB_QP_PKEY_INDEX);
>
> -	kfree(attr);
>  	return ret;
>  }
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]