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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <Ytp3dl5l9jkp94lU@shell.armlinux.org.uk>
Date:   Fri, 22 Jul 2022 11:09:58 +0100
From:   "Russell King (Oracle)" <linux@...linux.org.uk>
To:     Arun Ramadoss <arun.ramadoss@...rochip.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        Woojung Huh <woojung.huh@...rochip.com>,
        UNGLinuxDriver@...rochip.com, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [Patch net-next v1 3/9] net: dsa: microchip: add common duplex
 and flow control function

Hi,

On Fri, Jul 22, 2022 at 02:54:53PM +0530, Arun Ramadoss wrote:
> +void ksz_set_fullduplex(struct ksz_device *dev, int port, bool val)
> +{
> +	const u8 *bitval = dev->info->xmii_ctrl0;
> +	const u16 *regs = dev->info->regs;
> +	u8 data8;
> +
> +	ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> +	data8 &= ~P_MII_DUPLEX_M;
> +
> +	if (val)
> +		data8 |= FIELD_PREP(P_MII_DUPLEX_M,
> +				    bitval[P_MII_FULL_DUPLEX]);
> +	else
> +		data8 |= FIELD_PREP(P_MII_DUPLEX_M,
> +				    bitval[P_MII_HALF_DUPLEX]);
> +
> +	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +
> +void ksz_set_tx_pause(struct ksz_device *dev, int port, bool val)
> +{
> +	const u32 *masks = dev->info->masks;
> +	const u16 *regs = dev->info->regs;
> +	u8 data8;
> +
> +	ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> +	if (val)
> +		data8 |= masks[P_MII_TX_FLOW_CTRL];
> +	else
> +		data8 &= ~masks[P_MII_TX_FLOW_CTRL];
> +
> +	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +
> +void ksz_set_rx_pause(struct ksz_device *dev, int port, bool val)
> +{
> +	const u32 *masks = dev->info->masks;
> +	const u16 *regs = dev->info->regs;
> +	u8 data8;
> +
> +	ksz_pread8(dev, port, regs[P_XMII_CTRL_0], &data8);
> +
> +	if (val)
> +		data8 |= masks[P_MII_RX_FLOW_CTRL];
> +	else
> +		data8 &= ~masks[P_MII_RX_FLOW_CTRL];
> +
> +	ksz_pwrite8(dev, port, regs[P_XMII_CTRL_0], data8);
> +}
> +

Having looked through all the proposed patches and noticed that these
three functions are always called serially. What is the reason to make
these separate functions which all read the same register, modify a
single bit, and then write it back.

What we end up with is the following sequence:

- read P_XMII_CTRL_0
- udpate P_MII_HALF_DUPLEX bit
- write P_XMII_CTRL_0
- read P_XMII_CTRL_0
- update P_MII_TX_FLOW_CTRL bit
- write P_XMII_CTRL_0
- read P_XMII_CTRL_0
- update P_MII_RX_FLOW_CTRL bit
- write P_XMII_CTRL_0

whereas the original code did:

- read P_XMII_CTRL_0
- udpate P_MII_HALF_DUPLEX, P_MII_TX_FLOW_CTRL and P_MII_RX_FLOW_CTRL
  bits
- write P_XMII_CTRL_0

which was much more efficient, not only in terms of CPU cycles, but also
IO cycles and code size.

You could do this instead:

	u8 mask, val, ctrl0;

	mask = P_MII_DUPLEX_M | masks[P_MII_TX_FLOW_CTRL] |
	       masks[P_MII_RX_FLOW_CTRL];

	if (duplex == DUPLEX_FULL)
		val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_FULL_DUPLEX]);
	else
		val = FIELD_PREP(P_MII_DUPLEX_M, bitval[P_MII_HALF_DUPLEX]);

	if (tx_pause)
		val |= masks[P_MII_TX_FLOW_CTRL];
	
	if (rx_pause)
		val |= masks[P_MII_RX_FLOW_CTRL];
	
	ksz_pread8(dev, port, REG_PORT_XMII_CTRL_0, &ctrl0);
	ctrl0 = (ctrl0 & ~mask) | val;
	ksz_pwrite8(dev, port, REG_PORT_XMII_CTRL_0, ctrl0);

and maybe convert that last three lines into a helper, ksz_pmodify8()
which could be useful in other parts of the driver where you do a
read-modify-write operation on a register.

Thanks.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ