Re: [PATCH RFC net-next 3/3] net: dsa: add basic initial driver for MxL862xx switches

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

On Wed, Dec 03, 2025 at 03:07:20AM +0100, Andrew Lunn wrote:
> > +/**
> > + * struct mxl862xx_ss_sp_tag
> > + * @pid: port ID (1~16)
> > + * @mask: bit value 1 to indicate valid field
> > + *	0 - rx
> > + *	1 - tx
> > + *	2 - rx_pen
> > + *	3 - tx_pen
> > + * @rx: RX special tag mode
> > + *	0 - packet does NOT have special tag and special tag is NOT inserted
> > + *	1 - packet does NOT have special tag and special tag is inserted
> > + *	2 - packet has special tag and special tag is NOT inserted
> > + * @tx: TX special tag mode
> > + *	0 - packet does NOT have special tag and special tag is NOT removed
> > + *	1 - packet has special tag and special tag is replaced
> > + *	2 - packet has special tag and special tag is NOT removed
> > + *	3 - packet has special tag and special tag is removed
> > + * @rx_pen: RX special tag info over preamble
> > + *	0 - special tag info inserted from byte 2 to 7 are all 0
> > + *	1 - special tag byte 5 is 16, other bytes from 2 to 7 are 0
> > + *	2 - special tag byte 5 is from preamble field, others are 0
> > + *	3 - special tag byte 2 to 7 are from preabmle field
> > + * @tx_pen: TX special tag info over preamble
> > + *	0 - disabled
> > + *	1 - enabled
> > + */
> > +struct mxl862xx_ss_sp_tag {
> > +	u8 pid;
> > +	u8 mask;
> > +	u8 rx;
> > +	u8 tx;
> > +	u8 rx_pen;
> > +	u8 tx_pen;
> > +} __packed;
> > +
> > +/**
> > + * enum mxl862xx_logical_port_mode - Logical port mode
> > + * @MXL862XX_LOGICAL_PORT_8BIT_WLAN: WLAN with 8-bit station ID
> > + * @MXL862XX_LOGICAL_PORT_9BIT_WLAN: WLAN with 9-bit station ID
> > + * @MXL862XX_LOGICAL_PORT_GPON: GPON OMCI context
> > + * @MXL862XX_LOGICAL_PORT_EPON: EPON context
> > + * @MXL862XX_LOGICAL_PORT_GINT: G.INT context
> > + * @MXL862XX_LOGICAL_PORT_OTHER: Others
> > + */
> > +enum mxl862xx_logical_port_mode {
> > +	MXL862XX_LOGICAL_PORT_8BIT_WLAN = 0,
> > +	MXL862XX_LOGICAL_PORT_9BIT_WLAN,
> > +	MXL862XX_LOGICAL_PORT_GPON,
> > +	MXL862XX_LOGICAL_PORT_EPON,
> > +	MXL862XX_LOGICAL_PORT_GINT,
> > +	MXL862XX_LOGICAL_PORT_OTHER = 0xFF,
> > +};
> > +
> > +/**
> > + * struct mxl862xx_ctp_port_assignment - CTP Port Assignment/association with logical port
> > + * @logical_port_id: Logical Port Id. The valid range is hardware dependent
> > + * @first_ctp_port_id: First CTP Port ID mapped to above logical port ID
> > + * @number_of_ctp_port: Total number of CTP Ports mapped above logical port ID
> > + * @mode: See &enum mxl862xx_logical_port_mode
> > + * @bridge_port_id: Bridge ID (FID)
> > + */
> > +struct mxl862xx_ctp_port_assignment {
> > +	u8 logical_port_id;
> > +	__le16 first_ctp_port_id;
> > +	__le16 number_of_ctp_port;
> > +	enum mxl862xx_logical_port_mode mode;
> > +	__le16 bridge_port_id;
> > +} __packed;
> 
> Does the C standard define the size of an enum? Do you assume this is
> a byte?

It does not. Some architectures are allowed to choose the storage size
of enum depending on the range of values.

> > +static int mxl862xx_send_cmd(struct mxl862xx_priv *dev, u16 cmd, u16 size,
> > +			     s16 *presult)
> > +{
> > +	int ret;
> > +
> > +	ret = __mdiobus_c45_write(dev->bus, dev->sw_addr, MXL862XX_MMD_DEV,
> > +				  MXL862XX_MMD_REG_LEN_RET, size);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __mdiobus_c45_write(dev->bus, dev->sw_addr, MXL862XX_MMD_DEV,
> > +				  MXL862XX_MMD_REG_CTRL, cmd | CTRL_BUSY_MASK);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = mxl862xx_busy_wait(dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = __mdiobus_c45_read(dev->bus, dev->sw_addr, MXL862XX_MMD_DEV,
> > +				 MXL862XX_MMD_REG_LEN_RET);
> > +	if (ret < 0)
> > +		return ret;

Error codes go via this path.

> > +
> > +	*presult = ret;

Register values via this, and if the sign bit is set, *presult is
negative.

> > +	ret = mxl862xx_send_cmd(priv, cmd, size, &result);
> > +	if (ret < 0)
> > +		goto out;
> > +
> > +	if (result < 0) {
> > +		ret = result;
> > +		goto out;
> > +	}
> 
> If i'm reading mxl862xx_send_cmd() correct, result is the value of a
> register. It seems unlikely this is a Linux error code?

result here is the register value, and a negative value is the value
from the register. So I agree - this assigns a register value to
"ret" which gets promoted from s16 to int (sign extension) and thus
gets returned as a Linux error code. So yes, this doesn't seem right.

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