netdev - Re: [PATCH net-next v2 1/7] net: ngbe: implement phylink to handle PHY device

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] [day] [month] [year] [list]

Message-ID: <ZW2loxTO6oKNYLew@shell.armlinux.org.uk>
Date: Mon, 4 Dec 2023 10:10:43 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Jiawen Wu <jiawenwu@...stnetic.com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
	pabeni@...hat.com, andrew@...n.ch, netdev@...r.kernel.org,
	mengyuanlou@...-swift.com
Subject: Re: [PATCH net-next v2 1/7] net: ngbe: implement phylink to handle
 PHY device

On Mon, Dec 04, 2023 at 05:18:59PM +0800, Jiawen Wu wrote:
> Add phylink support for Wangxun 1Gb Ethernet controller.
> 
> Signed-off-by: Jiawen Wu <jiawenwu@...stnetic.com>
> ---
>  drivers/net/ethernet/wangxun/libwx/wx_type.h  |   1 +
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  21 ++-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 127 +++++++++++-------
>  3 files changed, 88 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 165e82de772e..4088637440c6 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -940,6 +940,7 @@ struct wx {
>  	int speed;
>  	int duplex;
>  	struct phy_device *phydev;

If you also embed struct phylink_config() in struct wx, then you can get
to it in the MAC operations using an inline function in wx_type.h:

static inline struct wx *phylink_to_wx(struct phylink_config *config)
{
	return container_of(config, struct wx, phylink_config);
}

which will be more efficient than doing the
netdev_priv(to_net_dev(config->dev)) dance. It's also what other network
drivers that use phylink do, so it brings consistency of implementation.

...
> -static void ngbe_handle_link_change(struct net_device *dev)
> +static void ngbe_phy_fixup(struct wx *wx)
>  {
> -	struct wx *wx = netdev_priv(dev);
> -	struct phy_device *phydev;
> -	u32 lan_speed, reg;
> +	struct phy_device *phydev = wx->phydev;
> +	struct ethtool_eee eee;
>  
> -	phydev = wx->phydev;
> -	if (!(wx->link != phydev->link ||
> -	      wx->speed != phydev->speed ||
> -	      wx->duplex != phydev->duplex))
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_10baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_100baseT_Half_BIT);
> +	phy_remove_link_mode(phydev, ETHTOOL_LINK_MODE_1000baseT_Half_BIT);

Phylink restricts the advertisement according to the capabilities
provided, do you should not need to do this. Please remove.

> +
> +	phydev->mac_managed_pm = true;

Please don't bypass phylink's management of this, instead set the
"mac_managed_pm" boolean in struct phylink_config.

...
> +static int ngbe_phylink_init(struct wx *wx)
>  {
> -	int ret;
> +	struct phylink_config *config;
> +	phy_interface_t phy_mode;
> +	struct phylink *phylink;
>  
> -	ret = phy_connect_direct(wx->netdev,
> -				 wx->phydev,
> -				 ngbe_handle_link_change,
> -				 PHY_INTERFACE_MODE_RGMII_ID);
> -	if (ret) {
> -		wx_err(wx, "PHY connect failed.\n");
> -		return ret;
> -	}
> +	config = devm_kzalloc(&wx->pdev->dev, sizeof(*config), GFP_KERNEL);
> +	if (!config)
> +		return -ENOMEM;
>  
> -	return 0;
> -}
> +	config->dev = &wx->netdev->dev;
> +	config->type = PHYLINK_NETDEV;
> +	config->mac_capabilities = MAC_1000FD | MAC_100FD |
> +				   MAC_SYM_PAUSE | MAC_ASYM_PAUSE;
>  
> -static void ngbe_phy_fixup(struct wx *wx)
> -{
> -	struct phy_device *phydev = wx->phydev;
> -	struct ethtool_eee eee;
> +	phy_mode = PHY_INTERFACE_MODE_RGMII_ID;
> +	__set_bit(PHY_INTERFACE_MODE_RGMII_ID, config->supported_interfaces);

As mentioned above, also set config->mac_managed_pm.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!