[PATCH 0/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[PATCH 0/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Peter Delevoryas]

NC-SI 1.2 isn't officially released yet, but the DMTF takes way too long
to finalize stuff, and there's hardware out there that actually supports
this command (Just the Broadcom 200G NIC afaik).

The work in progress spec document is here:

https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf

The command code is 0x58, the command has no data, and the response
returns a variable-length array of MAC addresses for the BMC.

I've tested this out using QEMU emulation (I added the Mellanox OEM Get
MAC Address command to libslirp a while ago [1], although the QEMU code
to use it is still not in upstream QEMU [2] [3]. I worked on some more
emulation code for this as well), and on the new Broadcom 200G NIC.

The Nvidia ConnectX-7 NIC doesn't support NC-SI 1.2 yet afaik. Neither
do older versions in newer firmware, they all just report NC-SI 1.1.

Let me know what I can do to change this patch to be more suitable for
upstreaming, I'm happy to work on it more!

Thanks,
Peter

[1] https://gitlab.freedesktop.org/slirp/libslirp/-/blob/0dd7f05095c0a77d9d2ec4764e8617192b4fa6ec/src/ncsi.c#L59
[2] https://github.com/facebook/openbmc/blob/a33dbcc25759f00baf113fd497c8d9db60eeed9e/common/recipes-devtools/qemu/qemu/0003-slirp-Add-mfr-id-to-netdev-options.patch
[3] https://github.com/facebook/openbmc/blob/a33dbcc25759f00baf113fd497c8d9db60eeed9e/common/recipes-devtools/qemu/qemu/0004-slirp-Add-oob-eth-addr-to-netdev-options.patch

Peter Delevoryas (3):
  net/ncsi: Simplify Kconfig/dts control flow
  net/ncsi: Fix netlink major/minor verison numbers
  net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

 net/ncsi/internal.h     |  7 ++--
 net/ncsi/ncsi-cmd.c     |  3 +-
 net/ncsi/ncsi-manage.c  | 29 ++++++-----------
 net/ncsi/ncsi-netlink.c |  4 +--
 net/ncsi/ncsi-pkt.h     | 17 ++++++++--
 net/ncsi/ncsi-rsp.c     | 71 +++++++++++++++++++++++++++++++++++++++--
 6 files changed, 102 insertions(+), 29 deletions(-)

-- 
2.30.2

[PATCH 1/3] net/ncsi: Simplify Kconfig/dts control flow

[Peter Delevoryas]

Background:

1. CONFIG_NCSI_OEM_CMD_KEEP_PHY

If this is enabled, we send an extra OEM Intel command in the probe
sequence immediately after discovering a channel (e.g. after "Clear
Initial State").

2. CONFIG_NCSI_OEM_CMD_GET_MAC

If this is enabled, we send one of 3 OEM "Get MAC Address" commands from
Broadcom, Mellanox (Nvidida), and Intel in the *configuration* sequence
for a channel.

3. mellanox,multi-host (or mlx,multi-host)

Introduced by this patch:

https://lore.kernel.org/all/20200108234341.2590674-1-vijaykhemka@fb.com/

Which was actually originally from cosmo.chou@quantatw.com:

https://github.com/facebook/openbmc-linux/commit/9f132a10ec48db84613519258cd8a317fb9c8f1b

Cosmo claimed that the Nvidia ConnectX-4 and ConnectX-6 NIC's don't
respond to Get Version ID, et. al in the probe sequence unless you send
the Set MC Affinity command first.

Problem Statement:

We've been using a combination of #ifdef code blocks and IS_ENABLED()
conditions to conditionally send these OEM commands.

It makes adding any new code around these commands hard to understand.

Solution:

In this patch, I just want to remove the conditionally compiled blocks
of code, and always use IS_ENABLED(...) to do dynamic control flow.

I don't think the small amount of code this adds to non-users of the OEM
Kconfigs is a big deal.

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
 net/ncsi/ncsi-manage.c | 20 +++-----------------
 1 file changed, 3 insertions(+), 17 deletions(-)

diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index 80713febfac6..f56795769893 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -689,8 +689,6 @@ 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];
@@ -716,10 +714,6 @@ static int ncsi_oem_keep_phy_intel(struct ncsi_cmd_arg *nca)
 	return ret;
 }
 
-#endif
-
-#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
-
 /* NCSI OEM Command APIs */
 static int ncsi_oem_gma_handler_bcm(struct ncsi_cmd_arg *nca)
 {
@@ -856,8 +850,6 @@ static int ncsi_gma_handler(struct ncsi_cmd_arg *nca, unsigned int mf_id)
 	return nch->handler(nca);
 }
 
-#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
-
 /* Determine if a given channel from the channel_queue should be used for Tx */
 static bool ncsi_channel_is_tx(struct ncsi_dev_priv *ndp,
 			       struct ncsi_channel *nc)
@@ -1039,20 +1031,18 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 			goto error;
 		}
 
-		nd->state = ncsi_dev_state_config_oem_gma;
+		nd->state = IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
+			  ? ncsi_dev_state_config_oem_gma
+			  : ncsi_dev_state_config_clear_vids;
 		break;
 	case ncsi_dev_state_config_oem_gma:
 		nd->state = ncsi_dev_state_config_clear_vids;
-		ret = -1;
 
-#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
 		nca.type = NCSI_PKT_CMD_OEM;
 		nca.package = np->id;
 		nca.channel = nc->id;
 		ndp->pending_req_num = 1;
 		ret = ncsi_gma_handler(&nca, nc->version.mf_id);
-#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
-
 		if (ret < 0)
 			schedule_work(&ndp->work);
 
@@ -1404,7 +1394,6 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
 
 		schedule_work(&ndp->work);
 		break;
-#if IS_ENABLED(CONFIG_NCSI_OEM_CMD_GET_MAC)
 	case ncsi_dev_state_probe_mlx_gma:
 		ndp->pending_req_num = 1;
 
@@ -1429,7 +1418,6 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
 
 		nd->state = ncsi_dev_state_probe_cis;
 		break;
-#endif /* CONFIG_NCSI_OEM_CMD_GET_MAC */
 	case ncsi_dev_state_probe_cis:
 		ndp->pending_req_num = NCSI_RESERVED_CHANNEL;
 
@@ -1447,7 +1435,6 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
 		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;
 
@@ -1460,7 +1447,6 @@ static void ncsi_probe_channel(struct ncsi_dev_priv *ndp)
 
 		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.30.2

[PATCH 2/3] net/ncsi: Fix netlink major/minor version numbers

[Peter Delevoryas]

The netlink interface for major and minor version numbers doesn't actually
return the major and minor version numbers.

It reports a u32 that contains the (major, minor, update, alpha1)
components as the major version number, and then alpha2 as the minor
version number.

For whatever reason, the u32 byte order was reversed (ntohl): maybe it was
assumed that the encoded value was a single big-endian u32, and alpha2 was
the minor version.

The correct way to get the supported NC-SI version from the network
controller is to parse the Get Version ID response as described in 8.4.44
of the NC-SI spec[1].

    Get Version ID Response Packet Format

              Bits
            +--------+--------+--------+--------+
     Bytes  | 31..24 | 23..16 | 15..8  | 7..0   |
    +-------+--------+--------+--------+--------+
    | 0..15 | NC-SI Header                      |
    +-------+--------+--------+--------+--------+
    | 16..19| Response code   | Reason code     |
    +-------+--------+--------+--------+--------+
    |20..23 | Major  | Minor  | Update | Alpha1 |
    +-------+--------+--------+--------+--------+
    |24..27 |         reserved         | Alpha2 |
    +-------+--------+--------+--------+--------+
    |            .... other stuff ....          |

The major, minor, and update fields are all binary-coded decimal (BCD)
encoded [2]. The spec provides examples below the Get Version ID response
format in section 8.4.44.1, but for practical purposes, this is an example
from a live network card:

    root@bmc:~# ncsi-util 0x15
    NC-SI Command Response:
    cmd: GET_VERSION_ID(0x15)
    Response: COMMAND_COMPLETED(0x0000)  Reason: NO_ERROR(0x0000)
    Payload length = 40

    20: 0xf1 0xf1 0xf0 0x00 <<<<<<<<< (major, minor, update, alpha1)
    24: 0x00 0x00 0x00 0x00 <<<<<<<<< (_, _, _, alpha2)

    28: 0x6d 0x6c 0x78 0x30
    32: 0x2e 0x31 0x00 0x00
    36: 0x00 0x00 0x00 0x00
    40: 0x16 0x1d 0x07 0xd2
    44: 0x10 0x1d 0x15 0xb3
    48: 0x00 0x17 0x15 0xb3
    52: 0x00 0x00 0x81 0x19

This should be parsed as "1.1.0".

"f" in the upper-nibble means to ignore it, contributing zero.

If both nibbles are "f", I think the whole field is supposed to be ignored.
Major and minor are "required", meaning they're not supposed to be "ff",
but the update field is "optional" so I think it can be ff. I think the
simplest thing to do is just set the major and minor to zero instead of
juggling some conditional logic or something.

bcd2bin() from "include/linux/bcd.h" seems to assume both nibbles are 0-9,
so I've provided a custom BCD decoding function.

Alpha1 and alpha2 are ISO/IEC 8859-1 encoded, which just means ASCII
characters as far as I can tell, although the full encoding table for
non-alphabetic characters is slightly different (I think).

I imagine the alpha fields are just supposed to be alphabetic characters,
but I haven't seen any network cards actually report a non-zero value for
either.

If people wrote software against this netlink behavior, and were parsing
the major and minor versions themselves from the u32, then this would
definitely break their code.

[1] https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.0.0.pdf
[2] https://en.wikipedia.org/wiki/Binary-coded_decimal
[2] https://en.wikipedia.org/wiki/ISO/IEC_8859-1

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
Fixes: 138635cc27c9 ("net/ncsi: NCSI response packet handler")
---
 net/ncsi/internal.h     |  7 +++++--
 net/ncsi/ncsi-netlink.c |  4 ++--
 net/ncsi/ncsi-pkt.h     |  7 +++++--
 net/ncsi/ncsi-rsp.c     | 26 ++++++++++++++++++++++++--
 4 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/net/ncsi/internal.h b/net/ncsi/internal.h
index 03757e76bb6b..374412ed780b 100644
--- a/net/ncsi/internal.h
+++ b/net/ncsi/internal.h
@@ -105,8 +105,11 @@ enum {
 
 
 struct ncsi_channel_version {
-	u32 version;		/* Supported BCD encoded NCSI version */
-	u32 alpha2;		/* Supported BCD encoded NCSI version */
+	u8   major;		/* NCSI version major */
+	u8   minor;		/* NCSI version minor */
+	u8   update;		/* NCSI version update */
+	char alpha1;		/* NCSI version alpha1 */
+	char alpha2;		/* NCSI version alpha2 */
 	u8  fw_name[12];	/* Firmware name string                */
 	u32 fw_version;		/* Firmware version                   */
 	u16 pci_ids[4];		/* PCI identification                 */
diff --git a/net/ncsi/ncsi-netlink.c b/net/ncsi/ncsi-netlink.c
index c189b4c8a182..db350b8f5d88 100644
--- a/net/ncsi/ncsi-netlink.c
+++ b/net/ncsi/ncsi-netlink.c
@@ -71,8 +71,8 @@ static int ncsi_write_channel_info(struct sk_buff *skb,
 	if (nc == nc->package->preferred_channel)
 		nla_put_flag(skb, NCSI_CHANNEL_ATTR_FORCED);
 
-	nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.version);
-	nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MINOR, nc->version.alpha2);
+	nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MAJOR, nc->version.major);
+	nla_put_u32(skb, NCSI_CHANNEL_ATTR_VERSION_MINOR, nc->version.minor);
 	nla_put_string(skb, NCSI_CHANNEL_ATTR_VERSION_STR, nc->version.fw_name);
 
 	vid_nest = nla_nest_start_noflag(skb, NCSI_CHANNEL_ATTR_VLAN_LIST);
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index ba66c7dc3a21..c9d1da34dc4d 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -197,9 +197,12 @@ struct ncsi_rsp_gls_pkt {
 /* Get Version ID */
 struct ncsi_rsp_gvi_pkt {
 	struct ncsi_rsp_pkt_hdr rsp;          /* Response header */
-	__be32                  ncsi_version; /* NCSI version    */
+	unsigned char           major;        /* NCSI version major */
+	unsigned char           minor;        /* NCSI version minor */
+	unsigned char           update;       /* NCSI version update */
+	unsigned char           alpha1;       /* NCSI version alpha1 */
 	unsigned char           reserved[3];  /* Reserved        */
-	unsigned char           alpha2;       /* NCSI version    */
+	unsigned char           alpha2;       /* NCSI version alpha2 */
 	unsigned char           fw_name[12];  /* f/w name string */
 	__be32                  fw_version;   /* f/w version     */
 	__be16                  pci_ids[4];   /* PCI IDs         */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 6447a09932f5..7a805b86a12d 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -19,6 +19,19 @@
 #include "ncsi-pkt.h"
 #include "ncsi-netlink.h"
 
+/* Nibbles within [0xA, 0xF] add zero "0" to the returned value.
+ * Optional fields (encoded as 0xFF) will default to zero.
+ */
+static u8 decode_bcd_u8(u8 x)
+{
+	int lo = x & 0xF;
+	int hi = x >> 4;
+
+	lo = lo < 0xA ? lo : 0;
+	hi = hi < 0xA ? hi : 0;
+	return lo + hi * 10;
+}
+
 static int ncsi_validate_rsp_pkt(struct ncsi_request *nr,
 				 unsigned short payload)
 {
@@ -804,9 +817,18 @@ static int ncsi_rsp_handler_gvi(struct ncsi_request *nr)
 	if (!nc)
 		return -ENODEV;
 
-	/* Update to channel's version info */
+	/* Update channel's version info
+	 *
+	 * Major, minor, and update fields are supposed to be
+	 * unsigned integers encoded as packed BCD.
+	 *
+	 * Alpha1 and alpha2 are ISO/IEC 8859-1 characters.
+	 */
 	ncv = &nc->version;
-	ncv->version = ntohl(rsp->ncsi_version);
+	ncv->major = decode_bcd_u8(rsp->major);
+	ncv->minor = decode_bcd_u8(rsp->minor);
+	ncv->update = decode_bcd_u8(rsp->update);
+	ncv->alpha1 = rsp->alpha1;
 	ncv->alpha2 = rsp->alpha2;
 	memcpy(ncv->fw_name, rsp->fw_name, 12);
 	ncv->fw_version = ntohl(rsp->fw_version);
-- 
2.30.2

[PATCH 3/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Peter Delevoryas]

This change adds support for the NC-SI 1.2 Get MC MAC Address command,
specified here:

https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf

It serves the exact same function as the existing OEM Get MAC Address
commands, so if a channel reports that it supports NC-SI 1.2, we prefer
to use the standard command rather than the OEM command.

Verified with an invalid MAC address and 2 valid ones:

[   55.137072] ftgmac100 1e690000.ftgmac eth0: NCSI: Received 3 provisioned MAC addresses
[   55.137614] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 0: 00:00:00:00:00:00
[   55.138026] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 1: fa:ce:b0:0c:20:22
[   55.138528] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 2: fa:ce:b0:0c:20:23
[   55.139241] ftgmac100 1e690000.ftgmac eth0: NCSI: Unable to assign 00:00:00:00:00:00 to device
[   55.140098] ftgmac100 1e690000.ftgmac eth0: NCSI: Set MAC address to fa:ce:b0:0c:20:22

IMPORTANT NOTE:

The code I'm submitting here is parsing the MAC addresses as if they are
transmitted in *reverse* order.

This is different from how every other NC-SI command is parsed in the
Linux kernel, even though the spec describes the format in the same way
for every command.

The *reason* for this is that I was able to test this code against the
new 200G Broadcom NIC, which reports that it supports NC-SI 1.2 in Get
Version ID and successfully responds to this command. It transmits the
MAC addresses in reverse byte order.

Nvidia's new 200G NIC doesn't support NC-SI 1.2 yet. I don't know how
they're planning to implement it.

Why did Broadcom do this? Well, they have some of the core NC-SI
specification maintainers there, and at some point they must have
decided that everyone (Linux kernel folks and Nvidia and previous
Broadcom firmware authors) is interpreting the specification
incorrectly. They created a 2nd edition of their OEM Get MAC Address
command that returns the MAC address in reverse order:

https://github.com/facebook/openbmc-linux/commit/a4a3a45809c6d43582a12a25bc45b95a79a4c034

That was never upstreamed, but it's supposed to replace the version we
have in the upstream 6.x kernel today.

So, to summarize: I chose the reverse encoding because that's what the
first NC-SI 1.2 supported NIC is doing, and apparently that might be the
correct way to interpret the spec as we go forward.

Signed-off-by: Peter Delevoryas <peter@pjd.dev>
---
 net/ncsi/ncsi-cmd.c    |  3 ++-
 net/ncsi/ncsi-manage.c |  9 +++++++--
 net/ncsi/ncsi-pkt.h    | 10 ++++++++++
 net/ncsi/ncsi-rsp.c    | 45 +++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 63 insertions(+), 4 deletions(-)

diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
index dda8b76b7798..7be177f55173 100644
--- a/net/ncsi/ncsi-cmd.c
+++ b/net/ncsi/ncsi-cmd.c
@@ -269,7 +269,8 @@ static struct ncsi_cmd_handler {
 	{ NCSI_PKT_CMD_GPS,    0, ncsi_cmd_handler_default },
 	{ NCSI_PKT_CMD_OEM,   -1, ncsi_cmd_handler_oem     },
 	{ NCSI_PKT_CMD_PLDM,   0, NULL                     },
-	{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
+	{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default },
+	{ NCSI_PKT_CMD_GMCMA,  0, ncsi_cmd_handler_default }
 };
 
 static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
index f56795769893..bc1887a2543d 100644
--- a/net/ncsi/ncsi-manage.c
+++ b/net/ncsi/ncsi-manage.c
@@ -1038,11 +1038,16 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
 	case ncsi_dev_state_config_oem_gma:
 		nd->state = ncsi_dev_state_config_clear_vids;
 
-		nca.type = NCSI_PKT_CMD_OEM;
 		nca.package = np->id;
 		nca.channel = nc->id;
 		ndp->pending_req_num = 1;
-		ret = ncsi_gma_handler(&nca, nc->version.mf_id);
+		if (nc->version.major >= 1 && nc->version.minor >= 2) {
+			nca.type = NCSI_PKT_CMD_GMCMA;
+			ret = ncsi_xmit_cmd(&nca);
+		} else {
+			nca.type = NCSI_PKT_CMD_OEM;
+			ret = ncsi_gma_handler(&nca, nc->version.mf_id);
+		}
 		if (ret < 0)
 			schedule_work(&ndp->work);
 
diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
index c9d1da34dc4d..f2f3b5c1b941 100644
--- a/net/ncsi/ncsi-pkt.h
+++ b/net/ncsi/ncsi-pkt.h
@@ -338,6 +338,14 @@ struct ncsi_rsp_gpuuid_pkt {
 	__be32                  checksum;
 };
 
+/* Get MC MAC Address */
+struct ncsi_rsp_gmcma_pkt {
+	struct ncsi_rsp_pkt_hdr rsp;
+	unsigned char           address_count;
+	unsigned char           reserved[3];
+	unsigned char           addresses[][ETH_ALEN];
+};
+
 /* AEN: Link State Change */
 struct ncsi_aen_lsc_pkt {
 	struct ncsi_aen_pkt_hdr aen;        /* AEN header      */
@@ -398,6 +406,7 @@ struct ncsi_aen_hncdsc_pkt {
 #define NCSI_PKT_CMD_GPUUID	0x52 /* Get package UUID                 */
 #define NCSI_PKT_CMD_QPNPR	0x56 /* Query Pending NC PLDM request */
 #define NCSI_PKT_CMD_SNPR	0x57 /* Send NC PLDM Reply  */
+#define NCSI_PKT_CMD_GMCMA	0x58 /* Get MC MAC Address */
 
 
 /* NCSI packet responses */
@@ -433,6 +442,7 @@ struct ncsi_aen_hncdsc_pkt {
 #define NCSI_PKT_RSP_GPUUID	(NCSI_PKT_CMD_GPUUID + 0x80)
 #define NCSI_PKT_RSP_QPNPR	(NCSI_PKT_CMD_QPNPR   + 0x80)
 #define NCSI_PKT_RSP_SNPR	(NCSI_PKT_CMD_SNPR   + 0x80)
+#define NCSI_PKT_RSP_GMCMA	(NCSI_PKT_CMD_GMCMA  + 0x80)
 
 /* NCSI response code/reason */
 #define NCSI_PKT_RSP_C_COMPLETED	0x0000 /* Command Completed        */
diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
index 7a805b86a12d..28a042688d0b 100644
--- a/net/ncsi/ncsi-rsp.c
+++ b/net/ncsi/ncsi-rsp.c
@@ -1140,6 +1140,48 @@ static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
 	return ret;
 }
 
+static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
+{
+	struct ncsi_dev_priv *ndp = nr->ndp;
+	struct net_device *ndev = ndp->ndev.dev;
+	struct ncsi_rsp_gmcma_pkt *rsp;
+	struct sockaddr saddr;
+	int ret = -1;
+	int i;
+	int j;
+
+	rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
+	saddr.sa_family = ndev->type;
+	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
+
+	netdev_warn(ndev, "NCSI: Received %d provisioned MAC addresses\n",
+		    rsp->address_count);
+	for (i = 0; i < rsp->address_count; i++) {
+		netdev_warn(ndev, "NCSI: MAC address %d: "
+			    "%02x:%02x:%02x:%02x:%02x:%02x\n", i,
+			    rsp->addresses[i][5], rsp->addresses[i][4],
+			    rsp->addresses[i][3], rsp->addresses[i][2],
+			    rsp->addresses[i][1], rsp->addresses[i][0]);
+	}
+
+	for (i = 0; i < rsp->address_count; i++) {
+		for (j = 0; j < ETH_ALEN; j++) {
+			saddr.sa_data[j] = rsp->addresses[i][ETH_ALEN - j - 1];
+		}
+		ret = ndev->netdev_ops->ndo_set_mac_address(ndev, &saddr);
+		if (ret < 0) {
+			netdev_warn(ndev, "NCSI: Unable to assign %pM to "
+				    "device\n", saddr.sa_data);
+			continue;
+		}
+		netdev_warn(ndev, "NCSI: Set MAC address to %pM\n", saddr.sa_data);
+		break;
+	}
+
+	ndp->gma_flag = ret == 0;
+	return ret;
+}
+
 static struct ncsi_rsp_handler {
 	unsigned char	type;
 	int             payload;
@@ -1176,7 +1218,8 @@ static struct ncsi_rsp_handler {
 	{ NCSI_PKT_RSP_PLDM,   -1, ncsi_rsp_handler_pldm    },
 	{ NCSI_PKT_RSP_GPUUID, 20, ncsi_rsp_handler_gpuuid  },
 	{ NCSI_PKT_RSP_QPNPR,  -1, ncsi_rsp_handler_pldm    },
-	{ NCSI_PKT_RSP_SNPR,   -1, ncsi_rsp_handler_pldm    }
+	{ NCSI_PKT_RSP_SNPR,   -1, ncsi_rsp_handler_pldm    },
+	{ NCSI_PKT_RSP_GMCMA,  -1, ncsi_rsp_handler_gmcma   },
 };
 
 int ncsi_rcv_rsp(struct sk_buff *skb, struct net_device *dev,
-- 
2.30.2

Re: [PATCH 0/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Paolo Abeni]

On Tue, 2022-12-20 at 21:22 -0800, Peter Delevoryas wrote:
> NC-SI 1.2 isn't officially released yet, but the DMTF takes way too long
> to finalize stuff, and there's hardware out there that actually supports
> this command (Just the Broadcom 200G NIC afaik).
> 
> The work in progress spec document is here:
> 
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf
> 
> The command code is 0x58, the command has no data, and the response
> returns a variable-length array of MAC addresses for the BMC.
> 
> I've tested this out using QEMU emulation (I added the Mellanox OEM Get
> MAC Address command to libslirp a while ago [1], although the QEMU code
> to use it is still not in upstream QEMU [2] [3]. I worked on some more
> emulation code for this as well), and on the new Broadcom 200G NIC.
> 
> The Nvidia ConnectX-7 NIC doesn't support NC-SI 1.2 yet afaik. Neither
> do older versions in newer firmware, they all just report NC-SI 1.1.
> 
> Let me know what I can do to change this patch to be more suitable for
> upstreaming, I'm happy to work on it more!

This series is targeting the net-next tree, you should include such tag
into the patch subjected.

We have already submitted the networking pull request to Linus
for v6.2 and therefore net-next is closed for new drivers, features,
code refactoring and optimizations. We are currently accepting
bug fixes only.

Please repost when net-next reopens after Jan 2nd.

RFC patches sent for review only are obviously welcome at any time.

Thanks,

Paolo

Re: [PATCH 3/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Paolo Abeni]

On Tue, 2022-12-20 at 21:22 -0800, Peter Delevoryas wrote:
> This change adds support for the NC-SI 1.2 Get MC MAC Address command,
> specified here:
> 
> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf
> 
> It serves the exact same function as the existing OEM Get MAC Address
> commands, so if a channel reports that it supports NC-SI 1.2, we prefer
> to use the standard command rather than the OEM command.
> 
> Verified with an invalid MAC address and 2 valid ones:
> 
> [   55.137072] ftgmac100 1e690000.ftgmac eth0: NCSI: Received 3 provisioned MAC addresses
> [   55.137614] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 0: 00:00:00:00:00:00
> [   55.138026] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 1: fa:ce:b0:0c:20:22
> [   55.138528] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 2: fa:ce:b0:0c:20:23
> [   55.139241] ftgmac100 1e690000.ftgmac eth0: NCSI: Unable to assign 00:00:00:00:00:00 to device
> [   55.140098] ftgmac100 1e690000.ftgmac eth0: NCSI: Set MAC address to fa:ce:b0:0c:20:22
> 
> IMPORTANT NOTE:
> 
> The code I'm submitting here is parsing the MAC addresses as if they are
> transmitted in *reverse* order.
> 
> This is different from how every other NC-SI command is parsed in the
> Linux kernel, even though the spec describes the format in the same way
> for every command.
> 
> The *reason* for this is that I was able to test this code against the
> new 200G Broadcom NIC, which reports that it supports NC-SI 1.2 in Get
> Version ID and successfully responds to this command. It transmits the
> MAC addresses in reverse byte order.
> 
> Nvidia's new 200G NIC doesn't support NC-SI 1.2 yet. I don't know how
> they're planning to implement it.

All the above looks like a good reason to wait for at least a
stable/documented H/W implementation, before pushing code to the
networking core.

>  net/ncsi/ncsi-cmd.c    |  3 ++-
>  net/ncsi/ncsi-manage.c |  9 +++++++--
>  net/ncsi/ncsi-pkt.h    | 10 ++++++++++
>  net/ncsi/ncsi-rsp.c    | 45 +++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 63 insertions(+), 4 deletions(-)
> 
> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
> index dda8b76b7798..7be177f55173 100644
> --- a/net/ncsi/ncsi-cmd.c
> +++ b/net/ncsi/ncsi-cmd.c
> @@ -269,7 +269,8 @@ static struct ncsi_cmd_handler {
>  	{ NCSI_PKT_CMD_GPS,    0, ncsi_cmd_handler_default },
>  	{ NCSI_PKT_CMD_OEM,   -1, ncsi_cmd_handler_oem     },
>  	{ NCSI_PKT_CMD_PLDM,   0, NULL                     },
> -	{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
> +	{ NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default },
> +	{ NCSI_PKT_CMD_GMCMA,  0, ncsi_cmd_handler_default }
>  };
>  
>  static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
> index f56795769893..bc1887a2543d 100644
> --- a/net/ncsi/ncsi-manage.c
> +++ b/net/ncsi/ncsi-manage.c
> @@ -1038,11 +1038,16 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>  	case ncsi_dev_state_config_oem_gma:
>  		nd->state = ncsi_dev_state_config_clear_vids;
>  
> -		nca.type = NCSI_PKT_CMD_OEM;
>  		nca.package = np->id;
>  		nca.channel = nc->id;
>  		ndp->pending_req_num = 1;
> -		ret = ncsi_gma_handler(&nca, nc->version.mf_id);
> +		if (nc->version.major >= 1 && nc->version.minor >= 2) {
> +			nca.type = NCSI_PKT_CMD_GMCMA;
> +			ret = ncsi_xmit_cmd(&nca);
> +		} else {
> +			nca.type = NCSI_PKT_CMD_OEM;
> +			ret = ncsi_gma_handler(&nca, nc->version.mf_id);
> +		}
>  		if (ret < 0)
>  			schedule_work(&ndp->work);
>  
> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
> index c9d1da34dc4d..f2f3b5c1b941 100644
> --- a/net/ncsi/ncsi-pkt.h
> +++ b/net/ncsi/ncsi-pkt.h
> @@ -338,6 +338,14 @@ struct ncsi_rsp_gpuuid_pkt {
>  	__be32                  checksum;
>  };
>  
> +/* Get MC MAC Address */
> +struct ncsi_rsp_gmcma_pkt {
> +	struct ncsi_rsp_pkt_hdr rsp;
> +	unsigned char           address_count;
> +	unsigned char           reserved[3];
> +	unsigned char           addresses[][ETH_ALEN];
> +};
> +
>  /* AEN: Link State Change */
>  struct ncsi_aen_lsc_pkt {
>  	struct ncsi_aen_pkt_hdr aen;        /* AEN header      */
> @@ -398,6 +406,7 @@ struct ncsi_aen_hncdsc_pkt {
>  #define NCSI_PKT_CMD_GPUUID	0x52 /* Get package UUID                 */
>  #define NCSI_PKT_CMD_QPNPR	0x56 /* Query Pending NC PLDM request */
>  #define NCSI_PKT_CMD_SNPR	0x57 /* Send NC PLDM Reply  */
> +#define NCSI_PKT_CMD_GMCMA	0x58 /* Get MC MAC Address */
>  
>  
>  /* NCSI packet responses */
> @@ -433,6 +442,7 @@ struct ncsi_aen_hncdsc_pkt {
>  #define NCSI_PKT_RSP_GPUUID	(NCSI_PKT_CMD_GPUUID + 0x80)
>  #define NCSI_PKT_RSP_QPNPR	(NCSI_PKT_CMD_QPNPR   + 0x80)
>  #define NCSI_PKT_RSP_SNPR	(NCSI_PKT_CMD_SNPR   + 0x80)
> +#define NCSI_PKT_RSP_GMCMA	(NCSI_PKT_CMD_GMCMA  + 0x80)
>  
>  /* NCSI response code/reason */
>  #define NCSI_PKT_RSP_C_COMPLETED	0x0000 /* Command Completed        */
> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
> index 7a805b86a12d..28a042688d0b 100644
> --- a/net/ncsi/ncsi-rsp.c
> +++ b/net/ncsi/ncsi-rsp.c
> @@ -1140,6 +1140,48 @@ static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
>  	return ret;
>  }
>  
> +static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
> +{
> +	struct ncsi_dev_priv *ndp = nr->ndp;
> +	struct net_device *ndev = ndp->ndev.dev;
> +	struct ncsi_rsp_gmcma_pkt *rsp;
> +	struct sockaddr saddr;
> +	int ret = -1;
> +	int i;
> +	int j;
> +
> +	rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
> +	saddr.sa_family = ndev->type;
> +	ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
> +
> +	netdev_warn(ndev, "NCSI: Received %d provisioned MAC addresses\n",
> +		    rsp->address_count);
> +	for (i = 0; i < rsp->address_count; i++) {
> +		netdev_warn(ndev, "NCSI: MAC address %d: "
> +			    "%02x:%02x:%02x:%02x:%02x:%02x\n", i,
> +			    rsp->addresses[i][5], rsp->addresses[i][4],
> +			    rsp->addresses[i][3], rsp->addresses[i][2],
> +			    rsp->addresses[i][1], rsp->addresses[i][0]);
> +	}

You must avoid this kind of debug messages on 'warn' level (more
below). You could consider pr_debug() instead or completely drop the
message.

Cheers,

Paolo

Re: [PATCH 0/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Peter Delevoryas]

> On Dec 22, 2022, at 2:44 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Tue, 2022-12-20 at 21:22 -0800, Peter Delevoryas wrote:
>> NC-SI 1.2 isn't officially released yet, but the DMTF takes way too long
>> to finalize stuff, and there's hardware out there that actually supports
>> this command (Just the Broadcom 200G NIC afaik).
>> 
>> The work in progress spec document is here:
>> 
>> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf
>> 
>> The command code is 0x58, the command has no data, and the response
>> returns a variable-length array of MAC addresses for the BMC.
>> 
>> I've tested this out using QEMU emulation (I added the Mellanox OEM Get
>> MAC Address command to libslirp a while ago [1], although the QEMU code
>> to use it is still not in upstream QEMU [2] [3]. I worked on some more
>> emulation code for this as well), and on the new Broadcom 200G NIC.
>> 
>> The Nvidia ConnectX-7 NIC doesn't support NC-SI 1.2 yet afaik. Neither
>> do older versions in newer firmware, they all just report NC-SI 1.1.
>> 
>> Let me know what I can do to change this patch to be more suitable for
>> upstreaming, I'm happy to work on it more!
> 
> This series is targeting the net-next tree, you should include such tag
> into the patch subjected.
> 
> We have already submitted the networking pull request to Linus
> for v6.2 and therefore net-next is closed for new drivers, features,
> code refactoring and optimizations. We are currently accepting
> bug fixes only.
> Please repost when net-next reopens after Jan 2nd.

I see, thanks, I’ll resubmit later.

> 
> RFC patches sent for review only are obviously welcome at any time.

Oh good point, I should have submitted it as an RFC.

> 
> Thanks,
> 
> Paolo
> 

Re: [PATCH 3/3] net/ncsi: Add NC-SI 1.2 Get MC MAC Address command

[Peter Delevoryas]

> On Dec 22, 2022, at 2:53 AM, Paolo Abeni <pabeni@redhat.com> wrote:
> 
> On Tue, 2022-12-20 at 21:22 -0800, Peter Delevoryas wrote:
>> This change adds support for the NC-SI 1.2 Get MC MAC Address command,
>> specified here:
>> 
>> https://www.dmtf.org/sites/default/files/standards/documents/DSP0222_1.2WIP90_0.pdf
>> 
>> It serves the exact same function as the existing OEM Get MAC Address
>> commands, so if a channel reports that it supports NC-SI 1.2, we prefer
>> to use the standard command rather than the OEM command.
>> 
>> Verified with an invalid MAC address and 2 valid ones:
>> 
>> [   55.137072] ftgmac100 1e690000.ftgmac eth0: NCSI: Received 3 provisioned MAC addresses
>> [   55.137614] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 0: 00:00:00:00:00:00
>> [   55.138026] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 1: fa:ce:b0:0c:20:22
>> [   55.138528] ftgmac100 1e690000.ftgmac eth0: NCSI: MAC address 2: fa:ce:b0:0c:20:23
>> [   55.139241] ftgmac100 1e690000.ftgmac eth0: NCSI: Unable to assign 00:00:00:00:00:00 to device
>> [   55.140098] ftgmac100 1e690000.ftgmac eth0: NCSI: Set MAC address to fa:ce:b0:0c:20:22
>> 
>> IMPORTANT NOTE:
>> 
>> The code I'm submitting here is parsing the MAC addresses as if they are
>> transmitted in *reverse* order.
>> 
>> This is different from how every other NC-SI command is parsed in the
>> Linux kernel, even though the spec describes the format in the same way
>> for every command.
>> 
>> The *reason* for this is that I was able to test this code against the
>> new 200G Broadcom NIC, which reports that it supports NC-SI 1.2 in Get
>> Version ID and successfully responds to this command. It transmits the
>> MAC addresses in reverse byte order.
>> 
>> Nvidia's new 200G NIC doesn't support NC-SI 1.2 yet. I don't know how
>> they're planning to implement it.
> 
> All the above looks like a good reason to wait for at least a
> stable/documented H/W implementation, before pushing code to the
> networking core.

I guess that’s a good point.

> 
>> net/ncsi/ncsi-cmd.c    |  3 ++-
>> net/ncsi/ncsi-manage.c |  9 +++++++--
>> net/ncsi/ncsi-pkt.h    | 10 ++++++++++
>> net/ncsi/ncsi-rsp.c    | 45 +++++++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 63 insertions(+), 4 deletions(-)
>> 
>> diff --git a/net/ncsi/ncsi-cmd.c b/net/ncsi/ncsi-cmd.c
>> index dda8b76b7798..7be177f55173 100644
>> --- a/net/ncsi/ncsi-cmd.c
>> +++ b/net/ncsi/ncsi-cmd.c
>> @@ -269,7 +269,8 @@ static struct ncsi_cmd_handler {
>>    { NCSI_PKT_CMD_GPS,    0, ncsi_cmd_handler_default },
>>    { NCSI_PKT_CMD_OEM,   -1, ncsi_cmd_handler_oem     },
>>    { NCSI_PKT_CMD_PLDM,   0, NULL                     },
>> -    { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default }
>> +    { NCSI_PKT_CMD_GPUUID, 0, ncsi_cmd_handler_default },
>> +    { NCSI_PKT_CMD_GMCMA,  0, ncsi_cmd_handler_default }
>> };
>> 
>> static struct ncsi_request *ncsi_alloc_command(struct ncsi_cmd_arg *nca)
>> diff --git a/net/ncsi/ncsi-manage.c b/net/ncsi/ncsi-manage.c
>> index f56795769893..bc1887a2543d 100644
>> --- a/net/ncsi/ncsi-manage.c
>> +++ b/net/ncsi/ncsi-manage.c
>> @@ -1038,11 +1038,16 @@ static void ncsi_configure_channel(struct ncsi_dev_priv *ndp)
>>    case ncsi_dev_state_config_oem_gma:
>>        nd->state = ncsi_dev_state_config_clear_vids;
>> 
>> -        nca.type = NCSI_PKT_CMD_OEM;
>>        nca.package = np->id;
>>        nca.channel = nc->id;
>>        ndp->pending_req_num = 1;
>> -        ret = ncsi_gma_handler(&nca, nc->version.mf_id);
>> +        if (nc->version.major >= 1 && nc->version.minor >= 2) {
>> +            nca.type = NCSI_PKT_CMD_GMCMA;
>> +            ret = ncsi_xmit_cmd(&nca);
>> +        } else {
>> +            nca.type = NCSI_PKT_CMD_OEM;
>> +            ret = ncsi_gma_handler(&nca, nc->version.mf_id);
>> +        }
>>        if (ret < 0)
>>            schedule_work(&ndp->work);
>> 
>> diff --git a/net/ncsi/ncsi-pkt.h b/net/ncsi/ncsi-pkt.h
>> index c9d1da34dc4d..f2f3b5c1b941 100644
>> --- a/net/ncsi/ncsi-pkt.h
>> +++ b/net/ncsi/ncsi-pkt.h
>> @@ -338,6 +338,14 @@ struct ncsi_rsp_gpuuid_pkt {
>>    __be32                  checksum;
>> };
>> 
>> +/* Get MC MAC Address */
>> +struct ncsi_rsp_gmcma_pkt {
>> +    struct ncsi_rsp_pkt_hdr rsp;
>> +    unsigned char           address_count;
>> +    unsigned char           reserved[3];
>> +    unsigned char           addresses[][ETH_ALEN];
>> +};
>> +
>> /* AEN: Link State Change */
>> struct ncsi_aen_lsc_pkt {
>>    struct ncsi_aen_pkt_hdr aen;        /* AEN header      */
>> @@ -398,6 +406,7 @@ struct ncsi_aen_hncdsc_pkt {
>> #define NCSI_PKT_CMD_GPUUID    0x52 /* Get package UUID                 */
>> #define NCSI_PKT_CMD_QPNPR    0x56 /* Query Pending NC PLDM request */
>> #define NCSI_PKT_CMD_SNPR    0x57 /* Send NC PLDM Reply  */
>> +#define NCSI_PKT_CMD_GMCMA    0x58 /* Get MC MAC Address */
>> 
>> 
>> /* NCSI packet responses */
>> @@ -433,6 +442,7 @@ struct ncsi_aen_hncdsc_pkt {
>> #define NCSI_PKT_RSP_GPUUID    (NCSI_PKT_CMD_GPUUID + 0x80)
>> #define NCSI_PKT_RSP_QPNPR    (NCSI_PKT_CMD_QPNPR   + 0x80)
>> #define NCSI_PKT_RSP_SNPR    (NCSI_PKT_CMD_SNPR   + 0x80)
>> +#define NCSI_PKT_RSP_GMCMA    (NCSI_PKT_CMD_GMCMA  + 0x80)
>> 
>> /* NCSI response code/reason */
>> #define NCSI_PKT_RSP_C_COMPLETED    0x0000 /* Command Completed        */
>> diff --git a/net/ncsi/ncsi-rsp.c b/net/ncsi/ncsi-rsp.c
>> index 7a805b86a12d..28a042688d0b 100644
>> --- a/net/ncsi/ncsi-rsp.c
>> +++ b/net/ncsi/ncsi-rsp.c
>> @@ -1140,6 +1140,48 @@ static int ncsi_rsp_handler_netlink(struct ncsi_request *nr)
>>    return ret;
>> }
>> 
>> +static int ncsi_rsp_handler_gmcma(struct ncsi_request *nr)
>> +{
>> +    struct ncsi_dev_priv *ndp = nr->ndp;
>> +    struct net_device *ndev = ndp->ndev.dev;
>> +    struct ncsi_rsp_gmcma_pkt *rsp;
>> +    struct sockaddr saddr;
>> +    int ret = -1;
>> +    int i;
>> +    int j;
>> +
>> +    rsp = (struct ncsi_rsp_gmcma_pkt *)skb_network_header(nr->rsp);
>> +    saddr.sa_family = ndev->type;
>> +    ndev->priv_flags |= IFF_LIVE_ADDR_CHANGE;
>> +
>> +    netdev_warn(ndev, "NCSI: Received %d provisioned MAC addresses\n",
>> +            rsp->address_count);
>> +    for (i = 0; i < rsp->address_count; i++) {
>> +        netdev_warn(ndev, "NCSI: MAC address %d: "
>> +                "%02x:%02x:%02x:%02x:%02x:%02x\n", i,
>> +                rsp->addresses[i][5], rsp->addresses[i][4],
>> +                rsp->addresses[i][3], rsp->addresses[i][2],
>> +                rsp->addresses[i][1], rsp->addresses[i][0]);
>> +    }
> 
> You must avoid this kind of debug messages on 'warn' level (more
> below). You could consider pr_debug() instead or completely drop the
> message.

Oh ok, I’ll change it to a debug message when I resubmit it.

Thanks for your comments!

> 
> Cheers,
> 
> Paolo
>