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
> Thanks for the overview. We look forward to your patches; please > include the same cc list as this series. > I think it makes sense to have some OEM NCSI handing purely in the > kenrel. This would allow eg. the MAC address of an interface to be > correct at boot, without requiring userspace to come up first. > I have also heard stories of temperature sensors over NCSI. Those > would make sense to be hwmon drivers, which again would mean handling > them in the kernel. > Justin, Vijay, can you please list the known NCSI OEM > commands/extensions that we plan on implementing? In our implementation, All OEM commands are transparent to Kernel. We don't plan to add any specific handler in the Kernel. Thanks, Justin
Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
On Thu, 2018-09-27 at 13:43 +1000, Samuel Mendoza-Jonas wrote: > On Mon, 2018-09-24 at 17:08 -0700, 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 handles 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 > > > > Signed-off-by: Vijay Khemka > > 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). > > Some comments below: Whoops, forgot to re-add netdev. > > > --- > > 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. > > > 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? > > > + > > 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? > > > + else > > + len += nca->payload; > > + > > + cmd = skb_put_zero(skb, len); > > + memcpy(cmd->data, nca->bytes, nca->payload); > > + ncsi_cmd_build_header(&cmd->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 }, > >
Re: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
On Thu, 27 Sep 2018 at 00:39, wrote: > > > > 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. > > Thanks for the direction Sam! Justin, if you don't mind, can you share the > > patches you have to add the support? This actually would solve a few other > > things we are trying to accomplish. > > > Basically, I add a new flag to indicate the request is coming from the > Netlink interface to allow the command handler and response handler to react. > #define NCSI_REQ_FLAG_NETLINK_DRIVEN2 > > 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_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: > Detected error -> ncsi_send_netlink_err () -> Netlink interface (err msg) -> > user space application > > I will clean up some code and send out the patch. Thanks for the overview. We look forward to your patches; please include the same cc list as this series. I think it makes sense to have some OEM NCSI handing purely in the kenrel. This would allow eg. the MAC address of an interface to be correct at boot, without requiring userspace to come up first. I have also heard stories of temperature sensors over NCSI. Those would make sense to be hwmon drivers, which again would mean handling them in the kernel. Justin, Vijay, can you please list the known NCSI OEM commands/extensions that we plan on implementing? Cheers, Joel
RE: [PATCH] net/ncsi: Add NCSI OEM command for FB Tiogapass
> > 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. > Thanks for the direction Sam! Justin, if you don't mind, can you share the > patches you have to add the support? This actually would solve a few other > things we are trying to accomplish. Basically, I add a new flag to indicate the request is coming from the Netlink interface to allow the command handler and response handler to react. #define NCSI_REQ_FLAG_NETLINK_DRIVEN2 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_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: Detected error -> ncsi_send_netlink_err () -> Netlink interface (err msg) -> user space application I will clean up some code and send out the patch. Thanks, Justin
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
On Tue, 2018-09-25 at 18:16 +, Vijay Khemka wrote: > 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 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 Justin - could you weigh in here and give a description of your intended changes? Are you able to post your changes upstream so we can compare? Regards, Samuel > > 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/n
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); > + ncsi_cmd_build_header(&cmd->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,