Re: [PATCH net-next v5] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
On 10/10/18, 11:12 AM, "justin.l...@dell.com" wrote:
The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
to send NC-SI command to the network card.
Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and
response.
The work flow is as below.
Request:
User space application
-> Netlink interface (msg)
-> new Netlink handler - ncsi_send_cmd_nl()
-> ncsi_xmit_cmd()
Response:
Response received - ncsi_rcv_rsp()
-> internal response handler - ncsi_rsp_handler_xxx()
-> ncsi_rsp_handler_netlink()
-> ncsi_send_netlink_rsp ()
-> Netlink interface (msg)
-> user space application
Command timeout - ncsi_request_timeout()
-> ncsi_send_netlink_timeout ()
-> Netlink interface (msg with zero data length)
-> user space application
Error:
Error detected
-> ncsi_send_netlink_err ()
-> Netlink interface (err msg)
-> user space application
Signed-off-by: Justin Lee
Reviewed-by : Vijay Khemka
Re: [PATCH net-next v4] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
Hi Justin,
Please see minor comments below.
Regards
-Vijay
On 10/10/18, 9:01 AM, "justin.l...@dell.com" wrote:
The new command (NCSI_CMD_SEND_CMD) is added to allow user space application
to send NC-SI command to the network card.
Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and
response.
The work flow is as below.
Request:
User space application
-> Netlink interface (msg)
-> new Netlink handler - ncsi_send_cmd_nl()
-> ncsi_xmit_cmd()
Response:
Response received - ncsi_rcv_rsp()
-> internal response handler - ncsi_rsp_handler_xxx()
-> ncsi_rsp_handler_netlink()
-> ncsi_send_netlink_rsp ()
-> Netlink interface (msg)
-> user space application
Command timeout - ncsi_request_timeout()
-> ncsi_send_netlink_timeout ()
-> Netlink interface (msg with zero data length)
-> user space application
Error:
Error detected
-> ncsi_send_netlink_err ()
-> Netlink interface (err msg)
-> user space application
Signed-off-by: Justin Lee
---
V4: Update comments and remove some debug message.
V3: Based on
https://urldefense.proofpoint.com/v2/url?u=http-3A__patchwork.ozlabs.org_patch_979688_=DwIGaQ=5VD0RTtNlTh3ycd41b3MUw=v9MU0Ki9pWnTXCWwjHPVgpnCR80vXkkcrIaqU7USl5g=38KSrF_ThuoRmouLx-xKKkkpk9EfhNFEEqXbduQqQpE=GRXR1sIEzNo7xDKyUWS9t1Ita6vxEX_llzMx2azueVc=
to remove the duplicated code.
V2: Remove non-related debug message and clean up the code.
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -175,6 +175,8 @@ struct ncsi_package;
#define NCSI_RESERVED_CHANNEL 0x1f
#define NCSI_CHANNEL_INDEX(c) ((c) & ((1 << NCSI_PACKAGE_SHIFT) - 1))
#define NCSI_TO_CHANNEL(p, c) (((p) << NCSI_PACKAGE_SHIFT) | (c))
+#define NCSI_MAX_PACKAGE 8
+#define NCSI_MAX_CHANNEL 32
struct ncsi_channel {
unsigned char id;
@@ -219,12 +221,17 @@ struct ncsi_request {
unsigned charid; /* Request ID - 0 to 255 */
bool used;/* Request that has been assigned */
unsigned int flags; /* NCSI request property */
-#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
+#define NCSI_REQ_FLAG_EVENT_DRIVEN 1
Why extra space added above?
+#define NCSI_REQ_FLAG_NETLINK_DRIVEN 2
struct ncsi_dev_priv *ndp;/* Associated NCSI device */
struct sk_buff *cmd;/* Associated NCSI command packet */
struct sk_buff *rsp;/* Associated NCSI response packet */
struct timer_listtimer; /* Timer on waiting for response */
bool enabled; /* Time has been enabled or not*/
+
+ u32 snd_seq; /* netlink sending sequence number */
+ u32 snd_portid; /* netlink portid of sender*/
+ struct nlmsghdr nlhdr; /* netlink message header */
};
Extra line inside structure may not be required.
enum {
@@ -310,6 +317,7 @@ struct ncsi_cmd_arg {
unsigned int dwords[4];
};
unsigned char*data; /* NCSI OEM data */
+ struct genl_info *info; /* Netlink information */
};
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index 45f33d6..62e191f 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -20,6 +20,7 @@
#include
#include "internal.h"
+#include "ncsi-pkt.h"
#include "ncsi-netlink.h"
static struct genl_family ncsi_genl_family;
@@ -29,6 +30,7 @@ static const struct nla_policy
ncsi_genl_policy[NCSI_ATTR_MAX + 1] = {
[NCSI_ATTR_PACKAGE_LIST] = { .type = NLA_NESTED },
[NCSI_ATTR_PACKAGE_ID] ={ .type = NLA_U32 },
[NCSI_ATTR_CHANNEL_ID] ={ .type = NLA_U32 },
+ [NCSI_ATTR_DATA] = { .type = NLA_BINARY, .len = 2048 },
};
static struct ncsi_dev_priv *ndp_from_ifindex(struct net *net, u32 ifindex)
@@ -366,6 +368,198 @@ static int ncsi_clear_interface_nl(struct sk_buff
*msg, struct genl_info *info)
return 0;
}
+static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info)
+{
+ struct ncsi_dev_priv *ndp;
+
+ struct ncsi_cmd_arg nca;
+ struct ncsi_pkt_hdr *hdr;
+
+ u32 package_id, channel_id;
+ unsigned char *data;
+ int len, ret;
+
+ if (!info || !info->attrs) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (!info->attrs[NCSI_ATTR_IFINDEX]) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ if (!info->attrs[NCSI_ATTR_PACKAGE_ID]) {
+
Re: [PATCH v3] net/ncsi: Add NCSI OEM command support
On 10/4/18, 9:48 PM, "Samuel Mendoza-Jonas" wrote:
On Wed, 2018-10-03 at 16:32 -0700, Vijay Khemka wrote:
> This patch adds OEM commands and response handling. It also defines OEM
> command and response structure as per NCSI specification along with its
> handlers.
>
> ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> commands
> ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
>
> Signed-off-by: Vijay Khemka
Reviewed-by: Samuel Mendoza-Jonas
Technically this patch should also be marked [PATCH net-next] to target
David's next tree.
Thanks Sam, I will mark it and send it again to larger audience.
[PATCH v3] net/ncsi: Add NCSI OEM command support
This patch adds OEM commands and response handling. It also defines OEM
command and response structure as per NCSI specification along with its
handlers.
ncsi_cmd_handler_oem: This is a generic command request handler for OEM
commands
ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
Signed-off-by: Vijay Khemka
---
net/ncsi/internal.h | 5 +
net/ncsi/ncsi-cmd.c | 30 +++---
net/ncsi/ncsi-pkt.h | 14 ++
net/ncsi/ncsi-rsp.c | 43 ++-
4 files changed, 88 insertions(+), 4 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e3965cef..3d0a33b874f5 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -68,6 +68,10 @@ enum {
NCSI_MODE_MAX
};
+/* OEM Vendor Manufacture ID */
+#define NCSI_OEM_MFR_MLX_ID 0x8119
+#define NCSI_OEM_MFR_BCM_ID 0x113d
+
struct ncsi_channel_version {
u32 version;/* Supported BCD encoded NCSI version */
u32 alpha2; /* Supported BCD encoded NCSI version */
@@ -305,6 +309,7 @@ struct ncsi_cmd_arg {
unsigned short words[8];
unsigned int dwords[4];
};
+ unsigned char*data; /* NCSI OEM data */
};
extern struct list_head ncsi_dev_list;
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63aae2..82b7d9201db8 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -211,6 +211,25 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
return 0;
}
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+ struct ncsi_cmd_arg *nca)
+{
+ struct ncsi_cmd_oem_pkt *cmd;
+ unsigned int len;
+
+ len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
+ if (nca->payload < 26)
+ len += 26;
+ else
+ len += nca->payload;
+
+ cmd = skb_put_zero(skb, len);
+ memcpy(>mfr_id, nca->data, nca->payload);
+ ncsi_cmd_build_header(>cmd.common, nca);
+
+ return 0;
+}
+
static struct ncsi_cmd_handler {
unsigned char type;
int payload;
@@ -244,7 +263,7 @@ static struct ncsi_cmd_handler {
{ NCSI_PKT_CMD_GNS,0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GPS,0, ncsi_cmd_handler_default },
- { NCSI_PKT_CMD_OEM,0, NULL },
+ { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
{ NCSI_PKT_CMD_PLDM, 0, NULL },
{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
};
@@ -316,8 +335,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
return -ENOENT;
}
- /* Get packet payload length and allocate the request */
- nca->payload = nch->payload;
+ /* Get packet payload length and allocate the request
+* It is expected that if length set as negative in
+* handler structure means caller is initializing it
+* and setting length in nca before calling xmit function
+*/
+ if (nch->payload >= 0)
+ nca->payload = nch->payload;
nr = ncsi_alloc_command(nca);
if (!nr)
return -ENOMEM;
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..0f2087c8d42a 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -151,6 +151,20 @@ struct ncsi_cmd_snfc_pkt {
unsigned char pad[22];
};
+/* OEM Request Command as per NCSI Specification */
+struct ncsi_cmd_oem_pkt {
+ struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[]; /* OEM Payload Data */
+};
+
+/* OEM Response Packet as per NCSI Specification */
+struct ncsi_rsp_oem_pkt {
+ struct ncsi_rsp_pkt_hdr rsp; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[]; /* Payload data */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp;/* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 930c1d3796f0..d66b34749027 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,6 +596,47 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+static struct ncsi_rsp_oem_handler {
+ unsigned intmfr_id;
+ int (*handler)(struct ncsi_request *nr);
+} ncsi_rsp_oem_handlers[] = {
+ { NCSI_OEM_MFR_MLX_ID, NULL },
+ { NCSI_OEM_MFR_BCM_ID, NULL }
+};
+
+/* Response handler for OEM command */
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_rsp_oem_handler *nrh
Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
Hi Justin,
Please see comments below
-Vijay
> > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > > index 7567ca63aae2..2f98533eba46 100644
> > > --- a/net/ncsi/ncsi-cmd.c
> > > +++ b/net/ncsi/ncsi-cmd.c
> > > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff
*skb,
> > > return 0;
> > > }
> > >
> > > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > > + struct ncsi_cmd_arg *nca)
> > > +{
> > > + struct ncsi_cmd_oem_pkt *cmd;
>
> >OEM command doesn't not have the fixed data size. Should we use pointer
instead?
> >struct ncsi_cmd_oem_pkt {
> > struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
> > __be32 mfr_id; /* Manufacture ID*/
> > unsigned char *data; /* OEM Payload Data */
> >};
> Yes, I agree that OEM command doesn't have fixed data but to map to
skbuff structure,
> I have defined above structure as per NCSI spec, We can certainly have a
MAX_DATA_LEN define.
The spec only defines manufacture ID field and the start offset of vendor
data.
If we just want to point to the vendor data, we don't need to specify the
max length and
we can use flexible array as the last member (correct the typo above).
struct ncsi_cmd_oem_pkt {
struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
__be32 mfr_id; /* Manufacture ID*/
unsigned char data[]; /* OEM Payload Data */
};
I agree and will add this change.
> > > + unsigned int len;
> > > +
> > > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > > + if (nca->payload < 26)
> > > + len += 26;
>
> >Why does it add 26? I knew the other place in ncsi_alloc_command() is
also add 26.
> >If it is less than 26, then it should be a fixed size of structure
ncsi_cmd_pkt (46), right?
> It adds 26 because It has already assigned len to hdr+4 which is 16+4 =
20 bytes. By
> adding 26 it makes it to 46. I am just being consistent with other
portion of code.
>
> > > + else
> > > + len += nca->payload;
> > > +
> > > + cmd = skb_put_zero(skb, len);
> > > + cmd->mfr_id = nca->dwords[0];
> > > + memcpy(cmd->data, >dwords[1], nca->payload - 4);
>
> >Netlink request is using the new nca->data pointer to pass the data as
the request payload
> >is not the same size and some command payload is bigger than 16 bytes.
> >Will you consider to use the same data pointer? So, we don't need to
have a checking here.
> >If the command is used less than 16 bytes, we can simply assigned
>bytes[0] to it.
> To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I
don't see any issue in
> Copying data from data pointer from nca but user needs to be aware if it
is less than 16 bytes then
> use bytes or use data pointer. To keep it simple, we should simply define
MAX_LEN.
I use the pointer to avoid copying the data again. OEM handler can always
process the flexible payload
through the data pointer. It can depend on the caller to use
stack/allocated buffer/nca buffer and
it will be straight forward if all OEM stuff is using the data pointer to
process the flexible
payload.
Do we use data pointer for payload less than 16 bytes as well in OEM. We copy
data only once while adding to skbuff.
I have no issue, I will make this change but wait for other comments.
>
> > > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > > index 91b4b66438df..1f338386810d 100644
> > > --- a/net/ncsi/ncsi-pkt.h
> > > +++ b/net/ncsi/ncsi-pkt.h
> > > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > > unsigned char pad[22];
> > > };
> > >
> > > +/* OEM Request Command as per NCSI Specification */
> > > +struct ncsi_cmd_oem_pkt {
> > > + struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
> > > + __be32 mfr_id; /* Manufacture ID*/
> > > + unsigned char data[64];/* OEM Payload Data */
> > > + __be32 checksum;/* Checksum */
> > > +};
> > > +
> > > +/* OEM Response Packet as per NCSI Specification */
> > > +struct ncsi_rsp_oem_pkt {
> > > + struct ncsi_rsp_pkt_hdr rsp; /* Command header*/
> > > + __be32 mfr_id; /* Manufacture ID*/
> > > + unsigned char data[64];/* Payload data */
> > > + __be32 checksum;/* Checksum */
> > > +};
> > > +
>
> >OEM command doesn't not have the fixed response data size too.
> >Should we use pointer instead?
>
> Here also we can
Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
Hi Justin,
Thanks for response. Please see my comments below.
-Vijay
> > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> > index 7567ca63aae2..2f98533eba46 100644
> > --- a/net/ncsi/ncsi-cmd.c
> > +++ b/net/ncsi/ncsi-cmd.c
> > @@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff
*skb,
> > return 0;
> > }
> >
> > +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> > + struct ncsi_cmd_arg *nca)
> > +{
> > + struct ncsi_cmd_oem_pkt *cmd;
>OEM command doesn't not have the fixed data size. Should we use pointer
instead?
>struct ncsi_cmd_oem_pkt {
> struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
> __be32 mfr_id; /* Manufacture ID*/
> unsigned char *data; /* OEM Payload Data */
>};
Yes, I agree that OEM command doesn't have fixed data but to map to skbuff
structure,
I have defined above structure as per NCSI spec, We can certainly have a
MAX_DATA_LEN define.
> > + unsigned int len;
> > +
> > + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> > + if (nca->payload < 26)
> > + len += 26;
>Why does it add 26? I knew the other place in ncsi_alloc_command() is also
add 26.
>If it is less than 26, then it should be a fixed size of structure
ncsi_cmd_pkt (46), right?
It adds 26 because It has already assigned len to hdr+4 which is 16+4 = 20
bytes. By
adding 26 it makes it to 46. I am just being consistent with other portion
of code.
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + cmd->mfr_id = nca->dwords[0];
> > + memcpy(cmd->data, >dwords[1], nca->payload - 4);
>Netlink request is using the new nca->data pointer to pass the data as the
request payload
>is not the same size and some command payload is bigger than 16 bytes.
>Will you consider to use the same data pointer? So, we don't need to have
a checking here.
>If the command is used less than 16 bytes, we can simply assigned
>bytes[0] to it.
To keep original structure, we can change 16 bytes to MAX_DATA_LEN. Or I
don't see any issue in
Copying data from data pointer from nca but user needs to be aware if it is
less than 16 bytes then
use bytes or use data pointer. To keep it simple, we should simply define
MAX_LEN.
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..1f338386810d 100644
> > --- a/net/ncsi/ncsi-pkt.h
> > +++ b/net/ncsi/ncsi-pkt.h
> > @@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
> > unsigned char pad[22];
> > };
> >
> > +/* OEM Request Command as per NCSI Specification */
> > +struct ncsi_cmd_oem_pkt {
> > + struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
> > + __be32 mfr_id; /* Manufacture ID*/
> > + unsigned char data[64];/* OEM Payload Data */
> > + __be32 checksum;/* Checksum */
> > +};
> > +
> > +/* OEM Response Packet as per NCSI Specification */
> > +struct ncsi_rsp_oem_pkt {
> > + struct ncsi_rsp_pkt_hdr rsp; /* Command header*/
> > + __be32 mfr_id; /* Manufacture ID*/
> > + unsigned char data[64];/* Payload data */
> > + __be32 checksum;/* Checksum */
> > +};
> > +
>OEM command doesn't not have the fixed response data size too.
>Should we use pointer instead?
Here also we can define MAX_DATA_LEN because data pointer won't map to skb
directly.
Re: [Potential Spoof] Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
On 9/28/18, 6:21 PM, "Linux-aspeed on behalf of Vijay Khemka" wrote: > On 9/28/18, 6:07 PM, "Vijay Khemka" wrote: > This patch adds OEM commands and response handling. It also defines OEM > command and response structure as per NCSI specification along with its > handlers. > > ncsi_cmd_handler_oem: This is a generic command request handler for OEM > commands > ncsi_rsp_handler_oem: This is a generic response handler for OEM commands This is a generic patch for OEM command handling, There will be another patch following this to handle specific OEM commands for each vendor. Currently I have defined 2 vendor/manufacturer id below in internal.h, more can be added here for other vendors. I have not defined ncsi_rsp_oem_handler in this patch as they are NULL, but there will be a defined handlers for each vendor in next patch. Sam, Joel, Justin, please review this patch. I have 2 more patch coming for Broadcom and Mellanox.
Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
> On 9/28/18, 6:07 PM, "Vijay Khemka" wrote:
> This patch adds OEM commands and response handling. It also defines OEM
> command and response structure as per NCSI specification along with its
> handlers.
>
> ncsi_cmd_handler_oem: This is a generic command request handler for OEM
> commands
> ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
This is a generic patch for OEM command handling, There will be another patch
following this to handle specific OEM commands for each vendor. Currently I
have
defined 2 vendor/manufacturer id below in internal.h, more can be added here
for
other vendors. I have not defined ncsi_rsp_oem_handler in this patch as they
are
NULL, but there will be a defined handlers for each vendor in next patch.
Signed-off-by: Vijay Khemka
---
net/ncsi/internal.h | 4
net/ncsi/ncsi-cmd.c | 31 ---
net/ncsi/ncsi-pkt.h | 16
net/ncsi/ncsi-rsp.c | 44 +++-
4 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e3965cef..c16cb7223064 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -68,6 +68,10 @@ enum {
NCSI_MODE_MAX
};
+/* OEM Vendor Manufacture ID */
+#define NCSI_OEM_MFR_MLX_ID 0x8119
+#define NCSI_OEM_MFR_BCM_ID 0x113d
+
struct ncsi_channel_version {
u32 version;/* Supported BCD encoded NCSI version */
u32 alpha2; /* Supported BCD encoded NCSI version */
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63aae2..2f98533eba46 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
return 0;
}
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+ struct ncsi_cmd_arg *nca)
+{
+ struct ncsi_cmd_oem_pkt *cmd;
+ unsigned int len;
+
+ len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
+ if (nca->payload < 26)
+ len += 26;
+ else
+ len += nca->payload;
+
+ cmd = skb_put_zero(skb, len);
+ cmd->mfr_id = nca->dwords[0];
+ memcpy(cmd->data, >dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(>cmd.common, nca);
+
+ return 0;
+}
+
static struct ncsi_cmd_handler {
unsigned char type;
int payload;
@@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
{ NCSI_PKT_CMD_GNS,0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GPS,0, ncsi_cmd_handler_default },
- { NCSI_PKT_CMD_OEM,0, NULL },
+ { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
{ NCSI_PKT_CMD_PLDM, 0, NULL },
{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
};
@@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
return -ENOENT;
}
- /* Get packet payload length and allocate the request */
- nca->payload = nch->payload;
+ /* Get packet payload length and allocate the request
+* It is expected that if length set as negative in
+* handler structure means caller is initializing it
+* and setting length in nca before calling xmit function
+*/
+ if (nch->payload >= 0)
+ nca->payload = nch->payload;
nr = ncsi_alloc_command(nca);
if (!nr)
return -ENOMEM;
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..1f338386810d 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
unsigned char pad[22];
};
+/* OEM Request Command as per NCSI Specification */
+struct ncsi_cmd_oem_pkt {
+ struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[64];/* OEM Payload Data */
+ __be32 checksum;/* Checksum */
+};
+
+/* OEM Response Packet as per NCSI Specification */
+struct ncsi_rsp_oem_pkt {
+ struct ncsi_rsp_pkt_hdr rsp; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[64];/* Payload data */
+ __be32 checksum;/* Checksum */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp;/* Response h
[PATCH v2] net/ncsi: Add NCSI OEM command support
This patch adds OEM commands and response handling. It also defines OEM
command and response structure as per NCSI specification along with its
handlers.
ncsi_cmd_handler_oem: This is a generic command request handler for OEM
commands
ncsi_rsp_handler_oem: This is a generic response handler for OEM commands
Signed-off-by: Vijay Khemka
---
net/ncsi/internal.h | 4
net/ncsi/ncsi-cmd.c | 31 ---
net/ncsi/ncsi-pkt.h | 16
net/ncsi/ncsi-rsp.c | 44 +++-
4 files changed, 91 insertions(+), 4 deletions(-)
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 8055e3965cef..c16cb7223064 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -68,6 +68,10 @@ enum {
NCSI_MODE_MAX
};
+/* OEM Vendor Manufacture ID */
+#define NCSI_OEM_MFR_MLX_ID 0x8119
+#define NCSI_OEM_MFR_BCM_ID 0x113d
+
struct ncsi_channel_version {
u32 version;/* Supported BCD encoded NCSI version */
u32 alpha2; /* Supported BCD encoded NCSI version */
diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index 7567ca63aae2..2f98533eba46 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -211,6 +211,26 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
return 0;
}
+static int ncsi_cmd_handler_oem(struct sk_buff *skb,
+ struct ncsi_cmd_arg *nca)
+{
+ struct ncsi_cmd_oem_pkt *cmd;
+ unsigned int len;
+
+ len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
+ if (nca->payload < 26)
+ len += 26;
+ else
+ len += nca->payload;
+
+ cmd = skb_put_zero(skb, len);
+ cmd->mfr_id = nca->dwords[0];
+ memcpy(cmd->data, >dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(>cmd.common, nca);
+
+ return 0;
+}
+
static struct ncsi_cmd_handler {
unsigned char type;
int payload;
@@ -244,7 +264,7 @@ static struct ncsi_cmd_handler {
{ NCSI_PKT_CMD_GNS,0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GNPTS, 0, ncsi_cmd_handler_default },
{ NCSI_PKT_CMD_GPS,0, ncsi_cmd_handler_default },
- { NCSI_PKT_CMD_OEM,0, NULL },
+ { NCSI_PKT_CMD_OEM, -1, ncsi_cmd_handler_oem },
{ NCSI_PKT_CMD_PLDM, 0, NULL },
{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
};
@@ -316,8 +336,13 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
return -ENOENT;
}
- /* Get packet payload length and allocate the request */
- nca->payload = nch->payload;
+ /* Get packet payload length and allocate the request
+* It is expected that if length set as negative in
+* handler structure means caller is initializing it
+* and setting length in nca before calling xmit function
+*/
+ if (nch->payload >= 0)
+ nca->payload = nch->payload;
nr = ncsi_alloc_command(nca);
if (!nr)
return -ENOMEM;
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..1f338386810d 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -151,6 +151,22 @@ struct ncsi_cmd_snfc_pkt {
unsigned char pad[22];
};
+/* OEM Request Command as per NCSI Specification */
+struct ncsi_cmd_oem_pkt {
+ struct ncsi_cmd_pkt_hdr cmd; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[64];/* OEM Payload Data */
+ __be32 checksum;/* Checksum */
+};
+
+/* OEM Response Packet as per NCSI Specification */
+struct ncsi_rsp_oem_pkt {
+ struct ncsi_rsp_pkt_hdr rsp; /* Command header*/
+ __be32 mfr_id; /* Manufacture ID*/
+ unsigned char data[64];/* Payload data */
+ __be32 checksum;/* Checksum */
+};
+
/* Get Link Status */
struct ncsi_rsp_gls_pkt {
struct ncsi_rsp_pkt_hdr rsp;/* Response header */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 930c1d3796f0..22664ebdc93a 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,6 +596,48 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
return 0;
}
+static struct ncsi_rsp_oem_handler {
+ unsigned intmfr_id;
+ int (*handler)(struct ncsi_request *nr);
+} ncsi_rsp_oem_handlers[] = {
+ { NCSI_OEM_MFR_MLX_ID, NULL },
+ { NCSI_OEM_MFR_BCM_ID, NULL }
+};
+
+
+/* Response handler for OEM command */
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+ struct ncsi_rsp_oem_pkt *rsp;
+ struct ncsi_rsp_oem_handler *nrh = NULL;
+ unsigned int mfr_id, i;
+
+ /* Get the response hea
Re: [PATCH net v2] net/ncsi: Extend NC-SI Netlink interface to allow user space to send NC-SI command
>On 9/28/18, 11:16 AM, "justin.l...@dell.com" wrote: > The new command (NCSI_CMD_SEND_CMD) is added to allow user space application > to send NC-SI command to the network card. > Also, add a new attribute (NCSI_ATTR_DATA) for transferring request and response. > > The work flow is as below. > > Request: > User space application -> Netlink interface (msg) -> new Netlink handler - ncsi_send_cmd_nl() -> ncsi_xmit_cmd() > Response: > Response received - ncsi_rcv_rsp() -> internal response handler - ncsi_rsp_handler_xxx() -> ncsi_rsp_handler_netlink() -> ncsi_send_netlink_rsp () -> Netlink interface (msg) -> user space application > Command timeout - ncsi_request_timeout() -> ncsi_send_netlink_timeout () -> Netlink interface (msg with zero data length) -> user space application > Error: > Error detected -> ncsi_send_netlink_err () -> Netlink interface (err msg) -> user space application I will request to keep 2 patch, one for OEM handler and other one for netlink user space handling.
Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
On 9/26/18, 8:44 PM, "Samuel Mendoza-Jonas" wrote:
>Hi Vijay,
>Having had a chance to take a closer look, there is probably room for
> both this patchset and Justin's potential changes to coexist; while
> Justin's is a more general solution for sending arbitrary commands, the
>approach of this patch is also useful for handling commands we want
>included in the configure process (such as get-mac-address).
Hi Sam,
I have created a more generic approach based patch, will send you after clean
up. I agree we need both Justin patch as well as mine to handle OEM specific
command. I am creating 2 patches one for generic OEM command and response
handling and another is OEM vendor specific command handling. Please see my
responses to your comments below.
Some comments below:
> ---
> net/ncsi/Kconfig | 3 ++
> net/ncsi/internal.h| 11 +--
> net/ncsi/ncsi-cmd.c| 24 +--
> net/ncsi/ncsi-manage.c | 68 ++
> net/ncsi/ncsi-pkt.h| 16 ++
> net/ncsi/ncsi-rsp.c| 33 +++-
> 6 files changed, 149 insertions(+), 6 deletions(-)
>
> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 08a8a6031fd7..b8bf89fea7c8 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -10,3 +10,6 @@ config NET_NCSI
> support. Enable this only if your system connects to a network
> device via NCSI and the ethernet driver you're using supports
> the protocol explicitly.
> +config NCSI_OEM_CMD_GET_MAC
> + bool "Get NCSI OEM MAC Address"
> + depends on NET_NCSI
For the moment this isn't too bad but I wonder if in the future it would
be more flexible to have something like NCSI_OEM_CMD_MELLANOX etc, so we
could selectively enable a class of OEM commands based on vendor rather
than per-command.
Certainly, we can have like that and will be an addition to above config.
Above config is to enable retrieving and setting mac address from device
during channel configuration. And then we can have vendor selection like
MELLANOX, Broadcom etc. But I will rather check GV ID under this config
and see if there is available function for specific vendor. This can be
revised
and made more generic based on vendor.
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e3965cef..da17958e6a4b 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -68,6 +68,10 @@ enum {
> NCSI_MODE_MAX
> };
>
> +#define NCSI_OEM_MFR_MLX_ID 0x8119
> +#define NCSI_OEM_MLX_CMD_GET_MAC0x1b00
> +#define NCSI_OEM_MLX_CMD_SET_AFFINITY 0x010700
I gather this is part of the OEM command but it would be good to
describe
what these bits mean. Is this command documented anywhere by Mellanox?
I will add more description here, please see in next patch. Yes Mellanox
specification describes these commands.
> +
> struct ncsi_channel_version {
> u32 version;/* Supported BCD encoded NCSI version */
> u32 alpha2; /* Supported BCD encoded NCSI version */
> @@ -236,6 +240,7 @@ enum {
> ncsi_dev_state_probe_dp,
> ncsi_dev_state_config_sp= 0x0301,
> ncsi_dev_state_config_cis,
> + ncsi_dev_state_config_oem_gma,
> ncsi_dev_state_config_clear_vids,
> ncsi_dev_state_config_svf,
> ncsi_dev_state_config_ev,
> @@ -301,9 +306,9 @@ struct ncsi_cmd_arg {
> unsigned short payload; /* Command packet payload length */
> unsigned int req_flags; /* NCSI request properties */
> union {
> - unsigned char bytes[16]; /* Command packet specific data */
> - unsigned short words[8];
> - unsigned int dwords[4];
> + unsigned char bytes[64]; /* Command packet specific data */
> + unsigned short words[32];
> + unsigned int dwords[16];
> };
> };
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63aae2..3205e22c1734 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -211,6 +211,25 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_oem_pkt *cmd;
> + unsigned int len;
> +
> + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> + if (nca->payload < 26)
> + len += 26;
This will have already happened in ncsi_alloc_command(), is this check
needed?
Yes it is needed to find length for skbuff data to initialize.
ncsi_alloc_command() does the same and allocate skbuff.
Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
>Hi Vijay, > Thanks for the patch; before I get too into a review though I'd like to > loop in Justin (cc'd) who I know is also working on an OEM command patch. > The changes here are very specific (eg. a command specific config option > "CONFIG_NCSI_OEM_CMD_GET_MAC"), which is ok on a small scale but if we > start to add an increasing amount of commands could get out of hand. > As I understand Justin's version adds a generic handler, using the NCSI > Netlink interface to pass OEM commands and responses to and from > userspace, which does the actual packet handling. > It would be good to compare these two approaches first before committing > to any one path Hi Sam, My oem command handler is generic and can be used for any oem commands and oem response handler can be made more generic. We can certainly write a wrapper to support netlink oem command to receive form user space. There are Mellanox specific functions which sends Mellanox specific request. These needed as a part of initial configuration. We can remove Kconfig option with more generic approach. -Vijay
Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
Hi Joel,
Thanks, I am adding netdev mailing list here.
Yes, this command is supported for all Mellanox card. It is as per Mellanox
specification.
Regards
-Vijay
On 9/24/18, 5:30 PM, "Joel Stanley" wrote:
Hi Vijay,
On Tue, 25 Sep 2018 at 09:39, Vijay Khemka wrote:
>
> This patch adds OEM command to get mac address from NCSI device and and
> configure the same to the network card.
>
> ncsi_cmd_arg - Modified this structure to include bigger payload data.
> ncsi_cmd_handler_oem: This function handes oem command request
> ncsi_rsp_handler_oem: This function handles response for OEM command.
> get_mac_address_oem_mlx: This function will send OEM command to get
> mac address for Mellanox card
> set_mac_affinity_mlx: This will send OEM command to set Mac affinity
> for Mellanox card
Thanks for the patch. The code looks okay, but I wanted to get some
input from our NCSI maintainer as to how OEM commands would be
structured. Sam, can you please provide some review here?
Is the command supported on all melanox cards, just some, or does
TiogaPass have a special firmware that enables it?
We should include the netdev mailing list in this discussion as the
patch needs to be acceptable for upstream.
Cheers,
Joel
>
> Signed-off-by: Vijay Khemka
> ---
> net/ncsi/Kconfig | 3 ++
> net/ncsi/internal.h| 11 +--
> net/ncsi/ncsi-cmd.c| 24 +--
> net/ncsi/ncsi-manage.c | 68 ++
> net/ncsi/ncsi-pkt.h| 16 ++
> net/ncsi/ncsi-rsp.c| 33 +++-
> 6 files changed, 149 insertions(+), 6 deletions(-)
>
> diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
> index 08a8a6031fd7..b8bf89fea7c8 100644
> --- a/net/ncsi/Kconfig
> +++ b/net/ncsi/Kconfig
> @@ -10,3 +10,6 @@ config NET_NCSI
> support. Enable this only if your system connects to a network
> device via NCSI and the ethernet driver you're using supports
> the protocol explicitly.
> +config NCSI_OEM_CMD_GET_MAC
> + bool "Get NCSI OEM MAC Address"
> + depends on NET_NCSI
> diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
> index 8055e3965cef..da17958e6a4b 100644
> --- a/net/ncsi/internal.h
> +++ b/net/ncsi/internal.h
> @@ -68,6 +68,10 @@ enum {
> NCSI_MODE_MAX
> };
>
> +#define NCSI_OEM_MFR_MLX_ID 0x8119
> +#define NCSI_OEM_MLX_CMD_GET_MAC0x1b00
> +#define NCSI_OEM_MLX_CMD_SET_AFFINITY 0x010700
> +
> struct ncsi_channel_version {
> u32 version;/* Supported BCD encoded NCSI version */
> u32 alpha2; /* Supported BCD encoded NCSI version */
> @@ -236,6 +240,7 @@ enum {
> ncsi_dev_state_probe_dp,
> ncsi_dev_state_config_sp= 0x0301,
> ncsi_dev_state_config_cis,
> + ncsi_dev_state_config_oem_gma,
> ncsi_dev_state_config_clear_vids,
> ncsi_dev_state_config_svf,
> ncsi_dev_state_config_ev,
> @@ -301,9 +306,9 @@ struct ncsi_cmd_arg {
> unsigned short payload; /* Command packet payload
length */
> unsigned int req_flags; /* NCSI request properties
*/
> union {
> - unsigned char bytes[16]; /* Command packet specific data
*/
> - unsigned short words[8];
> - unsigned int dwords[4];
> + unsigned char bytes[64]; /* Command packet specific data
*/
> + unsigned short words[32];
> + unsigned int dwords[16];
> };
> };
>
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index 7567ca63aae2..3205e22c1734 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -211,6 +211,25 @@ static int ncsi_cmd_handler_snfc(struct sk_buff *skb,
> return 0;
> }
>
> +static int ncsi_cmd_handler_oem(struct sk_buff *skb,
> + struct ncsi_cmd_arg *nca)
> +{
> + struct ncsi_cmd_oem_pkt *cmd;
> + unsigned int len;
> +
> + len = sizeof(struct ncsi_cmd_pkt_hdr) + 4;
> + if (nca->payload < 26)
> + len += 26;
> + else
> + len += nca->payload;
> +
> + cmd = skb_put_zero(skb, len);
> + memcpy(cmd->data, nca->bytes, nca->payload);
