| 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: <9b557a13-080a-853b-e65e-a39b75e1026f@seco.com>
Date: Thu, 18 Nov 2021 15:38:02 -0500
From: Sean Anderson <sean.anderson@...o.com>
To: Marek Behún <kabel@...nel.org>,
netdev@...r.kernel.org, Andrew Lunn <andrew@...n.ch>
Cc: Jakub Kicinski <kuba@...nel.org>,
David Miller <davem@...emloft.net>,
Russell King <rmk+kernel@...linux.org.uk>,
Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next 4/8] net: phylink: update supported_interfaces
with modes from fwnode
On 11/17/21 5:50 PM, Marek Behún wrote:
> Now that the 'phy-mode' property can be a string array containing more
> PHY modes (all that are supported by the board), update the bitmap of
> interfaces supported by the MAC with this property.
>
> Normally this would be a simple intersection (of interfaces supported by
> the current implementation of the driver and interfaces supported by the
> board), but we need to keep being backwards compatible with older DTs,
> which may only define one mode, since, as Russell King says,
> conventionally phy-mode has meant "this is the mode we want to operate
> the PHY interface in" which was fine when PHYs didn't change their
> mode depending on the media speed
>
> An example is DT defining
> phy-mode = "sgmii";
> but the board supporting also 1000base-x and 2500base-x.
>
> Add the following logic to keep this backwards compatiblity:
> - if more PHY modes are defined, do a simple intersection
> - if one PHY mode is defined:
> - if it is sgmii, 1000base-x or 2500base-x, add all three and then do
> the intersection
> - if it is 10gbase-r or usxgmii, add both, and also 5gbase-r,
> 2500base-x, 1000base-x and sgmii, and then do the intersection
>
> This is simple enough and should work for all boards.
>
> Nonetheless it is possible (although extremely unlikely, in my opinion)
> that a board will be found that (for example) defines
> phy-mode = "sgmii";
> and the MAC drivers supports sgmii, 1000base-x and 2500base-x, but the
> board DOESN'T support 2500base-x, because of electrical reasons (since
> the frequency is 2.5x of sgmii).
> Our code will in this case incorrectly infer also support for
> 2500base-x. To avoid this, the board maintainer should either change DTS
> to
> phy-mode = "sgmii", "1000base-x";
> and update device tree on all boards, or, if that is impossible, add a
> fix into the function we are introducing in this commit.
Can you touch on the 5G/10G stuff as well in the message?
> Signed-off-by: Marek Behún <kabel@...nel.org>
> ---
> drivers/net/phy/phylink.c | 63 +++++++++++++++++++++++++++++++++++++++
> include/linux/phy.h | 6 ++++
> 2 files changed, 69 insertions(+)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index f7156b6868e7..6d7c216a5dea 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -563,6 +563,67 @@ static int phylink_parse_fixedlink(struct phylink *pl,
> return 0;
> }
>
> +static void phylink_update_phy_modes(struct phylink *pl,
> + struct fwnode_handle *fwnode)
> +{
> + unsigned long *supported = pl->config->supported_interfaces;
> + DECLARE_PHY_INTERFACE_MASK(modes);
> +
> + if (fwnode_get_phy_modes(fwnode, modes) < 0)
> + return;
> +
> + if (phy_interface_empty(modes))
> + return;
> +
> + /* If supported is empty, just copy modes defined in fwnode. */
> + if (phy_interface_empty(supported))
> + return phy_interface_copy(supported, modes);
> +
> + /* We want the intersection of given supported modes with those defined
> + * in DT.
> + *
> + * Some older device-trees mention only one of `sgmii`, `1000base-x` or
> + * `2500base-x`, while supporting all three. Other mention `10gbase-r`
> + * or `usxgmii`, while supporting both, and also `sgmii`, `1000base-x`,
> + * `2500base-x` and `5gbase-r`.
> + * For backwards compatibility with these older DTs, make it so that if
> + * one of these modes is mentioned in DT and MAC supports more of them,
> + * keep all that are supported according to the logic above.
> + *
> + * Nonetheless it is possible that a device may support only one mode,
> + * for example 1000base-x, due to strapping pins or some other reasons.
> + * If a specific device supports only the mode mentioned in DT, the
> + * exception should be made here with of_machine_is_compatible().
> + */
> + if (bitmap_weight(modes, PHY_INTERFACE_MODE_MAX) == 1) {
> + bool lower = false;
> +
> + if (test_bit(PHY_INTERFACE_MODE_10GBASER, modes) ||
> + test_bit(PHY_INTERFACE_MODE_USXGMII, modes)) { > + if (test_bit(PHY_INTERFACE_MODE_5GBASER, supported))
> + __set_bit(PHY_INTERFACE_MODE_5GBASER, modes);
> + if (test_bit(PHY_INTERFACE_MODE_10GBASER, supported))
> + __set_bit(PHY_INTERFACE_MODE_10GBASER, modes);
> + if (test_bit(PHY_INTERFACE_MODE_USXGMII, supported))
> + __set_bit(PHY_INTERFACE_MODE_USXGMII, modes);
How about
DECLARE_PHY_INTERFACE_MASK(upper_modes);
__set_bit(upper_modes, PHY_INTERFACE_MODE_5GBASER);
__set_bit(upper_modes, PHY_INTERFACE_MODE_10GBASER);
__set_bit(upper_modes, PHY_INTERFACE_MODE_USXGMII);
phy_interface_and(upper_modes, supported, upper_modes);
phy_interface_or(modes, modes, upper_modes);
same linecount but less duplication
> + lower = true;
> + }
> +
> + if (lower || (test_bit(PHY_INTERFACE_MODE_SGMII, modes) ||
> + test_bit(PHY_INTERFACE_MODE_1000BASEX, modes) ||
> + test_bit(PHY_INTERFACE_MODE_2500BASEX, modes))) {
> + if (test_bit(PHY_INTERFACE_MODE_SGMII, supported))
> + __set_bit(PHY_INTERFACE_MODE_SGMII, modes);
> + if (test_bit(PHY_INTERFACE_MODE_1000BASEX, supported))
> + __set_bit(PHY_INTERFACE_MODE_1000BASEX, modes);
> + if (test_bit(PHY_INTERFACE_MODE_2500BASEX, supported))
> + __set_bit(PHY_INTERFACE_MODE_2500BASEX, modes);
ditto
--Sean
> + }
> + }
> +
> + phy_interface_and(supported, supported, modes);
> +}
> +
> static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
> {
> struct fwnode_handle *dn;
> @@ -1156,6 +1217,8 @@ struct phylink *phylink_create(struct phylink_config *config,
> __set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
> timer_setup(&pl->link_poll, phylink_fixed_poll, 0);
>
> + phylink_update_phy_modes(pl, fwnode);
> +
> bitmap_fill(pl->supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> linkmode_copy(pl->link_config.advertising, pl->supported);
> phylink_validate(pl, pl->supported, &pl->link_config);
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 1e57cdd95da3..83ae15ab1676 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -169,6 +169,12 @@ static inline bool phy_interface_empty(const unsigned long *intf)
> return bitmap_empty(intf, PHY_INTERFACE_MODE_MAX);
> }
>
> +static inline void phy_interface_copy(unsigned long *dst,
> + const unsigned long *src)
> +{
> + bitmap_copy(dst, src, PHY_INTERFACE_MODE_MAX);
> +}
> +
> static inline void phy_interface_and(unsigned long *dst, const unsigned long *a,
> const unsigned long *b)
> {
>
Powered by blists - more mailing lists