[PATCH v2 0/3] net/ncsi: Add NCSI Intel OEM command to keep PHY link up

Add NCSI Intel OEM command to keep PHY link up and prevents any channel
resets during the host load on i210. Also includes dummy response handler for
Intel manufacturer id.

Changes from v1:
 1. sparse fixes about casts
 2. put it after ncsi_dev_state_probe_cis instead of
    ncsi_dev_state_probe_channel because sometimes channel is not ready
    after it
 3. inl -> intel

Ivan Mikhaylov (3):
  net/ncsi: fix restricted cast warning of sparse
  net/ncsi: add NCSI Intel OEM command to keep PHY up
  net/ncsi: add dummy response handler for Intel boards

 net/ncsi/Kconfig       |  6 +++++
 net/ncsi/internal.h    |  5 +++++
 net/ncsi/ncsi-manage.c | 51 +++++++++++++++++++++++++++++++++++++++---
 net/ncsi/ncsi-rsp.c    | 11 +++++++--
 4 files changed, 68 insertions(+), 5 deletions(-)

-- 
2.31.1

Sparse reports:
net/ncsi/ncsi-rsp.c:406:24: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:732:33: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:756:25: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:779:25: warning: cast to restricted __be32

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/ncsi-manage.c | 6 +++---
 net/ncsi/ncsi-rsp.c    | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index ca04b6df1341..42b54a3da2e6 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c

@@ -700,7 +700,7 @@ static int ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg *nca)
 	nca->payload = NCSI_OEM_BCM_CMD_GMA_LEN;
 
 	memset(data, 0, NCSI_OEM_BCM_CMD_GMA_LEN);
-	*(unsigned int *)data = ntohl(NCSI_OEM_MFR_BCM_ID);
+	*(unsigned int *)data = ntohl((__force __be32)NCSI_OEM_MFR_BCM_ID);
 	data[5] = NCSI_OEM_BCM_CMD_GMA;
 
 	nca->data = data;

@@ -724,7 +724,7 @@ static int ncsi_oem_gma_handler_mlx(struct ncsi_cmd_arg *nca)
 	nca->payload = NCSI_OEM_MLX_CMD_GMA_LEN;
 
 	memset(&u, 0, sizeof(u));
-	u.data_u32[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
+	u.data_u32[0] = ntohl((__force __be32)NCSI_OEM_MFR_MLX_ID);
 	u.data_u8[5] = NCSI_OEM_MLX_CMD_GMA;
 	u.data_u8[6] = NCSI_OEM_MLX_CMD_GMA_PARAM;
 

@@ -747,7 +747,7 @@ static int ncsi_oem_smaf_mlx(struct ncsi_cmd_arg *nca)
 	int ret = 0;
 
 	memset(&u, 0, sizeof(u));
-	u.data_u32[0] = ntohl(NCSI_OEM_MFR_MLX_ID);
+	u.data_u32[0] = ntohl((__force __be32)NCSI_OEM_MFR_MLX_ID);
 	u.data_u8[5] = NCSI_OEM_MLX_CMD_SMAF;
 	u.data_u8[6] = NCSI_OEM_MLX_CMD_SMAF_PARAM;
 	memcpy(&u.data_u8[MLX_SMAF_MAC_ADDR_OFFSET],

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 888ccc2d4e34..04bc50be5c01 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c

@@ -403,7 +403,7 @@ static int ncsi_rsp_handler_ev(struct ncsi_request *nr)
 	/* Update to VLAN mode */
 	cmd = (struct ncsi_cmd_ev_pkt *)skb_network_header(nr->cmd);
 	ncm->enable = 1;
-	ncm->data[0] = ntohl(cmd->mode);
+	ncm->data[0] = ntohl((__force __be32)cmd->mode);
 
 	return 0;
 }

-- 
2.31.1

Add the dummy response handler for Intel boards to prevent incorrect
handling of OEM commands.

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/ncsi-rsp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 04bc50be5c01..d48374894817 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c

@@ -699,12 +699,19 @@ static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 	return 0;
 }
 
+/* Response handler for Intel card */
+static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
+{
+	return 0;
+}
+
 static struct ncsi_rsp_oem_handler {
 	unsigned int	mfr_id;
 	int		(*handler)(struct ncsi_request *nr);
 } ncsi_rsp_oem_handlers[] = {
 	{ NCSI_OEM_MFR_MLX_ID, ncsi_rsp_handler_oem_mlx },
-	{ NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
+	{ NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm },
+	{ NCSI_OEM_MFR_INTEL_ID, ncsi_rsp_handler_oem_intel }
 };
 
 /* Response handler for OEM command */

-- 
2.31.1

This allows to keep PHY link up and prevents any channel resets during
the host load.

It is KEEP_PHY_LINK_UP option(Veto bit) in i210 datasheet which
block PHY reset and power state changes.

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/Kconfig       |  6 ++++++
 net/ncsi/internal.h    |  5 +++++
 net/ncsi/ncsi-manage.c | 45 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 93309081f5a4..ea1dd32b6b1f 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig

@@ -17,3 +17,9 @@ config NCSI_OEM_CMD_GET_MAC
 	help
 	  This allows to get MAC address from NCSI firmware and set them back to
 		controller.
+config NCSI_OEM_CMD_KEEP_PHY
+	bool "Keep PHY Link up"
+	depends on NET_NCSI
+	help
+	  This allows to keep PHY link up and prevents any channel resets during
+	  the host load.

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index cbbb0de4750a..0b6cfd3b31e0 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h

@@ -78,6 +78,9 @@ enum {
 /* OEM Vendor Manufacture ID */
 #define NCSI_OEM_MFR_MLX_ID             0x8119
 #define NCSI_OEM_MFR_BCM_ID             0x113d
+#define NCSI_OEM_MFR_INTEL_ID           0x157
+/* Intel specific OEM command */
+#define NCSI_OEM_INTEL_CMD_KEEP_PHY     0x20   /* CMD ID for Keep PHY up */
 /* Broadcom specific OEM Command */
 #define NCSI_OEM_BCM_CMD_GMA            0x01   /* CMD ID for Get MAC */
 /* Mellanox specific OEM Command */

@@ -86,6 +89,7 @@ enum {
 #define NCSI_OEM_MLX_CMD_SMAF           0x01   /* CMD ID for Set MC Affinity */
 #define NCSI_OEM_MLX_CMD_SMAF_PARAM     0x07   /* Parameter for SMAF         */
 /* OEM Command payload lengths*/
+#define NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN 7
 #define NCSI_OEM_BCM_CMD_GMA_LEN        12
 #define NCSI_OEM_MLX_CMD_GMA_LEN        8
 #define NCSI_OEM_MLX_CMD_SMAF_LEN        60

@@ -271,6 +275,7 @@ enum {
 	ncsi_dev_state_probe_mlx_gma,
 	ncsi_dev_state_probe_mlx_smaf,
 	ncsi_dev_state_probe_cis,
+	ncsi_dev_state_probe_keep_phy,
 	ncsi_dev_state_probe_gvi,
 	ncsi_dev_state_probe_gc,
 	ncsi_dev_state_probe_gls,

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 42b54a3da2e6..89c7742cd72e 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c

@@ -689,6 +689,35 @@ static int set_one_vid(struct ncsi_dev_priv *ndp, struct ncsi_channel *nc,
 	return 0;
 }
 
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY)
+
+static int ncsi_oem_keep_phy_intel(struct ncsi_cmd_arg *nca)
+{
+	unsigned char data[NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN];
+	int ret = 0;
+
+	nca->payload = NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN;
+
+	memset(data, 0, NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN);
+	*(unsigned int *)data = ntohl((__force __be32)NCSI_OEM_MFR_INTEL_ID);
+
+	data[4] = NCSI_OEM_INTEL_CMD_KEEP_PHY;
+
+	/* PHY Link up attribute */
+	data[6] = 0x1;
+
+	nca->data = data;
+
+	ret = ncsi_xmit_cmd(nca);
+	if (ret)
+		netdev_err(nca->ndp->ndev.dev,
+			   "NCSI: Failed to transmit cmd 0x%x during configure\n",
+			   nca->type);
+	return ret;
+}
+
+#endif
+
 #if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
 
 /* NCSI OEM Command APIs */

@@ -1391,8 +1420,24 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
 				goto error;
 		}
 
+		nd->state = ncsi_dev_state_probe_gvi;
+		if (IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY))
+			nd->state = ncsi_dev_state_probe_keep_phy;
+		break;
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY)
+	case ncsi_dev_state_probe_keep_phy:
+		ndp->pending_req_num = 1;
+
+		nca.type = NCSI_PKT_CMD_OEM;
+		nca.package = ndp->active_package->id;
+		nca.channel = 0;
+		ret = ncsi_oem_keep_phy_intel(&nca);
+		if (ret)
+			goto error;
+
 		nd->state = ncsi_dev_state_probe_gvi;
 		break;
+#endif /* CONFIG_NCSI_OEM_CMD_KEEP_PHY */
 	case ncsi_dev_state_probe_gvi:
 	case ncsi_dev_state_probe_gc:
 	case ncsi_dev_state_probe_gls:

-- 
2.31.1

Hello:

This series was applied to netdev/net.git (refs/heads/master):

On Thu, 8 Jul 2021 15:27:51 +0300 you wrote:

Add NCSI Intel OEM command to keep PHY link up and prevents any channel
resets during the host load on i210. Also includes dummy response handler for
Intel manufacturer id.

Changes from v1:
 1. sparse fixes about casts
 2. put it after ncsi_dev_state_probe_cis instead of
    ncsi_dev_state_probe_channel because sometimes channel is not ready
    after it
 3. inl -> intel

[...]

Here is the summary with links:
  - [v2,1/3] net/ncsi: fix restricted cast warning of sparse
    https://git.kernel.org/netdev/net/c/27fa107d3b8d
  - [v2,2/3] net/ncsi: add NCSI Intel OEM command to keep PHY up
    https://git.kernel.org/netdev/net/c/abd2fddc94a6
  - [v2,3/3] net/ncsi: add dummy response handler for Intel boards
    https://git.kernel.org/netdev/net/c/163f5de509a8

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Thu, 8 Jul 2021 at 12:27, Ivan Mikhaylov [off-list ref] wrote:

Sparse reports:
net/ncsi/ncsi-rsp.c:406:24: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:732:33: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:756:25: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:779:25: warning: cast to restricted __be32

Strange, I don't get these warnings from sparse on my system.

$ sparse --version
0.6.3 (Debian: 0.6.3-2)

This looks wrong, the value you're passing isn't big endian. It would
make more sense if the byte swap was ntohl, as it's coming from the
cpu and going into the NCSI packet.

On Thu, 8 Jul 2021 at 12:27, Ivan Mikhaylov [off-list ref] wrote:

This allows to keep PHY link up and prevents any channel resets during
the host load.

It is KEEP_PHY_LINK_UP option(Veto bit) in i210 datasheet which
block PHY reset and power state changes.

How about using runtime configuration over using kconfig for this, so
the same kernel config can be used on different machines. Something
device tree based?

Another option is to use the netlink handler to send the OEM command
from userspace. Eddie has worked on this for an IBM machine, and I've
asked him to post those changes. I would prefer the kernel option
though.

On Thu, 8 Jul 2021 at 12:28, Ivan Mikhaylov [off-list ref] wrote:

Add the dummy response handler for Intel boards to prevent incorrect
handling of OEM commands.

What do you mean?

Is this to handle the response to the link up OEM command? If so,
include it in the same patch.

Can you check that the response is to the link up command and print a
warning if not?

On Mon, 2021-07-12 at 10:01 +0000, Joel Stanley wrote:

On Thu, 8 Jul 2021 at 12:27, Ivan Mikhaylov [off-list ref]
wrote:

quoted

This allows to keep PHY link up and prevents any channel resets
during
the host load.

It is KEEP_PHY_LINK_UP option(Veto bit) in i210 datasheet which
block PHY reset and power state changes.

How about using runtime configuration over using kconfig for this, so
the same kernel config can be used on different machines. Something
device tree based?

Another option is to use the netlink handler to send the OEM command
from userspace. Eddie has worked on this for an IBM machine, and I've
asked him to post those changes. I would prefer the kernel option
though.

For reference that is here: 
https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-networkd/+/36592

Thanks,
Eddie

quoted

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/Kconfig       |  6 ++++++
 net/ncsi/internal.h    |  5 +++++
 net/ncsi/ncsi-manage.c | 45
++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 56 insertions(+)

diff --git a/net/ncsi/Kconfig b/net/ncsi/Kconfig
index 93309081f5a4..ea1dd32b6b1f 100644
--- a/net/ncsi/Kconfig
+++ b/net/ncsi/Kconfig

@@ -17,3 +17,9 @@ config NCSI_OEM_CMD_GET_MAC
        help
          This allows to get MAC address from NCSI firmware and set

them back to
                controller.
+config NCSI_OEM_CMD_KEEP_PHY
+       bool "Keep PHY Link up"
+       depends on NET_NCSI
+       help
+         This allows to keep PHY link up and prevents any channel
resets during
+         the host load.

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index cbbb0de4750a..0b6cfd3b31e0 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h

@@ -78,6 +78,9 @@ enum {
 /* OEM Vendor Manufacture ID */
 #define NCSI_OEM_MFR_MLX_ID             0x8119
 #define NCSI_OEM_MFR_BCM_ID             0x113d
+#define NCSI_OEM_MFR_INTEL_ID           0x157
+/* Intel specific OEM command */
+#define NCSI_OEM_INTEL_CMD_KEEP_PHY     0x20   /* CMD ID for Keep

PHY up */
 /* Broadcom specific OEM Command */
 #define NCSI_OEM_BCM_CMD_GMA            0x01   /* CMD ID for Get
MAC */
 /* Mellanox specific OEM Command */

@@ -86,6 +89,7 @@ enum {
 #define NCSI_OEM_MLX_CMD_SMAF           0x01   /* CMD ID for Set

MC Affinity */
 #define NCSI_OEM_MLX_CMD_SMAF_PARAM     0x07   /* Parameter for
SMAF         */
 /* OEM Command payload lengths*/
+#define NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN 7
 #define NCSI_OEM_BCM_CMD_GMA_LEN        12
 #define NCSI_OEM_MLX_CMD_GMA_LEN        8
 #define NCSI_OEM_MLX_CMD_SMAF_LEN        60

@@ -271,6 +275,7 @@ enum {
        ncsi_dev_state_probe_mlx_gma,
        ncsi_dev_state_probe_mlx_smaf,
        ncsi_dev_state_probe_cis,
+       ncsi_dev_state_probe_keep_phy,
        ncsi_dev_state_probe_gvi,
        ncsi_dev_state_probe_gc,
        ncsi_dev_state_probe_gls,

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 42b54a3da2e6..89c7742cd72e 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c

@@ -689,6 +689,35 @@ static int set_one_vid(struct ncsi_dev_priv

*ndp, struct ncsi_channel *nc,
        return 0;
 }

+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY)
+
+static int ncsi_oem_keep_phy_intel(struct ncsi_cmd_arg *nca)
+{
+       unsigned char data[NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN];
+       int ret = 0;
+
+       nca->payload = NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN;
+
+       memset(data, 0, NCSI_OEM_INTEL_CMD_KEEP_PHY_LEN);
+       *(unsigned int *)data = ntohl((__force
__be32)NCSI_OEM_MFR_INTEL_ID);
+
+       data[4] = NCSI_OEM_INTEL_CMD_KEEP_PHY;
+
+       /* PHY Link up attribute */
+       data[6] = 0x1;
+
+       nca->data = data;
+
+       ret = ncsi_xmit_cmd(nca);
+       if (ret)
+               netdev_err(nca->ndp->ndev.dev,
+                          "NCSI: Failed to transmit cmd 0x%x
during configure\n",
+                          nca->type);
+       return ret;
+}
+
+#endif
+
 #if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)

 /* NCSI OEM Command APIs */

@@ -1391,8 +1420,24 @@ static void ncsi_probe_channel(struct

ncsi_dev_priv *ndp)
                                goto error;
                }

+               nd->state = ncsi_dev_state_probe_gvi;
+               if (IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY))
+                       nd->state = ncsi_dev_state_probe_keep_phy;
+               break;
+#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_KEEP_PHY)
+       case ncsi_dev_state_probe_keep_phy:
+               ndp->pending_req_num = 1;
+
+               nca.type = NCSI_PKT_CMD_OEM;
+               nca.package = ndp->active_package->id;
+               nca.channel = 0;
+               ret = ncsi_oem_keep_phy_intel(&nca);
+               if (ret)
+                       goto error;
+
                nd->state = ncsi_dev_state_probe_gvi;
                break;
+#endif /* CONFIG_NCSI_OEM_CMD_KEEP_PHY */
        case ncsi_dev_state_probe_gvi:
        case ncsi_dev_state_probe_gc:
        case ncsi_dev_state_probe_gls:
--
2.31.1

On Mon, 2021-07-12 at 09:09 +0000, Joel Stanley wrote:

On Thu, 8 Jul 2021 at 12:27, Ivan Mikhaylov [off-list ref] wrote:

quoted

Sparse reports:
net/ncsi/ncsi-rsp.c:406:24: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:732:33: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:756:25: warning: cast to restricted __be32
net/ncsi/ncsi-manage.c:779:25: warning: cast to restricted __be32

Strange, I don't get these warnings from sparse on my system.

https://patchwork.hopto.org/static/nipa/510079/12355969/build_32bit/stderr
All those errors from ntohl. David linked this one in first series of patches.

$ sparse --version
0.6.3 (Debian: 0.6.3-2)

quoted

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/ncsi-manage.c | 6 +++---
 net/ncsi/ncsi-rsp.c    | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index ca04b6df1341..42b54a3da2e6 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c

@@ -700,7 +700,7 @@ static int ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg

*nca)
        nca->payload = NCSI_OEM_BCM_CMD_GMA_LEN;

        memset(data, 0, NCSI_OEM_BCM_CMD_GMA_LEN);
-       *(unsigned int *)data = ntohl(NCSI_OEM_MFR_BCM_ID);
+       *(unsigned int *)data = ntohl((__force __be32)NCSI_OEM_MFR_BCM_ID);

This looks wrong, the value you're passing isn't big endian. It would
make more sense if the byte swap was ntohl, as it's coming from the
cpu and going into the NCSI packet.

Is there any other ways to deal with those sparse errors?

Thanks.

On Mon, 2021-07-12 at 10:03 +0000, Joel Stanley wrote:

On Thu, 8 Jul 2021 at 12:28, Ivan Mikhaylov [off-list ref] wrote:

quoted

Add the dummy response handler for Intel boards to prevent incorrect
handling of OEM commands.

What do you mean?

When you don't have proper OEM handler for your MFR_ID, you'll get this as
example:
[   39.073873] ftgmac100 1e660000.ethernet eth1: Received unrecognized OEM
packet with MFR-ID (0x157)
[   39.082974] ftgmac100 1e660000.ethernet eth1: NCSI: Handler for packet type
0xd0 returned -2

Is this to handle the response to the link up OEM command? If so,
include it in the same patch.

It is not the response, it's provides same way of handling as for broadcom and
mellanox manufacturers.

Can you check that the response is to the link up command and print a
warning if not?

Yes, I can. As example, ncsi_oem_smaf_mlx doesn't check the response, for me
it's like unidirectional commands, same for this one.

Thanks.

On Mon, 2021-07-12 at 10:01 +0000, Joel Stanley wrote:

On Thu, 8 Jul 2021 at 12:27, Ivan Mikhaylov [off-list ref] wrote:

quoted

This allows to keep PHY link up and prevents any channel resets during
the host load.

It is KEEP_PHY_LINK_UP option(Veto bit) in i210 datasheet which
block PHY reset and power state changes.

How about using runtime configuration over using kconfig for this, so
the same kernel config can be used on different machines. Something
device tree based?

As I see there is already the way with Kconfig option, with previous
broadcom/mellanox get mac address and set affinity for mellanox commands.
I'm not sure about dts based solution. As I see there is two ways:
1. make everything related OEM into dts based options
2. or this way, with Kconfig

Another option is to use the netlink handler to send the OEM command
from userspace. Eddie has worked on this for an IBM machine, and I've
asked him to post those changes. I would prefer the kernel option
though.

I like the idea, it may help with debugging.

Thanks.

Hello,

On Thu, Jul 08, 2021 at 03:27:54PM +0300, Ivan Mikhaylov wrote:

Add the dummy response handler for Intel boards to prevent incorrect
handling of OEM commands.

It would be much nicer if it wasn't dummy but provide means of
obtaining the MAC properly, in a similar way to the other supported
network cards.

I have a patch I can share but not ready to send for proper mainlining
due to time constraints. Feel free to take it over and send as part of
your patch series.

From 6c717bbb75442c83bd11b37b7644f9ce187ee7e9 Mon Sep 17 00:00:00 2001
From: Brad Ho <redacted>
Date: Thu, 25 Feb 2021 00:53:03 -0800
Subject: [PATCH] Add get MAC address through NCSI command to get INTEL i210
 MAC address

Signed-off-by: Brad Ho <redacted>
Signed-off-by: Paul Fertser <fercerpav@gmail.com>
---
 net/ncsi/internal.h    |  5 ++++
 net/ncsi/ncsi-manage.c | 25 ++++++++++++++++-
 net/ncsi/ncsi-pkt.h    |  6 ++++
 net/ncsi/ncsi-rsp.c    | 62 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index e37102546be6..8a6a8127156b 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h

@@ -78,6 +78,7 @@ enum {
 /* OEM Vendor Manufacture ID */
 #define NCSI_OEM_MFR_MLX_ID             0x8119
 #define NCSI_OEM_MFR_BCM_ID             0x113d
+#define NCSI_OEM_MFR_INTEL_ID           0x0157
 /* Broadcom specific OEM Command */
 #define NCSI_OEM_BCM_CMD_GMA            0x01   /* CMD ID for Get MAC */
 /* Mellanox specific OEM Command */

@@ -85,16 +86,20 @@ enum {
 #define NCSI_OEM_MLX_CMD_GMA_PARAM      0x1b   /* Parameter for GMA  */
 #define NCSI_OEM_MLX_CMD_SMAF           0x01   /* CMD ID for Set MC Affinity */
 #define NCSI_OEM_MLX_CMD_SMAF_PARAM     0x07   /* Parameter for SMAF         */
+/* Intel specific OEM Command */
+#define NCSI_OEM_INTEL_CMD_GMA          0x06   /* CMD ID for Get MAC */
 /* OEM Command payload lengths*/
 #define NCSI_OEM_BCM_CMD_GMA_LEN        12
 #define NCSI_OEM_MLX_CMD_GMA_LEN        8
 #define NCSI_OEM_MLX_CMD_SMAF_LEN        60
+#define NCSI_OEM_INTEL_CMD_GMA_LEN      5
 /* Offset in OEM request */
 #define MLX_SMAF_MAC_ADDR_OFFSET         8     /* Offset for MAC in SMAF    */
 #define MLX_SMAF_MED_SUPPORT_OFFSET      14    /* Offset for medium in SMAF */
 /* Mac address offset in OEM response */
 #define BCM_MAC_ADDR_OFFSET             28
 #define MLX_MAC_ADDR_OFFSET             8
+#define INTEL_MAC_ADDR_OFFSET           1
 
 
 struct ncsi_channel_version {

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 1f387be7827b..fb25ae22ea3d 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c

@@ -760,13 +760,36 @@ static int ncsi_oem_smaf_mlx(struct ncsi_cmd_arg *nca)
 	return ret;
 }
 
+static int ncsi_oem_gma_handler_intel(struct ncsi_cmd_arg *nca)
+{
+	unsigned char data[NCSI_OEM_INTEL_CMD_GMA_LEN];
+	int ret = 0;
+
+	nca->payload = NCSI_OEM_INTEL_CMD_GMA_LEN;
+
+	memset(data, 0, NCSI_OEM_INTEL_CMD_GMA_LEN);
+	*(unsigned int *)data = ntohl(NCSI_OEM_MFR_INTEL_ID);
+	data[4] = NCSI_OEM_INTEL_CMD_GMA;
+
+	nca->data = data;
+
+	ret = ncsi_xmit_cmd(nca);
+	if (ret)
+		netdev_err(nca->ndp->ndev.dev,
+			   "NCSI: Failed to transmit cmd 0x%x during configure\n",
+			   nca->type);
+
+	return ret;
+}
+
 /* OEM Command handlers initialization */
 static struct ncsi_oem_gma_handler {
 	unsigned int	mfr_id;
 	int		(*handler)(struct ncsi_cmd_arg *nca);
 } ncsi_oem_gma_handlers[] = {
 	{ NCSI_OEM_MFR_BCM_ID, ncsi_oem_gma_handler_bcm },
-	{ NCSI_OEM_MFR_MLX_ID, ncsi_oem_gma_handler_mlx }
+	{ NCSI_OEM_MFR_MLX_ID, ncsi_oem_gma_handler_mlx },
+	{ NCSI_OEM_MFR_INTEL_ID, ncsi_oem_gma_handler_intel }
 };
 
 static int ncsi_gma_handler(struct ncsi_cmd_arg *nca, unsigned int mf_id)

diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index 80938b338fee..ba66c7dc3a21 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h

@@ -178,6 +178,12 @@ struct ncsi_rsp_oem_bcm_pkt {
 	unsigned char           data[];      /* Cmd specific Data */
 };
 
+/* Intel Response Data */
+struct ncsi_rsp_oem_intel_pkt {
+	unsigned char           cmd;         /* OEM Command ID    */
+	unsigned char           data[];      /* Cmd specific Data */
+};
+
 /* 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 a94bb59793f0..b36c22ec4c3f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c

@@ -699,12 +699,72 @@ static int ncsi_rsp_handler_oem_bcm(struct ncsi_request *nr)
 	return 0;
 }
 
+/* Response handler for Intel command Get Mac Address */
+static int ncsi_rsp_handler_oem_intel_gma(struct ncsi_request *nr)
+{
+	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct net_device *ndev = ndp->ndev.dev;
+	const struct net_device_ops *ops = ndev->netdev_ops;
+	struct ncsi_rsp_oem_pkt *rsp;
+	struct sockaddr saddr;
+	int ret = 0;
+
+	/* Get the response header */
+	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+
+	saddr.sa_family = ndev->type;
+	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+	memcpy(saddr.sa_data, &rsp->data[INTEL_MAC_ADDR_OFFSET], ETH_ALEN);
+	/* Increase mac address by 1 for BMC's address */
+	eth_addr_inc((u8 *)saddr.sa_data);
+	if (!is_valid_ether_addr((const u8 *)saddr.sa_data))
+		return -ENXIO;
+
+	/* Set the flag for GMA command which should only be called once */
+	ndp->gma_flag = 1;
+
+	ret = ops->ndo_set_mac_address(ndev, &saddr);
+	if (ret < 0)
+		netdev_warn(ndev, "NCSI: 'Writing mac address to device failed\n");
+
+	return ret;
+}
+
+/* Response handler for Intel card */
+static int ncsi_rsp_handler_oem_intel(struct ncsi_request *nr)
+{
+	struct ncsi_rsp_oem_intel_pkt *intel;
+	struct ncsi_rsp_oem_pkt *rsp;
+
+	/* Get the response header */
+	rsp = (struct ncsi_rsp_oem_pkt *)skb_network_header(nr->rsp);
+	intel = (struct ncsi_rsp_oem_intel_pkt *)(rsp->data);
+
+#if 0 //For debug use
+    #define NCSI_INTEL_GMA_LEN 6
+    int i = 0;
+
+    printk("[Error] %s, %d, intel->cmd = %x\n", __func__, __LINE__, intel->cmd);
+    for(i ; i < NCSI_INTEL_GMA_LEN; i++)
+    {
+        printk("[Error] %s, %d, rsp->data[%d] = %x\n", __func__, __LINE__, i, rsp->data[i]);
+        printk("[Error] %s, %d, intel_rsp->data[%d] = %x\n", __func__, __LINE__, i, intel->data[i]);
+    }
+#endif
+    
+	if (intel->cmd == NCSI_OEM_INTEL_CMD_GMA)
+		return ncsi_rsp_handler_oem_intel_gma(nr);
+
+	return 0;
+}
+
 static struct ncsi_rsp_oem_handler {
 	unsigned int	mfr_id;
 	int		(*handler)(struct ncsi_request *nr);
 } ncsi_rsp_oem_handlers[] = {
 	{ NCSI_OEM_MFR_MLX_ID, ncsi_rsp_handler_oem_mlx },
-	{ NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
+	{ NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm },
+	{ NCSI_OEM_MFR_INTEL_ID, ncsi_rsp_handler_oem_intel }
 };
 
 /* Response handler for OEM command */

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

Hello,

On Thu, Jul 08, 2021 at 03:27:51PM +0300, Ivan Mikhaylov wrote:

Add NCSI Intel OEM command to keep PHY link up and prevents any channel
resets during the host load on i210.

There're multiple things to consider here and I have hesitations about
the way you propose to solve the issue.

While the host is booted up and fully functional it assumes it has
full proper control of network cards, and sometimes it really needs to
reset them to e.g. recover from crashed firmware. The PHY resets might
also make sense in certain cases, and so in general having this "link
up" bit set all the time might be breaking assumptions.

As far as I can tell the Intel developers assumed you would enable
this bit just prior to powering on the host and turn off after all the
POST codes are transferred and we can assume the host system is done
with the UEFI stage and the real OS took over.

OpenBMC seems to have all the necessary hooks to do it that way, and
you have a netlink command to send whatever you need for that from the
userspace, e.g. with the "C version" ncsi-netlink command to set this
bit just run:

ncsi-netlink -l 3 -c 0 -p 0 -o 0x50 0x00 0x00 0x01 0x57 0x20 0x00 0x01

https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-networkd/+/36592
would provide an OpenBMC-specific way too.


There's another related thing to consider here: by default I210 has
power-saving modes enabled and so when BMC is booting the link is
established only in 100BASE-T mode. With this configuration and this
bit always set you'd be always stuck to that, never getting Gigabit
speeds.

For server motherboards I propose to configure I210 with this:
./eeupdate64e /all /ww 0x13 0x0081 # disable Low Power Link Up
./eeupdate64e /all /ww 0x20 0x2004 # enable 1000 in non-D0a
(it's a one-time operation that's best to be performed along with the
initial I210 flashing)


Ivan, so far I have an impression that the user-space solution would
be much easier, flexible and manageable and that there's no need for
this command to be in Linux at all.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

On Tue, 2021-07-20 at 12:41 +0300, Paul Fertser wrote:

Paul, I know about 'get mac address' and it was in my todo list. You can put it
before or after this patch series whenever you want, it doesn't interfere with
this one. Anyways, thanks for sharing it.

Thanks.

On Tue, Jul 20, 2021 at 04:21:31PM +0300, Ivan Mikhaylov wrote:

Paul, I know about 'get mac address' and it was in my todo list. You can put it
before or after this patch series whenever you want, it doesn't interfere with
this one. Anyways, thanks for sharing it.

The patch in question modifies the same file in the same place, see:

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index a94bb59793f0..b36c22ec4c3f 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c

...
 static struct ncsi_rsp_oem_handler {
        unsigned int    mfr_id;
        int             (*handler)(struct ncsi_request *nr);
 } ncsi_rsp_oem_handlers[] = {
        { NCSI_OEM_MFR_MLX_ID, ncsi_rsp_handler_oem_mlx },
-       { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
+       { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm },
+       { NCSI_OEM_MFR_INTEL_ID, ncsi_rsp_handler_oem_intel }
 };

And your patch:

Signed-off-by: Ivan Mikhaylov <redacted>
---
 net/ncsi/ncsi-rsp.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 04bc50be5c01..d48374894817 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c

...
 static struct ncsi_rsp_oem_handler {
        unsigned int    mfr_id;
        int             (*handler)(struct ncsi_request *nr);
 } ncsi_rsp_oem_handlers[] = {
        { NCSI_OEM_MFR_MLX_ID, ncsi_rsp_handler_oem_mlx },
-       { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm }
+       { NCSI_OEM_MFR_BCM_ID, ncsi_rsp_handler_oem_bcm },
+       { NCSI_OEM_MFR_INTEL_ID, ncsi_rsp_handler_oem_intel }
 };

so it does conflict. I suggest if you decide to continue with this
series rather than the userspace solution to include the MAC fetching
patch in your submission instead of the stub handler.

--
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com

On Tue, 2021-07-20 at 12:53 +0300, Paul Fertser wrote:

Hello,

On Thu, Jul 08, 2021 at 03:27:51PM +0300, Ivan Mikhaylov wrote:

quoted

Add NCSI Intel OEM command to keep PHY link up and prevents any channel
resets during the host load on i210.

There're multiple things to consider here and I have hesitations about
the way you propose to solve the issue.

While the host is booted up and fully functional it assumes it has
full proper control of network cards, and sometimes it really needs to
reset them to e.g. recover from crashed firmware. The PHY resets might
also make sense in certain cases, and so in general having this "link
up" bit set all the time might be breaking assumptions.

Paul, what kind of assumption it would break? You know that you set that option
in your kernel, anyways you can look at /proc/config.gz if you have hesitations.
In other ways, if you're saying about possible runtime control, there is ncsi-
netlink control and solution from phosphor-networkd which is on review stage.
Joel proposed it as DTS option which may help at runtime. Some of those commands
should be applied after channel probe as I think including phy reset control.

As far as I can tell the Intel developers assumed you would enable
this bit just prior to powering on the host and turn off after all the
POST codes are transferred and we can assume the host system is done
with the UEFI stage and the real OS took over.

OpenBMC seems to have all the necessary hooks to do it that way, and
you have a netlink command to send whatever you need for that from the
userspace, e.g. with the "C version" ncsi-netlink command to set this
bit just run:

ncsi-netlink -l 3 -c 0 -p 0 -o 0x50 0x00 0x00 0x01 0x57 0x20 0x00 0x01

https://gerrit.openbmc-project.xyz/c/openbmc/phosphor-networkd/+/36592
would provide an OpenBMC-specific way too.

I know about it, Eddie posted that link before.

There's another related thing to consider here: by default I210 has
power-saving modes enabled and so when BMC is booting the link is
established only in 100BASE-T mode. With this configuration and this
bit always set you'd be always stuck to that, never getting Gigabit
speeds.

For server motherboards I propose to configure I210 with this:
./eeupdate64e /all /ww 0x13 0x0081 # disable Low Power Link Up
./eeupdate64e /all /ww 0x20 0x2004 # enable 1000 in non-D0a
(it's a one-time operation that's best to be performed along with the
initial I210 flashing)

Good to know, thanks.

Ivan, so far I have an impression that the user-space solution would
be much easier, flexible and manageable and that there's no need for
this command to be in Linux at all.

You may not have such things on your image with suitable env which you can rely
on. There is smaf for mellanox which is done in the same way for example.

Thanks.

On Tue, Jul 20, 2021 at 05:00:40PM +0300, Ivan Mikhaylov wrote:

quoted

While the host is booted up and fully functional it assumes it has
full proper control of network cards, and sometimes it really needs to
reset them to e.g. recover from crashed firmware. The PHY resets might
also make sense in certain cases, and so in general having this "link
up" bit set all the time might be breaking assumptions.

Paul, what kind of assumption it would break?

The host OS drivers assume they can fully control PCIe network
cards. Doing anything (including inhibiting PHY resets) behind its
back might break assumptions the driver authors had. This bit in
question certainly makes the card behave in an unusual way, so no
wonder Intel didn't enable it by default.

I do not claim I know for a fact it's problematic but it doesn't feel
like "the right thing" so some edge cases might expose issues.

Joel proposed it as DTS option which may help at runtime.

Sorry, I'm not following. If BMC is fully booted it's able to
configure NC-SI appropriately by a userspace action coordinated with
other BMC tasks. If BMC is not yet ready then we can't communicate
with it via Ethernet anyway. So I can't see when exactly is it going
to be helpful.

Some of those commands should be applied after channel probe as I
think including phy reset control.

Do you have any other commands in mind? So far I assumed we're
discussing just the one to mask PHY resets.

quoted

Ivan, so far I have an impression that the user-space solution would
be much easier, flexible and manageable and that there's no need for
this command to be in Linux at all.

You may not have such things on your image with suitable env which you can rely
on. There is smaf for mellanox which is done in the same way for example.

I can hardly imagine why an OS running on BMC would be using this code
in question and appropriate DT configuration but not having the right
means in userspace to control it. What would be the usecase?

If the network subsystem maintainers think this is a good idea, all
things considered, I'm fine with it. I210 losing link exactly at the
time when you need it (to enter the UEFI interactive menu) is
super-annoying, so probably any fix is better than none :)

Thank you for discussion.

-- 
Be free, use free (http://www.gnu.org/philosophy/free-sw.html) software!
mailto:fercerpav@gmail.com