Smatch report for drivers/net/phy/marvell10g.c

[Marek Behún]

On Wed, Aug 06, 2025 at 05:36:27PM +0200, Marek Vasut wrote:
> On 8/6/25 4:53 PM, Tom Rini wrote:
> > On Tue, Aug 05, 2025 at 05:30:49PM +0100, Andrew Goodbody wrote:
> > 
> > > I have these Smatch reports.
> > > 
> > > drivers/net/phy/marvell10g.c:384 mv3310_select_mactype() warn: was ||
> > > intended here instead of &&?
> > > drivers/net/phy/marvell10g.c:387 mv3310_select_mactype() warn: was ||
> > > intended here instead of &&?
> > > drivers/net/phy/marvell10g.c:390 mv3310_select_mactype() warn: was ||
> > > intended here instead of &&?
> > > 
> > > Now there is obviously incorrect code there as it cannot work correctly as
> > > is, phydev->interface cannot equal two different enums at the same time, but
> > > simply swapping the && for || is not going to work either. Could anyone
> > > please point me in the direction of a fix?
> > 
> > Adding Marek as r8a779f0_spider and r8a779f4_s4sk are the only two
> > boards which enable CONFIG_PHY_MARVELL_10G (aside, ./tools/qconfig.py -b
> > and then ./tools/qconfig.py -f CONFIG_FOO is handy to see what boards
> > enable a given option and helpful when get_maintainer.pl has nothing
> > specific).
> They use the 2xxx series PHY, so this code is avoided. However, it seems
> even linux-next has the same behavior in .select_mactype() callback . They
> use test_bit() in there, but otherwise also use && instead of || which looks
> odd. Maybe that is a genuine bug which got ported over from Linux and needs
> to be fixed both there and here ?

In linux there is a bitmap of supported interface modes given to the
function. Then for example we test whether 10gbase-r and sgmii are both
supported by the host (SoC):

  ...
  else if (test_bit(PHY_INTERFACE_MODE_SGMII, interfaces) &&
           test_bit(PHY_INTERFACE_MODE_10GBASER, interfaces))
          return MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER;
  ...

If both 10gbase-r and sgmii are supported by the SoC, we tell the PHY to
use mactype 4 (MV_V2_33X0_PORT_CTRL_MACTYPE_10GBASER).

Obiously this does not work in U-Boot it we translate it to
  interface == MODE_SGMII && interface == MODE_10GBASER
since the interface variable is not a bitmap of supported interfaces,
but rather the current interface mode (maybe from device-tree).

Looking at U-Boot's marvell10g code, I also see mv_test_bit macro
and how it is being used:

  #define mv_test_bit(iface, phydev) \
    ({ if ((phydev)->interface & (iface)) return 0; })

  mv_test_bit(PHY_INTERFACE_MODE_SGMII, phydev);

This is completely wrong, since the ->interface member is not a bitmap
(rather the currnet selected interface) nor are the PHY_INTERFACE_MODE_*
values one-bit values.

So this code will not work correctly at all in U-Boot.


Now as to why Linux also has interfaces bitmap in addition to the current
selected interface.

The reason why in Linux we use bitmap of supported interface modes is so
that we can select the best serdes mode switching behavior for the PHY.

For example if the host only supports 10gbase-r serdes mode, then the PHY
must always talk to the host in 10gbase-r mode, even if the RJ-45 copper
speed was autonegotiated to lower speed (i.e. 1gbps). (There is another
complication called rate matching that needs to be done in that case,
but this is not important now.)

If the host supports both 10gbase-r and sgmii serdes modes, we want the
PHY to switch to sgmii if the RJ-45 speed is 1000/100/10, and to switch
to 10gbase-r if the RJ-45 speed is 10000.

The register configuring this behavior is called MACTYPE in the Marvell
PHY.

The following table shows how the PHY will change serdes mode depedning
on the RJ-45 speed for some of the MACTYPE settings:

               mactype=4   mactype=6  mactype=7  mactype=0
  RJ-45 speed  sds mode    sds mode   sds mode   sds mode
  ---------------------------------------------------------
  10/100/1000  sgmii       10gbase-r  usxgmii    sgmii
  2500         2500base-x  10gbase-r  usxgmii    2500base-x
  5000         5gbase-r    10gbase-r  usxgmii    5gbase-r
  10000        10gbase-r   10gbase-r  usxgmii    rxaui

Marek