[PATCH] net: dsa: qca8k: fix kernel panic with legacy mdio mapping

When the mdio legacy mapping is used the mii_bus priv registred by DSA
refer to the dsa switch struct instead of the qca8k_priv struct and
cause a kernel panic. Create dedicated function when the internal
dedicated mdio driver is used to proprely handle the 2 different
implementation.

Fixes: 759bafb8a322 ("net: dsa: qca8k: add support for internal phy and internal mdio")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 1f63f50f73f1..a701323daf72 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c

@@ -643,10 +643,8 @@ qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
 }
 
 static int
-qca8k_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
 {
-	struct qca8k_priv *priv = salve_bus->priv;
-	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 	int ret;

@@ -682,10 +680,8 @@ qca8k_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
 }
 
 static int
-qca8k_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
+qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
 {
-	struct qca8k_priv *priv = salve_bus->priv;
-	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 	int ret;

@@ -726,6 +722,24 @@ qca8k_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
 	return ret;
 }
 
+static int
+qca8k_internal_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+{
+	struct qca8k_priv *priv = salve_bus->priv;
+	struct mii_bus *bus = priv->bus;
+
+	return qca8k_mdio_write(bus, phy, regnum, data);
+}
+
+static int
+qca8k_internal_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
+{
+	struct qca8k_priv *priv = salve_bus->priv;
+	struct mii_bus *bus = priv->bus;
+
+	return qca8k_mdio_read(bus, phy, regnum);
+}
+
 static int
 qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 {

@@ -775,8 +789,8 @@ qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
 
 	bus->priv = (void *)priv;
 	bus->name = "qca8k slave mii";
-	bus->read = qca8k_mdio_read;
-	bus->write = qca8k_mdio_write;
+	bus->read = qca8k_internal_mdio_read;
+	bus->write = qca8k_internal_mdio_write;
 	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
 		 ds->index);
 

-- 
2.32.0

On 9/11/2021 08:07, Ansuel Smith wrote:

When the mdio legacy mapping is used the mii_bus priv registred by DSA

typo: registered

refer to the dsa switch struct instead of the qca8k_priv struct and
cause a kernel panic.

typo: causes

Create dedicated function when the internal
dedicated mdio driver is used to proprely handle the 2 different
implementation.

typo: properly

You are only moving code here but while at it, mind fixing that typo?

-- 
Florian

On Sat, Sep 11, 2021 at 08:31:51AM -0700, Florian Fainelli wrote:

On 9/11/2021 08:07, Ansuel Smith wrote:

quoted

When the mdio legacy mapping is used the mii_bus priv registred by DSA

typo: registered

quoted

refer to the dsa switch struct instead of the qca8k_priv struct and
cause a kernel panic.

typo: causes

quoted

Create dedicated function when the internal
dedicated mdio driver is used to proprely handle the 2 different
implementation.

typo: properly

quoted

Fixes: 759bafb8a322 ("net: dsa: qca8k: add support for internal phy and internal mdio")
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
  drivers/net/dsa/qca8k.c | 30 ++++++++++++++++++++++--------
  1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 1f63f50f73f1..a701323daf72 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c

@@ -643,10 +643,8 @@ qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
  }
  static int
-qca8k_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
  {
-	struct qca8k_priv *priv = salve_bus->priv;
-	struct mii_bus *bus = priv->bus;
  	u16 r1, r2, page;
  	u32 val;
  	int ret;

@@ -682,10 +680,8 @@ qca8k_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
  }
  static int
-qca8k_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
+qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
  {
-	struct qca8k_priv *priv = salve_bus->priv;
-	struct mii_bus *bus = priv->bus;
  	u16 r1, r2, page;
  	u32 val;
  	int ret;

@@ -726,6 +722,24 @@ qca8k_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
  	return ret;
  }
+static int
+qca8k_internal_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+{
+	struct qca8k_priv *priv = salve_bus->priv;

You are only moving code here but while at it, mind fixing that typo?

I think I didn't understand what you mean here.
Sure I will fix the typo and sorry about it.
Aside from that anything wrong with the 2 new function or there is a
better fix that I can't think of.

quoted

+	struct mii_bus *bus = priv->bus;
+
+	return qca8k_mdio_write(bus, phy, regnum, data);
+}
+
+static int
+qca8k_internal_mdio_read(struct mii_bus *salve_bus, int phy, int regnum)
+{
+	struct qca8k_priv *priv = salve_bus->priv;
+	struct mii_bus *bus = priv->bus;
+
+	return qca8k_mdio_read(bus, phy, regnum);
+}
+
  static int
  qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
  {

@@ -775,8 +789,8 @@ qca8k_mdio_register(struct qca8k_priv *priv, struct device_node *mdio)
  	bus->priv = (void *)priv;
  	bus->name = "qca8k slave mii";
-	bus->read = qca8k_mdio_read;
-	bus->write = qca8k_mdio_write;
+	bus->read = qca8k_internal_mdio_read;
+	bus->write = qca8k_internal_mdio_write;
  	snprintf(bus->id, MII_BUS_ID_SIZE, "qca8k-%d",
  		 ds->index);

-- 
Florian

On Sat, Sep 11, 2021 at 05:36:40PM +0200, Ansuel Smith wrote:

quoted

quoted

+static int
+qca8k_internal_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+{
+	struct qca8k_priv *priv = salve_bus->priv;

You are only moving code here but while at it, mind fixing that typo?

I think I didn't understand what you mean here.
Sure I will fix the typo and sorry about it.
Aside from that anything wrong with the 2 new function or there is a
better fix that I can't think of.

"salve" is "hello" in Italian, and even though that is a greeting,
surely that is not what was meant, but rather "slave". So even though
that is less positive, it is at least a technical term with a clear
meaning, so I think Florian's request was to replace "salve" with
"slave" while you are moving the code anyway.

On Sat, Sep 11, 2021 at 06:40:55PM +0300, Vladimir Oltean wrote:

On Sat, Sep 11, 2021 at 05:36:40PM +0200, Ansuel Smith wrote:

quoted

quoted

quoted

+static int
+qca8k_internal_mdio_write(struct mii_bus *salve_bus, int phy, int regnum, u16 data)
+{
+	struct qca8k_priv *priv = salve_bus->priv;

You are only moving code here but while at it, mind fixing that typo?

I think I didn't understand what you mean here.
Sure I will fix the typo and sorry about it.
Aside from that anything wrong with the 2 new function or there is a
better fix that I can't think of.

"salve" is "hello" in Italian, and even though that is a greeting,
surely that is not what was meant, but rather "slave". So even though
that is less positive, it is at least a technical term with a clear
meaning, so I think Florian's request was to replace "salve" with
"slave" while you are moving the code anyway.

Ok this is embarrassing... Typo by vscode autocorrection... Fixing that
and sending v2. Again sorry.