[PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[Ben Wei]

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

RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[Justin.Lee1]

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

RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[Ben Wei]

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

RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[Justin.Lee1]

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

RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink

[Ben Wei]

> 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