linux-kernel - Re: [PATCH v9 net-next 06/11] net: dsa: microchip: add DSA support for microchip lan937x

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YjaAqXfiBrMggIdA@lunn.ch>
Date:   Sun, 20 Mar 2022 02:17:29 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
Cc:     netdev@...r.kernel.org, olteanv@...il.com, robh+dt@...nel.org,
        UNGLinuxDriver@...rochip.com, woojung.huh@...rochip.com,
        hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
        kuba@...nel.org, linux-kernel@...r.kernel.org,
        vivien.didelot@...il.com, f.fainelli@...il.com,
        devicetree@...r.kernel.org
Subject: Re: [PATCH v9 net-next 06/11] net: dsa: microchip: add DSA support
 for microchip lan937x

> +int lan937x_reset_switch(struct ksz_device *dev)
> +{
> +	u32 data32;
> +	u8 data8;
> +	int ret;
> +
> +	/* reset switch */
> +	ret = lan937x_cfg(dev, REG_SW_OPERATION, SW_RESET, true);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ksz_read8(dev, REG_SW_LUE_CTRL_1, &data8);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Enable Auto Aging */
> +	ret = ksz_write8(dev, REG_SW_LUE_CTRL_1, data8 | SW_LINK_AUTO_AGING);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* disable interrupts */
> +	ret = ksz_write32(dev, REG_SW_INT_MASK__4, SWITCH_INT_MASK);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, 0xFF);
> +	if (ret < 0)
> +		return ret;
> +
> +	return ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data32);

I would probably enable auto aging in the setup routing, not the
reset.  Disabling interrupts is less clear, maybe it also belongs in
setup?

> +static void lan937x_switch_exit(struct ksz_device *dev)
> +{
> +	lan937x_reset_switch(dev);
> +}

Humm. Does a reset leave the switch in a dumb mode, or is it totally
disabled?

> +int lan937x_internal_phy_write(struct ksz_device *dev, int addr, int reg,
> +			       u16 val)
> +{
> +	u16 temp, addr_base;
> +	unsigned int value;
> +	int ret;
> +
> +	/* Check for internal phy port */
> +	if (!lan937x_is_internal_phy_port(dev, addr))
> +		return -EOPNOTSUPP;
> +
> +	if (lan937x_is_internal_base_tx_phy_port(dev, addr))
> +		addr_base = REG_PORT_TX_PHY_CTRL_BASE;
> +	else
> +		addr_base = REG_PORT_T1_PHY_CTRL_BASE;
> +
> +	temp = PORT_CTRL_ADDR(addr, (addr_base + (reg << 2)));
> +
> +	ret = ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> +	if (ret < 0)
> +		return ret;

...

> +}
> +
> +int lan937x_internal_phy_read(struct ksz_device *dev, int addr, int reg,
> +			      u16 *val)
> +{
> +	u16 temp, addr_base;
> +	unsigned int value;
> +	int ret;
> +
> +	/* Check for internal phy port, return 0xffff for non-existent phy */
> +	if (!lan937x_is_internal_phy_port(dev, addr))
> +		return 0xffff;
> +
> +	if (lan937x_is_internal_base_tx_phy_port(dev, addr))
> +		addr_base = REG_PORT_TX_PHY_CTRL_BASE;
> +	else
> +		addr_base = REG_PORT_T1_PHY_CTRL_BASE;
> +
> +	/* get register address based on the logical port */
> +	temp = PORT_CTRL_ADDR(addr, (addr_base + (reg << 2)));
> +
> +	ret = ksz_write16(dev, REG_VPHY_IND_ADDR__2, temp);
> +	if (ret < 0)
> +		return ret;

Looks pretty similar to me. You should pull this out into a helper.


> +void lan937x_port_setup(struct ksz_device *dev, int port, bool cpu_port)
> +{
> +	struct dsa_switch *ds = dev->ds;
> +	u8 member;
> +
> +	/* enable tag tail for host port */
> +	if (cpu_port) {
> +		lan937x_port_cfg(dev, port, REG_PORT_CTRL_0,
> +				 PORT_TAIL_TAG_ENABLE, true);
> +	}

Checkpatch probably warns here that the {} are not needed.

> +
> +	/* disable frame check length field */
> +	lan937x_port_cfg(dev, port, REG_PORT_MAC_CTRL_0, PORT_FR_CHK_LENGTH,
> +			 false);
> +
> +	/* set back pressure for half duplex */
> +	lan937x_port_cfg(dev, port, REG_PORT_MAC_CTRL_1, PORT_BACK_PRESSURE,
> +			 true);
> +
> +	/* enable 802.1p priority */
> +	lan937x_port_cfg(dev, port, P_PRIO_CTRL, PORT_802_1P_PRIO_ENABLE, true);
> +
> +	if (!lan937x_is_internal_phy_port(dev, port)) {
> +		/* enable flow control */
> +		lan937x_port_cfg(dev, port, REG_PORT_XMII_CTRL_0,
> +				 PORT_TX_FLOW_CTRL | PORT_RX_FLOW_CTRL,
> +				 true);
> +	}

Here as well.

> +struct lan_alu_struct {
> +	/* entry 1 */
> +	u8	is_static:1;
> +	u8	is_src_filter:1;
> +	u8	is_dst_filter:1;
> +	u8	prio_age:3;
> +	u32	_reserv_0_1:23;
> +	u8	mstp:3;

I assume this works O.K, but bitfields are nornaly defined using one
type. I would of used u32 for them all. Is there some advantage of
missing u8 and u32?

> +	/* entry 2 */
> +	u8	is_override:1;
> +	u8	is_use_fid:1;
> +	u32	_reserv_1_1:22;
> +	u8	port_forward:8;
> +	/* entry 3 & 4*/
> +	u32	_reserv_2_1:9;
> +	u8	fid:7;
> +	u8	mac[ETH_ALEN];
> +};

  Andrew