[PATCH net v1 1/2] net: dsa: microchip: fix devicetree parsing of cpu node

From: Sven Van Asbroeck <redacted>

On the ksz8795, if the devicetree contains a cpu node,
devicetree parsing fails and the whole driver errors out.

Fix the devicetree parsing code by making it use the
correct number of ports.

Fixes: 912aae27c6af ("net: dsa: microchip: really look for phy-mode in port nodes")
Tested-by: Sven Van Asbroeck <redacted> # ksz8795
Signed-off-by: Sven Van Asbroeck <redacted>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 905b2032fa42

To: Woojung Huh <woojung.huh@microchip.com>
To: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
To: Andrew Lunn <andrew@lunn.ch>
To: Vivien Didelot <redacted>
To: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Helmut Grohne <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index e5f047129b15..17b804c44c53 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c

@@ -431,7 +431,7 @@ int ksz_switch_register(struct ksz_device *dev,
 				if (of_property_read_u32(port, "reg",
 							 &port_num))
 					continue;
-				if (port_num >= dev->port_cnt)
+				if (port_num >= dev->ds->num_ports)
 					return -EINVAL;
 				of_get_phy_mode(port,
 						&dev->ports[port_num].interface);

-- 
2.17.1

From: Sven Van Asbroeck <redacted>

Port counts in microchip dsa drivers can be quite confusing:
on the ksz8795, ksz_chip_data->port_cnt excludes the cpu port,
yet on the ksz9477, it includes the cpu port.

Add comments to document this situation explicitly.

Fixes: 912aae27c6af ("net: dsa: microchip: really look for phy-mode in port nodes")
Signed-off-by: Sven Van Asbroeck <redacted>
---

Tree: git://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git # 905b2032fa42

To: Woojung Huh <woojung.huh@microchip.com>
To: Microchip Linux Driver Support <UNGLinuxDriver@microchip.com>
To: Andrew Lunn <andrew@lunn.ch>
To: Vivien Didelot <redacted>
To: Florian Fainelli <f.fainelli@gmail.com>
To: Vladimir Oltean <olteanv@gmail.com>
To: "David S. Miller" <davem@davemloft.net>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Helmut Grohne <redacted>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

 drivers/net/dsa/microchip/ksz8795.c | 12 +++++++++---
 drivers/net/dsa/microchip/ksz9477.c | 16 ++++++++++++----
 2 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz8795.c b/drivers/net/dsa/microchip/ksz8795.c
index f5779e152377..99183347053f 100644
--- a/drivers/net/dsa/microchip/ksz8795.c
+++ b/drivers/net/dsa/microchip/ksz8795.c

@@ -1190,7 +1190,9 @@ static const struct ksz_chip_data ksz8795_switch_chips[] = {
 		.num_alus = 0,
 		.num_statics = 8,
 		.cpu_ports = 0x10,	/* can be configured as cpu port */
-		.port_cnt = 4,		/* total physical port count */
+		.port_cnt = 4,		/* total physical port count, excluding
+					 * the cpu port
+					 */
 	},
 	{
 		.chip_id = 0x8794,

@@ -1199,7 +1201,9 @@ static const struct ksz_chip_data ksz8795_switch_chips[] = {
 		.num_alus = 0,
 		.num_statics = 8,
 		.cpu_ports = 0x10,	/* can be configured as cpu port */
-		.port_cnt = 3,		/* total physical port count */
+		.port_cnt = 3,		/* total physical port count, excluding
+					 * the cpu port
+					 */
 	},
 	{
 		.chip_id = 0x8765,

@@ -1208,7 +1212,9 @@ static const struct ksz_chip_data ksz8795_switch_chips[] = {
 		.num_alus = 0,
 		.num_statics = 8,
 		.cpu_ports = 0x10,	/* can be configured as cpu port */
-		.port_cnt = 4,		/* total physical port count */
+		.port_cnt = 4,		/* total physical port count, excluding
+					 * the cpu port
+					 */
 	},
 };
 

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 2f5506ac7d19..e56122ffd495 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c

@@ -1482,7 +1482,9 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 		.num_alus = 4096,
 		.num_statics = 16,
 		.cpu_ports = 0x7F,	/* can be configured as cpu port */
-		.port_cnt = 7,		/* total physical port count */
+		.port_cnt = 7,		/* total physical port count, including
+					 * the cpu port
+					 */
 		.phy_errata_9477 = true,
 	},
 	{

@@ -1492,7 +1494,9 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 		.num_alus = 4096,
 		.num_statics = 16,
 		.cpu_ports = 0x7F,	/* can be configured as cpu port */
-		.port_cnt = 7,		/* total physical port count */
+		.port_cnt = 7,		/* total physical port count, including
+					 * the cpu port
+					 */
 		.phy_errata_9477 = true,
 	},
 	{

@@ -1502,7 +1506,9 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 		.num_alus = 4096,
 		.num_statics = 16,
 		.cpu_ports = 0x07,	/* can be configured as cpu port */
-		.port_cnt = 3,		/* total port count */
+		.port_cnt = 3,		/* total physical port count, including
+					 * the cpu port
+					 */
 	},
 	{
 		.chip_id = 0x00956700,

@@ -1511,7 +1517,9 @@ static const struct ksz_chip_data ksz9477_switch_chips[] = {
 		.num_alus = 4096,
 		.num_statics = 16,
 		.cpu_ports = 0x7F,	/* can be configured as cpu port */
-		.port_cnt = 7,		/* total physical port count */
+		.port_cnt = 7,		/* total physical port count, including
+					 * the cpu port
+					 */
 	},
 };
 

-- 
2.17.1

On Sat, Dec 05, 2020 at 10:28:14AM -0500, Sven Van Asbroeck wrote:

From: Sven Van Asbroeck <redacted>

Port counts in microchip dsa drivers can be quite confusing:
on the ksz8795, ksz_chip_data->port_cnt excludes the cpu port,
yet on the ksz9477, it includes the cpu port.

Add comments to document this situation explicitly.

Rather than document it, we should make it uniform. Unless there is a
valid reason to require them to mean different things.

      Andrew

On Tue, 8 Dec 2020 00:31:16 +0100 Andrew Lunn wrote:

On Sat, Dec 05, 2020 at 10:28:14AM -0500, Sven Van Asbroeck wrote:

quoted

From: Sven Van Asbroeck <redacted>

Port counts in microchip dsa drivers can be quite confusing:
on the ksz8795, ksz_chip_data->port_cnt excludes the cpu port,
yet on the ksz9477, it includes the cpu port.

Add comments to document this situation explicitly.  

Rather than document it, we should make it uniform. Unless there is a
valid reason to require them to mean different things.

Agreed.

I wonder if we should make this effort target net-next.

My concern is that for the 3 port switch the cpu_ports variable is set
to 0x10, the same as for the 4 port one. Which makes me worried that 
if we just allow the "+ 1" - the CPU port will not actually hit the
register offsets its supposed to on 3 port platforms.

Since configuring the CPU port never worked here (AFAICT) we can view
this as a new feature / config option (even tho an important one).

So let's move to net-next, and we can "do this right".

Does that sound sane?

Andrew, Jakub,

On Sat, Dec 5, 2020 at 10:28 AM Sven Van Asbroeck [off-list ref] wrote:

From: Sven Van Asbroeck <redacted>

On the ksz8795, if the devicetree contains a cpu node,
devicetree parsing fails and the whole driver errors out.

Fix the devicetree parsing code by making it use the
correct number of ports.

Fixes: 912aae27c6af ("net: dsa: microchip: really look for phy-mode in port nodes")
Tested-by: Sven Van Asbroeck <redacted> # ksz8795
Signed-off-by: Sven Van Asbroeck <redacted>
---

Any chance that this patch could still get merged?
I believe this will work fine on both ksz8795 and ksz9477, even though num_ports
is defined differently, because:

ksz8795:
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt + 1;
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz8795.c?h=v5.10-rc7#n1266

ksz9477:
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt;
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz9477.c?h=v5.10-rc7#n1585

Would it be possible to merge this into net, so users get working cpu nodes?
I don't think this will prevent you from harmonizing port_cnt in net-next.

On Tue, 8 Dec 2020 10:33:28 -0500 Sven Van Asbroeck wrote:

Andrew, Jakub,

On Sat, Dec 5, 2020 at 10:28 AM Sven Van Asbroeck [off-list ref] wrote:

quoted

From: Sven Van Asbroeck <redacted>

On the ksz8795, if the devicetree contains a cpu node,
devicetree parsing fails and the whole driver errors out.

Fix the devicetree parsing code by making it use the
correct number of ports.

Fixes: 912aae27c6af ("net: dsa: microchip: really look for phy-mode in port nodes")
Tested-by: Sven Van Asbroeck <redacted> # ksz8795
Signed-off-by: Sven Van Asbroeck <redacted>
---  

Any chance that this patch could still get merged?
I believe this will work fine on both ksz8795 and ksz9477, even though num_ports
is defined differently, because:

ksz8795:
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt + 1;
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz8795.c?h=v5.10-rc7#n1266

ksz9477:
/* set the real number of ports */
dev->ds->num_ports = dev->port_cnt;
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/net/dsa/microchip/ksz9477.c?h=v5.10-rc7#n1585

Would it be possible to merge this into net, so users get working cpu nodes?
I don't think this will prevent you from harmonizing port_cnt in net-next.

What I was talking about in the email yesterday was 0x8794 support
in ksz8795.c. Is the cpu port configuration going to work there?
Isn't the CPU port always port 5 (index 4)?

It sure as hell looked like it until commit c9f4633b93ea ("net: dsa:
microchip: remove usage of mib_port_count") came along. I wonder if 
ksz8794 works on net-next :/

On Tue, Dec 8, 2020 at 11:52 AM Jakub Kicinski [off-list ref] wrote:

What I was talking about in the email yesterday was 0x8794 support
in ksz8795.c. Is the cpu port configuration going to work there?
Isn't the CPU port always port 5 (index 4)?

Understood. You expressed concern that .cpu_ports = 0x10 even on the 3-port
switch.

I noticed that .cpu_ports is never used in ksz8794.c, so I hoped the fix
would be fine. But digging a bit deeper, I see in ksz8795.c:

        dev->mib_port_cnt = TOTAL_PORT_NUM; /* = 5 */
        ...
        dev->cpu_port = dev->mib_port_cnt - 1; /* = 4 */

And that is unlikely to work on the 3-port switch.
So yeah, you are right, my patch won't fix the general issues here :(