Re: [net-next RFC PATCH v2 2/3] net: phy: Add support for Aeonsemi AS21xxx PHYs

[Christian Marangi]

On Wed, Mar 26, 2025 at 01:53:39PM +0000, Russell King (Oracle) wrote:
> 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.
>

No I wasn't ignoring you, the description in v1 for this was very
generic and confusing so the idea was to post a real implementation so
we could discuss on the code in practice... My comments were done before
checking how phy_registration worked so they were only ideas (the
implementation changed a lot from what was my idea) Sorry if I gave this
impression while I was answering only to Andrew...

The problem of PHY firmware loaded by board firmware is that we
introduce lots of assumption on the table. Also doesn't that goes
against the idea that the kernel should not assume stuff set by the
bootloader (if they can be reset and are not part of the core system?)

>From what I'm seeing on devices that have this, SPI is never mounted and
bootloader doesn't provide support for this and the thing is loaded only
by the OS in a crap way.

Also the PHY doesn't keep the FW with an hardware reset and permit the
kernel to load an updated (probably fixed) firmware is only beneficial.
(there is plan to upstream firmware to linux-firmware)

I agree that the approach of declaring a "generic" PHY entry and "abuse"
the probe function is an HACK, but I also feel using match_phy_device
doesn't solve the problem.

Correct me if I'm wrong but match_phy_device will only give true or
false, it won't solve the problem of the PHY changing after the FW is
loaded.

This current approach permit to provide to the user the exact new OPs of
the PHY detected after FW is loaded.

Is it really that bad to introduce the idea that a PHY family require
some initial tuneup before it can correctly identified?

-- 
	Ansuel