| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <Z-QG4w425UuYXZOX@shell.armlinux.org.uk>
Date: Wed, 26 Mar 2025 13:53:39 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Christian Marangi <ansuelsmth@...il.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [net-next RFC PATCH v2 2/3] net: phy: Add support for Aeonsemi
AS21xxx PHYs
On Wed, Mar 26, 2025 at 01:23:58AM +0100, Christian Marangi wrote:
> +static int aeon_ipc_send_cmd(struct phy_device *phydev, u32 cmd,
> + u16 *ret_sts)
> +{
> + struct as21xxx_priv *priv = phydev->priv;
> + bool curr_parity;
> + int ret;
> +
> + /* The IPC sync by using a single parity bit.
> + * Each CMD have alternately this bit set or clear
> + * to understand correct flow and packet order.
> + */
> + curr_parity = priv->parity_status;
> + if (priv->parity_status)
> + cmd |= AEON_IPC_CMD_PARITY;
> +
> + /* Always update parity for next packet */
> + priv->parity_status = !priv->parity_status;
> +
> + ret = phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_CMD, cmd);
> + if (ret)
> + return ret;
> +
> + /* Wait for packet to be processed */
> + usleep_range(AEON_IPC_DELAY, AEON_IPC_DELAY + 5000);
> +
> + /* With no ret_sts, ignore waiting for packet completion
> + * (ipc parity bit sync)
> + */
> + if (!ret_sts)
> + return 0;
> +
> + ret = aeon_ipcs_wait_cmd(phydev, curr_parity);
> + if (ret)
> + return ret;
> +
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS);
> + if (ret < 0)
> + return ret;
> +
> + *ret_sts = ret;
> + if ((*ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_SUCCESS)
> + return -EFAULT;
> +
> + return 0;
> +}
> +
> +static int aeon_ipc_send_msg(struct phy_device *phydev, u16 opcode,
> + u16 *data, unsigned int data_len, u16 *ret_sts)
> +{
> + struct as21xxx_priv *priv = phydev->priv;
> + u32 cmd;
> + int ret;
> + int i;
> +
> + /* IPC have a max of 8 register to transfer data,
> + * make sure we never exceed this.
> + */
> + if (data_len > AEON_IPC_DATA_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->ipc_lock);
> +
> + for (i = 0; i < data_len / sizeof(u16); i++)
> + phy_write_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i),
> + data[i]);
> +
> + cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, data_len) |
> + FIELD_PREP(AEON_IPC_CMD_OPCODE, opcode);
> + ret = aeon_ipc_send_cmd(phydev, cmd, ret_sts);
> + if (ret)
> + phydev_err(phydev, "failed to send ipc msg for %x: %d\n", opcode, ret);
> +
> + mutex_unlock(&priv->ipc_lock);
> +
> + return ret;
> +}
> +
> +static int aeon_ipc_rcv_msg(struct phy_device *phydev, u16 ret_sts,
> + u16 *data)
> +{
> + unsigned int size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> + struct as21xxx_priv *priv = phydev->priv;
> + int ret;
> + int i;
> +
> + if ((ret_sts & AEON_IPC_STS_STATUS) == AEON_IPC_STS_STATUS_ERROR)
> + return -EINVAL;
> +
> + /* Prevent IPC from stack smashing the kernel */
> + if (size > AEON_IPC_DATA_MAX)
> + return -EINVAL;
> +
> + mutex_lock(&priv->ipc_lock);
> +
> + for (i = 0; i < DIV_ROUND_UP(size, sizeof(u16)); i++) {
> + ret = phy_read_mmd(phydev, MDIO_MMD_VEND1, VEND1_IPC_DATA(i));
> + if (ret < 0) {
> + size = ret;
> + goto out;
> + }
> +
> + data[i] = ret;
> + }
> +
> +out:
> + mutex_unlock(&priv->ipc_lock);
> +
> + return size;
> +}
> +
> +/* Logic to sync parity bit with IPC.
> + * We send 2 NOP cmd with same partity and we wait for IPC
> + * to handle the packet only for the second one. This way
> + * we make sure we are sync for every next cmd.
> + */
> +static int aeon_ipc_sync_parity(struct phy_device *phydev)
> +{
> + struct as21xxx_priv *priv = phydev->priv;
> + u16 ret_sts;
> + u32 cmd;
> + int ret;
> +
> + mutex_lock(&priv->ipc_lock);
> +
> + /* Send NOP with no parity */
> + cmd = FIELD_PREP(AEON_IPC_CMD_SIZE, 0) |
> + FIELD_PREP(AEON_IPC_CMD_OPCODE, IPC_CMD_NOOP);
> + aeon_ipc_send_cmd(phydev, cmd, NULL);
> +
> + /* Reset packet parity */
> + priv->parity_status = false;
> +
> + /* Send second NOP with no parity */
> + ret = aeon_ipc_send_cmd(phydev, cmd, &ret_sts);
> +
> + mutex_unlock(&priv->ipc_lock);
> +
> + /* We expect to return -EFAULT */
> + if (ret != -EFAULT)
> + return ret;
> +
> + if ((ret_sts & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_READY)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +static int aeon_ipc_get_fw_version(struct phy_device *phydev)
> +{
> + u16 ret_data[8], data[1];
> + u16 ret_sts;
> + int ret;
> +
> + data[0] = IPC_INFO_VERSION;
> + ret = aeon_ipc_send_msg(phydev, IPC_CMD_INFO, data, sizeof(data),
> + &ret_sts);
> + if (ret)
> + return ret;
> +
> + ret = aeon_ipc_rcv_msg(phydev, ret_sts, ret_data);
> + if (ret < 0)
> + return ret;
> +
> + phydev_info(phydev, "Firmware Version: %s\n", (char *)ret_data);
> +
> + return 0;
> +}
> +
> +static int aeon_dpc_ra_enable(struct phy_device *phydev)
> +{
> + u16 data[2];
> + u16 ret_sts;
> +
> + data[0] = IPC_CFG_PARAM_DIRECT;
> + data[1] = IPC_CFG_PARAM_DIRECT_DPC_RA;
> +
> + return aeon_ipc_send_msg(phydev, IPC_CMD_CFG_PARAM, data,
> + sizeof(data), &ret_sts);
> +}
> +
> +static int as21xxx_probe(struct phy_device *phydev)
> +{
> + struct as21xxx_priv *priv;
> + int ret;
> +
> + priv = devm_kzalloc(&phydev->mdio.dev,
> + sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> + phydev->priv = priv;
> +
> + ret = devm_mutex_init(&phydev->mdio.dev,
> + &priv->ipc_lock);
> + if (ret)
> + return ret;
> +
> + ret = aeon_firmware_load(phydev);
> + if (ret)
> + return ret;
> +
> + ret = aeon_ipc_sync_parity(phydev);
> + if (ret)
> + return ret;
> +
> + ret = phy_set_bits_mmd(phydev, MDIO_MMD_VEND1, VEND1_PTP_CLK,
> + VEND1_PTP_CLK_EN);
> + if (ret)
> + return ret;
> +
> + ret = aeon_dpc_ra_enable(phydev);
> + if (ret)
> + return ret;
> +
> + ret = aeon_ipc_get_fw_version(phydev);
> + if (ret)
> + return ret;
> +
> + phydev->needs_reregister = true;
> +
> + return 0;
> +}
This probe function allocates devres resources that wil lbe freed when
it returns through the unbinding in patch 1. This is a recipe for
confusion - struct as21xxx_priv must never be used from any of the
"real" driver.
I would suggest:
1. document that devres resources will not be preserved when
phydev->needs_reregister is set true.
2. rename struct as21xxx_priv to struct as21xxx_fw_load_priv to make
it clear that it's for firmware loading.
3. use a prefix that uniquely identifies those functions that can only
be called with this structure populated.
4. set phydev->priv to NULL at the end of this probe function to ensure
no one dereferences the free'd pointer in a "real" driver, which
could lead to use-after-free errors.
In summary, I really don't like this approach - it feels too much of a
hack, _and_ introduces the potential for drivers that makes use of this
to get stuff really very wrong. In my opinion that's not a model that
we should add to the kernel.
I'll say again - why can't the PHY firmware be loaded by board firmware.
You've been silent on my feedback on this point. Given that you're
ignoring me... for this patch series...
Hard NAK.
until you start responding to my review comments.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists