RE: [PATCH net-next 12/15] net: enetc: add VF support for i.MX94 and i.MX95

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c

b/drivers/net/ethernet/freescale/enetc/enetc.c

quoted

index aa8a87124b103..fdceaf36daa72 100644

--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c

@@ -3745,6 +3745,13 @@ static const struct enetc_drvdata enetc_vf_data

= {

quoted

 	.eth_ops = &enetc_vf_ethtool_ops,
 };

+static const struct enetc_drvdata enetc4_vf_data = {
+	.sysclk_freq = ENETC_CLK_333M,
+	.tx_csum = true,
+	.max_frags = ENETC4_MAX_SKB_FRAGS,
+	.eth_ops = &enetc_vf_ethtool_ops,

[Severity: High]
Does this safely support ENETC v4 hardware? The enetc_vf_ethtool_ops
implements the get_regs callback using enetc_get_regs(), which
unconditionally reads registers defined in the enetc_si_regs array.

This array includes ENETC 1.0 specific Control Ring (CBDR) registers like
ENETC_SICBDRMR. Since ENETC v4 hardware uses NTMP instead of CBDR,
these
register offsets are invalid or reserved on i.MX94/i.MX95.

Reading reserved memory regions via ethtool -d can trigger a synchronous
external abort or kernel panic on some architectures. Should the driver
define an ENETC4-specific ethtool_ops for the VF or check hardware
revisions before reading these registers?

The VF registers of v1 and v4 are basically the same, so there is no such
issue.

quoted

+};
+

[ ... ]

quoted

diff --git a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

quoted

index 15c8b704b2b75..868ed694e1200 100644

--- a/drivers/net/ethernet/freescale/enetc/enetc4_pf.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc4_pf.c

@@ -314,11 +314,15 @@ static const struct enetc_pf_ops enetc4_pf_ops =

{

quoted

 static int enetc4_pf_struct_init(struct enetc_si *si)
 {
 	struct enetc_pf *pf = enetc_si_priv(si);
+	int err;

 	pf->si = si;
-	pf->total_vfs = pci_sriov_get_totalvfs(si->pdev);
 	pf->ops = &enetc4_pf_ops;

+	err = enetc_init_sriov_resources(pf);

[Severity: High]
Can this lead to an uninitialized mutex being used on standalone ENETC v4
devices that do not support VFs?

In enetc_init_sriov_resources(), there is an early return if pf->total_vfs
is 0:

int enetc_init_sriov_resources(struct enetc_pf *pf)
{
	...
	pf->total_vfs = pci_sriov_get_totalvfs(pf->si->pdev);
	if (!pf->total_vfs)
		return 0;
	...
	mutex_init(&pf->msg_lock);
	return 0;
}

If total_vfs is 0 (as is the case for standalone i.MX94 ENETC),
the msg_lock is not initialized. Later, when the network interface is
brought up or the promiscuous mode changes, enetc4_psi_do_set_rx_mode()
unconditionally locks this uninitialized mutex:

	mutex_lock(&pf->msg_lock);
	enetc4_pf_set_si_mac_promisc(hw, 0, UC, uc_promisc);
	...
	mutex_unlock(&pf->msg_lock);

Would it be better to initialize the mutex unconditionally, or condition
the locking on the device capabilities?

Yes, this is a bug, will fix it in v2.

--
pw-bot: cr