diff mbox series

[v5,net-next,7/8] sfc: add support for devlink port_function_hw_addr_get in ef100

Message ID 20230202111423.56831-8-alejandro.lucero-palau@amd.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series sfc: devlink support for ef100 | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Series has a cover letter
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 1 this patch: 1
netdev/cc_maintainers success CCed 7 of 7 maintainers
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 1 this patch: 1
netdev/checkpatch warning WARNING: line length of 81 exceeds 80 columns
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Lucero Palau, Alejandro Feb. 2, 2023, 11:14 a.m. UTC 
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 | 53 ++++++++++++++++++++++++++
 5 files changed, 90 insertions(+)

Comments

Jiri Pirko Feb. 2, 2023, 12:09 p.m. UTC | #1 
Thu, Feb 02, 2023 at 12:14:22PM 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

Don't talk about "this patch". Just tell the codebase what to do.


>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 | 53 ++++++++++++++++++++++++++
> 5 files changed, 90 insertions(+)
>
>diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
>index aa48c79a2149..becd21c2325d 100644
>--- a/drivers/net/ethernet/sfc/ef100_nic.c
>+++ b/drivers/net/ethernet/sfc/ef100_nic.c
>@@ -1122,6 +1122,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 6b5bc5d6955d..0b3083ef0ead 100644
>--- a/drivers/net/ethernet/sfc/ef100_rep.c
>+++ b/drivers/net/ethernet/sfc/ef100_rep.c
>@@ -361,6 +361,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 ae6add4b0855..a042525a2240 100644
>--- a/drivers/net/ethernet/sfc/ef100_rep.h
>+++ b/drivers/net/ethernet/sfc/ef100_rep.h
>@@ -76,4 +76,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 afdb19f0c774..c44547b9894e 100644
>--- a/drivers/net/ethernet/sfc/efx_devlink.c
>+++ b/drivers/net/ethernet/sfc/efx_devlink.c
>@@ -60,6 +60,56 @@ static int efx_devlink_add_port(struct efx_nic *efx,
> 
> 	return devl_port_register(efx->devlink, &mport->dl_port, mport->mport_id);
> }
>+
>+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;

Pointless init.


>+
>+	mport_desc = container_of(port, struct mae_mport_desc, dl_port);
>+
>+	if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) {
>+		rc = -EINVAL;
>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "Port not on local interface (mport: %u)",
>+				   mport_desc->mport_id);
>+		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) {
>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "No internal client_ID for port (mport: %u)",
>+				   mport_desc->mport_id);
>+		goto out;
>+	}
>+
>+	rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true);
>+	if (rc != 0)

why "if (rc)" is not enough here?

>+		NL_SET_ERR_MSG_FMT(extack,
>+				   "No available MAC for port (mport: %u)",
>+				   mport_desc->mport_id);

It is redundant to print mport_id which is exposed as devlink port id.
Please remove from the all the extack messages. No need to mention
"port" at all, as the user knows on which object he is performing the
command.

Also, perhaps it would sound better to say "No MAC available"?



>+out:
>+	*hw_addr_len = ETH_ALEN;

Odd. I think you should not touch hw_addr_len in case of error.


>+	return rc;
>+}
>+
> #endif
> 
> static int efx_devlink_info_nvram_partition(struct efx_nic *efx,
>@@ -522,6 +572,9 @@ static int efx_devlink_info_get(struct devlink *devlink,
> 
> static const struct devlink_ops sfc_devlink_ops = {
> 	.info_get			= efx_devlink_info_get,
>+#ifdef CONFIG_SFC_SRIOV
>+	.port_function_hw_addr_get	= efx_devlink_port_addr_get,
>+#endif
> };
> 
> #ifdef CONFIG_SFC_SRIOV
>-- 
>2.17.1
>
Lucero Palau, Alejandro Feb. 7, 2023, 3:01 p.m. UTC | #2 
On 2/2/23 12:09, Jiri Pirko wrote:
> Thu, Feb 02, 2023 at 12:14:22PM 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
> Don't talk about "this patch". Just tell the codebase what to do.

OK

>
>> 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 | 53 ++++++++++++++++++++++++++
>> 5 files changed, 90 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
>> index aa48c79a2149..becd21c2325d 100644
>> --- a/drivers/net/ethernet/sfc/ef100_nic.c
>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
>> @@ -1122,6 +1122,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 6b5bc5d6955d..0b3083ef0ead 100644
>> --- a/drivers/net/ethernet/sfc/ef100_rep.c
>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c
>> @@ -361,6 +361,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 ae6add4b0855..a042525a2240 100644
>> --- a/drivers/net/ethernet/sfc/ef100_rep.h
>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h
>> @@ -76,4 +76,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 afdb19f0c774..c44547b9894e 100644
>> --- a/drivers/net/ethernet/sfc/efx_devlink.c
>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c
>> @@ -60,6 +60,56 @@ static int efx_devlink_add_port(struct efx_nic *efx,
>>
>> 	return devl_port_register(efx->devlink, &mport->dl_port, mport->mport_id);
>> }
>> +
>> +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;
> Pointless init.
>
>

Right

>> +
>> +	mport_desc = container_of(port, struct mae_mport_desc, dl_port);
>> +
>> +	if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) {
>> +		rc = -EINVAL;
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "Port not on local interface (mport: %u)",
>> +				   mport_desc->mport_id);
>> +		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) {
>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "No internal client_ID for port (mport: %u)",
>> +				   mport_desc->mport_id);
>> +		goto out;
>> +	}
>> +
>> +	rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true);
>> +	if (rc != 0)
> why "if (rc)" is not enough here?
>

Right

>> +		NL_SET_ERR_MSG_FMT(extack,
>> +				   "No available MAC for port (mport: %u)",
>> +				   mport_desc->mport_id);
> It is redundant to print mport_id which is exposed as devlink port id.
> Please remove from the all the extack messages. No need to mention
> "port" at all, as the user knows on which object he is performing the
> command.

I think the idea was to have such a report to user space as input from a 
client to our support team which could help.

But I guess you are right, and that info would also be reported without 
specifically adding it here.


> Also, perhaps it would sound better to say "No MAC available"?
>
>

That subtle change could imply a total different meaning ... at least in 
my native language. But I will not fight this one :-) and change it in the
next version.

>> +out:
>> +	*hw_addr_len = ETH_ALEN;
> Odd. I think you should not touch hw_addr_len in case of error.
>

It does not harm. Does it?

Otherwise, I need another goto in previous error checking what seems odd 
for a single line.

>> +	return rc;
>> +}
>> +
>> #endif
>>
>> static int efx_devlink_info_nvram_partition(struct efx_nic *efx,
>> @@ -522,6 +572,9 @@ static int efx_devlink_info_get(struct devlink *devlink,
>>
>> static const struct devlink_ops sfc_devlink_ops = {
>> 	.info_get			= efx_devlink_info_get,
>> +#ifdef CONFIG_SFC_SRIOV
>> +	.port_function_hw_addr_get	= efx_devlink_port_addr_get,
>> +#endif
>> };
>>
>> #ifdef CONFIG_SFC_SRIOV
>> -- 
>> 2.17.1
>>
Lucero Palau, Alejandro Feb. 8, 2023, 8:50 a.m. UTC | #3 
On 2/7/23 15:01, Lucero Palau, Alejandro wrote:
> On 2/2/23 12:09, Jiri Pirko wrote:
>> Thu, Feb 02, 2023 at 12:14:22PM 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
>> Don't talk about "this patch". Just tell the codebase what to do.
> OK
>
>>> 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 | 53 ++++++++++++++++++++++++++
>>> 5 files changed, 90 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
>>> index aa48c79a2149..becd21c2325d 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_nic.c
>>> +++ b/drivers/net/ethernet/sfc/ef100_nic.c
>>> @@ -1122,6 +1122,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 6b5bc5d6955d..0b3083ef0ead 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_rep.c
>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.c
>>> @@ -361,6 +361,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 ae6add4b0855..a042525a2240 100644
>>> --- a/drivers/net/ethernet/sfc/ef100_rep.h
>>> +++ b/drivers/net/ethernet/sfc/ef100_rep.h
>>> @@ -76,4 +76,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 afdb19f0c774..c44547b9894e 100644
>>> --- a/drivers/net/ethernet/sfc/efx_devlink.c
>>> +++ b/drivers/net/ethernet/sfc/efx_devlink.c
>>> @@ -60,6 +60,56 @@ static int efx_devlink_add_port(struct efx_nic *efx,
>>>
>>> 	return devl_port_register(efx->devlink, &mport->dl_port, mport->mport_id);
>>> }
>>> +
>>> +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;
>> Pointless init.
>>
>>
> Right
>
>>> +
>>> +	mport_desc = container_of(port, struct mae_mport_desc, dl_port);
>>> +
>>> +	if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) {
>>> +		rc = -EINVAL;
>>> +		NL_SET_ERR_MSG_FMT(extack,
>>> +				   "Port not on local interface (mport: %u)",
>>> +				   mport_desc->mport_id);
>>> +		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) {
>>> +		NL_SET_ERR_MSG_FMT(extack,
>>> +				   "No internal client_ID for port (mport: %u)",
>>> +				   mport_desc->mport_id);
>>> +		goto out;
>>> +	}
>>> +
>>> +	rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true);
>>> +	if (rc != 0)
>> why "if (rc)" is not enough here?
>>
> Right
>
>>> +		NL_SET_ERR_MSG_FMT(extack,
>>> +				   "No available MAC for port (mport: %u)",
>>> +				   mport_desc->mport_id);
>> It is redundant to print mport_id which is exposed as devlink port id.
>> Please remove from the all the extack messages. No need to mention
>> "port" at all, as the user knows on which object he is performing the
>> command.
> I think the idea was to have such a report to user space as input from a
> client to our support team which could help.
>
> But I guess you are right, and that info would also be reported without
> specifically adding it here.
>
>
>> Also, perhaps it would sound better to say "No MAC available"?
>>
>>
> That subtle change could imply a total different meaning ... at least in
> my native language. But I will not fight this one :-) and change it in the
> next version.
>
>>> +out:
>>> +	*hw_addr_len = ETH_ALEN;
>> Odd. I think you should not touch hw_addr_len in case of error.
>>
> It does not harm. Does it?
>
> Otherwise, I need another goto in previous error checking what seems odd
> for a single line.


Silly me. I do not need the gotos at all!

>>> +	return rc;
>>> +}
>>> +
>>> #endif
>>>
>>> static int efx_devlink_info_nvram_partition(struct efx_nic *efx,
>>> @@ -522,6 +572,9 @@ static int efx_devlink_info_get(struct devlink *devlink,
>>>
>>> static const struct devlink_ops sfc_devlink_ops = {
>>> 	.info_get			= efx_devlink_info_get,
>>> +#ifdef CONFIG_SFC_SRIOV
>>> +	.port_function_hw_addr_get	= efx_devlink_port_addr_get,
>>> +#endif
>>> };
>>>
>>> #ifdef CONFIG_SFC_SRIOV
>>> -- 
>>> 2.17.1
>>>
diff mbox series

Patch

diff --git a/drivers/net/ethernet/sfc/ef100_nic.c b/drivers/net/ethernet/sfc/ef100_nic.c
index aa48c79a2149..becd21c2325d 100644
--- a/drivers/net/ethernet/sfc/ef100_nic.c
+++ b/drivers/net/ethernet/sfc/ef100_nic.c
@@ -1122,6 +1122,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 6b5bc5d6955d..0b3083ef0ead 100644
--- a/drivers/net/ethernet/sfc/ef100_rep.c
+++ b/drivers/net/ethernet/sfc/ef100_rep.c
@@ -361,6 +361,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 ae6add4b0855..a042525a2240 100644
--- a/drivers/net/ethernet/sfc/ef100_rep.h
+++ b/drivers/net/ethernet/sfc/ef100_rep.h
@@ -76,4 +76,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 afdb19f0c774..c44547b9894e 100644
--- a/drivers/net/ethernet/sfc/efx_devlink.c
+++ b/drivers/net/ethernet/sfc/efx_devlink.c
@@ -60,6 +60,56 @@  static int efx_devlink_add_port(struct efx_nic *efx,
 
 	return devl_port_register(efx->devlink, &mport->dl_port, mport->mport_id);
 }
+
+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 = container_of(port, struct mae_mport_desc, dl_port);
+
+	if (!ef100_mport_on_local_intf(devlink->efx, mport_desc)) {
+		rc = -EINVAL;
+		NL_SET_ERR_MSG_FMT(extack,
+				   "Port not on local interface (mport: %u)",
+				   mport_desc->mport_id);
+		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) {
+		NL_SET_ERR_MSG_FMT(extack,
+				   "No internal client_ID for port (mport: %u)",
+				   mport_desc->mport_id);
+		goto out;
+	}
+
+	rc = ef100_get_mac_address(devlink->efx, hw_addr, client_id, true);
+	if (rc != 0)
+		NL_SET_ERR_MSG_FMT(extack,
+				   "No available MAC for port (mport: %u)",
+				   mport_desc->mport_id);
+out:
+	*hw_addr_len = ETH_ALEN;
+	return rc;
+}
+
 #endif
 
 static int efx_devlink_info_nvram_partition(struct efx_nic *efx,
@@ -522,6 +572,9 @@  static int efx_devlink_info_get(struct devlink *devlink,
 
 static const struct devlink_ops sfc_devlink_ops = {
 	.info_get			= efx_devlink_info_get,
+#ifdef CONFIG_SFC_SRIOV
+	.port_function_hw_addr_get	= efx_devlink_port_addr_get,
+#endif
 };
 
 #ifdef CONFIG_SFC_SRIOV