Re: [net-next PATCH v7 5/6] net: phy: Add support for Aeonsemi AS21xxx PHYs

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Apr 10, 2025 at 11:53:35AM +0200, Christian Marangi wrote:
> +#define VEND1_IPC_DATA0			0x5808
> +#define VEND1_IPC_DATA1			0x5809
> +#define VEND1_IPC_DATA2			0x580a
> +#define VEND1_IPC_DATA3			0x580b
> +#define VEND1_IPC_DATA4			0x580c
> +#define VEND1_IPC_DATA5			0x580d
> +#define VEND1_IPC_DATA6			0x580e
> +#define VEND1_IPC_DATA7			0x580f

Do you use any of these other than the first? If not, please remove
them, maybe adding a comment before the first stating that there are
8 registers.

> +static int aeon_ipcs_wait_cmd(struct phy_device *phydev, bool parity_status)
> +{
> +	u16 val;
> +
> +	/* Exit condition logic:
> +	 * - Wait for parity bit equal
> +	 * - Wait for status success, error OR ready
> +	 */
> +	return phy_read_mmd_poll_timeout(phydev, MDIO_MMD_VEND1, VEND1_IPC_STS, val,
> +					 FIELD_GET(AEON_IPC_STS_PARITY, val) == parity_status &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_RCVD &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_PROCESS &&
> +					 (val & AEON_IPC_STS_STATUS) != AEON_IPC_STS_STATUS_BUSY,
> +					 AEON_IPC_DELAY, AEON_IPC_TIMEOUT, false);

Hmm. I'm wondering whether:

static bool aeon_ipc_ready(u16 val, bool parity_status)
{
	u16 status;

	if (FIELD_GET(AEON_IPC_STS_PARITY, val) != parity_status)
		return false;

	status = val & AEON_IPC_STS_STATUS;

	return status != AEON_IPC_STS_STATUS_RCVD &&
	       status != AEON_IPC_STS_STATUS_PROCESS &&
	       status != AEON_IPC_STS_STATUS_BUSY;
}

would be better, and then maybe you can fit the code into less than 80
columns. I'm not a fan of FIELD_PREP_CONST() when it causes differing
usage patterns like the above (FIELD_GET(AEON_IPC_STS_STATUS, val)
would match the coding style, and probably makes no difference to the
code emitted.)

> +}
> +
> +static int aeon_ipc_send_cmd(struct phy_device *phydev,
> +			     struct as21xxx_priv *priv,
> +			     u16 cmd, u16 *ret_sts)
> +{
> +	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 -EINVAL;
> +
> +	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;
> +	u16 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, priv, 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)
> +{
> +	struct as21xxx_priv *priv = phydev->priv;
> +	unsigned int size;
> +	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 */
> +	size = FIELD_GET(AEON_IPC_STS_SIZE, ret_sts);
> +	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);

I think the locking here is suspicious.

aeon_ipc_send_msg() takes the lock, writes the registers, and waits for
the success signal, returning the status, and then drops the lock.

Time passes.

aeon_ipc_rcv_msg() is then called, which extracts the number of bytes to
be read from the returned status, takes the lock, reads the registers,
and then drops the lock.

So, what can happen is:

	Thread 1			Thread 2
	aeon_ipc_send_msg()
					aeon_ipc_send_msg()
	aeon_ipc_rcv_msg()
					aeon_ipc_rcv_msg()

Which means thread 1 reads the reply from thread 2's message.

So, this lock does nothing to really protect against the IPC mechanism
against races.

I think you need to combine both into a single function that takes the
lock once and releases it once.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!