* [PATCH v2] net/ncsi: Add NCSI OEM command support
@ 2018-09-29 1:06 Vijay Khemka
2018-09-29 1:20 ` Vijay Khemka
2018-10-02 1:20 ` Samuel Mendoza-Jonas
0 siblings, 2 replies; 9+ messages in thread
From: Vijay Khemka @ 2018-09-29 1:06 UTC (permalink / raw)
To: Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari,
netdev @ vger . kernel . org, christian @ cmd . nu,
Samuel Mendoza-Jonas
Cc: Vijay Khemka
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 <vijaykhemka@fb.com>
---
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, &nca->dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(&cmd->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 int mfr_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 header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ mfr_id = ntohl(rsp->mfr_id);
+
+ /* Check for manufacturer id and Find the handler */
+ for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
+ if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
+ if (ncsi_rsp_oem_handlers[i].handler)
+ nrh = &ncsi_rsp_oem_handlers[i];
+ else
+ nrh = NULL;
+
+ break;
+ }
+ }
+
+ if (!nrh) {
+ netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
+ mfr_id);
+ return -ENOENT;
+ }
+
+ /* Process the packet */
+ return nrh->handler(nr);
+}
+
static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
{
struct ncsi_rsp_gvi_pkt *rsp;
@@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
{ NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
{ NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
{ NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
- { NCSI_PKT_RSP_OEM, 0, NULL },
+ { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
{ NCSI_PKT_RSP_PLDM, 0, NULL },
{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
};
--
2.17.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-09-29 1:06 [PATCH v2] net/ncsi: Add NCSI OEM command support Vijay Khemka
@ 2018-09-29 1:20 ` Vijay Khemka
2018-10-02 1:20 ` Samuel Mendoza-Jonas
1 sibling, 0 replies; 9+ messages in thread
From: Vijay Khemka @ 2018-09-29 1:20 UTC (permalink / raw)
To: Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari,
netdev @ vger . kernel . org, christian @ cmd . nu,
Samuel Mendoza-Jonas
> On 9/28/18, 6:07 PM, "Vijay Khemka" <vijaykhemka@fb.com> 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 <vijaykhemka@fb.com>
---
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, &nca->dwords[1], nca->payload - 4);
+ ncsi_cmd_build_header(&cmd->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 int mfr_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 header */
+ rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+ mfr_id = ntohl(rsp->mfr_id);
+
+ /* Check for manufacturer id and Find the handler */
+ for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
+ if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
+ if (ncsi_rsp_oem_handlers[i].handler)
+ nrh = &ncsi_rsp_oem_handlers[i];
+ else
+ nrh = NULL;
+
+ break;
+ }
+ }
+
+ if (!nrh) {
+ netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
+ mfr_id);
+ return -ENOENT;
+ }
+
+ /* Process the packet */
+ return nrh->handler(nr);
+}
+
static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
{
struct ncsi_rsp_gvi_pkt *rsp;
@@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
{ NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
{ NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
{ NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
- { NCSI_PKT_RSP_OEM, 0, NULL },
+ { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
{ NCSI_PKT_RSP_PLDM, 0, NULL },
{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
};
--
2.17.1
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-09-29 1:06 [PATCH v2] net/ncsi: Add NCSI OEM command support Vijay Khemka
@ 2018-10-02 1:20 ` Samuel Mendoza-Jonas
2018-10-02 1:20 ` Samuel Mendoza-Jonas
1 sibling, 0 replies; 9+ messages in thread
From: Samuel Mendoza-Jonas @ 2018-10-02 1:20 UTC (permalink / raw)
To: Vijay Khemka, Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari,
netdev @ vger . kernel . org, christian @ cmd . nu
On Fri, 2018-09-28 at 18:06 -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 <vijaykhemka@fb.com>
Hi Vijay - looks good to me, and should be a good common base for your
and Justin's changes.
Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
> 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, &nca->dwords[1], nca->payload - 4);
> + ncsi_cmd_build_header(&cmd->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 int mfr_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 header */
> + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> + mfr_id = ntohl(rsp->mfr_id);
> +
> + /* Check for manufacturer id and Find the handler */
> + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> + if (ncsi_rsp_oem_handlers[i].handler)
> + nrh = &ncsi_rsp_oem_handlers[i];
> + else
> + nrh = NULL;
> +
> + break;
> + }
> + }
> +
> + if (!nrh) {
> + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
> + mfr_id);
> + return -ENOENT;
> + }
> +
> + /* Process the packet */
> + return nrh->handler(nr);
> +}
> +
> static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> {
> struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
@ 2018-10-02 1:20 ` Samuel Mendoza-Jonas
0 siblings, 0 replies; 9+ messages in thread
From: Samuel Mendoza-Jonas @ 2018-10-02 1:20 UTC (permalink / raw)
To: Vijay Khemka, Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari,
netdev @ vger . kernel . org, christian @ cmd . nu
On Fri, 2018-09-28 at 18:06 -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 <vijaykhemka@fb.com>
Hi Vijay - looks good to me, and should be a good common base for your
and Justin's changes.
Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
> ---
> 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, &nca->dwords[1], nca->payload - 4);
> + ncsi_cmd_build_header(&cmd->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 int mfr_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 header */
> + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> + mfr_id = ntohl(rsp->mfr_id);
> +
> + /* Check for manufacturer id and Find the handler */
> + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> + if (ncsi_rsp_oem_handlers[i].handler)
> + nrh = &ncsi_rsp_oem_handlers[i];
> + else
> + nrh = NULL;
> +
> + break;
> + }
> + }
> +
> + if (!nrh) {
> + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
> + mfr_id);
> + return -ENOENT;
> + }
> +
> + /* Process the packet */
> + return nrh->handler(nr);
> +}
> +
> static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> {
> struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> - { NCSI_PKT_RSP_OEM, 0, NULL },
> + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> { NCSI_PKT_RSP_PLDM, 0, NULL },
> { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> };
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-10-02 1:20 ` Samuel Mendoza-Jonas
(?)
@ 2018-10-02 6:29 ` Christian Svensson
-1 siblings, 0 replies; 9+ messages in thread
From: Christian Svensson @ 2018-10-02 6:29 UTC (permalink / raw)
To: Samuel Mendoza-Jonas
Cc: Vijay Khemka, Justin . Lee1 @ Dell . com, joel @ jms . id . au,
linux-aspeed @ lists . ozlabs . org,
openbmc @ lists . ozlabs . org, Sai Dasari,
netdev @ vger . kernel . org
[-- Attachment #1: Type: text/plain, Size: 7373 bytes --]
I'm just going to say that I'm very excited about these patches. Just what
I needed, thanks for contributing them :-).
On Tue, Oct 2, 2018, 03:20 Samuel Mendoza-Jonas <sam@mendozajonas.com>
wrote:
> On Fri, 2018-09-28 at 18:06 -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 <vijaykhemka@fb.com>
>
> Hi Vijay - looks good to me, and should be a good common base for your
> and Justin's changes.
>
> Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>
> > ---
> > 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, &nca->dwords[1], nca->payload - 4);
> > + ncsi_cmd_build_header(&cmd->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 int mfr_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 header */
> > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > + mfr_id = ntohl(rsp->mfr_id);
> > +
> > + /* Check for manufacturer id and Find the handler */
> > + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> > + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> > + if (ncsi_rsp_oem_handlers[i].handler)
> > + nrh = &ncsi_rsp_oem_handlers[i];
> > + else
> > + nrh = NULL;
> > +
> > + break;
> > + }
> > + }
> > +
> > + if (!nrh) {
> > + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM
> packet with MFR-ID (0x%x)\n",
> > + mfr_id);
> > + return -ENOENT;
> > + }
> > +
> > + /* Process the packet */
> > + return nrh->handler(nr);
> > +}
> > +
> > static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> > { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> > { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> > { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> > - { NCSI_PKT_RSP_OEM, 0, NULL },
> > + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> > { NCSI_PKT_RSP_PLDM, 0, NULL },
> > { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> > };
>
>
>
[-- Attachment #2: Type: text/html, Size: 9607 bytes --]
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-10-02 1:20 ` Samuel Mendoza-Jonas
(?)
(?)
@ 2018-10-02 16:53 ` Justin.Lee1
2018-10-02 18:56 ` Vijay Khemka
-1 siblings, 1 reply; 9+ messages in thread
From: Justin.Lee1 @ 2018-10-02 16:53 UTC (permalink / raw)
To: sam, vijaykhemka, joel, linux-aspeed, openbmc, sdasari, netdev,
christian
Hi Vijay,
Looks good. Please see my comment below.
Thanks,
Justin
> On Fri, 2018-09-28 at 18:06 -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 <vijaykhemka@fb.com>
>
> Hi Vijay - looks good to me, and should be a good common base for your
> and Justin's changes.
>
> Reviewed-by: Samuel Mendoza-Jonas <sam@mendozajonas.com>
>
> > ---
> > 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;
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 */
};
> > + 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?
> > + else
> > + len += nca->payload;
> > +
> > + cmd = skb_put_zero(skb, len);
> > + cmd->mfr_id = nca->dwords[0];
> > + memcpy(cmd->data, &nca->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 &nca->bytes[0] to it.
> > + ncsi_cmd_build_header(&cmd->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 */
> > +};
> > +
OEM command doesn't not have the fixed response data size too.
Should we use pointer instead?
> > /* 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 int mfr_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 header */
> > + rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > + mfr_id = ntohl(rsp->mfr_id);
> > +
> > + /* Check for manufacturer id and Find the handler */
> > + for (i = 0; i < ARRAY_SIZE(ncsi_rsp_oem_handlers); i++) {
> > + if (ncsi_rsp_oem_handlers[i].mfr_id == mfr_id) {
> > + if (ncsi_rsp_oem_handlers[i].handler)
> > + nrh = &ncsi_rsp_oem_handlers[i];
> > + else
> > + nrh = NULL;
> > +
> > + break;
> > + }
> > + }
> > +
> > + if (!nrh) {
> > + netdev_err(nr->ndp->ndev.dev, "Received unrecognized OEM packet with MFR-ID (0x%x)\n",
> > + mfr_id);
> > + return -ENOENT;
> > + }
> > +
> > + /* Process the packet */
> > + return nrh->handler(nr);
> > +}
> > +
> > static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> > {
> > struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +974,7 @@ static struct ncsi_rsp_handler {
> > { NCSI_PKT_RSP_GNS, 172, ncsi_rsp_handler_gns },
> > { NCSI_PKT_RSP_GNPTS, 172, ncsi_rsp_handler_gnpts },
> > { NCSI_PKT_RSP_GPS, 8, ncsi_rsp_handler_gps },
> > - { NCSI_PKT_RSP_OEM, 0, NULL },
> > + { NCSI_PKT_RSP_OEM, -1, ncsi_rsp_handler_oem },
> > { NCSI_PKT_RSP_PLDM, 0, NULL },
> > { NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid }
> > };
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-10-02 16:53 ` Justin.Lee1
@ 2018-10-02 18:56 ` Vijay Khemka
2018-10-02 20:53 ` Justin.Lee1
0 siblings, 1 reply; 9+ messages in thread
From: Vijay Khemka @ 2018-10-02 18:56 UTC (permalink / raw)
To: Justin.Lee1, sam, joel, linux-aspeed, openbmc, Sai Dasari,
netdev, christian
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, &nca->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 &nca->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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* RE: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-10-02 18:56 ` Vijay Khemka
@ 2018-10-02 20:53 ` Justin.Lee1
2018-10-02 21:34 ` Vijay Khemka
0 siblings, 1 reply; 9+ messages in thread
From: Justin.Lee1 @ 2018-10-02 20:53 UTC (permalink / raw)
To: vijaykhemka, sam, joel, linux-aspeed, openbmc, sdasari, netdev,
christian
Hi Vijay,
Please see the comments below.
Thanks,
Justin
> 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.
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 */
};
> > > + 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, &nca->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 &nca->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.
>
> > > 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.
Suggest to change as below due to the flexible response.
struct ncsi_rsp_oem_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* Payload data */
};
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH v2] net/ncsi: Add NCSI OEM command support
2018-10-02 20:53 ` Justin.Lee1
@ 2018-10-02 21:34 ` Vijay Khemka
0 siblings, 0 replies; 9+ messages in thread
From: Vijay Khemka @ 2018-10-02 21:34 UTC (permalink / raw)
To: Justin.Lee1, sam, joel, linux-aspeed, openbmc, Sai Dasari,
netdev, christian
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, &nca->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 &nca->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 define MAX_DATA_LEN because data pointer won't map to skb directly.
Suggest to change as below due to the flexible response.
struct ncsi_rsp_oem_pkt {
struct ncsi_rsp_pkt_hdr rsp; /* Command header */
__be32 mfr_id; /* Manufacture ID */
unsigned char data[]; /* Payload data */
};
Sure done.
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2018-10-03 4:28 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-29 1:06 [PATCH v2] net/ncsi: Add NCSI OEM command support Vijay Khemka
2018-09-29 1:20 ` Vijay Khemka
2018-10-02 1:20 ` Samuel Mendoza-Jonas
2018-10-02 1:20 ` Samuel Mendoza-Jonas
2018-10-02 6:29 ` Christian Svensson
2018-10-02 16:53 ` Justin.Lee1
2018-10-02 18:56 ` Vijay Khemka
2018-10-02 20:53 ` Justin.Lee1
2018-10-02 21:34 ` Vijay Khemka
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.