* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-11 23:53 ` [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver Chris Packham
@ 2024-12-12 7:41 ` Andrew Lunn
2024-12-12 8:18 ` Christophe JAILLET
` (2 subsequent siblings)
3 siblings, 0 replies; 16+ messages in thread
From: Andrew Lunn @ 2024-12-12 7:41 UTC (permalink / raw)
To: Chris Packham
Cc: lee, robh, krzk+dt, conor+dt, andrew+netdev, davem, edumazet,
kuba, pabeni, tsbogend, hkallweit1, linux, markus.stockhausen,
devicetree, linux-kernel, netdev, linux-mips
> + /* get_phy_c45_ids() will stop the mdio bus scan if we return an error
> + * here. So even though the SMI controller indicates an error for an
> + * absent device don't proagate it here.
> + */
> + //if (val & BIT(25)) {
> + // err = -ENODEV;
> + // return err;
> + //}
Please don't leave committed out code. It should either be needed, and
uncommented, or deleted.
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> + u32 port_addr[5] = { };
> + u32 poll_sel[2] = { 0, 0 };
> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> + int i, err;
> +
> + for (i = 0; i < MAX_PORTS; i++) {
> + int pos;
> +
> + if (priv->smi_bus[i] > 3)
> + continue;
> +
> + pos = (i % 6) * 5;
> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
> +
> + pos = (i % 16) * 2;
> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> + }
> +
> + for (i = 0; i < MAX_SMI_BUSSES; i++) {
> + if (priv->smi_bus_isc45[i]) {
> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> + }
> + }
Please could you add some comments because i don't understand this at
all.
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> + port_addr, 5);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> + poll_sel, 2);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
> + glb_ctrl_mask, glb_ctrl_val);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct realtek_mdio_priv *priv;
> + struct fwnode_handle *child;
> + struct mii_bus *bus;
> + int err;
> +
> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "Reaktek Switch MDIO Bus";
> + bus->read_c45 = realtek_mdio_read_c45;
> + bus->write_c45 = realtek_mdio_write_c45;
> + bus->parent = dev;
> + priv = bus->priv;
> +
> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
> + if (err)
> + return err;
> +
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> + device_for_each_child_node(dev, child) {
> + u32 pn, smi_addr[2];
> +
> + err = fwnode_property_read_u32(child, "reg", &pn);
> + if (err)
> + return err;
> +
> + if (pn > MAX_PORTS)
> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> + if (err) {
> + smi_addr[0] = 0;
> + smi_addr[1] = pn;
> + }
Would returning -EINVAL be better here, since it indicates a bug in
the device tree?
> +
> + if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> + priv->smi_bus_isc45[smi_addr[0]] = true;
If it is not C45 then what? I don't see any support for C22.
Andrew
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-11 23:53 ` [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver Chris Packham
2024-12-12 7:41 ` Andrew Lunn
@ 2024-12-12 8:18 ` Christophe JAILLET
2024-12-12 9:59 ` Russell King (Oracle)
2024-12-13 19:59 ` Simon Horman
3 siblings, 0 replies; 16+ messages in thread
From: Christophe JAILLET @ 2024-12-12 8:18 UTC (permalink / raw)
To: Chris Packham, lee, robh, krzk+dt, conor+dt, andrew+netdev, davem,
edumazet, kuba, pabeni, tsbogend, hkallweit1, linux,
markus.stockhausen
Cc: devicetree, linux-kernel, netdev, linux-mips
Le 12/12/2024 à 00:53, Chris Packham a écrit :
> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> switches with integrated SoC. There are 4 physical SMI interfaces on the
> RTL9300 but access is done using the switch ports so a single MDIO bus
> is presented to the rest of the system.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
...
> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> + PHY_CTRL_RWOP | PHY_CTRL_TYPE | PHY_CTRL_CMD);
> + if (err)
> + return err;
> +
> + err = regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> + val, !(val & PHY_CTRL_CMD), 10, 100);
> + if (err)
> + return err;
> +
> + if (val & PHY_CTRL_FAIL) {
> + err = -ENXIO;
> + return err;
Nitpick: return -ENXIO; and remove the { }
> + }
> +
> + return err;
Nitpick: return 0;
> +}
> +
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> + u32 port_addr[5] = { };
> + u32 poll_sel[2] = { 0, 0 };
Nitpick: Why {} in on case and {0,0} in the other one?
> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> + int i, err;
> +
> + for (i = 0; i < MAX_PORTS; i++) {
> + int pos;
> +
> + if (priv->smi_bus[i] > 3)
> + continue;
> +
> + pos = (i % 6) * 5;
> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
> +
> + pos = (i % 16) * 2;
> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> + }
...
CJ
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-11 23:53 ` [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver Chris Packham
2024-12-12 7:41 ` Andrew Lunn
2024-12-12 8:18 ` Christophe JAILLET
@ 2024-12-12 9:59 ` Russell King (Oracle)
2024-12-13 0:51 ` Chris Packham
2024-12-13 19:59 ` Simon Horman
3 siblings, 1 reply; 16+ messages in thread
From: Russell King (Oracle) @ 2024-12-12 9:59 UTC (permalink / raw)
To: Chris Packham
Cc: lee, robh, krzk+dt, conor+dt, andrew+netdev, davem, edumazet,
kuba, pabeni, tsbogend, hkallweit1, markus.stockhausen,
devicetree, linux-kernel, netdev, linux-mips
On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham wrote:
> +#define SMI_GLB_CTRL 0x000
> +#define GLB_CTRL_INTF_SEL(intf) BIT(16 + (intf))
> +#define SMI_PORT0_15_POLLING_SEL 0x008
> +#define SMI_ACCESS_PHY_CTRL_0 0x170
> +#define SMI_ACCESS_PHY_CTRL_1 0x174
> +#define PHY_CTRL_RWOP BIT(2)
Presumably, reading the code, this bit is set when writing?
> +#define PHY_CTRL_TYPE BIT(1)
Presumably, reading the code, this bit indicates we want to use clause
45?
> +#define PHY_CTRL_CMD BIT(0)
> +#define PHY_CTRL_FAIL BIT(25)
> +#define SMI_ACCESS_PHY_CTRL_2 0x178
> +#define SMI_ACCESS_PHY_CTRL_3 0x17c
> +#define SMI_PORT0_5_ADDR_CTRL 0x180
> +
> +#define MAX_PORTS 32
> +#define MAX_SMI_BUSSES 4
> +
> +struct realtek_mdio_priv {
> + struct regmap *regmap;
> + u8 smi_bus[MAX_PORTS];
> + u8 smi_addr[MAX_PORTS];
> + bool smi_bus_isc45[MAX_SMI_BUSSES];
Not sure about the support for !C45 - you appear to set this if you
find a PHY as a child of this device which has the PHY C45 compatible,
but as you don't populate the C22 MDIO bus operations, I'm not sure
how a C22 PHY can work.
> + u32 reg_base;
> +};
> +
> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
> +{
> + u32 val;
> +
> + return regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> + val, !(val & PHY_CTRL_CMD), 10, 500);
> +}
> +
> +static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
> +{
> + struct realtek_mdio_priv *priv = bus->priv;
> + u32 val;
> + int err;
> +
> + err = realtek_mdio_wait_ready(priv);
> + if (err)
> + return err;
> +
> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
> + if (err)
> + return err;
> +
> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
> + dev_addr << 16 | (regnum & 0xffff));
> + if (err)
> + return err;
> +
> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
> + PHY_CTRL_TYPE | PHY_CTRL_CMD);
> + if (err)
> + return err;
Maybe consider using a local variable for "regmap" and "reg_base" to
reduce the line length/wrapping?
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> + u32 port_addr[5] = { };
> + u32 poll_sel[2] = { 0, 0 };
> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
Please use reverse Christmas tree order.
> + int i, err;
> +
> + for (i = 0; i < MAX_PORTS; i++) {
> + int pos;
> +
> + if (priv->smi_bus[i] > 3)
> + continue;
> +
> + pos = (i % 6) * 5;
> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
s/ / /
> +
> + pos = (i % 16) * 2;
> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> + }
> +
> + for (i = 0; i < MAX_SMI_BUSSES; i++) {
> + if (priv->smi_bus_isc45[i]) {
> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> + }
> + }
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> + port_addr, 5);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> + poll_sel, 2);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
> + glb_ctrl_mask, glb_ctrl_val);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct realtek_mdio_priv *priv;
> + struct fwnode_handle *child;
> + struct mii_bus *bus;
> + int err;
> +
> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "Reaktek Switch MDIO Bus";
> + bus->read_c45 = realtek_mdio_read_c45;
> + bus->write_c45 = realtek_mdio_write_c45;
> + bus->parent = dev;
> + priv = bus->priv;
> +
> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
> + if (err)
> + return err;
> +
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> + device_for_each_child_node(dev, child) {
> + u32 pn, smi_addr[2];
> +
> + err = fwnode_property_read_u32(child, "reg", &pn);
> + if (err)
> + return err;
> +
> + if (pn > MAX_PORTS)
> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
You validate the port number.
> +
> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> + if (err) {
> + smi_addr[0] = 0;
> + smi_addr[1] = pn;
> + }
You don't validate the "smi_addr", so:
realtek,smi-address = <4, ...>;
would silently overflow priv->smi_bus_isc45. However, I haven't checked
whether the binding would warn about this.
Thanks.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-12 9:59 ` Russell King (Oracle)
@ 2024-12-13 0:51 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-12-13 0:51 UTC (permalink / raw)
To: Russell King (Oracle)
Cc: lee, robh, krzk+dt, conor+dt, andrew+netdev, davem, edumazet,
kuba, pabeni, tsbogend, hkallweit1, markus.stockhausen,
devicetree, linux-kernel, netdev, linux-mips
Hi Russell,
On 12/12/2024 22:59, Russell King (Oracle) wrote:
> On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham wrote:
>> +#define SMI_GLB_CTRL 0x000
>> +#define GLB_CTRL_INTF_SEL(intf) BIT(16 + (intf))
>> +#define SMI_PORT0_15_POLLING_SEL 0x008
>> +#define SMI_ACCESS_PHY_CTRL_0 0x170
>> +#define SMI_ACCESS_PHY_CTRL_1 0x174
>> +#define PHY_CTRL_RWOP BIT(2)
> Presumably, reading the code, this bit is set when writing?
Correct. I've tried to use the bit field names from the datasheet. RWOP
0=read, 1=write.
>> +#define PHY_CTRL_TYPE BIT(1)
> Presumably, reading the code, this bit indicates we want to use clause
> 45?
Yes. Technically the datasheet says 0=normal register, 1=MMD register.
>> +#define PHY_CTRL_CMD BIT(0)
>> +#define PHY_CTRL_FAIL BIT(25)
>> +#define SMI_ACCESS_PHY_CTRL_2 0x178
>> +#define SMI_ACCESS_PHY_CTRL_3 0x17c
>> +#define SMI_PORT0_5_ADDR_CTRL 0x180
>> +
>> +#define MAX_PORTS 32
>> +#define MAX_SMI_BUSSES 4
>> +
>> +struct realtek_mdio_priv {
>> + struct regmap *regmap;
>> + u8 smi_bus[MAX_PORTS];
>> + u8 smi_addr[MAX_PORTS];
>> + bool smi_bus_isc45[MAX_SMI_BUSSES];
> Not sure about the support for !C45 - you appear to set this if you
> find a PHY as a child of this device which has the PHY C45 compatible,
> but as you don't populate the C22 MDIO bus operations, I'm not sure
> how a C22 PHY can work.
Oops, yes I forgot to come back to C22. Most of the hardware I have
access to uses C45 so that's been my main test setup. I'll include C22
support in v2.
>> + u32 reg_base;
>> +};
>> +
>> +static int realtek_mdio_wait_ready(struct realtek_mdio_priv *priv)
>> +{
>> + u32 val;
>> +
>> + return regmap_read_poll_timeout(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
>> + val, !(val & PHY_CTRL_CMD), 10, 500);
>> +}
>> +
>> +static int realtek_mdio_read_c45(struct mii_bus *bus, int phy_id, int dev_addr, int regnum)
>> +{
>> + struct realtek_mdio_priv *priv = bus->priv;
>> + u32 val;
>> + int err;
>> +
>> + err = realtek_mdio_wait_ready(priv);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_2, phy_id << 16);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_3,
>> + dev_addr << 16 | (regnum & 0xffff));
>> + if (err)
>> + return err;
>> +
>> + err = regmap_write(priv->regmap, priv->reg_base + SMI_ACCESS_PHY_CTRL_1,
>> + PHY_CTRL_TYPE | PHY_CTRL_CMD);
>> + if (err)
>> + return err;
> Maybe consider using a local variable for "regmap" and "reg_base" to
> reduce the line length/wrapping?
Ok
>> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
>> +{
>> + u32 port_addr[5] = { };
>> + u32 poll_sel[2] = { 0, 0 };
>> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> Please use reverse Christmas tree order.
Ok.
>> + int i, err;
>> +
>> + for (i = 0; i < MAX_PORTS; i++) {
>> + int pos;
>> +
>> + if (priv->smi_bus[i] > 3)
>> + continue;
>> +
>> + pos = (i % 6) * 5;
>> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
> s/ / /
Ok.
>> +
>> + pos = (i % 16) * 2;
>> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
>> + }
>> +
>> + for (i = 0; i < MAX_SMI_BUSSES; i++) {
>> + if (priv->smi_bus_isc45[i]) {
>> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
>> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>> + }
>> + }
>> +
>> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
>> + port_addr, 5);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
>> + poll_sel, 2);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
>> + glb_ctrl_mask, glb_ctrl_val);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> +static int realtek_mdiobus_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct realtek_mdio_priv *priv;
>> + struct fwnode_handle *child;
>> + struct mii_bus *bus;
>> + int err;
>> +
>> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
>> + if (!bus)
>> + return -ENOMEM;
>> +
>> + bus->name = "Reaktek Switch MDIO Bus";
>> + bus->read_c45 = realtek_mdio_read_c45;
>> + bus->write_c45 = realtek_mdio_write_c45;
>> + bus->parent = dev;
>> + priv = bus->priv;
>> +
>> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
>> + if (IS_ERR(priv->regmap))
>> + return PTR_ERR(priv->regmap);
>> +
>> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
>> + if (err)
>> + return err;
>> +
>> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
>> +
>> + device_for_each_child_node(dev, child) {
>> + u32 pn, smi_addr[2];
>> +
>> + err = fwnode_property_read_u32(child, "reg", &pn);
>> + if (err)
>> + return err;
>> +
>> + if (pn > MAX_PORTS)
>> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> You validate the port number.
>
>> +
>> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
>> + if (err) {
>> + smi_addr[0] = 0;
>> + smi_addr[1] = pn;
>> + }
> You don't validate the "smi_addr", so:
>
> realtek,smi-address = <4, ...>;
>
> would silently overflow priv->smi_bus_isc45. However, I haven't checked
> whether the binding would warn about this.
I'll make sure the smi bus and phy address are within an appropriate range.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-11 23:53 ` [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver Chris Packham
` (2 preceding siblings ...)
2024-12-12 9:59 ` Russell King (Oracle)
@ 2024-12-13 19:59 ` Simon Horman
2024-12-15 20:27 ` Chris Packham
3 siblings, 1 reply; 16+ messages in thread
From: Simon Horman @ 2024-12-13 19:59 UTC (permalink / raw)
To: Chris Packham
Cc: lee, robh, krzk+dt, conor+dt, andrew+netdev, davem, edumazet,
kuba, pabeni, tsbogend, hkallweit1, linux, markus.stockhausen,
devicetree, linux-kernel, netdev, linux-mips
On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham wrote:
> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
> switches with integrated SoC. There are 4 physical SMI interfaces on the
> RTL9300 but access is done using the switch ports so a single MDIO bus
> is presented to the rest of the system.
>
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
...
> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
...
> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
> +{
> + u32 port_addr[5] = { };
> + u32 poll_sel[2] = { 0, 0 };
> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
> + int i, err;
> +
> + for (i = 0; i < MAX_PORTS; i++) {
> + int pos;
> +
> + if (priv->smi_bus[i] > 3)
> + continue;
> +
> + pos = (i % 6) * 5;
> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
Hi Chris,
The maximum index of port_addr accessed above is
(MAX_PORTS - 1) / 6 = (32 - 1) / 6 = 5.
But port_addr only has five elements (maximum index of 4).
So this will overflow.
Flagged by Smatch.
> +
> + pos = (i % 16) * 2;
> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
> + }
> +
> + for (i = 0; i < MAX_SMI_BUSSES; i++) {
> + if (priv->smi_bus_isc45[i]) {
> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
> + }
> + }
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
> + port_addr, 5);
> + if (err)
> + return err;
> +
> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
> + poll_sel, 2);
> + if (err)
> + return err;
> +
> + err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
> + glb_ctrl_mask, glb_ctrl_val);
> + if (err)
> + return err;
> +
> + return 0;
> +}
> +
> +static int realtek_mdiobus_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct realtek_mdio_priv *priv;
> + struct fwnode_handle *child;
> + struct mii_bus *bus;
> + int err;
> +
> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
> + if (!bus)
> + return -ENOMEM;
> +
> + bus->name = "Reaktek Switch MDIO Bus";
> + bus->read_c45 = realtek_mdio_read_c45;
> + bus->write_c45 = realtek_mdio_write_c45;
> + bus->parent = dev;
> + priv = bus->priv;
> +
> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
> + if (IS_ERR(priv->regmap))
> + return PTR_ERR(priv->regmap);
> +
> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
> + if (err)
> + return err;
> +
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
> +
> + device_for_each_child_node(dev, child) {
> + u32 pn, smi_addr[2];
> +
> + err = fwnode_property_read_u32(child, "reg", &pn);
> + if (err)
> + return err;
> +
> + if (pn > MAX_PORTS)
> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
> +
> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
> + if (err) {
> + smi_addr[0] = 0;
> + smi_addr[1] = pn;
> + }
> +
> + if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
> + priv->smi_bus_isc45[smi_addr[0]] = true;
> +
> + priv->smi_bus[pn] = smi_addr[0];
> + priv->smi_addr[pn] = smi_addr[1];
The condition about 15 lines above ensures that the maximum value of pn
is MAX_PORTS. But if this is the case then the above will overflow
both smi_bus and smi_addr as they each have MAX_PORTS elements
(maximum index of MAX_PORTS - 1).
I suspect the condition above should be updated to:
if (pn >= MAX_PORTS)
return ...
Also flagged by Smatch.
> + }
> +
> + err = realtek_mdiobus_init(priv);
> + if (err)
> + return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
> +
> + err = devm_of_mdiobus_register(dev, bus, dev->of_node);
> + if (err)
> + return dev_err_probe(dev, err, "cannot register MDIO bus\n");
> +
> + return 0;
> +}
...
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH 4/4] net: mdio: Add RTL9300 MDIO driver
2024-12-13 19:59 ` Simon Horman
@ 2024-12-15 20:27 ` Chris Packham
0 siblings, 0 replies; 16+ messages in thread
From: Chris Packham @ 2024-12-15 20:27 UTC (permalink / raw)
To: Simon Horman
Cc: lee, robh, krzk+dt, conor+dt, andrew+netdev, davem, edumazet,
kuba, pabeni, tsbogend, hkallweit1, linux, markus.stockhausen,
devicetree, linux-kernel, netdev, linux-mips
On 14/12/2024 08:59, Simon Horman wrote:
> On Thu, Dec 12, 2024 at 12:53:42PM +1300, Chris Packham wrote:
>> Add a driver for the MDIO controller on the RTL9300 family of Ethernet
>> switches with integrated SoC. There are 4 physical SMI interfaces on the
>> RTL9300 but access is done using the switch ports so a single MDIO bus
>> is presented to the rest of the system.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ...
>
>> diff --git a/drivers/net/mdio/mdio-realtek-rtl.c b/drivers/net/mdio/mdio-realtek-rtl.c
> ...
>
>> +static int realtek_mdiobus_init(struct realtek_mdio_priv *priv)
>> +{
>> + u32 port_addr[5] = { };
>> + u32 poll_sel[2] = { 0, 0 };
>> + u32 glb_ctrl_mask = 0, glb_ctrl_val = 0;
>> + int i, err;
>> +
>> + for (i = 0; i < MAX_PORTS; i++) {
>> + int pos;
>> +
>> + if (priv->smi_bus[i] > 3)
>> + continue;
>> +
>> + pos = (i % 6) * 5;
>> + port_addr[i / 6] |= priv->smi_addr[i] << pos;
> Hi Chris,
>
> The maximum index of port_addr accessed above is
> (MAX_PORTS - 1) / 6 = (32 - 1) / 6 = 5.
> But port_addr only has five elements (maximum index of 4).
> So this will overflow.
>
> Flagged by Smatch.
Drat. It's more complicated than that. The maximum number of _physical_
ports on the RTL9300 is 28 (i.e. 0-27). In some other places port 28 is
used to mean the CPU port (i.e. the DMA interface) and in others it just
uses 32 possible ports because that makes the tables entries align
nicely. Since this is just the MDIO interface, setting MAX_PORTS to 28
here seems like the sensible thing to do.
>> +
>> + pos = (i % 16) * 2;
>> + poll_sel[i / 16] |= priv->smi_bus[i] << pos;
>> + }
>> +
>> + for (i = 0; i < MAX_SMI_BUSSES; i++) {
>> + if (priv->smi_bus_isc45[i]) {
>> + glb_ctrl_mask |= GLB_CTRL_INTF_SEL(i);
>> + glb_ctrl_val |= GLB_CTRL_INTF_SEL(i);
>> + }
>> + }
>> +
>> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_5_ADDR_CTRL,
>> + port_addr, 5);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_bulk_write(priv->regmap, priv->reg_base + SMI_PORT0_15_POLLING_SEL,
>> + poll_sel, 2);
>> + if (err)
>> + return err;
>> +
>> + err = regmap_update_bits(priv->regmap, priv->reg_base + SMI_GLB_CTRL,
>> + glb_ctrl_mask, glb_ctrl_val);
>> + if (err)
>> + return err;
>> +
>> + return 0;
>> +}
>> +
>> +static int realtek_mdiobus_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct realtek_mdio_priv *priv;
>> + struct fwnode_handle *child;
>> + struct mii_bus *bus;
>> + int err;
>> +
>> + bus = devm_mdiobus_alloc_size(dev, sizeof(*priv));
>> + if (!bus)
>> + return -ENOMEM;
>> +
>> + bus->name = "Reaktek Switch MDIO Bus";
>> + bus->read_c45 = realtek_mdio_read_c45;
>> + bus->write_c45 = realtek_mdio_write_c45;
>> + bus->parent = dev;
>> + priv = bus->priv;
>> +
>> + priv->regmap = syscon_node_to_regmap(dev->parent->of_node);
>> + if (IS_ERR(priv->regmap))
>> + return PTR_ERR(priv->regmap);
>> +
>> + err = device_property_read_u32(dev, "reg", &priv->reg_base);
>> + if (err)
>> + return err;
>> +
>> + snprintf(bus->id, MII_BUS_ID_SIZE, "%s", dev_name(dev));
>> +
>> + device_for_each_child_node(dev, child) {
>> + u32 pn, smi_addr[2];
>> +
>> + err = fwnode_property_read_u32(child, "reg", &pn);
>> + if (err)
>> + return err;
>> +
>> + if (pn > MAX_PORTS)
>> + return dev_err_probe(dev, -EINVAL, "illegal port number %d\n", pn);
>> +
>> + err = fwnode_property_read_u32_array(child, "realtek,smi-address", smi_addr, 2);
>> + if (err) {
>> + smi_addr[0] = 0;
>> + smi_addr[1] = pn;
>> + }
>> +
>> + if (fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45"))
>> + priv->smi_bus_isc45[smi_addr[0]] = true;
>> +
>> + priv->smi_bus[pn] = smi_addr[0];
>> + priv->smi_addr[pn] = smi_addr[1];
> The condition about 15 lines above ensures that the maximum value of pn
> is MAX_PORTS. But if this is the case then the above will overflow
> both smi_bus and smi_addr as they each have MAX_PORTS elements
> (maximum index of MAX_PORTS - 1).
>
> I suspect the condition above should be updated to:
>
> if (pn >= MAX_PORTS)
> return ...
>
> Also flagged by Smatch.
Good catch thanks. Will fix in v2.
>> + }
>> +
>> + err = realtek_mdiobus_init(priv);
>> + if (err)
>> + return dev_err_probe(dev, err, "failed to initialise MDIO bus controller\n");
>> +
>> + err = devm_of_mdiobus_register(dev, bus, dev->of_node);
>> + if (err)
>> + return dev_err_probe(dev, err, "cannot register MDIO bus\n");
>> +
>> + return 0;
>> +}
> ...
^ permalink raw reply [flat|nested] 16+ messages in thread