| 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
| ||
|
Message-ID: <61d9f92e-78d8-5d14-50d1-1ed886ec0e17@seco.com>
Date: Tue, 26 Oct 2021 13:28:15 -0400
From: Sean Anderson <sean.anderson@...o.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>,
Nicolas Ferre <nicolas.ferre@...rochip.com>
Cc: netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
Jakub Kicinski <kuba@...nel.org>,
Claudiu Beznea <claudiu.beznea@...rochip.com>,
Antoine Tenart <atenart@...nel.org>
Subject: Re: [PATCH v4] net: macb: Fix several edge cases in validate
On 10/26/21 1:04 PM, Russell King (Oracle) wrote:
> On Tue, Oct 26, 2021 at 06:37:18PM +0200, Nicolas Ferre wrote:
>> I like it as well. Thanks a lot for these enhancements.
>
> So, would this work - have I understood all these capability flags
> correctly? To aid this process, I've included the resulting code
> below as well as the diff.
>
> Should we only be setting have_10g if _all_ of MACB_CAPS_HIGH_SPEED,
> MACB_CAPS_PCS, and MACB_CAPS_GIGABIT_MODE_AVAILABLE are set and we
> have gem, or is what I have below sufficient (does it matter if
> MACB_CAPS_PCS is clear?)
>
> static void macb_validate(struct phylink_config *config,
> unsigned long *supported,
> struct phylink_link_state *state)
> {
> struct net_device *ndev = to_net_dev(config->dev);
> __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> bool have_1g = false, have_10g = false;
> struct macb *bp = netdev_priv(ndev);
>
> if (macb_is_gem(bp) &&
> (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
> if (bp->caps & MACB_CAPS_PCS)
> have_1g = true;
> if (bp->caps & MACB_CAPS_HIGH_SPEED)
> have_10g = true;
> }
This is incorrect. (R)GMII does not need a PCS. Only SGMII needs one. So it really should be
if (macb_is_gem(bp) &&
(bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
have_1g = true;
if (bp->caps & MACB_CAPS_PCS)
have_sgmii = true;
if (bp->caps & MACB_CAPS_HIGH_SPEED)
have_10g = true;
}
>
> switch (state->interface) {
> case PHY_INTERFACE_MODE_NA:
> case PHY_INTERFACE_MODE_MII:
> case PHY_INTERFACE_MODE_RMII:
> break;
>
> case PHY_INTERFACE_MODE_GMII:
> case PHY_INTERFACE_MODE_RGMII:
> case PHY_INTERFACE_MODE_RGMII_ID:
> case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> case PHY_INTERFACE_MODE_SGMII:
And then SGMII needs its own case here.
> if (!have_1g) {
> linkmode_zero(supported);
> return;
> }
> break;
>
> case PHY_INTERFACE_MODE_10GBASER:
> if (!have_10g) {
> linkmode_zero(supported);
> return;
> }
> break;
>
> default:
> linkmode_zero(supported);
> return;
> }
>
> phylink_set_port_modes(mask);
> phylink_set(mask, Autoneg);
> phylink_set(mask, Asym_Pause);
>
> switch (state->interface) {
> case PHY_INTERFACE_MODE_NA:
> case PHY_INTERFACE_MODE_10GBASER:
> if (have_10g) {
> phylink_set_10g_modes(mask);
> phylink_set(mask, 10000baseKR_Full);
> }
> if (state->interface != PHY_INTERFACE_MODE_NA)
> break;
> fallthrough;
>
> case PHY_INTERFACE_MODE_GMII:
> case PHY_INTERFACE_MODE_RGMII:
> case PHY_INTERFACE_MODE_RGMII_ID:
> case PHY_INTERFACE_MODE_RGMII_RXID:
> case PHY_INTERFACE_MODE_RGMII_TXID:
> case PHY_INTERFACE_MODE_SGMII:
> if (have_1g) {
> phylink_set(mask, 1000baseT_Full);
> phylink_set(mask, 1000baseX_Full);
>
> if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> phylink_set(mask, 1000baseT_Half);
> }
> fallthrough;
Actually, according to the Zynq UltraScale+ Devices Register Reference [1], the PCS does not support 10/100. So should SGMII even fall through here?
[1] https://www.xilinx.com/html_docs/registers/ug1087/gem___pcs_control.html
>
> default:
> phylink_set(mask, 10baseT_Half);
> phylink_set(mask, 10baseT_Full);
> phylink_set(mask, 100baseT_Half);
> phylink_set(mask, 100baseT_Full);
> break;
> }
>
> linkmode_and(supported, supported, mask);
> linkmode_and(state->advertising, state->advertising, mask);
> }
>
>
> diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c
> index 309371abfe23..36fcd5563a71 100644
> --- a/drivers/net/ethernet/cadence/macb_main.c
> +++ b/drivers/net/ethernet/cadence/macb_main.c
> @@ -512,30 +512,43 @@ static void macb_validate(struct phylink_config *config,
> {
> struct net_device *ndev = to_net_dev(config->dev);
> __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> + bool have_1g = false, have_10g = false;
> struct macb *bp = netdev_priv(ndev);
>
> - /* We only support MII, RMII, GMII, RGMII & SGMII. */
> - if (state->interface != PHY_INTERFACE_MODE_NA &&
> - state->interface != PHY_INTERFACE_MODE_MII &&
> - state->interface != PHY_INTERFACE_MODE_RMII &&
> - state->interface != PHY_INTERFACE_MODE_GMII &&
> - state->interface != PHY_INTERFACE_MODE_SGMII &&
> - state->interface != PHY_INTERFACE_MODE_10GBASER &&
> - !phy_interface_mode_is_rgmii(state->interface)) {
> - linkmode_zero(supported);
> - return;
> + if (macb_is_gem(bp) &&
> + (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE)) {
> + if (bp->caps & MACB_CAPS_PCS)
> + have_1g = true;
> + if (bp->caps & MACB_CAPS_HIGH_SPEED)
> + have_10g = true;
> }
>
> - if (!macb_is_gem(bp) &&
> - (state->interface == PHY_INTERFACE_MODE_GMII ||
> - phy_interface_mode_is_rgmii(state->interface))) {
> - linkmode_zero(supported);
> - return;
> - }
> + switch (state->interface) {
> + case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_MII:
> + case PHY_INTERFACE_MODE_RMII:
> + break;
> +
> + case PHY_INTERFACE_MODE_GMII:
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_SGMII:
> + if (!have_1g) {
> + linkmode_zero(supported);
> + return;
> + }
> + break;
> +
> + case PHY_INTERFACE_MODE_10GBASER:
> + if (!have_10g) {
> + linkmode_zero(supported);
> + return;
> + }
> + break;
>
> - if (state->interface == PHY_INTERFACE_MODE_10GBASER &&
> - !(bp->caps & MACB_CAPS_HIGH_SPEED &&
> - bp->caps & MACB_CAPS_PCS)) {
> + default:
> linkmode_zero(supported);
> return;
> }
> @@ -544,32 +557,40 @@ static void macb_validate(struct phylink_config *config,
> phylink_set(mask, Autoneg);
> phylink_set(mask, Asym_Pause);
>
> - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> - (state->interface == PHY_INTERFACE_MODE_NA ||
> - state->interface == PHY_INTERFACE_MODE_10GBASER)) {
> - phylink_set_10g_modes(mask);
> - phylink_set(mask, 10000baseKR_Full);
> + switch (state->interface) {
> + case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_10GBASER:
> + if (have_10g) {
> + phylink_set_10g_modes(mask);
> + phylink_set(mask, 10000baseKR_Full);
> + }
> if (state->interface != PHY_INTERFACE_MODE_NA)
> - goto out;
> - }
> + break;
> + fallthrough;
> +
> + case PHY_INTERFACE_MODE_GMII:
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_SGMII:
> + if (have_1g) {
> + phylink_set(mask, 1000baseT_Full);
> + phylink_set(mask, 1000baseX_Full);
> +
> + if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> + phylink_set(mask, 1000baseT_Half);
> + }
> + fallthrough;
>
> - phylink_set(mask, 10baseT_Half);
> - phylink_set(mask, 10baseT_Full);
> - phylink_set(mask, 100baseT_Half);
> - phylink_set(mask, 100baseT_Full);
> -
> - if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE &&
> - (state->interface == PHY_INTERFACE_MODE_NA ||
> - state->interface == PHY_INTERFACE_MODE_GMII ||
> - state->interface == PHY_INTERFACE_MODE_SGMII ||
> - phy_interface_mode_is_rgmii(state->interface))) {
> - phylink_set(mask, 1000baseT_Full);
> - phylink_set(mask, 1000baseX_Full);
> -
> - if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> - phylink_set(mask, 1000baseT_Half);
> + default:
> + phylink_set(mask, 10baseT_Half);
> + phylink_set(mask, 10baseT_Full);
> + phylink_set(mask, 100baseT_Half);
> + phylink_set(mask, 100baseT_Full);
> + break;
> }
> -out:
> +
> linkmode_and(supported, supported, mask);
> linkmode_and(state->advertising, state->advertising, mask);
> }
>
Powered by blists - more mailing lists