netdev - Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20191221102217.GA30801@lunn.ch>
Date:   Sat, 21 Dec 2019 11:22:17 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Vivien Didelot <vivien.didelot@...il.com>
Cc:     Baruch Siach <baruch@...s.co.il>,
        Marek Behún <marek.behun@....cz>,
        netdev@...r.kernel.org,
        Denis Odintsov <d.odintsov@...viangames.com>
Subject: Re: [PATCH] net: dsa: mv88e6xxx: force cmode write on 6141/6341

> Andrew,
> 
> We tend to avoid caching values in the mv88e6xxx driver the more we can and
> query the hardware instead to avoid errors like this. We can consider calling a
> new mv88e6xxx_port_get_cmode() helper when needed (e.g. in higher level callers
> like mv88e6xxx_serdes_power() and mv88e6xxx_serdes_irq_thread_fn()) and pass
> the value down to the routines previously accessing chip->ports[port].cmode,
> as a new argument. I can prepare a patch doing this. What do you think?

Hi Vivien

There is one problem area. The lower ports on the 6390X, port
2-7. They can have two different cmode values, 0x1f for internal PHY,
and 0x09 for 1000BaseX when using an SFP. The switch will swap between
external and internal depending on which gets link first. But in order
for the SFP to get link, the SERDES needs to be powered on. And we
currently decide to power it on, and enable interrupts, via the
cmode. In the normal case, this works, e.g. ports 9 and 10 of 6390,
the fibre port of 6352. The cmode of 6390X is directly writable, we
get the phy-mode from DT and write the correct value. For 6352,
strapping is used, cmode is read only, but has the correct value.

But for the lower ports of 6390X, the hardware cmode defaults to 0x1f
until the fibre gets link, and then it changes to 0x09. It is not
writable. The current generic code does write to the register, which
in this case is pointless since it is read only, and it updates the
cached value, which is R/W. Later, when the port is enabled, we look
at the cached value, and based on that, decide is the SERDES should be
powered up, interrupts enabled. At this point, the cached value is
what we have in DT, but the hardware cmode is probably still 0x1f,
internal PHY. If we were to use the hardware value, we would never
enable the SERDES and so never get link.

This is all a bit hidden in the code. So if you want to remove the
cache, you need to add something in its place. Maybe explicitly keep
the configured value in the port structure, and modify the code using
cmode so that it either looks at the actual cmode, or the configured
cmode, depending on what it is trying to achieve.

ZII devel C is a good test bed for this, port 3 has both a copper PHY
wired to an RJ45 socket, and an SFF.

      Andrew