net/ncsi: Add NCSI OEM command for FB Tiogapass

Message ID 20180925000840.4111212-1-vijaykhemka@fb.com
State Not Applicable, archived
Headers show
Series
  • net/ncsi: Add NCSI OEM command for FB Tiogapass
Related show

Commit Message

Vijay Khemka Sept. 25, 2018, 12:08 a.m.
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 <vijaykhemka@fb.com>
---
 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(-)

Comments

Vijay Khemka Sept. 25, 2018, 12:15 a.m. | #1
Hi,
I am sending this patch to mailing list only not maintainer as I need feedback on this patch before I ask for upstream in kernel. Below is the background of problem.

We are using 2 different cards Mellanox and Broadcom for our Tiogapass BMC. We have a requirement of getting mac address from card itself by sending OEM command and then set it to device. We get these mac address as a part of NCSI configuration and set once received response. I have tried to make code as generic as possible. I need your feedback on this patch.

Regards
-Vijay

On 9/24/18, 5:09 PM, "Vijay Khemka" <vijaykhemka@fb.com> 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 <vijaykhemka@fb.com>
    ---
     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_MAC        0x1b00
    +#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,    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 }
     };
    @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
     	}
     
     	/* Get packet payload length and allocate the request */
    -	nca->payload = nch->payload;
    +	if (nch->payload >= 0)
    +		nca->payload = nch->payload;
     	nr = ncsi_alloc_command(nca);
     	if (!nr)
     		return -ENOMEM;
    diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
    index 091284760d21..3b2b86560cc8 100644
    --- a/net/ncsi/ncsi-manage.c
    +++ b/net/ncsi/ncsi-manage.c
    @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
     	return 0;
     }
     
    +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    +/* NCSI Facebook OEM APIs */
    +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
    +{
    +	struct ncsi_cmd_arg nca;
    +	int ret = 0;
    +
    +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    +	nca.ndp = ndp;
    +	nca.channel = ndp->active_channel->id;
    +	nca.package = ndp->active_package->id;
    +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    +	nca.type = NCSI_PKT_CMD_OEM;
    +	nca.payload = 8;
    +
    +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
    +
    +	ret = ncsi_xmit_cmd(&nca);
    +	if (ret)
    +		netdev_err(ndp->ndev.dev,
    +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
    +			   nca.type);
    +}
    +
    +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
    +{
    +	struct ncsi_cmd_arg nca;
    +	int ret = 0;
    +
    +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    +	nca.ndp = ndp;
    +	nca.channel = ndp->active_channel->id;
    +	nca.package = ndp->active_package->id;
    +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    +	nca.type = NCSI_PKT_CMD_OEM;
    +	nca.payload = 60;
    +
    +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
    +
    +	memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
    +	nca.bytes[14] = 0x09;
    +
    +	ret = ncsi_xmit_cmd(&nca);
    +	if (ret)
    +		netdev_err(ndp->ndev.dev,
    +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
    +				   nca.type);
    +}
    +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
    +
     static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
     {
     	struct ncsi_dev *nd = &ndp->ndev;
    @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
     			goto error;
     		}
     
    +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    +		/* Check Manufacture id if it is Mellanox then
    +		 * get and set mac address. To Do: Add code for
    +		 * other types of card if required
    +		 */
    +		if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
    +			nd->state = ncsi_dev_state_config_oem_gma;
    +		else
    +			nd->state = ncsi_dev_state_config_clear_vids;
    +		break;
    +	case ncsi_dev_state_config_oem_gma:
    +		ndp->pending_req_num = 2;
    +		get_mac_address_oem_mlx(ndp);
    +		msleep(500);
    +		set_mac_affinity_mlx(ndp);
    +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
     		nd->state = ncsi_dev_state_config_clear_vids;
     		break;
     	case ncsi_dev_state_config_clear_vids:
    diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
    index 91b4b66438df..0653a893eb12 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 */
    +struct ncsi_cmd_oem_pkt {
    +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    +	unsigned char           data[64];    /* OEM Payload Data  */
    +	__be32                  checksum;    /* Checksum          */
    +};
    +
    +/* Oem Response Packet */
    +struct ncsi_rsp_oem_pkt {
    +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
    +	__be32                  mfr_id;      /* Manufacture ID    */
    +	__be32                  oem_cmd;     /* oem command       */
    +	unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
    --- a/net/ncsi/ncsi-rsp.c
    +++ b/net/ncsi/ncsi-rsp.c
    @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
     	return 0;
     }
     
    +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
    +{
    +	struct ncsi_rsp_oem_pkt *rsp;
    +	struct ncsi_dev_priv *ndp = nr->ndp;
    +	struct net_device *ndev = ndp->ndev.dev;
    +	int ret = 0;
    +	unsigned int oem_cmd, mfr_id;
    +	const struct net_device_ops *ops = ndev->netdev_ops;
    +	struct sockaddr saddr;
    +
    +	/* Get the response header */
    +	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
    +
    +	oem_cmd = ntohl(rsp->oem_cmd);
    +	mfr_id = ntohl(rsp->mfr_id);
    +
    +	/* Check for Mellanox manufacturer id */
    +	if (mfr_id != NCSI_OEM_MFR_MLX_ID)
    +		return 0;
    +
    +	if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
    +		saddr.sa_family = ndev->type;
    +		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
    +		memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
    +		ret = ops->ndo_set_mac_address(ndev, &saddr);
    +		if (ret < 0)
    +			netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
    +	}
    +	return ret;
    +}
    +
     static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
     {
     	struct ncsi_rsp_gvi_pkt *rsp;
    @@ -932,7 +963,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
Joel Stanley Sept. 25, 2018, 12:30 a.m. | #2
Hi Vijay,

On Tue, 25 Sep 2018 at 09:39, Vijay Khemka <vijaykhemka@fb.com> 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 <vijaykhemka@fb.com>
> ---
>  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_MAC        0x1b00
> +#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,    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 }
>  };
> @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
>         }
>
>         /* Get packet payload length and allocate the request */
> -       nca->payload = nch->payload;
> +       if (nch->payload >= 0)
> +               nca->payload = nch->payload;
>         nr = ncsi_alloc_command(nca);
>         if (!nr)
>                 return -ENOMEM;
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 091284760d21..3b2b86560cc8 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
>         return 0;
>  }
>
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> +/* NCSI Facebook OEM APIs */
> +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
> +{
> +       struct ncsi_cmd_arg nca;
> +       int ret = 0;
> +
> +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> +       nca.ndp = ndp;
> +       nca.channel = ndp->active_channel->id;
> +       nca.package = ndp->active_package->id;
> +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> +       nca.type = NCSI_PKT_CMD_OEM;
> +       nca.payload = 8;
> +
> +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
> +
> +       ret = ncsi_xmit_cmd(&nca);
> +       if (ret)
> +               netdev_err(ndp->ndev.dev,
> +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
> +                          nca.type);
> +}
> +
> +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
> +{
> +       struct ncsi_cmd_arg nca;
> +       int ret = 0;
> +
> +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> +       nca.ndp = ndp;
> +       nca.channel = ndp->active_channel->id;
> +       nca.package = ndp->active_package->id;
> +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> +       nca.type = NCSI_PKT_CMD_OEM;
> +       nca.payload = 60;
> +
> +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
> +
> +       memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
> +       nca.bytes[14] = 0x09;
> +
> +       ret = ncsi_xmit_cmd(&nca);
> +       if (ret)
> +               netdev_err(ndp->ndev.dev,
> +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
> +                                  nca.type);
> +}
> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> +
>  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>  {
>         struct ncsi_dev *nd = &ndp->ndev;
> @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>                         goto error;
>                 }
>
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> +               /* Check Manufacture id if it is Mellanox then
> +                * get and set mac address. To Do: Add code for
> +                * other types of card if required
> +                */
> +               if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
> +                       nd->state = ncsi_dev_state_config_oem_gma;
> +               else
> +                       nd->state = ncsi_dev_state_config_clear_vids;
> +               break;
> +       case ncsi_dev_state_config_oem_gma:
> +               ndp->pending_req_num = 2;
> +               get_mac_address_oem_mlx(ndp);
> +               msleep(500);
> +               set_mac_affinity_mlx(ndp);
> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>                 nd->state = ncsi_dev_state_config_clear_vids;
>                 break;
>         case ncsi_dev_state_config_clear_vids:
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index 91b4b66438df..0653a893eb12 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 */
> +struct ncsi_cmd_oem_pkt {
> +       struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
> +       unsigned char           data[64];    /* OEM Payload Data  */
> +       __be32                  checksum;    /* Checksum          */
> +};
> +
> +/* Oem Response Packet */
> +struct ncsi_rsp_oem_pkt {
> +       struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
> +       __be32                  mfr_id;      /* Manufacture ID    */
> +       __be32                  oem_cmd;     /* oem command       */
> +       unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
>         return 0;
>  }
>
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> +       struct ncsi_rsp_oem_pkt *rsp;
> +       struct ncsi_dev_priv *ndp = nr->ndp;
> +       struct net_device *ndev = ndp->ndev.dev;
> +       int ret = 0;
> +       unsigned int oem_cmd, mfr_id;
> +       const struct net_device_ops *ops = ndev->netdev_ops;
> +       struct sockaddr saddr;
> +
> +       /* Get the response header */
> +       rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> +
> +       oem_cmd = ntohl(rsp->oem_cmd);
> +       mfr_id = ntohl(rsp->mfr_id);
> +
> +       /* Check for Mellanox manufacturer id */
> +       if (mfr_id != NCSI_OEM_MFR_MLX_ID)
> +               return 0;
> +
> +       if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
> +               saddr.sa_family = ndev->type;
> +               ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> +               memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
> +               ret = ops->ndo_set_mac_address(ndev, &saddr);
> +               if (ret < 0)
> +                       netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
> +       }
> +       return ret;
> +}
> +
>  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
>  {
>         struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +963,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
>
Vijay Khemka Sept. 25, 2018, 6:16 p.m. | #3
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" <joel@jms.id.au> wrote:

    Hi Vijay,
    
    On Tue, 25 Sep 2018 at 09:39, Vijay Khemka <vijaykhemka@fb.com> 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 <vijaykhemka@fb.com>
    > ---
    >  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_MAC        0x1b00
    > +#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,    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 }
    >  };
    > @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
    >         }
    >
    >         /* Get packet payload length and allocate the request */
    > -       nca->payload = nch->payload;
    > +       if (nch->payload >= 0)
    > +               nca->payload = nch->payload;
    >         nr = ncsi_alloc_command(nca);
    >         if (!nr)
    >                 return -ENOMEM;
    > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
    > index 091284760d21..3b2b86560cc8 100644
    > --- a/net/ncsi/ncsi-manage.c
    > +++ b/net/ncsi/ncsi-manage.c
    > @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
    >         return 0;
    >  }
    >
    > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    > +/* NCSI Facebook OEM APIs */
    > +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
    > +{
    > +       struct ncsi_cmd_arg nca;
    > +       int ret = 0;
    > +
    > +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    > +       nca.ndp = ndp;
    > +       nca.channel = ndp->active_channel->id;
    > +       nca.package = ndp->active_package->id;
    > +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    > +       nca.type = NCSI_PKT_CMD_OEM;
    > +       nca.payload = 8;
    > +
    > +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    > +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
    > +
    > +       ret = ncsi_xmit_cmd(&nca);
    > +       if (ret)
    > +               netdev_err(ndp->ndev.dev,
    > +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
    > +                          nca.type);
    > +}
    > +
    > +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
    > +{
    > +       struct ncsi_cmd_arg nca;
    > +       int ret = 0;
    > +
    > +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    > +       nca.ndp = ndp;
    > +       nca.channel = ndp->active_channel->id;
    > +       nca.package = ndp->active_package->id;
    > +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    > +       nca.type = NCSI_PKT_CMD_OEM;
    > +       nca.payload = 60;
    > +
    > +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    > +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
    > +
    > +       memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
    > +       nca.bytes[14] = 0x09;
    > +
    > +       ret = ncsi_xmit_cmd(&nca);
    > +       if (ret)
    > +               netdev_err(ndp->ndev.dev,
    > +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
    > +                                  nca.type);
    > +}
    > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
    > +
    >  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
    >  {
    >         struct ncsi_dev *nd = &ndp->ndev;
    > @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
    >                         goto error;
    >                 }
    >
    > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    > +               /* Check Manufacture id if it is Mellanox then
    > +                * get and set mac address. To Do: Add code for
    > +                * other types of card if required
    > +                */
    > +               if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
    > +                       nd->state = ncsi_dev_state_config_oem_gma;
    > +               else
    > +                       nd->state = ncsi_dev_state_config_clear_vids;
    > +               break;
    > +       case ncsi_dev_state_config_oem_gma:
    > +               ndp->pending_req_num = 2;
    > +               get_mac_address_oem_mlx(ndp);
    > +               msleep(500);
    > +               set_mac_affinity_mlx(ndp);
    > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
    >                 nd->state = ncsi_dev_state_config_clear_vids;
    >                 break;
    >         case ncsi_dev_state_config_clear_vids:
    > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
    > index 91b4b66438df..0653a893eb12 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 */
    > +struct ncsi_cmd_oem_pkt {
    > +       struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    > +       unsigned char           data[64];    /* OEM Payload Data  */
    > +       __be32                  checksum;    /* Checksum          */
    > +};
    > +
    > +/* Oem Response Packet */
    > +struct ncsi_rsp_oem_pkt {
    > +       struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
    > +       __be32                  mfr_id;      /* Manufacture ID    */
    > +       __be32                  oem_cmd;     /* oem command       */
    > +       unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
    > --- a/net/ncsi/ncsi-rsp.c
    > +++ b/net/ncsi/ncsi-rsp.c
    > @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
    >         return 0;
    >  }
    >
    > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
    > +{
    > +       struct ncsi_rsp_oem_pkt *rsp;
    > +       struct ncsi_dev_priv *ndp = nr->ndp;
    > +       struct net_device *ndev = ndp->ndev.dev;
    > +       int ret = 0;
    > +       unsigned int oem_cmd, mfr_id;
    > +       const struct net_device_ops *ops = ndev->netdev_ops;
    > +       struct sockaddr saddr;
    > +
    > +       /* Get the response header */
    > +       rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
    > +
    > +       oem_cmd = ntohl(rsp->oem_cmd);
    > +       mfr_id = ntohl(rsp->mfr_id);
    > +
    > +       /* Check for Mellanox manufacturer id */
    > +       if (mfr_id != NCSI_OEM_MFR_MLX_ID)
    > +               return 0;
    > +
    > +       if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
    > +               saddr.sa_family = ndev->type;
    > +               ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
    > +               memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
    > +               ret = ops->ndo_set_mac_address(ndev, &saddr);
    > +               if (ret < 0)
    > +                       netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
    > +       }
    > +       return ret;
    > +}
    > +
    >  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
    >  {
    >         struct ncsi_rsp_gvi_pkt *rsp;
    > @@ -932,7 +963,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
    >
Samuel Mendoza-Jonas Sept. 26, 2018, 4:33 a.m. | #4
On Tue, 2018-09-25 at 18:16 +0000, 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" <joel@jms.id.au> wrote:
> 
>     Hi Vijay,
>     
>     On Tue, 25 Sep 2018 at 09:39, Vijay Khemka <vijaykhemka@fb.com> 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 <vijaykhemka@fb.com>
>     > ---
>     >  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_MAC        0x1b00
>     > +#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,    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 }
>     >  };
>     > @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
>     >         }
>     >
>     >         /* Get packet payload length and allocate the request */
>     > -       nca->payload = nch->payload;
>     > +       if (nch->payload >= 0)
>     > +               nca->payload = nch->payload;
>     >         nr = ncsi_alloc_command(nca);
>     >         if (!nr)
>     >                 return -ENOMEM;
>     > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>     > index 091284760d21..3b2b86560cc8 100644
>     > --- a/net/ncsi/ncsi-manage.c
>     > +++ b/net/ncsi/ncsi-manage.c
>     > @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
>     >         return 0;
>     >  }
>     >
>     > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
>     > +/* NCSI Facebook OEM APIs */
>     > +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
>     > +{
>     > +       struct ncsi_cmd_arg nca;
>     > +       int ret = 0;
>     > +
>     > +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
>     > +       nca.ndp = ndp;
>     > +       nca.channel = ndp->active_channel->id;
>     > +       nca.package = ndp->active_package->id;
>     > +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
>     > +       nca.type = NCSI_PKT_CMD_OEM;
>     > +       nca.payload = 8;
>     > +
>     > +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
>     > +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
>     > +
>     > +       ret = ncsi_xmit_cmd(&nca);
>     > +       if (ret)
>     > +               netdev_err(ndp->ndev.dev,
>     > +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
>     > +                          nca.type);
>     > +}
>     > +
>     > +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
>     > +{
>     > +       struct ncsi_cmd_arg nca;
>     > +       int ret = 0;
>     > +
>     > +       memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
>     > +       nca.ndp = ndp;
>     > +       nca.channel = ndp->active_channel->id;
>     > +       nca.package = ndp->active_package->id;
>     > +       nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
>     > +       nca.type = NCSI_PKT_CMD_OEM;
>     > +       nca.payload = 60;
>     > +
>     > +       nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
>     > +       nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
>     > +
>     > +       memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
>     > +       nca.bytes[14] = 0x09;
>     > +
>     > +       ret = ncsi_xmit_cmd(&nca);
>     > +       if (ret)
>     > +               netdev_err(ndp->ndev.dev,
>     > +                          "NCSI: Failed to transmit cmd 0x%x during probe\n",
>     > +                                  nca.type);
>     > +}
>     > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>     > +
>     >  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>     >  {
>     >         struct ncsi_dev *nd = &ndp->ndev;
>     > @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>     >                         goto error;
>     >                 }
>     >
>     > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
>     > +               /* Check Manufacture id if it is Mellanox then
>     > +                * get and set mac address. To Do: Add code for
>     > +                * other types of card if required
>     > +                */
>     > +               if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
>     > +                       nd->state = ncsi_dev_state_config_oem_gma;
>     > +               else
>     > +                       nd->state = ncsi_dev_state_config_clear_vids;
>     > +               break;
>     > +       case ncsi_dev_state_config_oem_gma:
>     > +               ndp->pending_req_num = 2;
>     > +               get_mac_address_oem_mlx(ndp);
>     > +               msleep(500);
>     > +               set_mac_affinity_mlx(ndp);
>     > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>     >                 nd->state = ncsi_dev_state_config_clear_vids;
>     >                 break;
>     >         case ncsi_dev_state_config_clear_vids:
>     > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
>     > index 91b4b66438df..0653a893eb12 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 */
>     > +struct ncsi_cmd_oem_pkt {
>     > +       struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
>     > +       unsigned char           data[64];    /* OEM Payload Data  */
>     > +       __be32                  checksum;    /* Checksum          */
>     > +};
>     > +
>     > +/* Oem Response Packet */
>     > +struct ncsi_rsp_oem_pkt {
>     > +       struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
>     > +       __be32                  mfr_id;      /* Manufacture ID    */
>     > +       __be32                  oem_cmd;     /* oem command       */
>     > +       unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
>     > --- a/net/ncsi/ncsi-rsp.c
>     > +++ b/net/ncsi/ncsi-rsp.c
>     > @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
>     >         return 0;
>     >  }
>     >
>     > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
>     > +{
>     > +       struct ncsi_rsp_oem_pkt *rsp;
>     > +       struct ncsi_dev_priv *ndp = nr->ndp;
>     > +       struct net_device *ndev = ndp->ndev.dev;
>     > +       int ret = 0;
>     > +       unsigned int oem_cmd, mfr_id;
>     > +       const struct net_device_ops *ops = ndev->netdev_ops;
>     > +       struct sockaddr saddr;
>     > +
>     > +       /* Get the response header */
>     > +       rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
>     > +
>     > +       oem_cmd = ntohl(rsp->oem_cmd);
>     > +       mfr_id = ntohl(rsp->mfr_id);
>     > +
>     > +       /* Check for Mellanox manufacturer id */
>     > +       if (mfr_id != NCSI_OEM_MFR_MLX_ID)
>     > +               return 0;
>     > +
>     > +       if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
>     > +               saddr.sa_family = ndev->type;
>     > +               ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>     > +               memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
>     > +               ret = ops->ndo_set_mac_address(ndev, &saddr);
>     > +               if (ret < 0)
>     > +                       netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
>     > +       }
>     > +       return ret;
>     > +}
>     > +
>     >  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
>     >  {
>     >         struct ncsi_rsp_gvi_pkt *rsp;
>     > @@ -932,7 +963,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
>     >
>     
>
Vijay Khemka Sept. 26, 2018, 5:07 p.m. | #5
>    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
Amithash Prasad Sept. 26, 2018, 6:07 p.m. | #6
>  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.
<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1">
<style type="text/css" style="display:none;"><!-- P {margin-top:0;margin-bottom:0;} --></style>
</head>
<body dir="ltr">
<div id="divtagdefaultwrapper" style="font-size: 12pt; color: rgb(0, 0, 0); font-family: Calibri, Helvetica, sans-serif, EmojiFont, &quot;Apple Color Emoji&quot;, &quot;Segoe UI Emoji&quot;, NotoColorEmoji, &quot;Segoe UI Symbol&quot;, &quot;Android Emoji&quot;, EmojiSymbols;" dir="ltr">
<p style="margin-top:0;margin-bottom:0"><span style="font-size: 11pt;">&nbsp;&gt;&nbsp; As I understand Justin's version adds a generic handler, using the NCSI</span><br>
</p>
<div style="color: rgb(0, 0, 0);">
<div class="BodyFragment"><font size="2"><span style="font-size:11pt;">
<div class="PlainText">&nbsp;&gt;&nbsp; Netlink interface to pass OEM commands and responses to and from<br>
&nbsp;&gt;&nbsp; userspace, which does the actual packet handling.<br>
</div>
<div class="PlainText">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.</div>
</span></font></div>
</div>
</div>
</body>
</html>
Justin.Lee1@Dell.com Sept. 26, 2018, 8:39 p.m. | #7
> >  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_DRIVEN	2

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
Joel Stanley Sept. 27, 2018, 3:20 a.m. | #8
On Thu, 27 Sep 2018 at 00:39, <Justin.Lee1@dell.com> 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_DRIVEN    2
>
> 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
Samuel Mendoza-Jonas Sept. 27, 2018, 3:43 a.m. | #9
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 <vijaykhemka@fb.com>

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:

> ---
>  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_MAC        0x1b00
> +#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 },
>  	{ 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 }
>  };
> @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
>  	}
>  
>  	/* Get packet payload length and allocate the request */
> -	nca->payload = nch->payload;
> +	if (nch->payload >= 0)
> +		nca->payload = nch->payload;

I think with this there is a chance of nca->payload being uninitialised
and then used in ncsi_alloc_command(). Can you describe what the aim here
is?

>  	nr = ncsi_alloc_command(nca);
>  	if (!nr)
>  		return -ENOMEM;
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index 091284760d21..3b2b86560cc8 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
>  	return 0;
>  }
>  
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> +/* NCSI Facebook OEM APIs */

Are these Facebook OEM commands or Mellanox OEM commands?

> +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
> +{
> +	struct ncsi_cmd_arg nca;
> +	int ret = 0;
> +
> +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> +	nca.ndp = ndp;
> +	nca.channel = ndp->active_channel->id;
> +	nca.package = ndp->active_package->id;
> +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> +	nca.type = NCSI_PKT_CMD_OEM;
> +	nca.payload = 8;
> +
> +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
> +
> +	ret = ncsi_xmit_cmd(&nca);
> +	if (ret)
> +		netdev_err(ndp->ndev.dev,
> +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
> +			   nca.type);
> +}
> +
> +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
> +{
> +	struct ncsi_cmd_arg nca;
> +	int ret = 0;
> +
> +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));

Ah I see we initialise nca, and thus payload here - the path between
these two points in the driver isn't super obvious (not this patch's
fault :) ), it might be better to explicitly mention/handle this where we
use payload later on.

> +	nca.ndp = ndp;
> +	nca.channel = ndp->active_channel->id;
> +	nca.package = ndp->active_package->id;

These should probably be set in ncsi_configure_channel (eg. see the CIS
state before these are called).

> +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> +	nca.type = NCSI_PKT_CMD_OEM;
> +	nca.payload = 60;
> +
> +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
> +
> +	memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
> +	nca.bytes[14] = 0x09;
> +
> +	ret = ncsi_xmit_cmd(&nca);
> +	if (ret)
> +		netdev_err(ndp->ndev.dev,
> +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
> +				   nca.type);
> +}
> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> +
>  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>  {
>  	struct ncsi_dev *nd = &ndp->ndev;
> @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>  			goto error;
>  		}
>  
> +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> +		/* Check Manufacture id if it is Mellanox then
> +		 * get and set mac address. To Do: Add code for
> +		 * other types of card if required
> +		 */
> +		if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
> +			nd->state = ncsi_dev_state_config_oem_gma;
> +		else
> +			nd->state = ncsi_dev_state_config_clear_vids;
> +		break;
> +	case ncsi_dev_state_config_oem_gma:
> +		ndp->pending_req_num = 2;
> +		get_mac_address_oem_mlx(ndp);
> +		msleep(500);

Is this msleep() required?

> +		set_mac_affinity_mlx(ndp);

Since this *sets* the MAC address, should we name the state and config
option more accurately? How does this OEM command interact with the NCSI
set-mac-address command?
Does this command depend on the previous get_mac_address_oem_mlx()
command succeeding?

> +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
>  		nd->state = ncsi_dev_state_config_clear_vids;
>  		break;
>  	case ncsi_dev_state_config_clear_vids:
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index 91b4b66438df..0653a893eb12 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 */

In general, s/Oem/OEM

> +struct ncsi_cmd_oem_pkt {
> +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
> +	unsigned char           data[64];    /* OEM Payload Data  */
> +	__be32                  checksum;    /* Checksum          */
> +};
> +
> +/* Oem Response Packet */
> +struct ncsi_rsp_oem_pkt {
> +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
> +	__be32                  mfr_id;      /* Manufacture ID    */
> +	__be32                  oem_cmd;     /* oem command       */
> +	unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
>  	return 0;
>  }
>  
> +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> +{
> +	struct ncsi_rsp_oem_pkt *rsp;
> +	struct ncsi_dev_priv *ndp = nr->ndp;
> +	struct net_device *ndev = ndp->ndev.dev;
> +	int ret = 0;
> +	unsigned int oem_cmd, mfr_id;
> +	const struct net_device_ops *ops = ndev->netdev_ops;
> +	struct sockaddr saddr;
> +
> +	/* Get the response header */
> +	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> +
> +	oem_cmd = ntohl(rsp->oem_cmd);
> +	mfr_id = ntohl(rsp->mfr_id);
> +
> +	/* Check for Mellanox manufacturer id */
> +	if (mfr_id != NCSI_OEM_MFR_MLX_ID)
> +		return 0;
> +
> +	if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
> +		saddr.sa_family = ndev->type;
> +		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> +		memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
> +		ret = ops->ndo_set_mac_address(ndev, &saddr);
> +		if (ret < 0)
> +			netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
> +	}
> +	return ret;
> +}
> +
>  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
>  {
>  	struct ncsi_rsp_gvi_pkt *rsp;
> @@ -932,7 +963,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  }
>  };
Samuel Mendoza-Jonas Sept. 27, 2018, 3:54 a.m. | #10
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 <vijaykhemka@fb.com>
> 
> 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_MAC        0x1b00
> > +#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 },
> >  	{ 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 }
> >  };
> > @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
> >  	}
> >  
> >  	/* Get packet payload length and allocate the request */
> > -	nca->payload = nch->payload;
> > +	if (nch->payload >= 0)
> > +		nca->payload = nch->payload;
> 
> I think with this there is a chance of nca->payload being uninitialised
> and then used in ncsi_alloc_command(). Can you describe what the aim here
> is?
> 
> >  	nr = ncsi_alloc_command(nca);
> >  	if (!nr)
> >  		return -ENOMEM;
> > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> > index 091284760d21..3b2b86560cc8 100644
> > --- a/net/ncsi/ncsi-manage.c
> > +++ b/net/ncsi/ncsi-manage.c
> > @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
> >  	return 0;
> >  }
> >  
> > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> > +/* NCSI Facebook OEM APIs */
> 
> Are these Facebook OEM commands or Mellanox OEM commands?
> 
> > +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
> > +{
> > +	struct ncsi_cmd_arg nca;
> > +	int ret = 0;
> > +
> > +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> > +	nca.ndp = ndp;
> > +	nca.channel = ndp->active_channel->id;
> > +	nca.package = ndp->active_package->id;
> > +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> > +	nca.type = NCSI_PKT_CMD_OEM;
> > +	nca.payload = 8;
> > +
> > +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> > +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
> > +
> > +	ret = ncsi_xmit_cmd(&nca);
> > +	if (ret)
> > +		netdev_err(ndp->ndev.dev,
> > +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
> > +			   nca.type);
> > +}
> > +
> > +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
> > +{
> > +	struct ncsi_cmd_arg nca;
> > +	int ret = 0;
> > +
> > +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
> 
> Ah I see we initialise nca, and thus payload here - the path between
> these two points in the driver isn't super obvious (not this patch's
> fault :) ), it might be better to explicitly mention/handle this where we
> use payload later on.
> 
> > +	nca.ndp = ndp;
> > +	nca.channel = ndp->active_channel->id;
> > +	nca.package = ndp->active_package->id;
> 
> These should probably be set in ncsi_configure_channel (eg. see the CIS
> state before these are called).
> 
> > +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
> > +	nca.type = NCSI_PKT_CMD_OEM;
> > +	nca.payload = 60;
> > +
> > +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
> > +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
> > +
> > +	memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
> > +	nca.bytes[14] = 0x09;
> > +
> > +	ret = ncsi_xmit_cmd(&nca);
> > +	if (ret)
> > +		netdev_err(ndp->ndev.dev,
> > +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
> > +				   nca.type);
> > +}
> > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> > +
> >  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> >  {
> >  	struct ncsi_dev *nd = &ndp->ndev;
> > @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
> >  			goto error;
> >  		}
> >  
> > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
> > +		/* Check Manufacture id if it is Mellanox then
> > +		 * get and set mac address. To Do: Add code for
> > +		 * other types of card if required
> > +		 */
> > +		if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
> > +			nd->state = ncsi_dev_state_config_oem_gma;
> > +		else
> > +			nd->state = ncsi_dev_state_config_clear_vids;
> > +		break;
> > +	case ncsi_dev_state_config_oem_gma:
> > +		ndp->pending_req_num = 2;
> > +		get_mac_address_oem_mlx(ndp);
> > +		msleep(500);
> 
> Is this msleep() required?
> 
> > +		set_mac_affinity_mlx(ndp);
> 
> Since this *sets* the MAC address, should we name the state and config
> option more accurately? How does this OEM command interact with the NCSI
> set-mac-address command?
> Does this command depend on the previous get_mac_address_oem_mlx()
> command succeeding?
> 
> > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
> >  		nd->state = ncsi_dev_state_config_clear_vids;
> >  		break;
> >  	case ncsi_dev_state_config_clear_vids:
> > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> > index 91b4b66438df..0653a893eb12 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 */
> 
> In general, s/Oem/OEM
> 
> > +struct ncsi_cmd_oem_pkt {
> > +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
> > +	unsigned char           data[64];    /* OEM Payload Data  */
> > +	__be32                  checksum;    /* Checksum          */
> > +};
> > +
> > +/* Oem Response Packet */
> > +struct ncsi_rsp_oem_pkt {
> > +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
> > +	__be32                  mfr_id;      /* Manufacture ID    */
> > +	__be32                  oem_cmd;     /* oem command       */
> > +	unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
> > --- a/net/ncsi/ncsi-rsp.c
> > +++ b/net/ncsi/ncsi-rsp.c
> > @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
> >  	return 0;
> >  }
> >  
> > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
> > +{
> > +	struct ncsi_rsp_oem_pkt *rsp;
> > +	struct ncsi_dev_priv *ndp = nr->ndp;
> > +	struct net_device *ndev = ndp->ndev.dev;
> > +	int ret = 0;
> > +	unsigned int oem_cmd, mfr_id;
> > +	const struct net_device_ops *ops = ndev->netdev_ops;
> > +	struct sockaddr saddr;
> > +
> > +	/* Get the response header */
> > +	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
> > +
> > +	oem_cmd = ntohl(rsp->oem_cmd);
> > +	mfr_id = ntohl(rsp->mfr_id);
> > +
> > +	/* Check for Mellanox manufacturer id */
> > +	if (mfr_id != NCSI_OEM_MFR_MLX_ID)
> > +		return 0;
> > +
> > +	if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
> > +		saddr.sa_family = ndev->type;
> > +		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> > +		memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
> > +		ret = ops->ndo_set_mac_address(ndev, &saddr);
> > +		if (ret < 0)
> > +			netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
> > +	}
> > +	return ret;
> > +}
> > +
> >  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
> >  {
> >  	struct ncsi_rsp_gvi_pkt *rsp;
> > @@ -932,7 +963,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  }
> >  };
Justin.Lee1@Dell.com Sept. 27, 2018, 5:46 p.m. | #11
> 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
Vijay Khemka Sept. 28, 2018, 10:26 p.m. | #12
On 9/26/18, 8:44 PM, "Samuel Mendoza-Jonas" <sam@mendozajonas.com> 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_MAC        0x1b00
    > +#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.  

    > +	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 },
    >  	{ 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 }
    >  };
    > @@ -317,7 +336,8 @@ int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
    >  	}
    >  
    >  	/* Get packet payload length and allocate the request */
    > -	nca->payload = nch->payload;
    > +	if (nch->payload >= 0)
    > +		nca->payload = nch->payload;
    
        I think with this there is a chance of nca->payload being uninitialised
        and then used in ncsi_alloc_command(). Can you describe what the aim here
        is?
    I will add more description here.

    >  	nr = ncsi_alloc_command(nca);
    >  	if (!nr)
    >  		return -ENOMEM;
    > diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
    > index 091284760d21..3b2b86560cc8 100644
    > --- a/net/ncsi/ncsi-manage.c
    > +++ b/net/ncsi/ncsi-manage.c
    > @@ -635,6 +635,58 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
    >  	return 0;
    >  }
    >  
    > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    > +/* NCSI Facebook OEM APIs */
    
        Are these Facebook OEM commands or Mellanox OEM commands?
    Will be taken care in next patch

    > +static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
    > +{
    > +	struct ncsi_cmd_arg nca;
    > +	int ret = 0;
    > +
    > +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    > +	nca.ndp = ndp;
    > +	nca.channel = ndp->active_channel->id;
    > +	nca.package = ndp->active_package->id;
    > +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    > +	nca.type = NCSI_PKT_CMD_OEM;
    > +	nca.payload = 8;
    > +
    > +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    > +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
    > +
    > +	ret = ncsi_xmit_cmd(&nca);
    > +	if (ret)
    > +		netdev_err(ndp->ndev.dev,
    > +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
    > +			   nca.type);
    > +}
    > +
    > +static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
    > +{
    > +	struct ncsi_cmd_arg nca;
    > +	int ret = 0;
    > +
    > +	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
    
        Ah I see we initialise nca, and thus payload here - the path between
        these two points in the driver isn't super obvious (not this patch's
        fault :) ), it might be better to explicitly mention/handle this where we
        use payload later on.
    Yes, I agree, will add more details.

    > +	nca.ndp = ndp;
    > +	nca.channel = ndp->active_channel->id;
    > +	nca.package = ndp->active_package->id;
    
        These should probably be set in ncsi_configure_channel (eg. see the CIS
        state before these are called).
    These functions are being called from ncsi_configure_channel() only. We 
    can either initialize nca there in ncsi_configure_channel() function and pass 
    nca as function parameter or initialize nca inside function and populate 
    required field.

    > +	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
    > +	nca.type = NCSI_PKT_CMD_OEM;
    > +	nca.payload = 60;
    > +
    > +	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
    > +	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
    > +
    > +	memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
    > +	nca.bytes[14] = 0x09;
    > +
    > +	ret = ncsi_xmit_cmd(&nca);
    > +	if (ret)
    > +		netdev_err(ndp->ndev.dev,
    > +			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
    > +				   nca.type);
    > +}
    > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
    > +
    >  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
    >  {
    >  	struct ncsi_dev *nd = &ndp->ndev;
    > @@ -685,6 +737,22 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
    >  			goto error;
    >  		}
    >  
    > +#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
    > +		/* Check Manufacture id if it is Mellanox then
    > +		 * get and set mac address. To Do: Add code for
    > +		 * other types of card if required
    > +		 */
    > +		if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
    > +			nd->state = ncsi_dev_state_config_oem_gma;
    > +		else
    > +			nd->state = ncsi_dev_state_config_clear_vids;
    > +		break;
    > +	case ncsi_dev_state_config_oem_gma:
    > +		ndp->pending_req_num = 2;
    > +		get_mac_address_oem_mlx(ndp);
    > +		msleep(500);
    
        Is this msleep() required?
    It is required to make sure previous command completed. 
    Will try to handle it differently

    > +		set_mac_affinity_mlx(ndp);
    
        Since this *sets* the MAC address, should we name the state and config
        option more accurately? How does this OEM command interact with the NCSI
        set-mac-address command?
        Does this command depend on the previous get_mac_address_oem_mlx()
        command succeeding?
    It is independent of Set_mac_address. Yes it depends on succession 
    of get_mac_address_oem_mlx() that's why msleep was put there.
    
    > +#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
    >  		nd->state = ncsi_dev_state_config_clear_vids;
    >  		break;
    >  	case ncsi_dev_state_config_clear_vids:
    > diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
    > index 91b4b66438df..0653a893eb12 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 */
    
        In general, s/Oem/OEM
    Sure, will be done.

    > +struct ncsi_cmd_oem_pkt {
    > +	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
    > +	unsigned char           data[64];    /* OEM Payload Data  */
    > +	__be32                  checksum;    /* Checksum          */
    > +};
    > +
    > +/* Oem Response Packet */
    > +struct ncsi_rsp_oem_pkt {
    > +	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
    > +	__be32                  mfr_id;      /* Manufacture ID    */
    > +	__be32                  oem_cmd;     /* oem command       */
    > +	unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
    > --- a/net/ncsi/ncsi-rsp.c
    > +++ b/net/ncsi/ncsi-rsp.c
    > @@ -596,6 +596,37 @@ static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
    >  	return 0;
    >  }
    >  
    > +static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
    > +{
    > +	struct ncsi_rsp_oem_pkt *rsp;
    > +	struct ncsi_dev_priv *ndp = nr->ndp;
    > +	struct net_device *ndev = ndp->ndev.dev;
    > +	int ret = 0;
    > +	unsigned int oem_cmd, mfr_id;
    > +	const struct net_device_ops *ops = ndev->netdev_ops;
    > +	struct sockaddr saddr;
    > +
    > +	/* Get the response header */
    > +	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
    > +
    > +	oem_cmd = ntohl(rsp->oem_cmd);
    > +	mfr_id = ntohl(rsp->mfr_id);
    > +
    > +	/* Check for Mellanox manufacturer id */
    > +	if (mfr_id != NCSI_OEM_MFR_MLX_ID)
    > +		return 0;
    > +
    > +	if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
    > +		saddr.sa_family = ndev->type;
    > +		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
    > +		memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
    > +		ret = ops->ndo_set_mac_address(ndev, &saddr);
    > +		if (ret < 0)
    > +			netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
    > +	}
    > +	return ret;
    > +}
    > +
    >  static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
    >  {
    >  	struct ncsi_rsp_gvi_pkt *rsp;
    > @@ -932,7 +963,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  }
    >  };

Patch

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_MAC        0x1b00
+#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,    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 }
 };
@@ -317,7 +336,8 @@  int ncsi_xmit_cmd(struct ncsi_cmd_arg *nca)
 	}
 
 	/* Get packet payload length and allocate the request */
-	nca->payload = nch->payload;
+	if (nch->payload >= 0)
+		nca->payload = nch->payload;
 	nr = ncsi_alloc_command(nca);
 	if (!nr)
 		return -ENOMEM;
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 091284760d21..3b2b86560cc8 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -635,6 +635,58 @@  static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+/* NCSI Facebook OEM APIs */
+static void get_mac_address_oem_mlx(struct ncsi_dev_priv *ndp)
+{
+	struct ncsi_cmd_arg nca;
+	int ret = 0;
+
+	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
+	nca.ndp = ndp;
+	nca.channel = ndp->active_channel->id;
+	nca.package = ndp->active_package->id;
+	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
+	nca.type = NCSI_PKT_CMD_OEM;
+	nca.payload = 8;
+
+	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
+	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_GET_MAC);
+
+	ret = ncsi_xmit_cmd(&nca);
+	if (ret)
+		netdev_err(ndp->ndev.dev,
+			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
+			   nca.type);
+}
+
+static void set_mac_affinity_mlx(struct ncsi_dev_priv *ndp)
+{
+	struct ncsi_cmd_arg nca;
+	int ret = 0;
+
+	memset(&nca, 0, sizeof(struct ncsi_cmd_arg));
+	nca.ndp = ndp;
+	nca.channel = ndp->active_channel->id;
+	nca.package = ndp->active_package->id;
+	nca.req_flags = NCSI_REQ_FLAG_EVENT_DRIVEN;
+	nca.type = NCSI_PKT_CMD_OEM;
+	nca.payload = 60;
+
+	nca.dwords[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
+	nca.dwords[1] = ntohl(NCSI_OEM_MLX_CMD_SET_AFFINITY);
+
+	memcpy(&(nca.bytes[8]), ndp->ndev.dev->dev_addr, ETH_ALEN);
+	nca.bytes[14] = 0x09;
+
+	ret = ncsi_xmit_cmd(&nca);
+	if (ret)
+		netdev_err(ndp->ndev.dev,
+			   "NCSI: Failed to transmit cmd 0x%x during probe\n",
+				   nca.type);
+}
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
+
 static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 {
 	struct ncsi_dev *nd = &ndp->ndev;
@@ -685,6 +737,22 @@  static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 			goto error;
 		}
 
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+		/* Check Manufacture id if it is Mellanox then
+		 * get and set mac address. To Do: Add code for
+		 * other types of card if required
+		 */
+		if (nc->version.mf_id == NCSI_OEM_MFR_MLX_ID)
+			nd->state = ncsi_dev_state_config_oem_gma;
+		else
+			nd->state = ncsi_dev_state_config_clear_vids;
+		break;
+	case ncsi_dev_state_config_oem_gma:
+		ndp->pending_req_num = 2;
+		get_mac_address_oem_mlx(ndp);
+		msleep(500);
+		set_mac_affinity_mlx(ndp);
+#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
 		nd->state = ncsi_dev_state_config_clear_vids;
 		break;
 	case ncsi_dev_state_config_clear_vids:
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 91b4b66438df..0653a893eb12 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 */
+struct ncsi_cmd_oem_pkt {
+	struct ncsi_cmd_pkt_hdr cmd;         /* Command header    */
+	unsigned char           data[64];    /* OEM Payload Data  */
+	__be32                  checksum;    /* Checksum          */
+};
+
+/* Oem Response Packet */
+struct ncsi_rsp_oem_pkt {
+	struct ncsi_rsp_pkt_hdr rsp;         /* Command header    */
+	__be32                  mfr_id;      /* Manufacture ID    */
+	__be32                  oem_cmd;     /* oem command       */
+	unsigned char           data[32];    /* 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..3b94c96b9c7f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -596,6 +596,37 @@  static int ncsi_rsp_handler_snfc(struct ncsi_request *nr)
 	return 0;
 }
 
+static int ncsi_rsp_handler_oem(struct ncsi_request *nr)
+{
+	struct ncsi_rsp_oem_pkt *rsp;
+	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct net_device *ndev = ndp->ndev.dev;
+	int ret = 0;
+	unsigned int oem_cmd, mfr_id;
+	const struct net_device_ops *ops = ndev->netdev_ops;
+	struct sockaddr saddr;
+
+	/* Get the response header */
+	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+
+	oem_cmd = ntohl(rsp->oem_cmd);
+	mfr_id = ntohl(rsp->mfr_id);
+
+	/* Check for Mellanox manufacturer id */
+	if (mfr_id != NCSI_OEM_MFR_MLX_ID)
+		return 0;
+
+	if (oem_cmd == NCSI_OEM_MLX_CMD_GET_MAC) {
+		saddr.sa_family = ndev->type;
+		ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+		memcpy(saddr.sa_data, &(rsp->data[4]), ETH_ALEN);
+		ret = ops->ndo_set_mac_address(ndev, &saddr);
+		if (ret < 0)
+			netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
+	}
+	return ret;
+}
+
 static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
 {
 	struct ncsi_rsp_gvi_pkt *rsp;
@@ -932,7 +963,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  }
 };