| Message ID | 20230119113140.20208-7-alejandro.lucero-palau@amd.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | sfc: devlink support for ef100 | expand |
Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >implies to execute an MCDI command for getting the data from the >firmware for each devlink port. > >Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >--- > drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ > drivers/net/ethernet/sfc/ef100_nic.h | 1 + > drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ > drivers/net/ethernet/sfc/ef100_rep.h | 1 + > drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ > 5 files changed, 81 insertions(+) > >diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >index f4e913593f2b..4400ce622228 100644 >--- a/drivers/net/ethernet/sfc/ef100_nic.c >+++ b/drivers/net/ethernet/sfc/ef100_nic.c >@@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) > return rc; > } > >+/* MCDI commands are related to the same device issuing them. This function >+ * allows to do an MCDI command on behalf of another device, mainly PFs setting >+ * things for VFs. >+ */ >+int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >+{ >+ MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >+ MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >+ u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >+ size_t outlen; >+ int rc; >+ >+ MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >+ MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >+ MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >+ pciefn_flat); >+ >+ rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >+ outbuf, sizeof(outbuf), &outlen); >+ if (rc) >+ return rc; >+ if (outlen < sizeof(outbuf)) >+ return -EIO; >+ *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >+ return 0; >+} >+ > int ef100_probe_netdev_pf(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; >diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >index e59044072333..f1ed481c1260 100644 >--- a/drivers/net/ethernet/sfc/ef100_nic.h >+++ b/drivers/net/ethernet/sfc/ef100_nic.h >@@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); > > int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, > int client_handle, bool empty_ok); >+int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); > #endif /* EFX_EF100_NIC_H */ >diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >index ff0c8da61919..974c9ff901a0 100644 >--- a/drivers/net/ethernet/sfc/ef100_rep.c >+++ b/drivers/net/ethernet/sfc/ef100_rep.c >@@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, > mport_desc->interface_idx == nic_data->local_mae_intf; > } > >+bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >+{ >+ bool pcie_func; >+ >+ pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >+ return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >+} >+ > void efx_ef100_init_reps(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; >diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >index 9cca41614982..74853ccbc937 100644 >--- a/drivers/net/ethernet/sfc/ef100_rep.h >+++ b/drivers/net/ethernet/sfc/ef100_rep.h >@@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); > struct mae_mport_desc; > bool ef100_mport_on_local_intf(struct efx_nic *efx, > struct mae_mport_desc *mport_desc); >+bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); > #endif /* EF100_REP_H */ >diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >index bb19d3ad7ffd..2a57c4f6d2b2 100644 >--- a/drivers/net/ethernet/sfc/efx_devlink.c >+++ b/drivers/net/ethernet/sfc/efx_devlink.c >@@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, > return err; > } > >+static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >+ int *hw_addr_len, >+ struct netlink_ext_ack *extack) >+{ >+ struct efx_devlink *devlink = devlink_priv(port->devlink); >+ struct mae_mport_desc *mport_desc; >+ efx_qword_t pciefn; >+ u32 client_id; >+ int rc = 0; >+ >+ mport_desc = efx_mae_get_mport(devlink->efx, port->index); Dont use port->index, never. It's devlink internal. You have port pointer passed here. Usually, what drivers do is to embed the struct devlink_port in the driver port struct. Then you do just simple container of to get it here. Mlxsw example: static void *__dl_port(struct devlink_port *devlink_port) { return container_of(devlink_port, struct mlxsw_core_port, devlink_port); } static int mlxsw_devlink_port_split(struct devlink *devlink, struct devlink_port *port, unsigned int count, struct netlink_ext_ack *extack) { struct mlxsw_core_port *mlxsw_core_port = __dl_port(port); ... >+ if (!mport_desc) Tell the user what's wrong, extack is here for that. >+ return -EINVAL; >+ >+ if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >+ goto out; >+ >+ if (ef100_mport_is_vf(mport_desc)) >+ 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); >+ else >+ EFX_POPULATE_QWORD_3(pciefn, >+ PCIE_FUNCTION_PF, mport_desc->pf_idx, >+ PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >+ PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >+ >+ rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >+ if (rc) { >+ netif_err(devlink->efx, drv, devlink->efx->net_dev, >+ "Failed to get client ID for port index %u, rc %d\n", >+ port->index, rc); Don't write to dmesg, use extack msg instead. >+ return rc; >+ } >+ >+ rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); Again, extack would be nice here if (rc) >+out: >+ *hw_addr_len = ETH_ALEN; >+ >+ return rc; >+} >+ > static int efx_devlink_info_get(struct devlink *devlink, > struct devlink_info_req *req, > struct netlink_ext_ack *extack) >@@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, > > static const struct devlink_ops sfc_devlink_ops = { > .info_get = efx_devlink_info_get, >+ .port_function_hw_addr_get = efx_devlink_port_addr_get, > }; > > static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >-- >2.17.1 >
Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >implies to execute an MCDI command for getting the data from the >firmware for each devlink port. > >Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >--- > drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ > drivers/net/ethernet/sfc/ef100_nic.h | 1 + > drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ > drivers/net/ethernet/sfc/ef100_rep.h | 1 + > drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ > 5 files changed, 81 insertions(+) > >diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >index f4e913593f2b..4400ce622228 100644 >--- a/drivers/net/ethernet/sfc/ef100_nic.c >+++ b/drivers/net/ethernet/sfc/ef100_nic.c >@@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) > return rc; > } > >+/* MCDI commands are related to the same device issuing them. This function >+ * allows to do an MCDI command on behalf of another device, mainly PFs setting >+ * things for VFs. >+ */ >+int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >+{ >+ MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >+ MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >+ u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >+ size_t outlen; >+ int rc; >+ >+ MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >+ MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >+ MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >+ pciefn_flat); >+ >+ rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >+ outbuf, sizeof(outbuf), &outlen); >+ if (rc) >+ return rc; >+ if (outlen < sizeof(outbuf)) >+ return -EIO; >+ *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >+ return 0; >+} >+ > int ef100_probe_netdev_pf(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; >diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >index e59044072333..f1ed481c1260 100644 >--- a/drivers/net/ethernet/sfc/ef100_nic.h >+++ b/drivers/net/ethernet/sfc/ef100_nic.h >@@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); > > int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, > int client_handle, bool empty_ok); >+int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); > #endif /* EFX_EF100_NIC_H */ >diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >index ff0c8da61919..974c9ff901a0 100644 >--- a/drivers/net/ethernet/sfc/ef100_rep.c >+++ b/drivers/net/ethernet/sfc/ef100_rep.c >@@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, > mport_desc->interface_idx == nic_data->local_mae_intf; > } > >+bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >+{ >+ bool pcie_func; >+ >+ pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >+ return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >+} >+ > void efx_ef100_init_reps(struct efx_nic *efx) > { > struct ef100_nic_data *nic_data = efx->nic_data; >diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >index 9cca41614982..74853ccbc937 100644 >--- a/drivers/net/ethernet/sfc/ef100_rep.h >+++ b/drivers/net/ethernet/sfc/ef100_rep.h >@@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); > struct mae_mport_desc; > bool ef100_mport_on_local_intf(struct efx_nic *efx, > struct mae_mport_desc *mport_desc); >+bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); > #endif /* EF100_REP_H */ >diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >index bb19d3ad7ffd..2a57c4f6d2b2 100644 >--- a/drivers/net/ethernet/sfc/efx_devlink.c >+++ b/drivers/net/ethernet/sfc/efx_devlink.c >@@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, > return err; > } > >+static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >+ int *hw_addr_len, >+ struct netlink_ext_ack *extack) >+{ >+ struct efx_devlink *devlink = devlink_priv(port->devlink); >+ struct mae_mport_desc *mport_desc; >+ efx_qword_t pciefn; >+ u32 client_id; >+ int rc = 0; >+ >+ mport_desc = efx_mae_get_mport(devlink->efx, port->index); I may be missing something, where do you fail with -EOPNOTSUPP in case this is called for PHYSICAL port ? >+ if (!mport_desc) >+ return -EINVAL; >+ >+ if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >+ goto out; >+ >+ if (ef100_mport_is_vf(mport_desc)) >+ 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); >+ else >+ EFX_POPULATE_QWORD_3(pciefn, >+ PCIE_FUNCTION_PF, mport_desc->pf_idx, >+ PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >+ PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >+ >+ rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >+ if (rc) { >+ netif_err(devlink->efx, drv, devlink->efx->net_dev, >+ "Failed to get client ID for port index %u, rc %d\n", >+ port->index, rc); >+ return rc; >+ } >+ >+ rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >+out: >+ *hw_addr_len = ETH_ALEN; >+ >+ return rc; >+} >+ > static int efx_devlink_info_get(struct devlink *devlink, > struct devlink_info_req *req, > struct netlink_ext_ack *extack) >@@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, > > static const struct devlink_ops sfc_devlink_ops = { > .info_get = efx_devlink_info_get, >+ .port_function_hw_addr_get = efx_devlink_port_addr_get, > }; > > static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >-- >2.17.1 >
On 1/19/23 12:25, Jiri Pirko wrote: > Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >> implies to execute an MCDI command for getting the data from the >> firmware for each devlink port. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> --- >> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >> 5 files changed, 81 insertions(+) >> >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >> index f4e913593f2b..4400ce622228 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.c >> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >> return rc; >> } >> >> +/* MCDI commands are related to the same device issuing them. This function >> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >> + * things for VFs. >> + */ >> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >> +{ >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >> + size_t outlen; >> + int rc; >> + >> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >> + pciefn_flat); >> + >> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >> + outbuf, sizeof(outbuf), &outlen); >> + if (rc) >> + return rc; >> + if (outlen < sizeof(outbuf)) >> + return -EIO; >> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >> + return 0; >> +} >> + >> int ef100_probe_netdev_pf(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >> index e59044072333..f1ed481c1260 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.h >> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >> >> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >> int client_handle, bool empty_ok); >> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >> #endif /* EFX_EF100_NIC_H */ >> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >> index ff0c8da61919..974c9ff901a0 100644 >> --- a/drivers/net/ethernet/sfc/ef100_rep.c >> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >> mport_desc->interface_idx == nic_data->local_mae_intf; >> } >> >> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >> +{ >> + bool pcie_func; >> + >> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >> +} >> + >> void efx_ef100_init_reps(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >> index 9cca41614982..74853ccbc937 100644 >> --- a/drivers/net/ethernet/sfc/ef100_rep.h >> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >> struct mae_mport_desc; >> bool ef100_mport_on_local_intf(struct efx_nic *efx, >> struct mae_mport_desc *mport_desc); >> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >> #endif /* EF100_REP_H */ >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >> --- a/drivers/net/ethernet/sfc/efx_devlink.c >> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >> return err; >> } >> >> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >> + int *hw_addr_len, >> + struct netlink_ext_ack *extack) >> +{ >> + struct efx_devlink *devlink = devlink_priv(port->devlink); >> + struct mae_mport_desc *mport_desc; >> + efx_qword_t pciefn; >> + u32 client_id; >> + int rc = 0; >> + >> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); > Dont use port->index, never. It's devlink internal. You have port > pointer passed here. Usually, what drivers do is to embed > the struct devlink_port in the driver port struct. Then you do just > simple container of to get it here. Mlxsw example: I do not understand this. Port index is set by the driver not by the devlink interface implementation. Why can I not use it? > static void *__dl_port(struct devlink_port *devlink_port) > { > return container_of(devlink_port, struct mlxsw_core_port, devlink_port); > } > > static int mlxsw_devlink_port_split(struct devlink *devlink, > struct devlink_port *port, > unsigned int count, > struct netlink_ext_ack *extack) > { > struct mlxsw_core_port *mlxsw_core_port = __dl_port(port); > ... > > > >> + if (!mport_desc) > Tell the user what's wrong, extack is here for that. > I'll do. > >> + return -EINVAL; >> + >> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >> + goto out; >> + >> + if (ef100_mport_is_vf(mport_desc)) >> + 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); >> + else >> + EFX_POPULATE_QWORD_3(pciefn, >> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >> + >> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >> + if (rc) { >> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >> + "Failed to get client ID for port index %u, rc %d\n", >> + port->index, rc); > Don't write to dmesg, use extack msg instead. I'll do. Thanks > >> + return rc; >> + } >> + >> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); > Again, extack would be nice here if (rc) > Again, I'll do. >> +out: >> + *hw_addr_len = ETH_ALEN; >> + >> + return rc; >> +} >> + >> static int efx_devlink_info_get(struct devlink *devlink, >> struct devlink_info_req *req, >> struct netlink_ext_ack *extack) >> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >> >> static const struct devlink_ops sfc_devlink_ops = { >> .info_get = efx_devlink_info_get, >> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >> }; >> >> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >> -- >> 2.17.1 >>
On 1/19/23 12:37, Jiri Pirko wrote: > Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >> implies to execute an MCDI command for getting the data from the >> firmware for each devlink port. >> >> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >> --- >> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >> 5 files changed, 81 insertions(+) >> >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >> index f4e913593f2b..4400ce622228 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.c >> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >> return rc; >> } >> >> +/* MCDI commands are related to the same device issuing them. This function >> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >> + * things for VFs. >> + */ >> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >> +{ >> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >> + size_t outlen; >> + int rc; >> + >> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >> + pciefn_flat); >> + >> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >> + outbuf, sizeof(outbuf), &outlen); >> + if (rc) >> + return rc; >> + if (outlen < sizeof(outbuf)) >> + return -EIO; >> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >> + return 0; >> +} >> + >> int ef100_probe_netdev_pf(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >> index e59044072333..f1ed481c1260 100644 >> --- a/drivers/net/ethernet/sfc/ef100_nic.h >> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >> >> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >> int client_handle, bool empty_ok); >> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >> #endif /* EFX_EF100_NIC_H */ >> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >> index ff0c8da61919..974c9ff901a0 100644 >> --- a/drivers/net/ethernet/sfc/ef100_rep.c >> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >> mport_desc->interface_idx == nic_data->local_mae_intf; >> } >> >> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >> +{ >> + bool pcie_func; >> + >> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >> +} >> + >> void efx_ef100_init_reps(struct efx_nic *efx) >> { >> struct ef100_nic_data *nic_data = efx->nic_data; >> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >> index 9cca41614982..74853ccbc937 100644 >> --- a/drivers/net/ethernet/sfc/ef100_rep.h >> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >> struct mae_mport_desc; >> bool ef100_mport_on_local_intf(struct efx_nic *efx, >> struct mae_mport_desc *mport_desc); >> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >> #endif /* EF100_REP_H */ >> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >> --- a/drivers/net/ethernet/sfc/efx_devlink.c >> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >> return err; >> } >> >> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >> + int *hw_addr_len, >> + struct netlink_ext_ack *extack) >> +{ >> + struct efx_devlink *devlink = devlink_priv(port->devlink); >> + struct mae_mport_desc *mport_desc; >> + efx_qword_t pciefn; >> + u32 client_id; >> + int rc = 0; >> + >> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); > I may be missing something, where do you fail with -EOPNOTSUPP > in case this is called for PHYSICAL port ? > We do not create a devlink port for the physical port. I'm aware this is not "fully compliant" with devlink design idea, just trying to use it for our current urgent necessities by now. Do you think this could be a problem? > >> + if (!mport_desc) >> + return -EINVAL; >> + >> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >> + goto out; >> + >> + if (ef100_mport_is_vf(mport_desc)) >> + 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); >> + else >> + EFX_POPULATE_QWORD_3(pciefn, >> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >> + >> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >> + if (rc) { >> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >> + "Failed to get client ID for port index %u, rc %d\n", >> + port->index, rc); >> + return rc; >> + } >> + >> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >> +out: >> + *hw_addr_len = ETH_ALEN; >> + >> + return rc; >> +} >> + >> static int efx_devlink_info_get(struct devlink *devlink, >> struct devlink_info_req *req, >> struct netlink_ext_ack *extack) >> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >> >> static const struct devlink_ops sfc_devlink_ops = { >> .info_get = efx_devlink_info_get, >> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >> }; >> >> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >> -- >> 2.17.1 >>
Thu, Jan 19, 2023 at 03:59:40PM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/19/23 12:25, Jiri Pirko wrote: >> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>> implies to execute an MCDI command for getting the data from the >>> firmware for each devlink port. >>> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> --- >>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>> 5 files changed, 81 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>> index f4e913593f2b..4400ce622228 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>> return rc; >>> } >>> >>> +/* MCDI commands are related to the same device issuing them. This function >>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>> + * things for VFs. >>> + */ >>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>> +{ >>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>> + size_t outlen; >>> + int rc; >>> + >>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>> + pciefn_flat); >>> + >>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>> + outbuf, sizeof(outbuf), &outlen); >>> + if (rc) >>> + return rc; >>> + if (outlen < sizeof(outbuf)) >>> + return -EIO; >>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>> + return 0; >>> +} >>> + >>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>> index e59044072333..f1ed481c1260 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>> >>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>> int client_handle, bool empty_ok); >>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>> #endif /* EFX_EF100_NIC_H */ >>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>> index ff0c8da61919..974c9ff901a0 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>> mport_desc->interface_idx == nic_data->local_mae_intf; >>> } >>> >>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>> +{ >>> + bool pcie_func; >>> + >>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>> +} >>> + >>> void efx_ef100_init_reps(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>> index 9cca41614982..74853ccbc937 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>> struct mae_mport_desc; >>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>> struct mae_mport_desc *mport_desc); >>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>> #endif /* EF100_REP_H */ >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>> return err; >>> } >>> >>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>> + int *hw_addr_len, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>> + struct mae_mport_desc *mport_desc; >>> + efx_qword_t pciefn; >>> + u32 client_id; >>> + int rc = 0; >>> + >>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >> Dont use port->index, never. It's devlink internal. You have port >> pointer passed here. Usually, what drivers do is to embed >> the struct devlink_port in the driver port struct. Then you do just >> simple container of to get it here. Mlxsw example: > >I do not understand this. Port index is set by the driver not by the >devlink interface implementation. > >Why can I not use it? Well you can, but things could be done much nicer, when you use devlink_port pointer. Also, better not to access devlink_port struct internals, driver should never need it. > >> static void *__dl_port(struct devlink_port *devlink_port) >> { >> return container_of(devlink_port, struct mlxsw_core_port, devlink_port); >> } >> >> static int mlxsw_devlink_port_split(struct devlink *devlink, >> struct devlink_port *port, >> unsigned int count, >> struct netlink_ext_ack *extack) >> { >> struct mlxsw_core_port *mlxsw_core_port = __dl_port(port); >> ... >> >> >> >>> + if (!mport_desc) >> Tell the user what's wrong, extack is here for that. >> >I'll do. >> >>> + return -EINVAL; >>> + >>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>> + goto out; >>> + >>> + if (ef100_mport_is_vf(mport_desc)) >>> + 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); >>> + else >>> + EFX_POPULATE_QWORD_3(pciefn, >>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>> + >>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>> + if (rc) { >>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>> + "Failed to get client ID for port index %u, rc %d\n", >>> + port->index, rc); >> Don't write to dmesg, use extack msg instead. > > >I'll do. > >Thanks > >> >>> + return rc; >>> + } >>> + >>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >> Again, extack would be nice here if (rc) >> >Again, I'll do. >>> +out: >>> + *hw_addr_len = ETH_ALEN; >>> + >>> + return rc; >>> +} >>> + >>> static int efx_devlink_info_get(struct devlink *devlink, >>> struct devlink_info_req *req, >>> struct netlink_ext_ack *extack) >>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>> >>> static const struct devlink_ops sfc_devlink_ops = { >>> .info_get = efx_devlink_info_get, >>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>> }; >>> >>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>> -- >>> 2.17.1 >>> >
Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/19/23 12:37, Jiri Pirko wrote: >> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>> implies to execute an MCDI command for getting the data from the >>> firmware for each devlink port. >>> >>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>> --- >>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>> 5 files changed, 81 insertions(+) >>> >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>> index f4e913593f2b..4400ce622228 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>> return rc; >>> } >>> >>> +/* MCDI commands are related to the same device issuing them. This function >>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>> + * things for VFs. >>> + */ >>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>> +{ >>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>> + size_t outlen; >>> + int rc; >>> + >>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>> + pciefn_flat); >>> + >>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>> + outbuf, sizeof(outbuf), &outlen); >>> + if (rc) >>> + return rc; >>> + if (outlen < sizeof(outbuf)) >>> + return -EIO; >>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>> + return 0; >>> +} >>> + >>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>> index e59044072333..f1ed481c1260 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>> >>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>> int client_handle, bool empty_ok); >>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>> #endif /* EFX_EF100_NIC_H */ >>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>> index ff0c8da61919..974c9ff901a0 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>> mport_desc->interface_idx == nic_data->local_mae_intf; >>> } >>> >>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>> +{ >>> + bool pcie_func; >>> + >>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>> +} >>> + >>> void efx_ef100_init_reps(struct efx_nic *efx) >>> { >>> struct ef100_nic_data *nic_data = efx->nic_data; >>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>> index 9cca41614982..74853ccbc937 100644 >>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>> struct mae_mport_desc; >>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>> struct mae_mport_desc *mport_desc); >>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>> #endif /* EF100_REP_H */ >>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>> return err; >>> } >>> >>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>> + int *hw_addr_len, >>> + struct netlink_ext_ack *extack) >>> +{ >>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>> + struct mae_mport_desc *mport_desc; >>> + efx_qword_t pciefn; >>> + u32 client_id; >>> + int rc = 0; >>> + >>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >> I may be missing something, where do you fail with -EOPNOTSUPP >> in case this is called for PHYSICAL port ? >> > >We do not create a devlink port for the physical port. Well, you do: + switch (mport->mport_type) { + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + attrs.phys.port_number = mport->port_idx; + devlink_port_attrs_set(dl_port, &attrs); + break; + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, + mport->vf_idx, + external); + } else { + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, + external); + } + break; > >I'm aware this is not "fully compliant" with devlink design idea, just >trying to use it for our current urgent necessities by now. > >Do you think this could be a problem? If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not uplink, it is a problem :) > > >> >>> + if (!mport_desc) >>> + return -EINVAL; >>> + >>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>> + goto out; >>> + >>> + if (ef100_mport_is_vf(mport_desc)) >>> + 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); >>> + else >>> + EFX_POPULATE_QWORD_3(pciefn, >>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>> + >>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>> + if (rc) { >>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>> + "Failed to get client ID for port index %u, rc %d\n", >>> + port->index, rc); >>> + return rc; >>> + } >>> + >>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>> +out: >>> + *hw_addr_len = ETH_ALEN; >>> + >>> + return rc; >>> +} >>> + >>> static int efx_devlink_info_get(struct devlink *devlink, >>> struct devlink_info_req *req, >>> struct netlink_ext_ack *extack) >>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>> >>> static const struct devlink_ops sfc_devlink_ops = { >>> .info_get = efx_devlink_info_get, >>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>> }; >>> >>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>> -- >>> 2.17.1 >>> >
On 1/20/23 11:08, Jiri Pirko wrote: > Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: >> On 1/19/23 12:37, Jiri Pirko wrote: >>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>> implies to execute an MCDI command for getting the data from the >>>> firmware for each devlink port. >>>> >>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>> --- >>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>> 5 files changed, 81 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>> index f4e913593f2b..4400ce622228 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>> return rc; >>>> } >>>> >>>> +/* MCDI commands are related to the same device issuing them. This function >>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>> + * things for VFs. >>>> + */ >>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>> +{ >>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>> + size_t outlen; >>>> + int rc; >>>> + >>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>> + pciefn_flat); >>>> + >>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>> + outbuf, sizeof(outbuf), &outlen); >>>> + if (rc) >>>> + return rc; >>>> + if (outlen < sizeof(outbuf)) >>>> + return -EIO; >>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>> + return 0; >>>> +} >>>> + >>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>> { >>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>> index e59044072333..f1ed481c1260 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>> >>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>> int client_handle, bool empty_ok); >>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>> #endif /* EFX_EF100_NIC_H */ >>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>> index ff0c8da61919..974c9ff901a0 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>> } >>>> >>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>> +{ >>>> + bool pcie_func; >>>> + >>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>> +} >>>> + >>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>> { >>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>> index 9cca41614982..74853ccbc937 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>> struct mae_mport_desc; >>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>> struct mae_mport_desc *mport_desc); >>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>> #endif /* EF100_REP_H */ >>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>> return err; >>>> } >>>> >>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>> + int *hw_addr_len, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>> + struct mae_mport_desc *mport_desc; >>>> + efx_qword_t pciefn; >>>> + u32 client_id; >>>> + int rc = 0; >>>> + >>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>> I may be missing something, where do you fail with -EOPNOTSUPP >>> in case this is called for PHYSICAL port ? >>> >> We do not create a devlink port for the physical port. > Well, you do: > > + switch (mport->mport_type) { > + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: > + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Right. I was relying on devlink-port output which does not show the physical port in my machine and completely forgot about it. I'm a bit confused at this point. Let me look at this further. > > + attrs.phys.port_number = mport->port_idx; > + devlink_port_attrs_set(dl_port, &attrs); > + break; > + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: > + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { > + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, > + mport->vf_idx, > + external); > + } else { > + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, > + external); > + } > + break; > > > > >> I'm aware this is not "fully compliant" with devlink design idea, just >> trying to use it for our current urgent necessities by now. >> >> Do you think this could be a problem? > If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not > uplink, it is a problem :) > > > >> >>>> + if (!mport_desc) >>>> + return -EINVAL; >>>> + >>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>> + goto out; >>>> + >>>> + if (ef100_mport_is_vf(mport_desc)) >>>> + 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); >>>> + else >>>> + EFX_POPULATE_QWORD_3(pciefn, >>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>> + >>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>> + if (rc) { >>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>> + "Failed to get client ID for port index %u, rc %d\n", >>>> + port->index, rc); >>>> + return rc; >>>> + } >>>> + >>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>>> +out: >>>> + *hw_addr_len = ETH_ALEN; >>>> + >>>> + return rc; >>>> +} >>>> + >>>> static int efx_devlink_info_get(struct devlink *devlink, >>>> struct devlink_info_req *req, >>>> struct netlink_ext_ack *extack) >>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>> >>>> static const struct devlink_ops sfc_devlink_ops = { >>>> .info_get = efx_devlink_info_get, >>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>> }; >>>> >>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>> -- >>>> 2.17.1 >>>>
On 1/20/23 12:48, Lucero Palau, Alejandro wrote: > On 1/20/23 11:08, Jiri Pirko wrote: >> Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: >>> On 1/19/23 12:37, Jiri Pirko wrote: >>>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>>> implies to execute an MCDI command for getting the data from the >>>>> firmware for each devlink port. >>>>> >>>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>>> --- >>>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>>> 5 files changed, 81 insertions(+) >>>>> >>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>>> index f4e913593f2b..4400ce622228 100644 >>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>>> return rc; >>>>> } >>>>> >>>>> +/* MCDI commands are related to the same device issuing them. This function >>>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>>> + * things for VFs. >>>>> + */ >>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>>> +{ >>>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>>> + size_t outlen; >>>>> + int rc; >>>>> + >>>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>>> + pciefn_flat); >>>>> + >>>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>>> + outbuf, sizeof(outbuf), &outlen); >>>>> + if (rc) >>>>> + return rc; >>>>> + if (outlen < sizeof(outbuf)) >>>>> + return -EIO; >>>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>>> + return 0; >>>>> +} >>>>> + >>>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>>> { >>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>>> index e59044072333..f1ed481c1260 100644 >>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>>> >>>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>>> int client_handle, bool empty_ok); >>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>>> #endif /* EFX_EF100_NIC_H */ >>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>>> index ff0c8da61919..974c9ff901a0 100644 >>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>>> } >>>>> >>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>>> +{ >>>>> + bool pcie_func; >>>>> + >>>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>>> +} >>>>> + >>>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>>> { >>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>>> index 9cca41614982..74853ccbc937 100644 >>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>>> struct mae_mport_desc; >>>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>> struct mae_mport_desc *mport_desc); >>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>>> #endif /* EF100_REP_H */ >>>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>>> return err; >>>>> } >>>>> >>>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>>> + int *hw_addr_len, >>>>> + struct netlink_ext_ack *extack) >>>>> +{ >>>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>>> + struct mae_mport_desc *mport_desc; >>>>> + efx_qword_t pciefn; >>>>> + u32 client_id; >>>>> + int rc = 0; >>>>> + >>>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>>> I may be missing something, where do you fail with -EOPNOTSUPP >>>> in case this is called for PHYSICAL port ? >>>> >>> We do not create a devlink port for the physical port. >> Well, you do: >> >> + switch (mport->mport_type) { >> + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: >> + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; >> >> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Right. > > I was relying on devlink-port output which does not show the physical > port in my machine and completely forgot about it. > > I'm a bit confused at this point. Let me look at this further. > The reason devlink show/info does not report the PHYSICAL port is simple: current code does not create a devlink port for such mport. I say current code because in my first implementation (not upstreamed) I just created devlink ports when the mports where enumerated, what turned out to be a bit complicated for dealing with VFs creation/destruction, and after looking at how other drivers do this, just before the related netdev is registered, I went for that same approach. That leaves no option for the physical mport registered. I could add that creation at PF devlink port creation, if we consider this a necessity. I know the ideal devlink support in our driver should likely be more devlink design compliant, but it is also true drivers should make use of it as decided for fulfilling their necessities as long as it does not create confusion to users. Our current devlink necessity is for dealing with setting VFs mac address as required during previous representors patchset review: https://lore.kernel.org/netdev/20220728092008.2117846e@kernel.org/ Do you see a problem not creating such a devlink port by now? >> + attrs.phys.port_number = mport->port_idx; >> + devlink_port_attrs_set(dl_port, &attrs); >> + break; >> + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: >> + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { >> + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, >> + mport->vf_idx, >> + external); >> + } else { >> + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, >> + external); >> + } >> + break; >> >> >> >> >>> I'm aware this is not "fully compliant" with devlink design idea, just >>> trying to use it for our current urgent necessities by now. >>> >>> Do you think this could be a problem? >> If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not >> uplink, it is a problem :) >> >> >> >>>>> + if (!mport_desc) >>>>> + return -EINVAL; >>>>> + >>>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>>> + goto out; >>>>> + >>>>> + if (ef100_mport_is_vf(mport_desc)) >>>>> + 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); >>>>> + else >>>>> + EFX_POPULATE_QWORD_3(pciefn, >>>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>>> + >>>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>>> + if (rc) { >>>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>>> + "Failed to get client ID for port index %u, rc %d\n", >>>>> + port->index, rc); >>>>> + return rc; >>>>> + } >>>>> + >>>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>>>> +out: >>>>> + *hw_addr_len = ETH_ALEN; >>>>> + >>>>> + return rc; >>>>> +} >>>>> + >>>>> static int efx_devlink_info_get(struct devlink *devlink, >>>>> struct devlink_info_req *req, >>>>> struct netlink_ext_ack *extack) >>>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>>> >>>>> static const struct devlink_ops sfc_devlink_ops = { >>>>> .info_get = efx_devlink_info_get, >>>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>>> }; >>>>> >>>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>>> -- >>>>> 2.17.1 >>>>>
On 1/20/23 11:05, Jiri Pirko wrote: > Thu, Jan 19, 2023 at 03:59:40PM CET, alejandro.lucero-palau@amd.com wrote: >> On 1/19/23 12:25, Jiri Pirko wrote: >>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>> implies to execute an MCDI command for getting the data from the >>>> firmware for each devlink port. >>>> >>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>> --- >>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>> 5 files changed, 81 insertions(+) >>>> >>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>> index f4e913593f2b..4400ce622228 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>> return rc; >>>> } >>>> >>>> +/* MCDI commands are related to the same device issuing them. This function >>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>> + * things for VFs. >>>> + */ >>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>> +{ >>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>> + size_t outlen; >>>> + int rc; >>>> + >>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>> + pciefn_flat); >>>> + >>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>> + outbuf, sizeof(outbuf), &outlen); >>>> + if (rc) >>>> + return rc; >>>> + if (outlen < sizeof(outbuf)) >>>> + return -EIO; >>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>> + return 0; >>>> +} >>>> + >>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>> { >>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>> index e59044072333..f1ed481c1260 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>> >>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>> int client_handle, bool empty_ok); >>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>> #endif /* EFX_EF100_NIC_H */ >>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>> index ff0c8da61919..974c9ff901a0 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>> } >>>> >>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>> +{ >>>> + bool pcie_func; >>>> + >>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>> +} >>>> + >>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>> { >>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>> index 9cca41614982..74853ccbc937 100644 >>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>> struct mae_mport_desc; >>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>> struct mae_mport_desc *mport_desc); >>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>> #endif /* EF100_REP_H */ >>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>> return err; >>>> } >>>> >>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>> + int *hw_addr_len, >>>> + struct netlink_ext_ack *extack) >>>> +{ >>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>> + struct mae_mport_desc *mport_desc; >>>> + efx_qword_t pciefn; >>>> + u32 client_id; >>>> + int rc = 0; >>>> + >>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>> Dont use port->index, never. It's devlink internal. You have port >>> pointer passed here. Usually, what drivers do is to embed >>> the struct devlink_port in the driver port struct. Then you do just >>> simple container of to get it here. Mlxsw example: >> I do not understand this. Port index is set by the driver not by the >> devlink interface implementation. >> >> Why can I not use it? > Well you can, but things could be done much nicer, when you use > devlink_port pointer. Also, better not to access devlink_port struct > internals, driver should never need it. > I think adding a new field in mport struct for this is not hard, so I'll follow your advice and change it in the next patchset version. Thanks. >>> static void *__dl_port(struct devlink_port *devlink_port) >>> { >>> return container_of(devlink_port, struct mlxsw_core_port, devlink_port); >>> } >>> >>> static int mlxsw_devlink_port_split(struct devlink *devlink, >>> struct devlink_port *port, >>> unsigned int count, >>> struct netlink_ext_ack *extack) >>> { >>> struct mlxsw_core_port *mlxsw_core_port = __dl_port(port); >>> ... >>> >>> >>> >>>> + if (!mport_desc) >>> Tell the user what's wrong, extack is here for that. >>> >> I'll do. >>>> + return -EINVAL; >>>> + >>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>> + goto out; >>>> + >>>> + if (ef100_mport_is_vf(mport_desc)) >>>> + 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); >>>> + else >>>> + EFX_POPULATE_QWORD_3(pciefn, >>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>> + >>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>> + if (rc) { >>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>> + "Failed to get client ID for port index %u, rc %d\n", >>>> + port->index, rc); >>> Don't write to dmesg, use extack msg instead. >> >> I'll do. >> >> Thanks >> >>>> + return rc; >>>> + } >>>> + >>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>> Again, extack would be nice here if (rc) >>> >> Again, I'll do. >>>> +out: >>>> + *hw_addr_len = ETH_ALEN; >>>> + >>>> + return rc; >>>> +} >>>> + >>>> static int efx_devlink_info_get(struct devlink *devlink, >>>> struct devlink_info_req *req, >>>> struct netlink_ext_ack *extack) >>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>> >>>> static const struct devlink_ops sfc_devlink_ops = { >>>> .info_get = efx_devlink_info_get, >>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>> }; >>>> >>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>> -- >>>> 2.17.1 >>>>
Sun, Jan 22, 2023 at 05:36:14PM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/20/23 12:48, Lucero Palau, Alejandro wrote: >> On 1/20/23 11:08, Jiri Pirko wrote: >>> Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: >>>> On 1/19/23 12:37, Jiri Pirko wrote: >>>>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>>>> implies to execute an MCDI command for getting the data from the >>>>>> firmware for each devlink port. >>>>>> >>>>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>>>> --- >>>>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>>>> 5 files changed, 81 insertions(+) >>>>>> >>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>> index f4e913593f2b..4400ce622228 100644 >>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>>>> return rc; >>>>>> } >>>>>> >>>>>> +/* MCDI commands are related to the same device issuing them. This function >>>>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>>>> + * things for VFs. >>>>>> + */ >>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>>>> +{ >>>>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>>>> + size_t outlen; >>>>>> + int rc; >>>>>> + >>>>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>>>> + pciefn_flat); >>>>>> + >>>>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>>>> + outbuf, sizeof(outbuf), &outlen); >>>>>> + if (rc) >>>>>> + return rc; >>>>>> + if (outlen < sizeof(outbuf)) >>>>>> + return -EIO; >>>>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>>>> { >>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>> index e59044072333..f1ed481c1260 100644 >>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>>>> >>>>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>>>> int client_handle, bool empty_ok); >>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>>>> #endif /* EFX_EF100_NIC_H */ >>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>> index ff0c8da61919..974c9ff901a0 100644 >>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>>>> } >>>>>> >>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>>>> +{ >>>>>> + bool pcie_func; >>>>>> + >>>>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>>>> +} >>>>>> + >>>>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>>>> { >>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>> index 9cca41614982..74853ccbc937 100644 >>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>>>> struct mae_mport_desc; >>>>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>> struct mae_mport_desc *mport_desc); >>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>>>> #endif /* EF100_REP_H */ >>>>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>>>> return err; >>>>>> } >>>>>> >>>>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>>>> + int *hw_addr_len, >>>>>> + struct netlink_ext_ack *extack) >>>>>> +{ >>>>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>>>> + struct mae_mport_desc *mport_desc; >>>>>> + efx_qword_t pciefn; >>>>>> + u32 client_id; >>>>>> + int rc = 0; >>>>>> + >>>>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>>>> I may be missing something, where do you fail with -EOPNOTSUPP >>>>> in case this is called for PHYSICAL port ? >>>>> >>>> We do not create a devlink port for the physical port. >>> Well, you do: >>> >>> + switch (mport->mport_type) { >>> + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: >>> + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; >>> >>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >> Right. >> >> I was relying on devlink-port output which does not show the physical >> port in my machine and completely forgot about it. >> >> I'm a bit confused at this point. Let me look at this further. >> >The reason devlink show/info does not report the PHYSICAL port is >simple: current code does not create a devlink port for such mport. > >I say current code because in my first implementation (not upstreamed) I >just created devlink ports when the mports where enumerated, what turned >out to be a bit complicated for dealing with VFs creation/destruction, >and after looking at how other drivers do this, just before the related >netdev is registered, I went for that same approach. That leaves no >option for the physical mport registered. > >I could add that creation at PF devlink port creation, if we consider >this a necessity. I know the ideal devlink support in our driver should >likely be more devlink design compliant, but it is also true drivers >should make use of it as decided for fulfilling their necessities as >long as it does not create confusion to users. Our current devlink >necessity is for dealing with setting VFs mac address as required during >previous representors patchset review: > >https://lore.kernel.org/netdev/20220728092008.2117846e@kernel.org/ > >Do you see a problem not creating such a devlink port by now? You don't have to create it. But loose the code setting PHYSICAL flavour. > >>> + attrs.phys.port_number = mport->port_idx; >>> + devlink_port_attrs_set(dl_port, &attrs); >>> + break; >>> + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: >>> + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { >>> + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, >>> + mport->vf_idx, >>> + external); >>> + } else { >>> + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, >>> + external); >>> + } >>> + break; >>> >>> >>> >>> >>>> I'm aware this is not "fully compliant" with devlink design idea, just >>>> trying to use it for our current urgent necessities by now. >>>> >>>> Do you think this could be a problem? >>> If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not >>> uplink, it is a problem :) >>> >>> >>> >>>>>> + if (!mport_desc) >>>>>> + return -EINVAL; >>>>>> + >>>>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>>>> + goto out; >>>>>> + >>>>>> + if (ef100_mport_is_vf(mport_desc)) >>>>>> + 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); >>>>>> + else >>>>>> + EFX_POPULATE_QWORD_3(pciefn, >>>>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>>>> + >>>>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>>>> + if (rc) { >>>>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>>>> + "Failed to get client ID for port index %u, rc %d\n", >>>>>> + port->index, rc); >>>>>> + return rc; >>>>>> + } >>>>>> + >>>>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>>>>> +out: >>>>>> + *hw_addr_len = ETH_ALEN; >>>>>> + >>>>>> + return rc; >>>>>> +} >>>>>> + >>>>>> static int efx_devlink_info_get(struct devlink *devlink, >>>>>> struct devlink_info_req *req, >>>>>> struct netlink_ext_ack *extack) >>>>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>>>> >>>>>> static const struct devlink_ops sfc_devlink_ops = { >>>>>> .info_get = efx_devlink_info_get, >>>>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>>>> }; >>>>>> >>>>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>>>> -- >>>>>> 2.17.1 >>>>>> >
On 1/23/23 08:38, Jiri Pirko wrote: > Sun, Jan 22, 2023 at 05:36:14PM CET, alejandro.lucero-palau@amd.com wrote: >> On 1/20/23 12:48, Lucero Palau, Alejandro wrote: >>> On 1/20/23 11:08, Jiri Pirko wrote: >>>> Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: >>>>> On 1/19/23 12:37, Jiri Pirko wrote: >>>>>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>>>>> implies to execute an MCDI command for getting the data from the >>>>>>> firmware for each devlink port. >>>>>>> >>>>>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>>>>> --- >>>>>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>>>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>>>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>>>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>>>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>>>>> 5 files changed, 81 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>> index f4e913593f2b..4400ce622228 100644 >>>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>>>>> return rc; >>>>>>> } >>>>>>> >>>>>>> +/* MCDI commands are related to the same device issuing them. This function >>>>>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>>>>> + * things for VFs. >>>>>>> + */ >>>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>>>>> +{ >>>>>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>>>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>>>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>>>>> + size_t outlen; >>>>>>> + int rc; >>>>>>> + >>>>>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>>>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>>>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>>>>> + pciefn_flat); >>>>>>> + >>>>>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>>>>> + outbuf, sizeof(outbuf), &outlen); >>>>>>> + if (rc) >>>>>>> + return rc; >>>>>>> + if (outlen < sizeof(outbuf)) >>>>>>> + return -EIO; >>>>>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>>>>> + return 0; >>>>>>> +} >>>>>>> + >>>>>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>>>>> { >>>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>> index e59044072333..f1ed481c1260 100644 >>>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>>>>> >>>>>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>>>>> int client_handle, bool empty_ok); >>>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>>>>> #endif /* EFX_EF100_NIC_H */ >>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>> index ff0c8da61919..974c9ff901a0 100644 >>>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>>>>> } >>>>>>> >>>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>>>>> +{ >>>>>>> + bool pcie_func; >>>>>>> + >>>>>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>>>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>>>>> +} >>>>>>> + >>>>>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>>>>> { >>>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>> index 9cca41614982..74853ccbc937 100644 >>>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>>>>> struct mae_mport_desc; >>>>>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>>> struct mae_mport_desc *mport_desc); >>>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>>>>> #endif /* EF100_REP_H */ >>>>>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>>>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>>>>> return err; >>>>>>> } >>>>>>> >>>>>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>>>>> + int *hw_addr_len, >>>>>>> + struct netlink_ext_ack *extack) >>>>>>> +{ >>>>>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>>>>> + struct mae_mport_desc *mport_desc; >>>>>>> + efx_qword_t pciefn; >>>>>>> + u32 client_id; >>>>>>> + int rc = 0; >>>>>>> + >>>>>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>>>>> I may be missing something, where do you fail with -EOPNOTSUPP >>>>>> in case this is called for PHYSICAL port ? >>>>>> >>>>> We do not create a devlink port for the physical port. >>>> Well, you do: >>>> >>>> + switch (mport->mport_type) { >>>> + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: >>>> + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; >>>> >>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>> Right. >>> >>> I was relying on devlink-port output which does not show the physical >>> port in my machine and completely forgot about it. >>> >>> I'm a bit confused at this point. Let me look at this further. >>> >> The reason devlink show/info does not report the PHYSICAL port is >> simple: current code does not create a devlink port for such mport. >> >> I say current code because in my first implementation (not upstreamed) I >> just created devlink ports when the mports where enumerated, what turned >> out to be a bit complicated for dealing with VFs creation/destruction, >> and after looking at how other drivers do this, just before the related >> netdev is registered, I went for that same approach. That leaves no >> option for the physical mport registered. >> >> I could add that creation at PF devlink port creation, if we consider >> this a necessity. I know the ideal devlink support in our driver should >> likely be more devlink design compliant, but it is also true drivers >> should make use of it as decided for fulfilling their necessities as >> long as it does not create confusion to users. Our current devlink >> necessity is for dealing with setting VFs mac address as required during >> previous representors patchset review: >> >> https://lore.kernel.org/netdev/20220728092008.2117846e@kernel.org/ >> >> Do you see a problem not creating such a devlink port by now? > You don't have to create it. But loose the code setting PHYSICAL > flavour. > I'll do so adding a comment discarding the physical port creation. Thanks. > >>>> + attrs.phys.port_number = mport->port_idx; >>>> + devlink_port_attrs_set(dl_port, &attrs); >>>> + break; >>>> + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: >>>> + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { >>>> + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, >>>> + mport->vf_idx, >>>> + external); >>>> + } else { >>>> + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, >>>> + external); >>>> + } >>>> + break; >>>> >>>> >>>> >>>> >>>>> I'm aware this is not "fully compliant" with devlink design idea, just >>>>> trying to use it for our current urgent necessities by now. >>>>> >>>>> Do you think this could be a problem? >>>> If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not >>>> uplink, it is a problem :) >>>> >>>> >>>> >>>>>>> + if (!mport_desc) >>>>>>> + return -EINVAL; >>>>>>> + >>>>>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>>>>> + goto out; >>>>>>> + >>>>>>> + if (ef100_mport_is_vf(mport_desc)) >>>>>>> + 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); >>>>>>> + else >>>>>>> + EFX_POPULATE_QWORD_3(pciefn, >>>>>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>>>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>>>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>>>>> + >>>>>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>>>>> + if (rc) { >>>>>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>>>>> + "Failed to get client ID for port index %u, rc %d\n", >>>>>>> + port->index, rc); >>>>>>> + return rc; >>>>>>> + } >>>>>>> + >>>>>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>>>>>> +out: >>>>>>> + *hw_addr_len = ETH_ALEN; >>>>>>> + >>>>>>> + return rc; >>>>>>> +} >>>>>>> + >>>>>>> static int efx_devlink_info_get(struct devlink *devlink, >>>>>>> struct devlink_info_req *req, >>>>>>> struct netlink_ext_ack *extack) >>>>>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>>>>> >>>>>>> static const struct devlink_ops sfc_devlink_ops = { >>>>>>> .info_get = efx_devlink_info_get, >>>>>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>>>>> }; >>>>>>> >>>>>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>>>>> -- >>>>>>> 2.17.1 >>>>>>>
Mon, Jan 23, 2023 at 10:45:47AM CET, alejandro.lucero-palau@amd.com wrote: > >On 1/23/23 08:38, Jiri Pirko wrote: >> Sun, Jan 22, 2023 at 05:36:14PM CET, alejandro.lucero-palau@amd.com wrote: >>> On 1/20/23 12:48, Lucero Palau, Alejandro wrote: >>>> On 1/20/23 11:08, Jiri Pirko wrote: >>>>> Thu, Jan 19, 2023 at 04:09:27PM CET, alejandro.lucero-palau@amd.com wrote: >>>>>> On 1/19/23 12:37, Jiri Pirko wrote: >>>>>>> Thu, Jan 19, 2023 at 12:31:39PM 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 obtaining the mac address linked to mports in ef100. This >>>>>>>> implies to execute an MCDI command for getting the data from the >>>>>>>> firmware for each devlink port. >>>>>>>> >>>>>>>> Signed-off-by: Alejandro Lucero <alejandro.lucero-palau@amd.com> >>>>>>>> --- >>>>>>>> drivers/net/ethernet/sfc/ef100_nic.c | 27 ++++++++++++++++ >>>>>>>> drivers/net/ethernet/sfc/ef100_nic.h | 1 + >>>>>>>> drivers/net/ethernet/sfc/ef100_rep.c | 8 +++++ >>>>>>>> drivers/net/ethernet/sfc/ef100_rep.h | 1 + >>>>>>>> drivers/net/ethernet/sfc/efx_devlink.c | 44 ++++++++++++++++++++++++++ >>>>>>>> 5 files changed, 81 insertions(+) >>>>>>>> >>>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>>> index f4e913593f2b..4400ce622228 100644 >>>>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c >>>>>>>> @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) >>>>>>>> return rc; >>>>>>>> } >>>>>>>> >>>>>>>> +/* MCDI commands are related to the same device issuing them. This function >>>>>>>> + * allows to do an MCDI command on behalf of another device, mainly PFs setting >>>>>>>> + * things for VFs. >>>>>>>> + */ >>>>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) >>>>>>>> +{ >>>>>>>> + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); >>>>>>>> + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); >>>>>>>> + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); >>>>>>>> + size_t outlen; >>>>>>>> + int rc; >>>>>>>> + >>>>>>>> + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, >>>>>>>> + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); >>>>>>>> + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, >>>>>>>> + pciefn_flat); >>>>>>>> + >>>>>>>> + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), >>>>>>>> + outbuf, sizeof(outbuf), &outlen); >>>>>>>> + if (rc) >>>>>>>> + return rc; >>>>>>>> + if (outlen < sizeof(outbuf)) >>>>>>>> + return -EIO; >>>>>>>> + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); >>>>>>>> + return 0; >>>>>>>> +} >>>>>>>> + >>>>>>>> int ef100_probe_netdev_pf(struct efx_nic *efx) >>>>>>>> { >>>>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>>> index e59044072333..f1ed481c1260 100644 >>>>>>>> --- a/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.h >>>>>>>> @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); >>>>>>>> >>>>>>>> int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, >>>>>>>> int client_handle, bool empty_ok); >>>>>>>> +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); >>>>>>>> #endif /* EFX_EF100_NIC_H */ >>>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>>> index ff0c8da61919..974c9ff901a0 100644 >>>>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c >>>>>>>> @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>>>> mport_desc->interface_idx == nic_data->local_mae_intf; >>>>>>>> } >>>>>>>> >>>>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) >>>>>>>> +{ >>>>>>>> + bool pcie_func; >>>>>>>> + >>>>>>>> + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); >>>>>>>> + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); >>>>>>>> +} >>>>>>>> + >>>>>>>> void efx_ef100_init_reps(struct efx_nic *efx) >>>>>>>> { >>>>>>>> struct ef100_nic_data *nic_data = efx->nic_data; >>>>>>>> diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>>> index 9cca41614982..74853ccbc937 100644 >>>>>>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h >>>>>>>> @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); >>>>>>>> struct mae_mport_desc; >>>>>>>> bool ef100_mport_on_local_intf(struct efx_nic *efx, >>>>>>>> struct mae_mport_desc *mport_desc); >>>>>>>> +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); >>>>>>>> #endif /* EF100_REP_H */ >>>>>>>> diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>>> index bb19d3ad7ffd..2a57c4f6d2b2 100644 >>>>>>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c >>>>>>>> @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, >>>>>>>> return err; >>>>>>>> } >>>>>>>> >>>>>>>> +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, >>>>>>>> + int *hw_addr_len, >>>>>>>> + struct netlink_ext_ack *extack) >>>>>>>> +{ >>>>>>>> + struct efx_devlink *devlink = devlink_priv(port->devlink); >>>>>>>> + struct mae_mport_desc *mport_desc; >>>>>>>> + efx_qword_t pciefn; >>>>>>>> + u32 client_id; >>>>>>>> + int rc = 0; >>>>>>>> + >>>>>>>> + mport_desc = efx_mae_get_mport(devlink->efx, port->index); >>>>>>> I may be missing something, where do you fail with -EOPNOTSUPP >>>>>>> in case this is called for PHYSICAL port ? >>>>>>> >>>>>> We do not create a devlink port for the physical port. >>>>> Well, you do: >>>>> >>>>> + switch (mport->mport_type) { >>>>> + case MAE_MPORT_DESC_MPORT_TYPE_NET_PORT: >>>>> + attrs.flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL; >>>>> >>>>> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ >>>> Right. >>>> >>>> I was relying on devlink-port output which does not show the physical >>>> port in my machine and completely forgot about it. >>>> >>>> I'm a bit confused at this point. Let me look at this further. >>>> >>> The reason devlink show/info does not report the PHYSICAL port is >>> simple: current code does not create a devlink port for such mport. >>> >>> I say current code because in my first implementation (not upstreamed) I >>> just created devlink ports when the mports where enumerated, what turned >>> out to be a bit complicated for dealing with VFs creation/destruction, >>> and after looking at how other drivers do this, just before the related >>> netdev is registered, I went for that same approach. That leaves no >>> option for the physical mport registered. >>> >>> I could add that creation at PF devlink port creation, if we consider >>> this a necessity. I know the ideal devlink support in our driver should >>> likely be more devlink design compliant, but it is also true drivers >>> should make use of it as decided for fulfilling their necessities as >>> long as it does not create confusion to users. Our current devlink >>> necessity is for dealing with setting VFs mac address as required during >>> previous representors patchset review: >>> >>> https://lore.kernel.org/netdev/20220728092008.2117846e@kernel.org/ >>> >>> Do you see a problem not creating such a devlink port by now? >> You don't have to create it. But loose the code setting PHYSICAL >> flavour. >> >I'll do so adding a comment discarding the physical port creation. Yes, and remove the related code. > > >Thanks. > >> >>>>> + attrs.phys.port_number = mport->port_idx; >>>>> + devlink_port_attrs_set(dl_port, &attrs); >>>>> + break; >>>>> + case MAE_MPORT_DESC_MPORT_TYPE_VNIC: >>>>> + if (mport->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL) { >>>>> + devlink_port_attrs_pci_vf_set(dl_port, 0, mport->pf_idx, >>>>> + mport->vf_idx, >>>>> + external); >>>>> + } else { >>>>> + devlink_port_attrs_pci_pf_set(dl_port, 0, mport->pf_idx, >>>>> + external); >>>>> + } >>>>> + break; >>>>> >>>>> >>>>> >>>>> >>>>>> I'm aware this is not "fully compliant" with devlink design idea, just >>>>>> trying to use it for our current urgent necessities by now. >>>>>> >>>>>> Do you think this could be a problem? >>>>> If you create port flavour = DEVLINK_PORT_FLAVOUR_PHYSICAL and it is not >>>>> uplink, it is a problem :) >>>>> >>>>> >>>>> >>>>>>>> + if (!mport_desc) >>>>>>>> + return -EINVAL; >>>>>>>> + >>>>>>>> + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) >>>>>>>> + goto out; >>>>>>>> + >>>>>>>> + if (ef100_mport_is_vf(mport_desc)) >>>>>>>> + 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); >>>>>>>> + else >>>>>>>> + EFX_POPULATE_QWORD_3(pciefn, >>>>>>>> + PCIE_FUNCTION_PF, mport_desc->pf_idx, >>>>>>>> + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, >>>>>>>> + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); >>>>>>>> + >>>>>>>> + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); >>>>>>>> + if (rc) { >>>>>>>> + netif_err(devlink->efx, drv, devlink->efx->net_dev, >>>>>>>> + "Failed to get client ID for port index %u, rc %d\n", >>>>>>>> + port->index, rc); >>>>>>>> + return rc; >>>>>>>> + } >>>>>>>> + >>>>>>>> + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); >>>>>>>> +out: >>>>>>>> + *hw_addr_len = ETH_ALEN; >>>>>>>> + >>>>>>>> + return rc; >>>>>>>> +} >>>>>>>> + >>>>>>>> static int efx_devlink_info_get(struct devlink *devlink, >>>>>>>> struct devlink_info_req *req, >>>>>>>> struct netlink_ext_ack *extack) >>>>>>>> @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, >>>>>>>> >>>>>>>> static const struct devlink_ops sfc_devlink_ops = { >>>>>>>> .info_get = efx_devlink_info_get, >>>>>>>> + .port_function_hw_addr_get = efx_devlink_port_addr_get, >>>>>>>> }; >>>>>>>> >>>>>>>> static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx) >>>>>>>> -- >>>>>>>> 2.17.1 >>>>>>>> >
diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c index f4e913593f2b..4400ce622228 100644 --- a/drivers/net/ethernet/sfc/ef100_nic.c +++ b/drivers/net/ethernet/sfc/ef100_nic.c @@ -1121,6 +1121,33 @@ static int ef100_probe_main(struct efx_nic *efx) return rc; } +/* MCDI commands are related to the same device issuing them. This function + * allows to do an MCDI command on behalf of another device, mainly PFs setting + * things for VFs. + */ +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id) +{ + MCDI_DECLARE_BUF(outbuf, MC_CMD_GET_CLIENT_HANDLE_OUT_LEN); + MCDI_DECLARE_BUF(inbuf, MC_CMD_GET_CLIENT_HANDLE_IN_LEN); + u64 pciefn_flat = le64_to_cpu(pciefn.u64[0]); + size_t outlen; + int rc; + + MCDI_SET_DWORD(inbuf, GET_CLIENT_HANDLE_IN_TYPE, + MC_CMD_GET_CLIENT_HANDLE_IN_TYPE_FUNC); + MCDI_SET_QWORD(inbuf, GET_CLIENT_HANDLE_IN_FUNC, + pciefn_flat); + + rc = efx_mcdi_rpc(efx, MC_CMD_GET_CLIENT_HANDLE, inbuf, sizeof(inbuf), + outbuf, sizeof(outbuf), &outlen); + if (rc) + return rc; + if (outlen < sizeof(outbuf)) + return -EIO; + *id = MCDI_DWORD(outbuf, GET_CLIENT_HANDLE_OUT_HANDLE); + return 0; +} + int ef100_probe_netdev_pf(struct efx_nic *efx) { struct ef100_nic_data *nic_data = efx->nic_data; diff --git a/drivers/net/ethernet/sfc/ef100_nic.h b/drivers/net/ethernet/sfc/ef100_nic.h index e59044072333..f1ed481c1260 100644 --- a/drivers/net/ethernet/sfc/ef100_nic.h +++ b/drivers/net/ethernet/sfc/ef100_nic.h @@ -94,4 +94,5 @@ int ef100_filter_table_probe(struct efx_nic *efx); int ef100_get_mac_address(struct efx_nic *efx, u8 *mac_address, int client_handle, bool empty_ok); +int efx_ef100_lookup_client_id(struct efx_nic *efx, efx_qword_t pciefn, u32 *id); #endif /* EFX_EF100_NIC_H */ diff --git a/drivers/net/ethernet/sfc/ef100_rep.c b/drivers/net/ethernet/sfc/ef100_rep.c index ff0c8da61919..974c9ff901a0 100644 --- a/drivers/net/ethernet/sfc/ef100_rep.c +++ b/drivers/net/ethernet/sfc/ef100_rep.c @@ -362,6 +362,14 @@ bool ef100_mport_on_local_intf(struct efx_nic *efx, mport_desc->interface_idx == nic_data->local_mae_intf; } +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc) +{ + bool pcie_func; + + pcie_func = ef100_mport_is_pcie_vnic(mport_desc); + return pcie_func && (mport_desc->vf_idx != MAE_MPORT_DESC_VF_IDX_NULL); +} + void efx_ef100_init_reps(struct efx_nic *efx) { struct ef100_nic_data *nic_data = efx->nic_data; diff --git a/drivers/net/ethernet/sfc/ef100_rep.h b/drivers/net/ethernet/sfc/ef100_rep.h index 9cca41614982..74853ccbc937 100644 --- a/drivers/net/ethernet/sfc/ef100_rep.h +++ b/drivers/net/ethernet/sfc/ef100_rep.h @@ -75,4 +75,5 @@ void efx_ef100_fini_reps(struct efx_nic *efx); struct mae_mport_desc; bool ef100_mport_on_local_intf(struct efx_nic *efx, struct mae_mport_desc *mport_desc); +bool ef100_mport_is_vf(struct mae_mport_desc *mport_desc); #endif /* EF100_REP_H */ diff --git a/drivers/net/ethernet/sfc/efx_devlink.c b/drivers/net/ethernet/sfc/efx_devlink.c index bb19d3ad7ffd..2a57c4f6d2b2 100644 --- a/drivers/net/ethernet/sfc/efx_devlink.c +++ b/drivers/net/ethernet/sfc/efx_devlink.c @@ -429,6 +429,49 @@ static int efx_devlink_add_port(struct efx_nic *efx, return err; } +static int efx_devlink_port_addr_get(struct devlink_port *port, u8 *hw_addr, + int *hw_addr_len, + struct netlink_ext_ack *extack) +{ + struct efx_devlink *devlink = devlink_priv(port->devlink); + struct mae_mport_desc *mport_desc; + efx_qword_t pciefn; + u32 client_id; + int rc = 0; + + mport_desc = efx_mae_get_mport(devlink->efx, port->index); + if (!mport_desc) + return -EINVAL; + + if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) + goto out; + + if (ef100_mport_is_vf(mport_desc)) + 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); + else + EFX_POPULATE_QWORD_3(pciefn, + PCIE_FUNCTION_PF, mport_desc->pf_idx, + PCIE_FUNCTION_VF, PCIE_FUNCTION_VF_NULL, + PCIE_FUNCTION_INTF, PCIE_INTERFACE_CALLER); + + rc = efx_ef100_lookup_client_id(devlink->efx, pciefn, &client_id); + if (rc) { + netif_err(devlink->efx, drv, devlink->efx->net_dev, + "Failed to get client ID for port index %u, rc %d\n", + port->index, rc); + return rc; + } + + rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true); +out: + *hw_addr_len = ETH_ALEN; + + return rc; +} + static int efx_devlink_info_get(struct devlink *devlink, struct devlink_info_req *req, struct netlink_ext_ack *extack) @@ -442,6 +485,7 @@ static int efx_devlink_info_get(struct devlink *devlink, static const struct devlink_ops sfc_devlink_ops = { .info_get = efx_devlink_info_get, + .port_function_hw_addr_get = efx_devlink_port_addr_get, }; static struct devlink_port *ef100_set_devlink_port(struct efx_nic *efx, u32 idx)