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
| ||
|
Message-ID: <ZCLmwm01FK7laSqs@makrotopia.org> Date: Tue, 28 Mar 2023 14:08:18 +0100 From: Daniel Golle <daniel@...rotopia.org> To: Andrew Lunn <andrew@...n.ch> Cc: netdev@...r.kernel.org, linux-mediatek@...ts.infradead.org, linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, 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>, Matthias Brugger <matthias.bgg@...il.com>, AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>, Sean Wang <sean.wang@...iatek.com>, Landen Chao <Landen.Chao@...iatek.com>, DENG Qingfang <dqfext@...il.com>, Philipp Zabel <p.zabel@...gutronix.de>, Sam Shih <Sam.Shih@...iatek.com>, Lorenzo Bianconi <lorenzo@...nel.org>, John Crispin <john@...ozen.org>, Felix Fietkau <nbd@....name> Subject: Re: [RFC PATCH net-next 2/2] net: dsa: mt7530: introduce MMIO driver for MT7988 SoC Hi Andrew, On Tue, Mar 28, 2023 at 03:58:50AM +0200, Andrew Lunn wrote: > > --- a/drivers/net/dsa/mt7530.c > > +++ b/drivers/net/dsa/mt7530.c > > @@ -118,6 +118,9 @@ core_write_mmd_indirect(struct mt7530_priv *priv, int prtad, > > struct mii_bus *bus = priv->bus; > > int ret; > > > > + if (!bus) > > + return 0; > > + > > /* Write the desired MMD Devad */ > > ret = bus->write(bus, 0, MII_MMD_CTRL, devad); > > if (ret < 0) > > @@ -147,11 +150,13 @@ core_write(struct mt7530_priv *priv, u32 reg, u32 val) > > { > > struct mii_bus *bus = priv->bus; > > > > - mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > > + if (bus) > > + mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > > > > core_write_mmd_indirect(priv, reg, MDIO_MMD_VEND2, val); > > > > - mutex_unlock(&bus->mdio_lock); > > + if (bus) > > + mutex_unlock(&bus->mdio_lock); > > } > > > > static void > > @@ -160,6 +165,9 @@ core_rmw(struct mt7530_priv *priv, u32 reg, u32 mask, u32 set) > > struct mii_bus *bus = priv->bus; > > u32 val; > > > > + if (!bus) > > + return; > > + > > mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); > > > > val = core_read_mmd_indirect(priv, reg, MDIO_MMD_VEND2); > > @@ -189,6 +197,11 @@ mt7530_mii_write(struct mt7530_priv *priv, u32 reg, u32 val) > > u16 page, r, lo, hi; > > int ret; > > > > + if (priv->base_addr) { > > + iowrite32(val, priv->base_addr + reg); > > + return 0; > > + } > > + > > page = (reg >> 6) & 0x3ff; > > r = (reg >> 2) & 0xf; > > lo = val & 0xffff; > > @@ -218,6 +231,9 @@ mt7530_mii_read(struct mt7530_priv *priv, u32 reg) > > u16 page, r, lo, hi; > > int ret; > > > > + if (priv->base_addr) > > + return ioread32(priv->base_addr + reg); > > + > > page = (reg >> 6) & 0x3ff; > > r = (reg >> 2) & 0xf; > > > > This looks pretty ugly. > > A much better way to do this is to use regmap. Take a look at xrs700x > and how it has both an i2c and an mdio version. I agree that using regmap would be better and I have evaluated that approach as well. As regmap doesn't allow lock-skipping and mt7530.c is much more complex than xrs700x in the way indirect access to its MDIO bus and interrupts work, using regmap accessors for everything would not be trivial. To illustrate what I'm talking about, let me show some examples in the current code for which I don't see a way to use regmap: 634) static int 635) mt7531_ind_c45_phy_read(struct mt7530_priv *priv, int port, int devad, 636) int regnum) 637) { 638) struct mii_bus *bus = priv->bus; 639) struct mt7530_dummy_poll p; 640) u32 reg, val; 641) int ret; 642) 643) INIT_MT7530_DUMMY_POLL(&p, priv, MT7531_PHY_IAC); 644) 645) mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED); 646) 647) ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val, 648) !(val & MT7531_PHY_ACS_ST), 20, 100000); 649) if (ret < 0) { 650) dev_err(priv->dev, "poll timeout\n"); 651) goto out; 652) } 653) 654) reg = MT7531_MDIO_CL45_ADDR | MT7531_MDIO_PHY_ADDR(port) | 655) MT7531_MDIO_DEV_ADDR(devad) | regnum; 656) mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST); 657) 658) ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val, 659) !(val & MT7531_PHY_ACS_ST), 20, 100000); 660) if (ret < 0) { 661) dev_err(priv->dev, "poll timeout\n"); 662) goto out; 663) } 664) 665) reg = MT7531_MDIO_CL45_READ | MT7531_MDIO_PHY_ADDR(port) | 666) MT7531_MDIO_DEV_ADDR(devad); 667) mt7530_mii_write(priv, MT7531_PHY_IAC, reg | MT7531_PHY_ACS_ST); 668) 669) ret = readx_poll_timeout(_mt7530_unlocked_read, &p, val, 670) !(val & MT7531_PHY_ACS_ST), 20, 100000); 671) if (ret < 0) { 672) dev_err(priv->dev, "poll timeout\n"); 673) goto out; 674) } 675) 676) ret = val & MT7531_MDIO_RW_DATA_MASK; 677) out: 678) mutex_unlock(&bus->mdio_lock); 679) 680) return ret; 681) } So here we can of course use regmap_read_poll_timeout and a bunch of readmap_write operations. However, each of them will individually acquire and release the mdio bus mutex while the current code acquires the lock at the top of the function and then uses unlocked operations. regmap currently doesn't offer any way to skip the locking and/or perform locking manually. regmap_read, regmap_write, regmap_update_bits, ... always acquire and release the lock on each operation. While in the above example this might just imply a performance hit, look here: 1958) static void 1959) mt7530_irq_bus_sync_unlock(struct irq_data *d) 1960) { 1961) struct mt7530_priv *priv = irq_data_get_irq_chip_data(d); 1962) 1963) mt7530_mii_write(priv, MT7530_SYS_INT_EN, priv->irq_enable); 1964) mutex_unlock(&priv->bus->mdio_lock); 1965) } So in this case the lock has previously been acquired and the register is written when releasing the lock. The above function is being used as .irq_bus_sync_unlock... Hence there are two ways to go: 1. Extend regmap to allow callers to perform locking manually and provide unlocked variants of all operations. This would allow us to then convert all the mt7530_{read,write,rmw}, mt7530_mii_{read,write}, ... calls to use regmap_* calls instead. 2. Use regmap to only provide locked access and keep open-coding the cases where unlocked access is used while manually acquiring the bus mutex in such cases, as it is the case now. 3. Use regmap to only provide unlocked access to beging with and keep taking care of locking in mt7530.c as it is now. This would be easy, but then all those ugly checks you criticized would still be needed. Ie. `if (priv->base_addr) ...` would of course go away, but all the other `if (priv->bus) ...` checks would have to stay. Let me know what you think and how I should proceed. Thank you! Best regards Daniel
Powered by blists - more mailing lists