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: <ZXBjwWjd1Jv8916K@shell.armlinux.org.uk>
Date: Wed, 6 Dec 2023 12:06:25 +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 v3 1/7] net: ngbe: implement phylink to handle
 PHY device

On Wed, Dec 06, 2023 at 05:53:49PM +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  |   8 ++
>  drivers/net/ethernet/wangxun/ngbe/ngbe_main.c |  20 ++-
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.c | 126 +++++++++++-------
>  drivers/net/ethernet/wangxun/ngbe/ngbe_mdio.h |   2 +-
>  4 files changed, 93 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/net/ethernet/wangxun/libwx/wx_type.h b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> index 165e82de772e..9225aaf029f8 100644
> --- a/drivers/net/ethernet/wangxun/libwx/wx_type.h
> +++ b/drivers/net/ethernet/wangxun/libwx/wx_type.h
> @@ -8,6 +8,7 @@
>  #include <linux/netdevice.h>
>  #include <linux/if_vlan.h>
>  #include <net/ip.h>
> +#include <linux/phylink.h>

Nit: would be better to keep linux/ includes together (and in
alphabetical order to prevent conflicts.)

> diff --git a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> index 8db804543e66..c61f4b9d79fa 100644
> --- a/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> +++ b/drivers/net/ethernet/wangxun/ngbe/ngbe_main.c
> @@ -9,6 +9,7 @@
>  #include <linux/etherdevice.h>
>  #include <net/ip.h>
>  #include <linux/phy.h>
> +#include <linux/phylink.h>
>  #include <linux/if_vlan.h>
>  
>  #include "../libwx/wx_type.h"

As wx_type.h includes linux/phylink.h, which is now fundamental for the
definition of one of the structures in wx_type.h, the include of
linux/phylink.h seems unnecessary here.

> @@ -336,7 +337,8 @@ static void ngbe_disable_device(struct wx *wx)
>  
>  static void ngbe_down(struct wx *wx)
>  {
> -	phy_stop(wx->phydev);
> +	phylink_stop(wx->phylink);
> +	phylink_disconnect_phy(wx->phylink);

I'm not sure why you're moving the PHY disconnection in this patch -
that seems like a separate change to the actual conversion to phylink.
For a pure conversion, you should be able to just replace the phylib
calls with their phylink equivalents.

It seems to me that there's two changes happening here: a conversion to
phylink and a re-ordering of the open/close methods particularly to do
with connecting and disconnecting the PHY. Either this needs to be
described in the commit message (the fact that it's happening and why)
or it should be two patches.

> -static void ngbe_phy_fixup(struct wx *wx)
> +void ngbe_phylink_start(struct wx *wx)
>  {
> -	struct phy_device *phydev = wx->phydev;
> -	struct ethtool_eee eee;
> -
> -	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);
> -
> -	phydev->mac_managed_pm = true;
> -	if (wx->mac_type != em_mac_type_mdi)
> -		return;
> -	/* disable EEE, internal phy does not support eee */
> -	memset(&eee, 0, sizeof(eee));
> -	phy_ethtool_set_eee(phydev, &eee);
> +	struct phylink *phylink = wx->phylink;
> +
> +	phylink_connect_phy(phylink, wx->phydev);

Note that phylink_connect_phy() can fail, so it's return value should
be checked.

Apart from the comments above, I think I'm fine with this.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ