diff mbox series

net: dsa: mv88e6xxx: don't use PHY_DETECT on internal PHY's

Message ID 20211011142720.42642-1-maarten.zanders@mind.be (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: mv88e6xxx: don't use PHY_DETECT on internal PHY's | expand

Checks

Context Check Description
netdev/cover_letter success Single patches do not need cover letters
netdev/fixes_present success Fixes tag not required for -next series
netdev/patch_count success Link
netdev/tree_selection success Guessed tree name to be net-next
netdev/subject_prefix warning Target tree name not specified in the subject
netdev/cc_maintainers success CCed 8 of 8 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Fixes tag looks correct
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 25 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success No static functions without inline keyword in header files

Commit Message

Maarten Zanders Oct. 11, 2021, 2:27 p.m. UTC 
mv88e6xxx_port_ppu_updates() interpretes data in the PORT_STS
register incorrectly for internal ports (ie no PPU). In these
cases, the PHY_DETECT bit indicates link status. This results
in forcing the MAC state whenever the PHY link goes down which
is not intended. As a side effect, LED's configured to show
link status stay lit even though the physical link is down.

Add a check in mac_link_down and mac_link_up to see if it
concerns an external port and only then, look at PPU status.

Fixes: 5d5b231da7ac (net: dsa: mv88e6xxx: use PHY_DETECT in mac_link_up/mac_link_down)
Reported-by: Maarten Zanders <m.zanders@televic.com>
Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
Signed-off-by: Maarten Zanders <maarten.zanders@mind.be>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

Comments

Russell King (Oracle) Oct. 17, 2021, 9:07 p.m. UTC | #1 
On Mon, Oct 11, 2021 at 04:27:20PM +0200, Maarten Zanders wrote:
> mv88e6xxx_port_ppu_updates() interpretes data in the PORT_STS
> register incorrectly for internal ports (ie no PPU). In these
> cases, the PHY_DETECT bit indicates link status. This results
> in forcing the MAC state whenever the PHY link goes down which
> is not intended. As a side effect, LED's configured to show
> link status stay lit even though the physical link is down.

I know this patch has been merged, but I'm going to say this anyway
for the record.

The description is not entirely correct. It is not true that internal
ports do not have the PHY_DETECT bit. 88E6176 and friends are documented
that bit 12 is always the PHY_DETECT bit even for internal ports. Bit 11
there is Link status.

Looking at the definitions in port.h, some switches are different
(88E6250 family) and do indeed use bit 12 as link status.

The point I'm making is that the commit description is not universally
true and only applies to a subset of mv88e6xxx supported switches. It is
a shame it wasn't better reviewed before merging.

At least the patch is harmless; we leave the PHY_DETECT bit set on the
internal ports, which means the PPU fetches the configuration and
configures the port appropriately. The change merely helps to ensure
that we don't force link status on the internal ports.
diff mbox series

Patch

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index ee3f32d1cf46..f5ce05d78e11 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -726,9 +726,13 @@  static void mv88e6xxx_mac_link_down(struct dsa_switch *ds, int port,
 
 	ops = chip->info->ops;
 
 	mv88e6xxx_reg_lock(chip);
-	if ((!mv88e6xxx_port_ppu_updates(chip, port) ||
-	     mode == MLO_AN_FIXED) && ops->port_sync_link)
+	/* Internal PHYs propagate their configuration directly to the MAC.
+	 * External PHYs depend on whether the PPU is enabled for this port.
+	 */
+	if (((!mv88e6xxx_phy_is_internal(ds, port) &&
+	      !mv88e6xxx_port_ppu_updates(chip, port)) ||
+	     mode == MLO_AN_FIXED) && ops->port_sync_link)
 		err = ops->port_sync_link(chip, port, mode, false);
 	mv88e6xxx_reg_unlock(chip);
 
@@ -750,7 +754,12 @@  static void mv88e6xxx_mac_link_up(struct dsa_switch *ds, int port,
 	ops = chip->info->ops;
 
 	mv88e6xxx_reg_lock(chip);
-	if (!mv88e6xxx_port_ppu_updates(chip, port) || mode == MLO_AN_FIXED) {
+	/* Internal PHYs propagate their configuration directly to the MAC.
+	 * External PHYs depend on whether the PPU is enabled for this port.
+	 */
+	if ((!mv88e6xxx_phy_is_internal(ds, port) &&
+	     !mv88e6xxx_port_ppu_updates(chip, port)) ||
+	    mode == MLO_AN_FIXED) {
 		/* FIXME: for an automedia port, should we force the link
 		 * down here - what if the link comes up due to "other" media
 		 * while we're bringing the port up, how is the exclusivity