[v4,01/50] IB: Add CNP opcode enumeration.

Commit Message

Marciniszyn, Mike July 30, 2015, 7:17 p.m. UTC

From: Dennis Dalessandro <[e-mail]>

This patch adds the value of the CNP opcode to the existing list of enumerated
opcodes.

Reviewed-by: Mike Marciniszyn <[e-mail]>
Signed-off-by: Dennis Dalessandro <[e-mail]>
---
 include/rdma/ib_pack.h |    1 +
 1 file changed, 1 insertion(+)


--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Christoph Lameter (Ampere) July 30, 2015, 8:35 p.m. UTC | #1

On Thu, 30 Jul 2015, Mike Marciniszyn wrote:

> This patch adds the value of the CNP opcode to the existing list of enumerated
> opcodes.

That is obvious and useless. Patches should have a meaningful
description and justify the changes.

Why do you add the CNP opcode and what in the world does it do? CNP is
what? And why do the other enum values not work for you?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marciniszyn, Mike July 30, 2015, 8:43 p.m. UTC | #2

> That is obvious and useless. Patches should have a meaningful description
> and justify the changes.
> 

The driver uses the CNP opcode for congestion control.

> Why do you add the CNP opcode and what in the world does it do? CNP is
> what? And why do the other enum values not work for you?

The driver supports congestion control in software vs. outboard firmware, so the opcode should be available in the appropriate kernel include file.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Lameter (Ampere) July 30, 2015, 8:56 p.m. UTC | #3

On Thu, 30 Jul 2015, Marciniszyn, Mike wrote:

> > That is obvious and useless. Patches should have a meaningful description
> > and justify the changes.
> >
>
> The driver uses the CNP opcode for congestion control.

And that requires a new transport protocol???

> > Why do you add the CNP opcode and what in the world does it do? CNP is
> > what? And why do the other enum values not work for you?
>
> The driver supports congestion control in software vs. outboard
> firmware, so the opcode should be available in the appropriate kernel
> include file.

So is CNP an operation or a protocol?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marciniszyn, Mike July 30, 2015, 9:13 p.m. UTC | #4

> > > That is obvious and useless. Patches should have a meaningful
> > > description and justify the changes.
> > >
> >
> > The driver uses the CNP opcode for congestion control.
> 
> And that requires a new transport protocol???
> 

The opcode is 0x80, which appears in the protocol part of the 8 bit opcode.  That is what is specified in A3.10.2 of the 1.3 spec.

That also happens to land in the upper bits of the opcode. 

Would this fit better with a  IB_OPCODE_CNP_TRANS (0x80) with a single opcode of IB_OPCODE_CNP_OP 0x00, combined with the IB_OPCODE macro to produce IB_OPCODE_CNP?

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Marciniszyn, Mike July 30, 2015, 9:18 p.m. UTC | #5

> The opcode is 0x80, which appears in the protocol part of the 8 bit opcode.
> That is what is specified in A3.10.2 of the 1.3 spec.
> 

Correction A10.3.2.

Mike
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Ira Weiny July 30, 2015, 10:09 p.m. UTC | #6

On Thu, Jul 30, 2015 at 09:18:17PM +0000, Marciniszyn, Mike wrote:
> > The opcode is 0x80, which appears in the protocol part of the 8 bit opcode.
> > That is what is specified in A3.10.2 of the 1.3 spec.
> > 
> 
> Correction A10.3.2.

This value also appears in Table 38 of section 9.2 where the other OpCodes
which appear in this enum are defined.

So it does appear to be the correct place to put this value.

Ira

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Lameter (Ampere) July 31, 2015, 2 p.m. UTC | #7

On Thu, 30 Jul 2015, ira.weiny wrote:

> On Thu, Jul 30, 2015 at 09:18:17PM +0000, Marciniszyn, Mike wrote:
> > > The opcode is 0x80, which appears in the protocol part of the 8 bit opcode.
> > > That is what is specified in A3.10.2 of the 1.3 spec.
> > >
> >
> > Correction A10.3.2.
>
> This value also appears in Table 38 of section 9.2 where the other OpCodes
> which appear in this enum are defined.
>
> So it does appear to be the correct place to put this value.

Ok some more wording please in the description and the ref to the
standard as a comment in the code?

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/include/rdma/ib_pack.h b/include/rdma/ib_pack.h
index b1f7592..03f5217 100644
--- a/include/rdma/ib_pack.h
+++ b/include/rdma/ib_pack.h
@@ -76,6 +76,7 @@  enum {
 	IB_OPCODE_UC                                = 0x20,
 	IB_OPCODE_RD                                = 0x40,
 	IB_OPCODE_UD                                = 0x60,
+	IB_OPCODE_CNP                               = 0x80,
 
 	/* operations -- just used to define real constants */
 	IB_OPCODE_SEND_FIRST                        = 0x00,