| 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: <20230317094629.nryf6qkuxp4nisul@skbuf>
Date: Fri, 17 Mar 2023 11:46:29 +0200
From: Vladimir Oltean <vladimir.oltean@....com>
To: netdev@...r.kernel.org
Cc: Andrew Lunn <andrew@...n.ch>,
Florian Fainelli <f.fainelli@...il.com>,
"David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>,
Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>,
Woojung Huh <woojung.huh@...rochip.com>,
UNGLinuxDriver@...rochip.com,
Arun Ramadoss <arun.ramadoss@...rochip.com>,
Russell King <linux@...linux.org.uk>,
Marek Vasut <marex@...x.de>,
Oleksij Rempel <linux@...pel-privat.de>,
linux-kernel@...r.kernel.org
Subject: Re: [RFC/RFT PATCH net-next 2/4] net: dsa: microchip: partial
conversion to regfields API for KSZ8795 (WIP)
On Thu, Mar 16, 2023 at 06:12:48PM +0200, Vladimir Oltean wrote:
> During review, Russell King has pointed out that the current way in
> which the KSZ common driver performs register access is error-prone.
>
> Based on the KSZ9477 software model where we have the XMII Port Control
> 0 Register (at offset 0xN300) and XMII Port Control 1 Register
> (at offset 0xN301), this driver also holds P_XMII_CTRL_0 and P_XMII_CTRL_1.
>
> However, on some switches, fields accessed through P_XMII_CTRL_0 (for
> example P_MII_100MBIT_M or P_MII_DUPLEX_M) and fields accessed through
> P_XMII_CTRL_1 (for example P_MII_SEL_M) may end up accessing the same
> hardware register. Or at least, that was certainly the impression after
> commit
> https://patchwork.kernel.org/project/netdevbpf/patch/20230315231916.2998480-1-vladimir.oltean@nxp.com/
> (sorry, can't reference the sha1sum of an unmerged commit), because for
> ksz8795_regs[], P_XMII_CTRL_0 and P_XMII_CTRL_1 now have the same value.
>
> But the reality is far more interesting. Opening a KSZ8795 datasheet, I
> see that out of the register fields accessed via P_XMII_CTRL_0:
> - what the driver names P_MII_SEL_M *is* actually "GMII/MII Mode Select"
> (bit 2) of the Port 5 Interface Control 6, address 0x56 (all good here)
> - what the driver names P_MII_100MBIT_M is actually "Switch SW5-MII/RMII
> Speed" (bit 4) of the Global Control 4 register, address 0x06.
>
> That is a huge problem, because the driver cannot access this register
> for KSZ8795 in its current form, even if that register exists. This
> creates an even stronger motivation to try to do something to normalize
> the way in which this driver abstracts away register field movement from
> one switch family to another.
>
> As I had proposed in that thread, reg_fields from regmap propose to
> solve exactly this problem. This patch contains a COMPLETELY UNTESTED
> rework of the driver, so that accesses done through the following
> registers (for demonstration purposes):
> - REG_IND_BYTE - a global register
> - REG_IND_CTRL_0 - another global register
> - P_LOCAL_CTRL - a port register
> - P_FORCE_CTRL - another port register
> - P_XMII_CTRL_0 and P_XMII_CTRL_1 - either port register, or global
> registers, depending on which manual you read!
>
> are converted to the regfields API.
>
> !! WARNING !! I only attempted to add a ksz_reg_fields structure for
> KSZ8795. The other switch families will currently crash!
>
> For easier partial migration, I have renamed the "REG_" or "P_" prefixes
> of the enum ksz_regs values into a common "RF_" (for reg field) prefix
> for a new enum type: ksz_rf. Eventually, enum ksz_regs, as well as the
> masks, should disappear completely, being replaced by reg fields.
>
> Link: https://lore.kernel.org/netdev/Y%2FYPfxg8Ackb8zmW@shell.armlinux.org.uk/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> diff --git a/drivers/net/dsa/microchip/ksz8863_smi.c b/drivers/net/dsa/microchip/ksz8863_smi.c
> index 5871f27451cb..f9d22a444146 100644
> --- a/drivers/net/dsa/microchip/ksz8863_smi.c
> +++ b/drivers/net/dsa/microchip/ksz8863_smi.c
> @@ -136,11 +136,16 @@ static const struct regmap_config ksz8863_regmap_config[] = {
>
> static int ksz8863_smi_probe(struct mdio_device *mdiodev)
> {
> + const struct ksz_chip_data *chip;
> struct regmap_config rc;
> struct ksz_device *dev;
> int ret;
> int i;
>
> + chip = device_get_match_data(ddev);
s/ddev/&mdiodev->dev/
> + if (!chip)
> + return -EINVAL;
> +
> dev = ksz_switch_alloc(&mdiodev->dev, mdiodev);
> if (!dev)
> return -ENOMEM;
> @@ -159,6 +164,10 @@ static int ksz8863_smi_probe(struct mdio_device *mdiodev)
> }
> }
>
> + ret = ksz_regfields_init(dev, chip);
> + if (ret)
> + return ret;
> +
> if (mdiodev->dev.platform_data)
> dev->pdata = mdiodev->dev.platform_data;
>
> diff --git a/drivers/net/dsa/microchip/ksz9477_i2c.c b/drivers/net/dsa/microchip/ksz9477_i2c.c
> index 497be833f707..2cbd76aed974 100644
> --- a/drivers/net/dsa/microchip/ksz9477_i2c.c
> +++ b/drivers/net/dsa/microchip/ksz9477_i2c.c
> @@ -16,10 +16,15 @@ KSZ_REGMAP_TABLE(ksz9477, not_used, 16, 0, 0);
>
> static int ksz9477_i2c_probe(struct i2c_client *i2c)
> {
> + const struct ksz_chip_data *chip;
> struct regmap_config rc;
> struct ksz_device *dev;
> int i, ret;
>
> + chip = device_get_match_data(ddev);
s/ddev/&i2c->dev/
> + if (!chip)
> + return -EINVAL;
> +
> dev = ksz_switch_alloc(&i2c->dev, i2c);
> if (!dev)
> return -ENOMEM;
> @@ -35,6 +40,10 @@ static int ksz9477_i2c_probe(struct i2c_client *i2c)
> }
> }
>
> + ret = ksz_regfields_init(dev, chip);
> + if (ret)
> + return ret;
> +
> if (i2c->dev.platform_data)
> dev->pdata = i2c->dev.platform_data;
>
Powered by blists - more mailing lists