linux-kernel - Re: [PATCH v2 net-next 5/9] net: dsa: microchip: add support for phylink management

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: <20210422200557.34lwobnmvhtba3wj@skbuf>
Date:   Thu, 22 Apr 2021 23:05:57 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
Cc:     andrew@...n.ch, netdev@...r.kernel.org, robh+dt@...nel.org,
        UNGLinuxDriver@...rochip.com, hkallweit1@...il.com,
        linux@...linux.org.uk, davem@...emloft.net, kuba@...nel.org,
        linux-kernel@...r.kernel.org, vivien.didelot@...il.com,
        f.fainelli@...il.com, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 net-next 5/9] net: dsa: microchip: add support for
 phylink management

On Thu, Apr 22, 2021 at 03:12:53PM +0530, Prasanna Vengateshan wrote:
> Support for phylink_validate() and reused KSZ commmon API for
> phylink_mac_link_down() operation
> 
> Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@...rochip.com>
> ---
>  drivers/net/dsa/microchip/lan937x_main.c | 42 ++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/net/dsa/microchip/lan937x_main.c b/drivers/net/dsa/microchip/lan937x_main.c
> index 944c6f4d6d60..93c392081423 100644
> --- a/drivers/net/dsa/microchip/lan937x_main.c
> +++ b/drivers/net/dsa/microchip/lan937x_main.c
> @@ -312,6 +312,46 @@ static int lan937x_get_max_mtu(struct dsa_switch *ds, int port)
>  	return FR_MAX_SIZE;
>  }
>  
> +static void lan937x_phylink_validate(struct dsa_switch *ds, int port,
> +				     unsigned long *supported,
> +			  struct phylink_link_state *state)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> +	if (phy_interface_mode_is_rgmii(state->interface) ||
> +	    state->interface == PHY_INTERFACE_MODE_SGMII ||
> +		state->interface == PHY_INTERFACE_MODE_RMII ||
> +		state->interface == PHY_INTERFACE_MODE_MII ||
> +		lan937x_is_internal_100BTX_phy_port(dev, port)) {
> +		phylink_set(mask, 10baseT_Half);
> +		phylink_set(mask, 10baseT_Full);
> +		phylink_set(mask, 100baseT_Half);
> +		phylink_set(mask, 100baseT_Full);
> +		phylink_set(mask, Autoneg);
> +		phylink_set_port_modes(mask);
> +		phylink_set(mask, Pause);
> +		phylink_set(mask, Asym_Pause);

Why do you advertise pause if you don't react to it (I commented on the
previous patch that you force flow control off)?

And the Microchip KSZ DSA phylink integration is the strangest I've ever
seen, bar none.

Do your RGMII ports work at 10/100/1000 Mbps? If yes, how?

Do your SGMII ports work in fixed-link, at 10/100/1000? How about with a
PHY that expects in-band autoneg?

> +	}
> +
> +	/*  For RGMII & SGMII interfaces */
> +	if (phy_interface_mode_is_rgmii(state->interface) ||
> +	    state->interface == PHY_INTERFACE_MODE_SGMII) {
> +		phylink_set(mask, 1000baseT_Full);
> +	}
> +
> +	/* For T1 PHY */
> +	if (lan937x_is_internal_t1_phy_port(dev, port)) {
> +		phylink_set(mask, 100baseT1_Full);
> +		phylink_set_port_modes(mask);
> +	}
> +
> +	bitmap_and(supported, supported, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +	bitmap_and(state->advertising, state->advertising, mask,
> +		   __ETHTOOL_LINK_MODE_MASK_NBITS);
> +}
> +
>  static int	lan937x_port_pre_bridge_flags(struct dsa_switch *ds, int port,
>  					      struct switchdev_brport_flags flags,
>  					 struct netlink_ext_ack *extack)
> @@ -340,6 +380,8 @@ const struct dsa_switch_ops lan937x_switch_ops = {
>  	.port_fast_age		= ksz_port_fast_age,
>  	.port_max_mtu		= lan937x_get_max_mtu,
>  	.port_change_mtu	= lan937x_change_mtu,
> +	.phylink_validate	= lan937x_phylink_validate,
> +	.phylink_mac_link_down	= ksz_mac_link_down,
>  };
>  
>  int lan937x_switch_register(struct ksz_device *dev)
> -- 
> 2.27.0
> 

Fundamentally, what is the purpose of this patch?