Message ID | CH2PR15MB368691D280F882864A6D356DA3A80@CH2PR15MB3686.namprd15.prod.outlook.com |
---|---|
State | Not Applicable, archived |
Headers | show |
Series | net/ncsi: add control packet payload to NC-SI commands from netlink | expand |
Hi Ben, I have similar fix locally with different approach as the command handler may have some expectation for those byes. We can use NCSI_PKT_CMD_OEM handler as it only copies data based on the payload length. diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index 5c3fad8..3b01f65 100644 --- a/net/ncsi/ncsi-cmd.c +++ b/net/ncsi/ncsi-cmd.c @@ -309,14 +309,19 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca) { + struct ncsi_cmd_handler *nch = NULL; struct ncsi_request *nr; + unsigned char type; struct ethhdr *eh; - struct ncsi_cmd_handler *nch = NULL; int i, ret; + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) + type = NCSI_PKT_CMD_OEM; + else + type = nca->type; /* Search for the handler */ for (i = 0; i < ARRAY_SIZE(ncsi_cmd_handlers); i++) { - if (ncsi_cmd_handlers[i].type == nca->type) { + if (ncsi_cmd_handlers[i].type == type) { if (ncsi_cmd_handlers[i].handler) nch = &ncsi_cmd_handlers[i]; else Thanks, Justin > This patch adds support for NCSI_CMD_SEND_CMD netlink command to load packet data payload (up to 16 bytes) to standard NC-SI commands. > > Packet data will be loaded from NCSI_ATTR_DATA attribute similar to NC-SI OEM commands > > Signed-off-by: Ben Wei <benwei@fb.com> > --- > net/ncsi/internal.h | 7 ++++--- > net/ncsi/ncsi-netlink.c | 9 +++++++++ > 2 files changed, 13 insertions(+), 3 deletions(-) > > diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h index 0b3f0673e1a2..4ff442faf5dc 100644 > --- a/net/ncsi/internal.h > +++ b/net/ncsi/internal.h > @@ -328,9 +328,10 @@ 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]; > +#define NCSI_MAX_DATA_BYTES 16 > + unsigned char bytes[NCSI_MAX_DATA_BYTES]; /* Command packet specific data */ > + unsigned short words[NCSI_MAX_DATA_BYTES / sizeof(unsigned short)]; > + unsigned int dwords[NCSI_MAX_DATA_BYTES / sizeof(unsigned int)]; > }; > 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 8b386d766e7d..7d2a43f30eb1 100644 > --- a/net/ncsi/ncsi-netlink.c > +++ b/net/ncsi/ncsi-netlink.c > @@ -459,6 +459,15 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info) > nca.payload = ntohs(hdr->length); > nca.data = data + sizeof(*hdr); > > + if (nca.payload <= NCSI_MAX_DATA_BYTES) { > + memcpy(nca.bytes, nca.data, nca.payload); > + } else { > + netdev_info(ndp->ndev.dev, "NCSI:payload size %u exceeds max %u\n", > + nca.payload, NCSI_MAX_DATA_BYTES); > + ret = -EINVAL; > + goto out_netlink; > + } > + > ret = ncsi_xmit_cmd(&nca); > out_netlink: > if (ret != 0) { > -- > 2.17.1
Hi Justin, > Hi Ben, > > I have similar fix locally with different approach as the command handler may have some expectation for those byes. > We can use NCSI_PKT_CMD_OEM handler as it only copies data based on the payload length. Great! Yes I was thinking the same, we just need some way to take data payload sent from netlink message and sent it over NC-SI. > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index 5c3fad8..3b01f65 100644 > --- a/net/ncsi/ncsi-cmd.c > +++ b/net/ncsi/ncsi-cmd.c > @@ -309,14 +309,19 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > > int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca) { > + struct ncsi_cmd_handler *nch = NULL; > struct ncsi_request *nr; > + unsigned char type; > struct ethhdr *eh; > - struct ncsi_cmd_handler *nch = NULL; > int i, ret; > > + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) > + type = NCSI_PKT_CMD_OEM; > + else > + type = nca->type; > /* Search for the handler */ > for (i = 0; i < ARRAY_SIZE(ncsi_cmd_handlers); i++) { > - if (ncsi_cmd_handlers[i].type == nca->type) { > + if (ncsi_cmd_handlers[i].type == type) { > if (ncsi_cmd_handlers[i].handler) > nch = &ncsi_cmd_handlers[i]; > else > So in this case NCSI_PKT_CMD_OEM would be the default handler for all NC-SI command over netlink (standard and OEM), correct? Should we rename this to something like NCSI_PKT_CMD_GENERIC for clarity perhaps? Do you plan to upstream this patch? Also do you have local patch to support NCSI_PKT_CMD_PLDM and the PLDM over NC-SI commands defined here (https://www.dmtf.org/sites/default/files/NC-SI_1.2_PLDM_Support_over_RBT_Commands_Proposal.pdf)? If not I can send my local changes - but I think we can use the same NCSI_PKT_CMD_OEM handler to transport PLDM payload over NC-SI. What do you think? (CC Deepak as I think once this is in place we can use pldmtool to send basic PLDM payloads over NC-SI) Regards, -Ben
Hi Ben, > Hi Justin, > > > Hi Ben, > > > > I have similar fix locally with different approach as the command handler may have some expectation for those byes. > > We can use NCSI_PKT_CMD_OEM handler as it only copies data based on the payload length. > > Great! Yes I was thinking the same, we just need some way to take data payload sent from netlink message and sent it over NC-SI. > > > > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index 5c3fad8..3b01f65 100644 > > --- a/net/ncsi/ncsi-cmd.c > > +++ b/net/ncsi/ncsi-cmd.c > > @@ -309,14 +309,19 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > > > > int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca) { > > + struct ncsi_cmd_handler *nch = NULL; > > struct ncsi_request *nr; > > + unsigned char type; > > struct ethhdr *eh; > > - struct ncsi_cmd_handler *nch = NULL; > > int i, ret; > > > > + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) > > + type = NCSI_PKT_CMD_OEM; > > + else > > + type = nca->type; > > /* Search for the handler */ > > for (i = 0; i < ARRAY_SIZE(ncsi_cmd_handlers); i++) { > > - if (ncsi_cmd_handlers[i].type == nca->type) { > > + if (ncsi_cmd_handlers[i].type == type) { > > if (ncsi_cmd_handlers[i].handler) > > nch = &ncsi_cmd_handlers[i]; > > else > > > > So in this case NCSI_PKT_CMD_OEM would be the default handler for all NC-SI command over netlink (standard and OEM), correct? Yes, that is correct. The handler for NCSI_PKT_CMD_OEM command is generic. > Should we rename this to something like NCSI_PKT_CMD_GENERIC for clarity perhaps? Do you plan to upstream this patch? NCSI_PKT_CMD_OEM is a real command type and it is defined by the NC-SI specific. We can add comments to indicate that we use the generic command handler from NCSI_PKT_CMD_OEM command. Does the change work for you? If so, I will prepare the patch. > > > Also do you have local patch to support NCSI_PKT_CMD_PLDM and the PLDM over NC-SI commands defined here (https://www.dmtf.org/sites/default/files/NC-SI_1.2_PLDM_Support_over_RBT_Commands_Proposal.pdf)? > If not I can send my local changes - but I think we can use the same NCSI_PKT_CMD_OEM handler to transport PLDM payload over NC-SI. > What do you think? No, I don't have any change currently to support these commands. It should be very similar to NCSI_PKT_CMD_OEM handler with some minor modification. > > (CC Deepak as I think once this is in place we can use pldmtool to send basic PLDM payloads over NC-SI) > > Regards, > -Ben Thanks, Justin
> Hi Ben, > > > Hi Justin, > > > > > Hi Ben, > > > > > > I have similar fix locally with different approach as the command handler may have some expectation for those byes. > > > We can use NCSI_PKT_CMD_OEM handler as it only copies data based on the payload length. > > > > Great! Yes I was thinking the same, we just need some way to take data payload sent from netlink message and sent it over NC-SI. > > > > > > > > diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c index 5c3fad8..3b01f65 100644 > > > --- a/net/ncsi/ncsi-cmd.c > > > +++ b/net/ncsi/ncsi-cmd.c > > > @@ -309,14 +309,19 @@ static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca) > > > > > > int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca) { > > > + struct ncsi_cmd_handler *nch = NULL; > > > struct ncsi_request *nr; > > > + unsigned char type; > > > struct ethhdr *eh; > > > - struct ncsi_cmd_handler *nch = NULL; > > > int i, ret; > > > > > > + if (nca->req_flags == NCSI_REQ_FLAG_NETLINK_DRIVEN) > > > + type = NCSI_PKT_CMD_OEM; > > > + else > > > + type = nca->type; > > > /* Search for the handler */ > > > for (i = 0; i < ARRAY_SIZE(ncsi_cmd_handlers); i++) { > > > - if (ncsi_cmd_handlers[i].type == nca->type) { > > > + if (ncsi_cmd_handlers[i].type == type) { > > > if (ncsi_cmd_handlers[i].handler) > > > nch = &ncsi_cmd_handlers[i]; > > > else > > > > > > > So in this case NCSI_PKT_CMD_OEM would be the default handler for all NC-SI command over netlink (standard and OEM), correct? > Yes, that is correct. The handler for NCSI_PKT_CMD_OEM command is generic. > > > Should we rename this to something like NCSI_PKT_CMD_GENERIC for clarity perhaps? Do you plan to upstream this patch? > NCSI_PKT_CMD_OEM is a real command type and it is defined by the NC-SI specific. > We can add comments to indicate that we use the generic command handler from NCSI_PKT_CMD_OEM command. > > Does the change work for you? If so, I will prepare the patch. Thanks Justin. I verified this change and it works, thanks! -Ben
diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h index 0b3f0673e1a2..4ff442faf5dc 100644 --- a/net/ncsi/internal.h +++ b/net/ncsi/internal.h @@ -328,9 +328,10 @@ 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]; +#define NCSI_MAX_DATA_BYTES 16 + unsigned char bytes[NCSI_MAX_DATA_BYTES]; /* Command packet specific data */ + unsigned short words[NCSI_MAX_DATA_BYTES / sizeof(unsigned short)]; + unsigned int dwords[NCSI_MAX_DATA_BYTES / sizeof(unsigned int)]; }; 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 8b386d766e7d..7d2a43f30eb1 100644 --- a/net/ncsi/ncsi-netlink.c +++ b/net/ncsi/ncsi-netlink.c @@ -459,6 +459,15 @@ static int ncsi_send_cmd_nl(struct sk_buff *msg, struct genl_info *info) nca.payload = ntohs(hdr->length); nca.data = data + sizeof(*hdr); + if (nca.payload <= NCSI_MAX_DATA_BYTES) { + memcpy(nca.bytes, nca.data, nca.payload); + } else { + netdev_info(ndp->ndev.dev, "NCSI:payload size %u exceeds max %u\n", + nca.payload, NCSI_MAX_DATA_BYTES); + ret = -EINVAL; + goto out_netlink; + } + ret = ncsi_xmit_cmd(&nca); out_netlink: if (ret != 0) {
This patch adds support for NCSI_CMD_SEND_CMD netlink command to load packet data payload (up to 16 bytes) to standard NC-SI commands. Packet data will be loaded from NCSI_ATTR_DATA attribute similar to NC-SI OEM commands Signed-off-by: Ben Wei <benwei@fb.com> --- net/ncsi/internal.h | 7 ++++--- net/ncsi/ncsi-netlink.c | 9 +++++++++ 2 files changed, 13 insertions(+), 3 deletions(-) -- 2.17.1