netdev - Re: question about drivers/net/dsa/sja1105/sja1105

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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <7fc8f8d5-285a-9ec0-23c5-c867347c4feb@gmail.com>
Date:   Fri, 3 Apr 2020 23:02:44 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>,
        Julia Lawall <julia.lawall@...ia.fr>
Cc:     netdev <netdev@...r.kernel.org>, Joe Perches <joe@...ches.com>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
Subject: Re: question about drivers/net/dsa/sja1105/sja1105_main.c

On 03.04.2020 16:36, Vladimir Oltean wrote:
> Hi Julia,
> 
> On Fri, 3 Apr 2020 at 16:46, Julia Lawall <julia.lawall@...ia.fr> wrote:
>>
>> Hello,
>>
>> The function sja1105_static_config_reload in sja1105_main.c contains the
>> code:
>>
>>                 if (!an_enabled) {
>>                         int speed = SPEED_UNKNOWN;
>>
>>                         if (bmcr & BMCR_SPEED1000)
>>                                 speed = SPEED_1000;
>>                         else if (bmcr & BMCR_SPEED100)
>>                                 speed = SPEED_100;
>>                         else if (bmcr & BMCR_SPEED10)
>>                                 speed = SPEED_10;
>>
>>                         sja1105_sgmii_pcs_force_speed(priv, speed);
>>                 }
>>
>> The last test bmcr & BMCR_SPEED10 does not look correct, because according
>> to include/uapi/linux/mii.h, BMCR_SPEED10 is 0.  What should be done
>> instead?
>>

It's right that this is not correct. You can check genphy_read_status_fixed()
for how it's done there.


>> thanks,
>> julia
> 
> Thanks for pointing out, you raise a good point.
> Correct usage would be:
> 
> include/uapi/linux/mii.h:
> #define BMCR_SPEED_MASK 0x2040
> 
> drivers/net/dsa/sja1105/sja1105_main.c:
>                          int speed = SPEED_UNKNOWN;
> 
>                          if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED1000)
>                                  speed = SPEED_1000;
>                          else if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED100)
>                                  speed = SPEED_100;
>                          else if (bmcr & BMCR_SPEED_MASK == BMCR_SPEED10)
>                                  speed = SPEED_10;
> 
> but the BMCR_SPEED_MASK doesn't exist, it looks like. I believe that
> is because drivers (or the PHY library) don't typically need to read
> the speed from the MII_BMCR register, they just need to write it. If
> the PHY library maintainers think there is any value in defining
> BMCR_SPEED_MASK as part of the UAPI, we can do that. Otherwise, the
> definition can be restricted to drivers/net/dsa/sja1105/sja1105.h.
> 
> Thanks,
> -Vladimir
>