IB/srp: Preparation for transmit ring response allocation

drivers/infiniband/ulp/srp/ib_srp.c +3 -1

@@ -989,19 +989,21 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
 					enum srp_request_type req_type)
 {
-	s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
+	s32 rsv;
+
+	rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TASK_MGMT_SQ_SIZE;
 
 	srp_send_completion(target->send_cq, target);
 
 	if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
 		return NULL;

Quoted 20 lines · Bart Van Assche

drivers/infiniband/ulp/srp/ib_srp.c +4 -2

@@ -989,19 +989,21 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
  static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
                                       enum srp_request_type req_type)
  {
-     s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
+     s32 rsv;
+
+     rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TASK_MGMT_SQ_SIZE;
 
       srp_send_completion(target->send_cq, target);
 
       if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
               return NULL;
 
-     if (target->req_lim < min) {
+     if (target->req_lim <= rsv) {
               ++target->zero_req_lim;
               return NULL;
       }
 

drivers/infiniband/ulp/srp/ib_srp.c static void srp_send_completion(struct ib_cq *cq, void *target_ptr) · line 1007

Quoted 3 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

-     return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+     return target->tx_ring[target->tx_head & SRP_SQ_MASK];
  }

Did you forget to remove the rest of the changes? The only change to this function should be s/SRP_SQ_SIZE/SRP_SQ_MASK/

Quoted 22 lines · David Dillow

Quoted 20 lines · Bart Van Assche

drivers/infiniband/ulp/srp/ib_srp.c +4 -2

@@ -989,19 +989,21 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
 static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
                                      enum srp_request_type req_type)
 {
-     s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
+     s32 rsv;
+
+     rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TASK_MGMT_SQ_SIZE;
 
      srp_send_completion(target->send_cq, target);
 
      if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
              return NULL;
 
-     if (target->req_lim < min) {
+     if (target->req_lim <= rsv) {
              ++target->zero_req_lim;
              return NULL;
      }
 

Quoted 6 lines

drivers/infiniband/ulp/srp/ib_srp.c static void srp_send_completion(struct ib_cq *cq, void *target_ptr) · line 1007

Quoted 3 lines

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

-     return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+     return target->tx_ring[target->tx_head & SRP_SQ_MASK];
 }

Did you forget to remove the rest of the changes? The only change to this function should be s/SRP_SQ_SIZE/SRP_SQ_MASK/

What are your objections against the above changes ? No functionality has been changed - all the above changes do is to introduce the symbolic constants SRP_TASK_MGMT_SQ_SIZE and SRP_SQ_MASK __srp_get_tx_iu().

Quoted 30 lines · Bart Van Assche

Quoted 22 lines · David Dillow

Quoted 20 lines · Bart Van Assche

(quoted diff) +4 -2

@@ -989,19 +989,21 @@ static void srp_send_completion(struct ib_cq *cq, void *target_ptr)
  static struct srp_iu *__srp_get_tx_iu(struct srp_target_port *target,
                                       enum srp_request_type req_type)
  {
-     s32 min = (req_type == SRP_REQ_TASK_MGMT) ? 1 : 2;
+     s32 rsv;
+
+     rsv = (req_type == SRP_REQ_TASK_MGMT) ? 0 : SRP_TASK_MGMT_SQ_SIZE;
 
       srp_send_completion(target->send_cq, target);
 
       if (target->tx_head - target->tx_tail >= SRP_SQ_SIZE)
               return NULL;
 
-     if (target->req_lim < min) {
+     if (target->req_lim <= rsv) {
               ++target->zero_req_lim;
               return NULL;
       }
 

Quoted 6 lines

Review static void srp_send_completion(struct ib_cq *cq, void *target_ptr) · line 1007

Quoted 3 lines

(quoted diff) +1 -1

-     return target->tx_ring[target->tx_head & SRP_SQ_SIZE];
+     return target->tx_ring[target->tx_head & SRP_SQ_MASK];
  }

Did you forget to remove the rest of the changes? The only change to this function should be s/SRP_SQ_SIZE/SRP_SQ_MASK/

Quoted 4 lines

What are your objections against the above changes ? No functionality has been changed - all the above changes do is to introduce the symbolic constants SRP_TASK_MGMT_SQ_SIZE and SRP_SQ_MASK __srp_get_tx_iu().

Style. Aesthetics. I was fine with SRP_SQ_MASK, but I didn't like the rest of the changes. Mainly, I seriously dislike the name SRP_TASK_MGMT_SQ_SIZE. But I've given up on the naming issue for now since I don't have a better suggestion, and you do remove a magic constant, which is good.

Put the assignment on the same line as the declaration and I'll deal with the name.

Quoted 2 lines · David Dillow

Put the assignment on the same line as the declaration and I'll deal with the name.

Putting the rsv assignment on the same line as its declaration would make that line exceed 80 columns and hence would trigger a checkpatch complaint, so it's better to keep it as it is now.

Quoted 7 lines · Bart Van Assche

Quoted 2 lines · David Dillow

Put the assignment on the same line as the declaration and I'll deal with the name.

Putting the rsv assignment on the same line as its declaration would make that line exceed 80 columns and hence would trigger a checkpatch complaint, so it's better to keep it as it is now.

I didn't check it with checkpatch, but vi tells me it is 78 characters.

Quoted 3 lines

Putting the rsv assignment on the same line as its declaration would make that line exceed 80 columns and hence would trigger a checkpatch complaint, so it's better to keep it as it is now.

I didn't check it with checkpatch, but vi tells me it is 78 characters.

 > > Putting the rsv assignment on the same line as its declaration would > > make that line exceed 80 columns and hence would trigger a checkpatch > > complaint, so it's better to keep it as it is now.

 > I didn't check it with checkpatch, but vi tells me it is 78 characters.

In any case I think the 80 column thing should be treated as a flexible guideline, not a strict rule.  If the code looks significantly better with an 85 or 90 column line than split across two lines, then I think it's better to keep it on one line.