| Message ID | 20230202111423.56831-9-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: devlink support for ef100 | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 62 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
Thu, Feb 02, 2023 at 12:14:23PM CET, alejandro.lucero-palau@amd.com wrote: >From: Alejandro Lucero <alejandro.lucero-palau@amd.com> > >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 <alejandro.lucero-palau@amd.com> Please check my notes to the previuous patch, most of them applies on this one as well. Couple more below. >--- > 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. >+ 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? >+ 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 >
On 2/2/23 12:13, Jiri Pirko wrote: > Thu, Feb 02, 2023 at 12:14:23PM CET, alejandro.lucero-palau@amd.com wrote: >> From: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> >> 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 <alejandro.lucero-palau@amd.com> > Please check my notes to the previuous patch, most of them applies on > this one as well. Couple more below. I'll do. > > >> --- >> 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. > >> + 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 ... >> + 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 >>
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)", + 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)", + 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 };