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

[Marek Vasut]

On 8/7/25 10:36 AM, Marek Behún wrote:
> 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
Thank you for the excellent detailed explanation. I have finally 
submitted a fix:

https://lore.kernel.org/u-boot/20260101165317.25595-1-marek.vasut+renesas@mailbox.org/