RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink
> 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 = _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
RE: [PATCH] net/ncsi: add control packet payload to NC-SI commands from netlink
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 = _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
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 = _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
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 = _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
> ---
> 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();
> out_netlink:
> if (ret != 0) {
> --
> 2.17.1
