netdev - Re: [PATCH net-next v4 6/6] net: tn40xx: add PHYLIB support

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <7bd09ce5-5844-4836-a044-c507f65c051d@lunn.ch>
Date: Wed, 8 May 2024 14:21:29 +0200
From: Andrew Lunn <andrew@...n.ch>
To: FUJITA Tomonori <fujita.tomonori@...il.com>
Cc: netdev@...r.kernel.org, kuba@...nel.org, jiri@...nulli.us,
	horms@...nel.org
Subject: Re: [PATCH net-next v4 6/6] net: tn40xx: add PHYLIB support

On Thu, May 02, 2024 at 08:05:52AM +0900, FUJITA Tomonori wrote:
> This patch adds supports for multiple PHY hardware with PHYLIB. The
> adapters with TN40xx chips use multiple PHY hardware; AMCC QT2025, TI
> TLK10232, Aqrate AQR105, and Marvell 88X3120, 88X3310, and MV88E2010.
> 
> For now, the PCI ID table of this driver enables adapters using only
> QT2025 PHY. I've tested this driver and the QT2025 PHY driver with
> Edimax EN-9320 10G adapter.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@...il.com>
> ---
>  drivers/net/ethernet/tehuti/Kconfig    |  1 +
>  drivers/net/ethernet/tehuti/Makefile   |  2 +-
>  drivers/net/ethernet/tehuti/tn40.c     | 34 ++++++++++---
>  drivers/net/ethernet/tehuti/tn40.h     |  7 +++
>  drivers/net/ethernet/tehuti/tn40_phy.c | 67 ++++++++++++++++++++++++++
>  5 files changed, 104 insertions(+), 7 deletions(-)
>  create mode 100644 drivers/net/ethernet/tehuti/tn40_phy.c
> 
> diff --git a/drivers/net/ethernet/tehuti/Kconfig b/drivers/net/ethernet/tehuti/Kconfig
> index 4198fd59e42e..6ad5d37eb0e4 100644
> --- a/drivers/net/ethernet/tehuti/Kconfig
> +++ b/drivers/net/ethernet/tehuti/Kconfig
> @@ -27,6 +27,7 @@ config TEHUTI_TN40
>  	tristate "Tehuti Networks TN40xx 10G Ethernet adapters"
>  	depends on PCI
>  	select FW_LOADER
> +	select PHYLINK
>  	help
>  	  This driver supports 10G Ethernet adapters using Tehuti Networks
>  	  TN40xx chips. Currently, adapters with Applied Micro Circuits
> diff --git a/drivers/net/ethernet/tehuti/Makefile b/drivers/net/ethernet/tehuti/Makefile
> index 7a0fe586a243..0d4f4d63a65c 100644
> --- a/drivers/net/ethernet/tehuti/Makefile
> +++ b/drivers/net/ethernet/tehuti/Makefile
> @@ -5,5 +5,5 @@
>  
>  obj-$(CONFIG_TEHUTI) += tehuti.o
>  
> -tn40xx-y := tn40.o tn40_mdio.o
> +tn40xx-y := tn40.o tn40_mdio.o tn40_phy.o
>  obj-$(CONFIG_TEHUTI_TN40) += tn40xx.o
> diff --git a/drivers/net/ethernet/tehuti/tn40.c b/drivers/net/ethernet/tehuti/tn40.c
> index db1f781b8063..bf9c00513a0c 100644
> --- a/drivers/net/ethernet/tehuti/tn40.c
> +++ b/drivers/net/ethernet/tehuti/tn40.c
> @@ -7,6 +7,7 @@
>  #include <linux/if_vlan.h>
>  #include <linux/netdevice.h>
>  #include <linux/pci.h>
> +#include <linux/phylink.h>
>  
>  #include "tn40.h"
>  
> @@ -1185,21 +1186,25 @@ static void tn40_link_changed(struct tn40_priv *priv)
>  	u32 link = tn40_read_reg(priv,
>  				 TN40_REG_MAC_LNK_STAT) & TN40_MAC_LINK_STAT;
>  	if (!link) {
> -		if (netif_carrier_ok(priv->ndev) && priv->link)
> +		if (netif_carrier_ok(priv->ndev) && priv->link) {
>  			netif_stop_queue(priv->ndev);
> +			phylink_mac_change(priv->phylink, false);
> +		}

What exactly does link_changed mean?

The normal use case for calling phylink_mac_change() is that you have
received an interrupt from something like the PCS, or the PHY. The MAC
driver itself cannot fully evaluate if the link is up because there
can be multiple parts in that decision. Is the SFP reporting LOS? Does
the PCS SERDES have sync, etc. So all you do is forward the interrupt
to phylink. phylink will then look at everything it knows about and
decide the state of the link, and maybe call one of the callbacks
indicating the link is now up/down.

>  		priv->link = 0;
>  		if (priv->link_loop_cnt++ > TN40_LINK_LOOP_MAX) {
>  			/* MAC reset */
>  			tn40_set_link_speed(priv, 0);
> +			tn40_set_link_speed(priv, priv->speed);
>  			priv->link_loop_cnt = 0;

This should move into the link_down callback.

> -	if (!netif_carrier_ok(priv->ndev) && !link)
> +	if (!netif_carrier_ok(priv->ndev) && !link) {
>  		netif_wake_queue(priv->ndev);

and this should be in the link_up callback.
> +static void tn40_link_up(struct phylink_config *config, struct phy_device *phy,
> +			 unsigned int mode, phy_interface_t interface,
> +			 int speed, int duplex, bool tx_pause, bool rx_pause)
> +{
> +	struct tn40_priv *priv = container_of(config, struct tn40_priv,
> +					      phylink_config);
> +
> +	priv->speed = speed;

This is where you should take any actions needed to make the MAC send
packets, at the correct rate.

> +}
> +
> +static void tn40_link_down(struct phylink_config *config, unsigned int mode,
> +			   phy_interface_t interface)
> +{

And here you should stop the MAC sending packets.

> +}

> +
> +static void tn40_mac_config(struct phylink_config *config, unsigned int mode,
> +			    const struct phylink_link_state *state)
> +{

I know at the moment you only support 10G. When you add support for
1G, this is where you will need to configure the MAC to swap between
the different modes. phylink will tell you which mode to use,
10GBaseX, 1000BaseX, SGMII, etc. You might want to move the existing
code for 10GBaseX here.

For the next version, please also Cc: Russell King, the phylink
Maintainer.

    Andrew