Re: [PATCH v5 net-next 8/8] sfc: add support for devlink port_function_hw_addr_set in ef100

On 2/2/23 12:13, Jiri Pirko wrote:

Thu, Feb 02, 2023 at 12:14:23PM CET, alejandro.lucero-palau@amd.com wrote:

quoted

From: Alejandro Lucero <redacted>

Using the builtin client handle id infrastructure, this patch adds
support for setting the mac address linked to mports in ef100. This
implies to execute an MCDI command for giving the address to the
firmware for the specific devlink port.

Signed-off-by: Alejandro Lucero <redacted>

Please check my notes to the previuous patch, most of them applies on
this one as well. Couple more below.

I'll do.

quoted

---
drivers/net/ethernet/sfc/efx_devlink.c | 50 ++++++++++++++++++++++++++
1 file changed, 50 insertions(+)

diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c
index c44547b9894e..bcb8543b43ba 100644
--- a/drivers/net/ethernet/sfc/efx_devlink.c
+++ b/drivers/net/ethernet/sfc/efx_devlink.c

@@ -110,6 +110,55 @@ static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr,

	return rc;
}

+static int efx_devlink_port_addr_set(struct devlink_port *port,
+				     const u8 *hw_addr, int hw_addr_len,
+				     struct netlink_ext_ack *extack)
+{
+	MCDI_DECLARE_BUF(inbuf, MC_CMD_SET_CLIENT_MAC_ADDRESSES_IN_LEN(1));
+	struct efx_devlink *devlink = devlink_priv(port->devlink);
+	struct mae_mport_desc *mport_desc;
+	efx_qword_t pciefn;
+	u32 client_id;
+	int rc;
+
+	mport_desc = container_of(port, struct mae_mport_desc, dl_port);
+
+	if (!ef100_mport_is_vf(mport_desc)) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "port mac change not allowed (mport: %u)",

"Port" with "P"? Be consistent with extack messages.
Also "MAC", as you used that in the previous patch.

OK.

quoted

+				   mport_desc->mport_id);
+		return -EPERM;
+	}
+
+	EFX_POPULATE_QWORD_3(pciefn,
+			     PCIE_FUNCTION_PF, PCIE_FUNCTION_PF_NULL,
+			     PCIE_FUNCTION_VF, mport_desc->vf_idx,
+			     PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER);
+
+	rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id);
+	if (rc) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "No internal client_ID for port (mport: %u)",
+				   mport_desc->mport_id);
+		return rc;
+	}
+
+	MCDI_SET_DWORD(inbuf, SET_CLIENT_MAC_ADDRESSES_IN_CLIENT_HANDLE,
+		       client_id);
+
+	ether_addr_copy(MCDI_PTR(inbuf, SET_CLIENT_MAC_ADDRESSES_IN_MAC_ADDRS),
+			hw_addr);
+
+	rc = efx_mcdi_rpc(devlink->efx, MC_CMD_SET_CLIENT_MAC_ADDRESSES, inbuf,
+			  sizeof(inbuf), NULL, 0, NULL);
+	if (rc)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "sfc MC_CMD_SET_CLIENT_MAC_ADDRESSES mcdi error (mport: %u)",

I have no clue why to put name of the driver in the extack. Don't do it.
Also, what does "MC_CMD_SET_CLIENT_MAC_ADDRESSES" tell to the user?

I was suggested to add that kind of information by my team, and I 
thought it was a good idea.

I guess it is sometimes not easy to realize this is for user space ...

quoted

+				   mport_desc->mport_id);
+
+	return rc;
+}
+
#endif

static int efx_devlink_info_nvram_partition(struct efx_nic *efx,

@@ -574,6 +623,7 @@ static const struct devlink_ops sfc_devlink_ops = {

	.info_get			= efx_devlink_info_get,
#ifdef CONFIG_SFC_SRIOV
	.port_function_hw_addr_get	= efx_devlink_port_addr_get,
+	.port_function_hw_addr_set	= efx_devlink_port_addr_set,
#endif
};

-- 
2.17.1