[PATCH net-next 0/6] net: ethernet: fs_enet: Cleanup and phylink conversion

[PATCH net-next 0/6] net: ethernet: fs_enet: Cleanup and phylink conversion

[Maxime Chevallier]

This series aims at improving the fs_enet code and port it's PHY
handling from direct phylib access to using phylink instead.

Although this driver is quite old, there are still some users out there,
running an upstream kernel. The development I'm doing is on an MPC885
device, which uses fs_enet, as well as a MPC866-based device.

The main motivation for that work is to eventually support ethernet interfaces
that have more than one PHY attached to the MAC upstream, for which
phylink might be a pre-requisite. That work isn't submitted yet, and the
final solution might not even require phylink.

Regardless, I do believe that this series is relevant, as it does some
cleanup to the driver, and having it use phylink brings some nice
improvements as it simplifies the DT parsing, fixed-link handling and
removes code in that driver that predates even phylib itself.

The series is structured in the following way :

- Patches 1 and 2 are cosmetic changes. The former converts the source
  to SPDX, while the latter has fs_enet-main.c pass checkpatch. Patch 2 is
  really not mandatory in this series, and I understand that this isn't
  the easiest or most pleasant patch to review. OTOH, this allows
  getting a clean checkpatch output for the main part of the driver.

- Patches 3, 4 and 5 drop some leftovers from back when the driver didn't
  use phylib, and brings the use of phylib macros.

- Patch 6 is the actual phylink port, which also cleans the bits of code
  that become irrelevant when using phylink.

Testing was done on an MPC866 and MPC885, any test on other platforms
that use fs_enet are more than welcome.

Thanks,

Maxime

Maxime Chevallier (6):
  net: ethernet: fs_enet: convert to SPDX
  net: ethernet: fs_enet: cosmetic cleanups
  net: ethernet: fs_enet: drop the .adjust_link custom fs_ops
  net: ethernet: fs_enet: drop unused phy_info and mii_if_info
  net: ethernet: fs_enet: fcc: use macros for speed and duplex values
  net: ethernet: fs_enet: phylink conversion

 .../net/ethernet/freescale/fs_enet/Kconfig    |   2 +-
 .../ethernet/freescale/fs_enet/fs_enet-main.c | 421 ++++++++----------
 .../net/ethernet/freescale/fs_enet/fs_enet.h  |  24 +-
 .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  16 +-
 .../net/ethernet/freescale/fs_enet/mac-fec.c  |  14 +-
 .../net/ethernet/freescale/fs_enet/mac-scc.c  |  10 +-
 .../ethernet/freescale/fs_enet/mii-bitbang.c  |   5 +-
 .../net/ethernet/freescale/fs_enet/mii-fec.c  |   5 +-
 8 files changed, 209 insertions(+), 288 deletions(-)

-- 
2.45.2

[PATCH net-next 1/6] net: ethernet: fs_enet: convert to SPDX

[Maxime Chevallier]

The ENET driver has SPDX tags in the header files, but they were missing
in the C files. Change the licence information to SPDX format.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 5 +----
 drivers/net/ethernet/freescale/fs_enet/mac-fcc.c      | 5 +----
 drivers/net/ethernet/freescale/fs_enet/mac-fec.c      | 5 +----
 drivers/net/ethernet/freescale/fs_enet/mac-scc.c      | 5 +----
 drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  | 5 +----
 drivers/net/ethernet/freescale/fs_enet/mii-fec.c      | 5 +----
 6 files changed, 6 insertions(+), 24 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index cf392faa6105..5bfdd43ffdeb 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
  *
@@ -9,10 +10,6 @@
  *
  * Heavily based on original FEC driver by Dan Malek <dan@embeddededge.com>
  * and modifications by Joakim Tjernlund <joakim.tjernlund@lumentis.se>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index e2ffac9eb2ad..add062928d99 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * FCC driver for Motorola MPC82xx (PQ2).
  *
@@ -6,10 +7,6 @@
  *
  * 2005 (c) MontaVista Software, Inc.
  * Vitaly Bordug <vbordug@ru.mvista.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
index cdc89d83cf07..f75acb3b358f 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Freescale Ethernet controllers
  *
@@ -6,10 +7,6 @@
  *
  * 2005 (c) MontaVista Software, Inc.
  * Vitaly Bordug <vbordug@ru.mvista.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
index 9e89ac2b6ce3..29ba0048396b 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Ethernet on Serial Communications Controller (SCC) driver for Motorola MPC8xx and MPC82xx.
  *
@@ -6,10 +7,6 @@
  *
  * 2005 (c) MontaVista Software, Inc.
  * Vitaly Bordug <vbordug@ru.mvista.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
index f965a2329055..2e210a003558 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
  *
@@ -6,10 +7,6 @@
  *
  * 2005 (c) MontaVista Software, Inc.
  * Vitaly Bordug <vbordug@ru.mvista.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
index 7bb69727952a..93d91e8ad0de 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
@@ -1,3 +1,4 @@
+// SPDX-License-Identifier: GPL-2.0-only
 /*
  * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
  *
@@ -6,10 +7,6 @@
  *
  * 2005 (c) MontaVista Software, Inc.
  * Vitaly Bordug <vbordug@ru.mvista.com>
- *
- * This file is licensed under the terms of the GNU General Public License
- * version 2. This program is licensed "as is" without any warranty of any
- * kind, whether express or implied.
  */
 
 #include <linux/module.h>
-- 
2.45.2

Re: [PATCH net-next 1/6] net: ethernet: fs_enet: convert to SPDX

[Christophe Leroy]

Le 28/08/2024 à 11:50, Maxime Chevallier a écrit :
> The ENET driver has SPDX tags in the header files, but they were missing
> in the C files. Change the licence information to SPDX format.

AFAIK you have to CC linux-spdx@vger.kernel.org for this kind of change.

> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 5 +----
>   drivers/net/ethernet/freescale/fs_enet/mac-fcc.c      | 5 +----
>   drivers/net/ethernet/freescale/fs_enet/mac-fec.c      | 5 +----
>   drivers/net/ethernet/freescale/fs_enet/mac-scc.c      | 5 +----
>   drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c  | 5 +----
>   drivers/net/ethernet/freescale/fs_enet/mii-fec.c      | 5 +----
>   6 files changed, 6 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index cf392faa6105..5bfdd43ffdeb 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
>    *
> @@ -9,10 +10,6 @@
>    *
>    * Heavily based on original FEC driver by Dan Malek <dan@embeddededge.com>
>    * and modifications by Joakim Tjernlund <joakim.tjernlund@lumentis.se>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> index e2ffac9eb2ad..add062928d99 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * FCC driver for Motorola MPC82xx (PQ2).
>    *
> @@ -6,10 +7,6 @@
>    *
>    * 2005 (c) MontaVista Software, Inc.
>    * Vitaly Bordug <vbordug@ru.mvista.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> index cdc89d83cf07..f75acb3b358f 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Freescale Ethernet controllers
>    *
> @@ -6,10 +7,6 @@
>    *
>    * 2005 (c) MontaVista Software, Inc.
>    * Vitaly Bordug <vbordug@ru.mvista.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> index 9e89ac2b6ce3..29ba0048396b 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Ethernet on Serial Communications Controller (SCC) driver for Motorola MPC8xx and MPC82xx.
>    *
> @@ -6,10 +7,6 @@
>    *
>    * 2005 (c) MontaVista Software, Inc.
>    * Vitaly Bordug <vbordug@ru.mvista.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> index f965a2329055..2e210a003558 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mii-bitbang.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
>    *
> @@ -6,10 +7,6 @@
>    *
>    * 2005 (c) MontaVista Software, Inc.
>    * Vitaly Bordug <vbordug@ru.mvista.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> index 7bb69727952a..93d91e8ad0de 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mii-fec.c
> @@ -1,3 +1,4 @@
> +// SPDX-License-Identifier: GPL-2.0-only
>   /*
>    * Combined Ethernet driver for Motorola MPC8xx and MPC82xx.
>    *
> @@ -6,10 +7,6 @@
>    *
>    * 2005 (c) MontaVista Software, Inc.
>    * Vitaly Bordug <vbordug@ru.mvista.com>
> - *
> - * This file is licensed under the terms of the GNU General Public License
> - * version 2. This program is licensed "as is" without any warranty of any
> - * kind, whether express or implied.
>    */
>   
>   #include <linux/module.h>

[PATCH net-next 2/6] net: ethernet: fs_enet: cosmetic cleanups

[Maxime Chevallier]

Due to the age of the driver and the slow recent activity on it, the code
has taken some layers of dust. Clean the main driver file up so that it
passes checkpatch and also conforms with the net coding style.

Changes include :
 - Re-ordering of the variable declarations for RCT
 - Fixing the comment styles to either one-line comments, or net-style
   comments
 - Adding braces around single-statement 'else' clauses
 - Aligning function/macro parameters on the opening parenthesis
 - Simplifying checks for NULL pointers
 - Splitting cascaded assignments into individual assignments
 - Fixing some typos
 - Fixing whitespace issues

This is a cosmetic change and doesn't introduce any change in behaviour.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../ethernet/freescale/fs_enet/fs_enet-main.c | 220 +++++++-----------
 1 file changed, 89 insertions(+), 131 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index 5bfdd43ffdeb..2b48a2a5e32d 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -81,15 +81,14 @@ static void skb_align(struct sk_buff *skb, int align)
 static int fs_enet_napi(struct napi_struct *napi, int budget)
 {
 	struct fs_enet_private *fep = container_of(napi, struct fs_enet_private, napi);
-	struct net_device *dev = fep->ndev;
 	const struct fs_platform_info *fpi = fep->fpi;
-	cbd_t __iomem *bdp;
+	struct net_device *dev = fep->ndev;
+	int curidx, dirtyidx, received = 0;
+	int do_wake = 0, do_restart = 0;
+	int tx_left = TX_RING_SIZE;
 	struct sk_buff *skb, *skbn;
-	int received = 0;
+	cbd_t __iomem *bdp;
 	u16 pkt_len, sc;
-	int curidx;
-	int dirtyidx, do_wake, do_restart;
-	int tx_left = TX_RING_SIZE;
 
 	spin_lock(&fep->tx_lock);
 	bdp = fep->dirty_tx;
@@ -97,7 +96,6 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 	/* clear status bits for napi*/
 	(*fep->ops->napi_clear_event)(dev);
 
-	do_wake = do_restart = 0;
 	while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0 && tx_left) {
 		dirtyidx = bdp - fep->tx_bd_base;
 
@@ -106,12 +104,9 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 
 		skb = fep->tx_skbuff[dirtyidx];
 
-		/*
-		 * Check for errors.
-		 */
+		 /* Check for errors. */
 		if (sc & (BD_ENET_TX_HB | BD_ENET_TX_LC |
 			  BD_ENET_TX_RL | BD_ENET_TX_UN | BD_ENET_TX_CSL)) {
-
 			if (sc & BD_ENET_TX_HB)	/* No heartbeat */
 				dev->stats.tx_heartbeat_errors++;
 			if (sc & BD_ENET_TX_LC)	/* Late collision */
@@ -127,16 +122,16 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 				dev->stats.tx_errors++;
 				do_restart = 1;
 			}
-		} else
+		} else {
 			dev->stats.tx_packets++;
+		}
 
 		if (sc & BD_ENET_TX_READY) {
 			dev_warn(fep->dev,
 				 "HEY! Enet xmit interrupt and TX_READY.\n");
 		}
 
-		/*
-		 * Deferred means some collisions occurred during transmit,
+		/* Deferred means some collisions occurred during transmit,
 		 * but we eventually sent the packet OK.
 		 */
 		if (sc & BD_ENET_TX_DEF)
@@ -150,25 +145,20 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 			dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
 					 CBDR_DATLEN(bdp), DMA_TO_DEVICE);
 
-		/*
-		 * Free the sk buffer associated with this last transmit.
-		 */
+		/* Free the sk buffer associated with this last transmit. */
 		if (skb) {
 			dev_kfree_skb(skb);
 			fep->tx_skbuff[dirtyidx] = NULL;
 		}
 
-		/*
-		 * Update pointer to next buffer descriptor to be transmitted.
+		/* Update pointer to next buffer descriptor to be transmitted.
 		 */
 		if ((sc & BD_ENET_TX_WRAP) == 0)
 			bdp++;
 		else
 			bdp = fep->tx_bd_base;
 
-		/*
-		 * Since we have freed up a buffer, the ring is no longer
-		 * full.
+		/* Since we have freed up a buffer, the ring is no longer full.
 		 */
 		if (++fep->tx_free == MAX_SKB_FRAGS)
 			do_wake = 1;
@@ -185,8 +175,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 	if (do_wake)
 		netif_wake_queue(dev);
 
-	/*
-	 * First, grab all of the stats for the incoming packet.
+	/* First, grab all of the stats for the incoming packet.
 	 * These get messed up if we get called due to a busy condition.
 	 */
 	bdp = fep->cur_rx;
@@ -195,16 +184,13 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 	       received < budget) {
 		curidx = bdp - fep->rx_bd_base;
 
-		/*
-		 * Since we have allocated space to hold a complete frame,
+		/* Since we have allocated space to hold a complete frame,
 		 * the last indicator should be set.
 		 */
 		if ((sc & BD_ENET_RX_LAST) == 0)
 			dev_warn(fep->dev, "rcv is not +last\n");
 
-		/*
-		 * Check for errors.
-		 */
+		/* Check for errors. */
 		if (sc & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_CL |
 			  BD_ENET_RX_NO | BD_ENET_RX_CR | BD_ENET_RX_OV)) {
 			dev->stats.rx_errors++;
@@ -225,9 +211,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 		} else {
 			skb = fep->rx_skbuff[curidx];
 
-			/*
-			 * Process the incoming frame.
-			 */
+			/* Process the incoming frame */
 			dev->stats.rx_packets++;
 			pkt_len = CBDR_DATLEN(bdp) - 4;	/* remove CRC */
 			dev->stats.rx_bytes += pkt_len + 4;
@@ -235,15 +219,15 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 			if (pkt_len <= fpi->rx_copybreak) {
 				/* +2 to make IP header L1 cache aligned */
 				skbn = netdev_alloc_skb(dev, pkt_len + 2);
-				if (skbn != NULL) {
+				if (skbn) {
 					skb_reserve(skbn, 2);	/* align IP header */
-					skb_copy_from_linear_data(skb,
-						      skbn->data, pkt_len);
+					skb_copy_from_linear_data(skb, skbn->data,
+								  pkt_len);
 					swap(skb, skbn);
 					dma_sync_single_for_cpu(fep->dev,
-						CBDR_BUFADDR(bdp),
-						L1_CACHE_ALIGN(pkt_len),
-						DMA_FROM_DEVICE);
+								CBDR_BUFADDR(bdp),
+								L1_CACHE_ALIGN(pkt_len),
+								DMA_FROM_DEVICE);
 				}
 			} else {
 				skbn = netdev_alloc_skb(dev, ENET_RX_FRSIZE);
@@ -253,20 +237,18 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 
 					skb_align(skbn, ENET_RX_ALIGN);
 
-					dma_unmap_single(fep->dev,
-						CBDR_BUFADDR(bdp),
-						L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
-						DMA_FROM_DEVICE);
+					dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
+							 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
+							 DMA_FROM_DEVICE);
 
-					dma = dma_map_single(fep->dev,
-						skbn->data,
-						L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
-						DMA_FROM_DEVICE);
+					dma = dma_map_single(fep->dev, skbn->data,
+							     L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
+							     DMA_FROM_DEVICE);
 					CBDW_BUFADDR(bdp, dma);
 				}
 			}
 
-			if (skbn != NULL) {
+			if (skbn) {
 				skb_put(skb, pkt_len);	/* Make room */
 				skb->protocol = eth_type_trans(skb, dev);
 				received++;
@@ -281,9 +263,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 		CBDW_DATLEN(bdp, 0);
 		CBDW_SC(bdp, (sc & ~BD_ENET_RX_STATS) | BD_ENET_RX_EMPTY);
 
-		/*
-		 * Update BD pointer to next entry.
-		 */
+		/* Update BD pointer to next entry */
 		if ((sc & BD_ENET_RX_WRAP) == 0)
 			bdp++;
 		else
@@ -305,19 +285,16 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
 	return budget;
 }
 
-/*
- * The interrupt handler.
+/* The interrupt handler.
  * This is called from the MPC core interrupt.
  */
 static irqreturn_t
 fs_enet_interrupt(int irq, void *dev_id)
 {
 	struct net_device *dev = dev_id;
+	u32 int_events, int_clr_events;
 	struct fs_enet_private *fep;
-	u32 int_events;
-	u32 int_clr_events;
-	int nr, napi_ok;
-	int handled;
+	int nr, napi_ok, handled;
 
 	fep = netdev_priv(dev);
 
@@ -339,12 +316,12 @@ fs_enet_interrupt(int irq, void *dev_id)
 			(*fep->ops->napi_disable)(dev);
 			(*fep->ops->clear_int_events)(dev, fep->ev_napi);
 
-			/* NOTE: it is possible for FCCs in NAPI mode    */
-			/* to submit a spurious interrupt while in poll  */
+			/* NOTE: it is possible for FCCs in NAPI mode
+			 * to submit a spurious interrupt while in poll
+			 */
 			if (napi_ok)
 				__napi_schedule(&fep->napi);
 		}
-
 	}
 
 	handled = nr > 0;
@@ -354,45 +331,40 @@ fs_enet_interrupt(int irq, void *dev_id)
 void fs_init_bds(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
-	cbd_t __iomem *bdp;
 	struct sk_buff *skb;
+	cbd_t __iomem *bdp;
 	int i;
 
 	fs_cleanup_bds(dev);
 
-	fep->dirty_tx = fep->cur_tx = fep->tx_bd_base;
+	fep->dirty_tx = fep->tx_bd_base;
+	fep->cur_tx = fep->tx_bd_base;
 	fep->tx_free = fep->tx_ring;
 	fep->cur_rx = fep->rx_bd_base;
 
-	/*
-	 * Initialize the receive buffer descriptors.
-	 */
+	/* Initialize the receive buffer descriptors */
 	for (i = 0, bdp = fep->rx_bd_base; i < fep->rx_ring; i++, bdp++) {
 		skb = netdev_alloc_skb(dev, ENET_RX_FRSIZE);
-		if (skb == NULL)
+		if (!skb)
 			break;
 
 		skb_align(skb, ENET_RX_ALIGN);
 		fep->rx_skbuff[i] = skb;
-		CBDW_BUFADDR(bdp,
-			dma_map_single(fep->dev, skb->data,
-				L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
-				DMA_FROM_DEVICE));
+		CBDW_BUFADDR(bdp, dma_map_single(fep->dev, skb->data,
+						 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
+						 DMA_FROM_DEVICE));
 		CBDW_DATLEN(bdp, 0);	/* zero */
 		CBDW_SC(bdp, BD_ENET_RX_EMPTY |
 			((i < fep->rx_ring - 1) ? 0 : BD_SC_WRAP));
 	}
-	/*
-	 * if we failed, fillup remainder
-	 */
+
+	/* if we failed, fillup remainder */
 	for (; i < fep->rx_ring; i++, bdp++) {
 		fep->rx_skbuff[i] = NULL;
 		CBDW_SC(bdp, (i < fep->rx_ring - 1) ? 0 : BD_SC_WRAP);
 	}
 
-	/*
-	 * ...and the same for transmit.
-	 */
+	/* ...and the same for transmit. */
 	for (i = 0, bdp = fep->tx_bd_base; i < fep->tx_ring; i++, bdp++) {
 		fep->tx_skbuff[i] = NULL;
 		CBDW_BUFADDR(bdp, 0);
@@ -408,32 +380,30 @@ void fs_cleanup_bds(struct net_device *dev)
 	cbd_t __iomem *bdp;
 	int i;
 
-	/*
-	 * Reset SKB transmit buffers.
-	 */
+	/* Reset SKB transmit buffers. */
 	for (i = 0, bdp = fep->tx_bd_base; i < fep->tx_ring; i++, bdp++) {
-		if ((skb = fep->tx_skbuff[i]) == NULL)
+		skb = fep->tx_skbuff[i];
+		if (!skb)
 			continue;
 
 		/* unmap */
 		dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
-				skb->len, DMA_TO_DEVICE);
+				 skb->len, DMA_TO_DEVICE);
 
 		fep->tx_skbuff[i] = NULL;
 		dev_kfree_skb(skb);
 	}
 
-	/*
-	 * Reset SKB receive buffers
-	 */
+	/* Reset SKB receive buffers */
 	for (i = 0, bdp = fep->rx_bd_base; i < fep->rx_ring; i++, bdp++) {
-		if ((skb = fep->rx_skbuff[i]) == NULL)
+		skb = fep->rx_skbuff[i];
+		if (!skb)
 			continue;
 
 		/* unmap */
 		dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
-			L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
-			DMA_FROM_DEVICE);
+				 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
+				 DMA_FROM_DEVICE);
 
 		fep->rx_skbuff[i] = NULL;
 
@@ -441,12 +411,8 @@ void fs_cleanup_bds(struct net_device *dev)
 	}
 }
 
-/**********************************************************************************/
-
 #ifdef CONFIG_FS_ENET_MPC5121_FEC
-/*
- * MPC5121 FEC requeries 4-byte alignment for TX data buffer!
- */
+/* MPC5121 FEC requires 4-byte alignment for TX data buffer! */
 static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
 					       struct sk_buff *skb)
 {
@@ -478,15 +444,12 @@ static netdev_tx_t
 fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+	int curidx, nr_frags, len;
 	cbd_t __iomem *bdp;
-	int curidx;
-	u16 sc;
-	int nr_frags;
 	skb_frag_t *frag;
-	int len;
+	u16 sc;
 #ifdef CONFIG_FS_ENET_MPC5121_FEC
-	int is_aligned = 1;
-	int i;
+	int i, is_aligned = 1;
 
 	if (!IS_ALIGNED((unsigned long)skb->data, 4)) {
 		is_aligned = 0;
@@ -504,8 +467,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	if (!is_aligned) {
 		skb = tx_skb_align_workaround(dev, skb);
 		if (!skb) {
-			/*
-			 * We have lost packet due to memory allocation error
+			/* We have lost packet due to memory allocation error
 			 * in tx_skb_align_workaround(). Hopefully original
 			 * skb is still valid, so try transmit it later.
 			 */
@@ -516,9 +478,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	spin_lock(&fep->tx_lock);
 
-	/*
-	 * Fill in a Tx ring entry
-	 */
+	/* Fill in a Tx ring entry */
 	bdp = fep->cur_tx;
 
 	nr_frags = skb_shinfo(skb)->nr_frags;
@@ -526,8 +486,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		netif_stop_queue(dev);
 		spin_unlock(&fep->tx_lock);
 
-		/*
-		 * Ooops.  All transmit buffers are full.  Bail out.
+		/* Ooops.  All transmit buffers are full.  Bail out.
 		 * This should not happen, since the tx queue should be stopped.
 		 */
 		dev_warn(fep->dev, "tx queue full!.\n");
@@ -540,12 +499,12 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.tx_bytes += len;
 	if (nr_frags)
 		len -= skb->data_len;
+
 	fep->tx_free -= nr_frags + 1;
-	/*
-	 * Push the data cache so the CPM does not get stale memory data.
+	/* Push the data cache so the CPM does not get stale memory data.
 	 */
 	CBDW_BUFADDR(bdp, dma_map_single(fep->dev,
-				skb->data, len, DMA_TO_DEVICE));
+					 skb->data, len, DMA_TO_DEVICE));
 	CBDW_DATLEN(bdp, len);
 
 	fep->mapped_as_page[curidx] = 0;
@@ -582,9 +541,11 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 
 	/* note that while FEC does not have this bit
 	 * it marks it as available for software use
-	 * yay for hw reuse :) */
+	 * yay for hw reuse :)
+	 */
 	if (skb->len <= 60)
 		sc |= BD_ENET_TX_PAD;
+
 	CBDC_SC(bdp, BD_ENET_TX_STATS);
 	CBDS_SC(bdp, sc);
 
@@ -596,6 +557,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
 		bdp++;
 	else
 		bdp = fep->tx_bd_base;
+
 	fep->cur_tx = bdp;
 
 	if (fep->tx_free < MAX_SKB_FRAGS)
@@ -644,9 +606,7 @@ static void fs_timeout(struct net_device *dev, unsigned int txqueue)
 	schedule_work(&fep->timeout_work);
 }
 
-/*-----------------------------------------------------------------------------
- *  generic link-change handler - should be sufficient for most cases
- *-----------------------------------------------------------------------------*/
+/* generic link-change handler - should be sufficient for most cases */
 static void generic_adjust_link(struct  net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
@@ -683,7 +643,6 @@ static void generic_adjust_link(struct  net_device *dev)
 		phy_print_status(phydev);
 }
 
-
 static void fs_adjust_link(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
@@ -691,7 +650,7 @@ static void fs_adjust_link(struct net_device *dev)
 
 	spin_lock_irqsave(&fep->lock, flags);
 
-	if(fep->ops->adjust_link)
+	if (fep->ops->adjust_link)
 		fep->ops->adjust_link(dev);
 	else
 		generic_adjust_link(dev);
@@ -728,8 +687,9 @@ static int fs_enet_open(struct net_device *dev)
 	int r;
 	int err;
 
-	/* to initialize the fep->cur_rx,... */
-	/* not doing this, will cause a crash in fs_enet_napi */
+	/* to initialize the fep->cur_rx,...
+	 * not doing this, will cause a crash in fs_enet_napi
+	 */
 	fs_init_bds(fep->ndev);
 
 	napi_enable(&fep->napi);
@@ -780,10 +740,8 @@ static int fs_enet_close(struct net_device *dev)
 	return 0;
 }
 
-/*************************************************************************/
-
 static void fs_get_drvinfo(struct net_device *dev,
-			    struct ethtool_drvinfo *info)
+			   struct ethtool_drvinfo *info)
 {
 	strscpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
 }
@@ -796,7 +754,7 @@ static int fs_get_regs_len(struct net_device *dev)
 }
 
 static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
-			 void *p)
+			void *p)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 	unsigned long flags;
@@ -815,12 +773,14 @@ static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
 static u32 fs_get_msglevel(struct net_device *dev)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+
 	return fep->msg_enable;
 }
 
 static void fs_set_msglevel(struct net_device *dev, u32 value)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
+
 	fep->msg_enable = value;
 }
 
@@ -877,8 +837,6 @@ static const struct ethtool_ops fs_ethtool_ops = {
 	.set_tunable = fs_set_tunable,
 };
 
-/**************************************************************************************/
-
 #ifdef CONFIG_FS_ENET_HAS_FEC
 #define IS_FEC(ops) ((ops) == &fs_fec_ops)
 #else
@@ -901,15 +859,14 @@ static const struct net_device_ops fs_enet_netdev_ops = {
 
 static int fs_enet_probe(struct platform_device *ofdev)
 {
+	int err, privsize, len, ret = -ENODEV;
+	const char *phy_connection_type;
+	struct fs_platform_info *fpi;
+	struct fs_enet_private *fep;
 	const struct fs_ops *ops;
 	struct net_device *ndev;
-	struct fs_enet_private *fep;
-	struct fs_platform_info *fpi;
 	const u32 *data;
 	struct clk *clk;
-	int err;
-	const char *phy_connection_type;
-	int privsize, len, ret = -ENODEV;
 
 	ops = device_get_match_data(&ofdev->dev);
 	if (!ops)
@@ -945,7 +902,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
 
 	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec")) {
 		phy_connection_type = of_get_property(ofdev->dev.of_node,
-						"phy-connection-type", NULL);
+						      "phy-connection-type",
+						      NULL);
 		if (phy_connection_type && !strcmp("rmii", phy_connection_type))
 			fpi->use_rmii = 1;
 	}
@@ -964,7 +922,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
 	}
 
 	privsize = sizeof(*fep) +
-	           sizeof(struct sk_buff **) *
+		   sizeof(struct sk_buff **) *
 		     (fpi->rx_ring + fpi->tx_ring) +
 		   sizeof(char) * fpi->tx_ring;
 
@@ -1111,9 +1069,9 @@ static struct platform_driver fs_enet_driver = {
 #ifdef CONFIG_NET_POLL_CONTROLLER
 static void fs_enet_netpoll(struct net_device *dev)
 {
-       disable_irq(dev->irq);
-       fs_enet_interrupt(dev->irq, dev);
-       enable_irq(dev->irq);
+	disable_irq(dev->irq);
+	fs_enet_interrupt(dev->irq, dev);
+	enable_irq(dev->irq);
 }
 #endif
 
-- 
2.45.2

Re: [PATCH net-next 2/6] net: ethernet: fs_enet: cosmetic cleanups

[Christophe Leroy]

Le 28/08/2024 à 11:50, Maxime Chevallier a écrit :
> Due to the age of the driver and the slow recent activity on it, the code
> has taken some layers of dust. Clean the main driver file up so that it
> passes checkpatch and also conforms with the net coding style.
> 
> Changes include :
>   - Re-ordering of the variable declarations for RCT
>   - Fixing the comment styles to either one-line comments, or net-style
>     comments
>   - Adding braces around single-statement 'else' clauses
>   - Aligning function/macro parameters on the opening parenthesis
>   - Simplifying checks for NULL pointers
>   - Splitting cascaded assignments into individual assignments
>   - Fixing some typos
>   - Fixing whitespace issues
> 
> This is a cosmetic change and doesn't introduce any change in behaviour.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   .../ethernet/freescale/fs_enet/fs_enet-main.c | 220 +++++++-----------
>   1 file changed, 89 insertions(+), 131 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index 5bfdd43ffdeb..2b48a2a5e32d 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -81,15 +81,14 @@ static void skb_align(struct sk_buff *skb, int align)
>   static int fs_enet_napi(struct napi_struct *napi, int budget)
>   {
>   	struct fs_enet_private *fep = container_of(napi, struct fs_enet_private, napi);
> -	struct net_device *dev = fep->ndev;
>   	const struct fs_platform_info *fpi = fep->fpi;
> -	cbd_t __iomem *bdp;
> +	struct net_device *dev = fep->ndev;
> +	int curidx, dirtyidx, received = 0;
> +	int do_wake = 0, do_restart = 0;
> +	int tx_left = TX_RING_SIZE;
>   	struct sk_buff *skb, *skbn;
> -	int received = 0;
> +	cbd_t __iomem *bdp;
>   	u16 pkt_len, sc;
> -	int curidx;
> -	int dirtyidx, do_wake, do_restart;
> -	int tx_left = TX_RING_SIZE;
>   
>   	spin_lock(&fep->tx_lock);
>   	bdp = fep->dirty_tx;
> @@ -97,7 +96,6 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   	/* clear status bits for napi*/
>   	(*fep->ops->napi_clear_event)(dev);
>   
> -	do_wake = do_restart = 0;
>   	while (((sc = CBDR_SC(bdp)) & BD_ENET_TX_READY) == 0 && tx_left) {
>   		dirtyidx = bdp - fep->tx_bd_base;
>   
> @@ -106,12 +104,9 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   
>   		skb = fep->tx_skbuff[dirtyidx];
>   
> -		/*
> -		 * Check for errors.
> -		 */
> +		 /* Check for errors. */
>   		if (sc & (BD_ENET_TX_HB | BD_ENET_TX_LC |
>   			  BD_ENET_TX_RL | BD_ENET_TX_UN | BD_ENET_TX_CSL)) {
> -
>   			if (sc & BD_ENET_TX_HB)	/* No heartbeat */
>   				dev->stats.tx_heartbeat_errors++;
>   			if (sc & BD_ENET_TX_LC)	/* Late collision */
> @@ -127,16 +122,16 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   				dev->stats.tx_errors++;
>   				do_restart = 1;
>   			}
> -		} else
> +		} else {
>   			dev->stats.tx_packets++;
> +		}
>   
>   		if (sc & BD_ENET_TX_READY) {
>   			dev_warn(fep->dev,
>   				 "HEY! Enet xmit interrupt and TX_READY.\n");
>   		}
>   
> -		/*
> -		 * Deferred means some collisions occurred during transmit,
> +		/* Deferred means some collisions occurred during transmit,
>   		 * but we eventually sent the packet OK.
>   		 */
>   		if (sc & BD_ENET_TX_DEF)
> @@ -150,25 +145,20 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   			dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
>   					 CBDR_DATLEN(bdp), DMA_TO_DEVICE);
>   
> -		/*
> -		 * Free the sk buffer associated with this last transmit.
> -		 */
> +		/* Free the sk buffer associated with this last transmit. */
>   		if (skb) {
>   			dev_kfree_skb(skb);
>   			fep->tx_skbuff[dirtyidx] = NULL;
>   		}
>   
> -		/*
> -		 * Update pointer to next buffer descriptor to be transmitted.
> +		/* Update pointer to next buffer descriptor to be transmitted.
>   		 */
>   		if ((sc & BD_ENET_TX_WRAP) == 0)
>   			bdp++;
>   		else
>   			bdp = fep->tx_bd_base;
>   
> -		/*
> -		 * Since we have freed up a buffer, the ring is no longer
> -		 * full.
> +		/* Since we have freed up a buffer, the ring is no longer full.
>   		 */
>   		if (++fep->tx_free == MAX_SKB_FRAGS)
>   			do_wake = 1;
> @@ -185,8 +175,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   	if (do_wake)
>   		netif_wake_queue(dev);
>   
> -	/*
> -	 * First, grab all of the stats for the incoming packet.
> +	/* First, grab all of the stats for the incoming packet.
>   	 * These get messed up if we get called due to a busy condition.
>   	 */
>   	bdp = fep->cur_rx;
> @@ -195,16 +184,13 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   	       received < budget) {
>   		curidx = bdp - fep->rx_bd_base;
>   
> -		/*
> -		 * Since we have allocated space to hold a complete frame,
> +		/* Since we have allocated space to hold a complete frame,
>   		 * the last indicator should be set.
>   		 */
>   		if ((sc & BD_ENET_RX_LAST) == 0)
>   			dev_warn(fep->dev, "rcv is not +last\n");
>   
> -		/*
> -		 * Check for errors.
> -		 */
> +		/* Check for errors. */
>   		if (sc & (BD_ENET_RX_LG | BD_ENET_RX_SH | BD_ENET_RX_CL |
>   			  BD_ENET_RX_NO | BD_ENET_RX_CR | BD_ENET_RX_OV)) {
>   			dev->stats.rx_errors++;
> @@ -225,9 +211,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   		} else {
>   			skb = fep->rx_skbuff[curidx];
>   
> -			/*
> -			 * Process the incoming frame.
> -			 */
> +			/* Process the incoming frame */
>   			dev->stats.rx_packets++;
>   			pkt_len = CBDR_DATLEN(bdp) - 4;	/* remove CRC */
>   			dev->stats.rx_bytes += pkt_len + 4;
> @@ -235,15 +219,15 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   			if (pkt_len <= fpi->rx_copybreak) {
>   				/* +2 to make IP header L1 cache aligned */
>   				skbn = netdev_alloc_skb(dev, pkt_len + 2);
> -				if (skbn != NULL) {
> +				if (skbn) {
>   					skb_reserve(skbn, 2);	/* align IP header */
> -					skb_copy_from_linear_data(skb,
> -						      skbn->data, pkt_len);
> +					skb_copy_from_linear_data(skb, skbn->data,
> +								  pkt_len);
>   					swap(skb, skbn);
>   					dma_sync_single_for_cpu(fep->dev,
> -						CBDR_BUFADDR(bdp),
> -						L1_CACHE_ALIGN(pkt_len),
> -						DMA_FROM_DEVICE);
> +								CBDR_BUFADDR(bdp),
> +								L1_CACHE_ALIGN(pkt_len),
> +								DMA_FROM_DEVICE);
>   				}
>   			} else {
>   				skbn = netdev_alloc_skb(dev, ENET_RX_FRSIZE);
> @@ -253,20 +237,18 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   
>   					skb_align(skbn, ENET_RX_ALIGN);
>   
> -					dma_unmap_single(fep->dev,
> -						CBDR_BUFADDR(bdp),
> -						L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> -						DMA_FROM_DEVICE);
> +					dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
> +							 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> +							 DMA_FROM_DEVICE);
>   
> -					dma = dma_map_single(fep->dev,
> -						skbn->data,
> -						L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> -						DMA_FROM_DEVICE);
> +					dma = dma_map_single(fep->dev, skbn->data,
> +							     L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> +							     DMA_FROM_DEVICE);
>   					CBDW_BUFADDR(bdp, dma);
>   				}
>   			}
>   
> -			if (skbn != NULL) {
> +			if (skbn) {
>   				skb_put(skb, pkt_len);	/* Make room */
>   				skb->protocol = eth_type_trans(skb, dev);
>   				received++;
> @@ -281,9 +263,7 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   		CBDW_DATLEN(bdp, 0);
>   		CBDW_SC(bdp, (sc & ~BD_ENET_RX_STATS) | BD_ENET_RX_EMPTY);
>   
> -		/*
> -		 * Update BD pointer to next entry.
> -		 */
> +		/* Update BD pointer to next entry */
>   		if ((sc & BD_ENET_RX_WRAP) == 0)
>   			bdp++;
>   		else
> @@ -305,19 +285,16 @@ static int fs_enet_napi(struct napi_struct *napi, int budget)
>   	return budget;
>   }
>   
> -/*
> - * The interrupt handler.
> +/* The interrupt handler.
>    * This is called from the MPC core interrupt.
>    */
>   static irqreturn_t
>   fs_enet_interrupt(int irq, void *dev_id)
>   {
>   	struct net_device *dev = dev_id;
> +	u32 int_events, int_clr_events;
>   	struct fs_enet_private *fep;
> -	u32 int_events;
> -	u32 int_clr_events;
> -	int nr, napi_ok;
> -	int handled;
> +	int nr, napi_ok, handled;
>   
>   	fep = netdev_priv(dev);
>   
> @@ -339,12 +316,12 @@ fs_enet_interrupt(int irq, void *dev_id)
>   			(*fep->ops->napi_disable)(dev);
>   			(*fep->ops->clear_int_events)(dev, fep->ev_napi);
>   
> -			/* NOTE: it is possible for FCCs in NAPI mode    */
> -			/* to submit a spurious interrupt while in poll  */
> +			/* NOTE: it is possible for FCCs in NAPI mode
> +			 * to submit a spurious interrupt while in poll
> +			 */
>   			if (napi_ok)
>   				__napi_schedule(&fep->napi);
>   		}
> -
>   	}
>   
>   	handled = nr > 0;
> @@ -354,45 +331,40 @@ fs_enet_interrupt(int irq, void *dev_id)
>   void fs_init_bds(struct net_device *dev)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> -	cbd_t __iomem *bdp;
>   	struct sk_buff *skb;
> +	cbd_t __iomem *bdp;
>   	int i;
>   
>   	fs_cleanup_bds(dev);
>   
> -	fep->dirty_tx = fep->cur_tx = fep->tx_bd_base;
> +	fep->dirty_tx = fep->tx_bd_base;
> +	fep->cur_tx = fep->tx_bd_base;
>   	fep->tx_free = fep->tx_ring;
>   	fep->cur_rx = fep->rx_bd_base;
>   
> -	/*
> -	 * Initialize the receive buffer descriptors.
> -	 */
> +	/* Initialize the receive buffer descriptors */
>   	for (i = 0, bdp = fep->rx_bd_base; i < fep->rx_ring; i++, bdp++) {
>   		skb = netdev_alloc_skb(dev, ENET_RX_FRSIZE);
> -		if (skb == NULL)
> +		if (!skb)
>   			break;
>   
>   		skb_align(skb, ENET_RX_ALIGN);
>   		fep->rx_skbuff[i] = skb;
> -		CBDW_BUFADDR(bdp,
> -			dma_map_single(fep->dev, skb->data,
> -				L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> -				DMA_FROM_DEVICE));
> +		CBDW_BUFADDR(bdp, dma_map_single(fep->dev, skb->data,
> +						 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> +						 DMA_FROM_DEVICE));
>   		CBDW_DATLEN(bdp, 0);	/* zero */
>   		CBDW_SC(bdp, BD_ENET_RX_EMPTY |
>   			((i < fep->rx_ring - 1) ? 0 : BD_SC_WRAP));
>   	}
> -	/*
> -	 * if we failed, fillup remainder
> -	 */
> +
> +	/* if we failed, fillup remainder */
>   	for (; i < fep->rx_ring; i++, bdp++) {
>   		fep->rx_skbuff[i] = NULL;
>   		CBDW_SC(bdp, (i < fep->rx_ring - 1) ? 0 : BD_SC_WRAP);
>   	}
>   
> -	/*
> -	 * ...and the same for transmit.
> -	 */
> +	/* ...and the same for transmit. */
>   	for (i = 0, bdp = fep->tx_bd_base; i < fep->tx_ring; i++, bdp++) {
>   		fep->tx_skbuff[i] = NULL;
>   		CBDW_BUFADDR(bdp, 0);
> @@ -408,32 +380,30 @@ void fs_cleanup_bds(struct net_device *dev)
>   	cbd_t __iomem *bdp;
>   	int i;
>   
> -	/*
> -	 * Reset SKB transmit buffers.
> -	 */
> +	/* Reset SKB transmit buffers. */
>   	for (i = 0, bdp = fep->tx_bd_base; i < fep->tx_ring; i++, bdp++) {
> -		if ((skb = fep->tx_skbuff[i]) == NULL)
> +		skb = fep->tx_skbuff[i];
> +		if (!skb)
>   			continue;
>   
>   		/* unmap */
>   		dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
> -				skb->len, DMA_TO_DEVICE);
> +				 skb->len, DMA_TO_DEVICE);
>   
>   		fep->tx_skbuff[i] = NULL;
>   		dev_kfree_skb(skb);
>   	}
>   
> -	/*
> -	 * Reset SKB receive buffers
> -	 */
> +	/* Reset SKB receive buffers */
>   	for (i = 0, bdp = fep->rx_bd_base; i < fep->rx_ring; i++, bdp++) {
> -		if ((skb = fep->rx_skbuff[i]) == NULL)
> +		skb = fep->rx_skbuff[i];
> +		if (!skb)
>   			continue;
>   
>   		/* unmap */
>   		dma_unmap_single(fep->dev, CBDR_BUFADDR(bdp),
> -			L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> -			DMA_FROM_DEVICE);
> +				 L1_CACHE_ALIGN(PKT_MAXBUF_SIZE),
> +				 DMA_FROM_DEVICE);
>   
>   		fep->rx_skbuff[i] = NULL;
>   
> @@ -441,12 +411,8 @@ void fs_cleanup_bds(struct net_device *dev)
>   	}
>   }
>   
> -/**********************************************************************************/
> -
>   #ifdef CONFIG_FS_ENET_MPC5121_FEC
> -/*
> - * MPC5121 FEC requeries 4-byte alignment for TX data buffer!
> - */
> +/* MPC5121 FEC requires 4-byte alignment for TX data buffer! */
>   static struct sk_buff *tx_skb_align_workaround(struct net_device *dev,
>   					       struct sk_buff *skb)
>   {
> @@ -478,15 +444,12 @@ static netdev_tx_t
>   fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> +	int curidx, nr_frags, len;
>   	cbd_t __iomem *bdp;
> -	int curidx;
> -	u16 sc;
> -	int nr_frags;
>   	skb_frag_t *frag;
> -	int len;
> +	u16 sc;
>   #ifdef CONFIG_FS_ENET_MPC5121_FEC
> -	int is_aligned = 1;
> -	int i;
> +	int i, is_aligned = 1;
>   
>   	if (!IS_ALIGNED((unsigned long)skb->data, 4)) {
>   		is_aligned = 0;
> @@ -504,8 +467,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	if (!is_aligned) {
>   		skb = tx_skb_align_workaround(dev, skb);
>   		if (!skb) {
> -			/*
> -			 * We have lost packet due to memory allocation error
> +			/* We have lost packet due to memory allocation error
>   			 * in tx_skb_align_workaround(). Hopefully original
>   			 * skb is still valid, so try transmit it later.
>   			 */
> @@ -516,9 +478,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   
>   	spin_lock(&fep->tx_lock);
>   
> -	/*
> -	 * Fill in a Tx ring entry
> -	 */
> +	/* Fill in a Tx ring entry */
>   	bdp = fep->cur_tx;
>   
>   	nr_frags = skb_shinfo(skb)->nr_frags;
> @@ -526,8 +486,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   		netif_stop_queue(dev);
>   		spin_unlock(&fep->tx_lock);
>   
> -		/*
> -		 * Ooops.  All transmit buffers are full.  Bail out.
> +		/* Ooops.  All transmit buffers are full.  Bail out.
>   		 * This should not happen, since the tx queue should be stopped.
>   		 */
>   		dev_warn(fep->dev, "tx queue full!.\n");
> @@ -540,12 +499,12 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   	dev->stats.tx_bytes += len;
>   	if (nr_frags)
>   		len -= skb->data_len;
> +
>   	fep->tx_free -= nr_frags + 1;
> -	/*
> -	 * Push the data cache so the CPM does not get stale memory data.
> +	/* Push the data cache so the CPM does not get stale memory data.
>   	 */
>   	CBDW_BUFADDR(bdp, dma_map_single(fep->dev,
> -				skb->data, len, DMA_TO_DEVICE));
> +					 skb->data, len, DMA_TO_DEVICE));
>   	CBDW_DATLEN(bdp, len);
>   
>   	fep->mapped_as_page[curidx] = 0;
> @@ -582,9 +541,11 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   
>   	/* note that while FEC does not have this bit
>   	 * it marks it as available for software use
> -	 * yay for hw reuse :) */
> +	 * yay for hw reuse :)
> +	 */
>   	if (skb->len <= 60)
>   		sc |= BD_ENET_TX_PAD;
> +
>   	CBDC_SC(bdp, BD_ENET_TX_STATS);
>   	CBDS_SC(bdp, sc);
>   
> @@ -596,6 +557,7 @@ fs_enet_start_xmit(struct sk_buff *skb, struct net_device *dev)
>   		bdp++;
>   	else
>   		bdp = fep->tx_bd_base;
> +
>   	fep->cur_tx = bdp;
>   
>   	if (fep->tx_free < MAX_SKB_FRAGS)
> @@ -644,9 +606,7 @@ static void fs_timeout(struct net_device *dev, unsigned int txqueue)
>   	schedule_work(&fep->timeout_work);
>   }
>   
> -/*-----------------------------------------------------------------------------
> - *  generic link-change handler - should be sufficient for most cases
> - *-----------------------------------------------------------------------------*/
> +/* generic link-change handler - should be sufficient for most cases */
>   static void generic_adjust_link(struct  net_device *dev)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> @@ -683,7 +643,6 @@ static void generic_adjust_link(struct  net_device *dev)
>   		phy_print_status(phydev);
>   }
>   
> -
>   static void fs_adjust_link(struct net_device *dev)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> @@ -691,7 +650,7 @@ static void fs_adjust_link(struct net_device *dev)
>   
>   	spin_lock_irqsave(&fep->lock, flags);
>   
> -	if(fep->ops->adjust_link)
> +	if (fep->ops->adjust_link)
>   		fep->ops->adjust_link(dev);
>   	else
>   		generic_adjust_link(dev);
> @@ -728,8 +687,9 @@ static int fs_enet_open(struct net_device *dev)
>   	int r;
>   	int err;
>   
> -	/* to initialize the fep->cur_rx,... */
> -	/* not doing this, will cause a crash in fs_enet_napi */
> +	/* to initialize the fep->cur_rx,...
> +	 * not doing this, will cause a crash in fs_enet_napi
> +	 */
>   	fs_init_bds(fep->ndev);
>   
>   	napi_enable(&fep->napi);
> @@ -780,10 +740,8 @@ static int fs_enet_close(struct net_device *dev)
>   	return 0;
>   }
>   
> -/*************************************************************************/
> -
>   static void fs_get_drvinfo(struct net_device *dev,
> -			    struct ethtool_drvinfo *info)
> +			   struct ethtool_drvinfo *info)
>   {
>   	strscpy(info->driver, DRV_MODULE_NAME, sizeof(info->driver));
>   }
> @@ -796,7 +754,7 @@ static int fs_get_regs_len(struct net_device *dev)
>   }
>   
>   static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
> -			 void *p)
> +			void *p)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
>   	unsigned long flags;
> @@ -815,12 +773,14 @@ static void fs_get_regs(struct net_device *dev, struct ethtool_regs *regs,
>   static u32 fs_get_msglevel(struct net_device *dev)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> +
>   	return fep->msg_enable;
>   }
>   
>   static void fs_set_msglevel(struct net_device *dev, u32 value)
>   {
>   	struct fs_enet_private *fep = netdev_priv(dev);
> +
>   	fep->msg_enable = value;
>   }
>   
> @@ -877,8 +837,6 @@ static const struct ethtool_ops fs_ethtool_ops = {
>   	.set_tunable = fs_set_tunable,
>   };
>   
> -/**************************************************************************************/
> -
>   #ifdef CONFIG_FS_ENET_HAS_FEC
>   #define IS_FEC(ops) ((ops) == &fs_fec_ops)
>   #else
> @@ -901,15 +859,14 @@ static const struct net_device_ops fs_enet_netdev_ops = {
>   
>   static int fs_enet_probe(struct platform_device *ofdev)
>   {
> +	int err, privsize, len, ret = -ENODEV;
> +	const char *phy_connection_type;
> +	struct fs_platform_info *fpi;
> +	struct fs_enet_private *fep;
>   	const struct fs_ops *ops;
>   	struct net_device *ndev;
> -	struct fs_enet_private *fep;
> -	struct fs_platform_info *fpi;
>   	const u32 *data;
>   	struct clk *clk;
> -	int err;
> -	const char *phy_connection_type;
> -	int privsize, len, ret = -ENODEV;
>   
>   	ops = device_get_match_data(&ofdev->dev);
>   	if (!ops)
> @@ -945,7 +902,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
>   
>   	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec")) {
>   		phy_connection_type = of_get_property(ofdev->dev.of_node,
> -						"phy-connection-type", NULL);
> +						      "phy-connection-type",
> +						      NULL);
>   		if (phy_connection_type && !strcmp("rmii", phy_connection_type))
>   			fpi->use_rmii = 1;
>   	}
> @@ -964,7 +922,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
>   	}
>   
>   	privsize = sizeof(*fep) +
> -	           sizeof(struct sk_buff **) *
> +		   sizeof(struct sk_buff **) *
>   		     (fpi->rx_ring + fpi->tx_ring) +
>   		   sizeof(char) * fpi->tx_ring;
>   
> @@ -1111,9 +1069,9 @@ static struct platform_driver fs_enet_driver = {
>   #ifdef CONFIG_NET_POLL_CONTROLLER
>   static void fs_enet_netpoll(struct net_device *dev)
>   {
> -       disable_irq(dev->irq);
> -       fs_enet_interrupt(dev->irq, dev);
> -       enable_irq(dev->irq);
> +	disable_irq(dev->irq);
> +	fs_enet_interrupt(dev->irq, dev);
> +	enable_irq(dev->irq);
>   }
>   #endif
>   

[PATCH net-next 3/6] net: ethernet: fs_enet: drop the .adjust_link custom fs_ops

[Maxime Chevallier]

There's no in-tree user for the fs_ops .adjust_link() function, so we
can always use the generic one in fe_enet-main.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 7 +------
 drivers/net/ethernet/freescale/fs_enet/fs_enet.h      | 1 -
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index 2b48a2a5e32d..caca81b3ccb6 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -649,12 +649,7 @@ static void fs_adjust_link(struct net_device *dev)
 	unsigned long flags;
 
 	spin_lock_irqsave(&fep->lock, flags);
-
-	if (fep->ops->adjust_link)
-		fep->ops->adjust_link(dev);
-	else
-		generic_adjust_link(dev);
-
+	generic_adjust_link(dev);
 	spin_unlock_irqrestore(&fep->lock, flags);
 }
 
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
index 21c07ac05225..abe4dc97e52a 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
@@ -77,7 +77,6 @@ struct fs_ops {
 	void (*free_bd)(struct net_device *dev);
 	void (*cleanup_data)(struct net_device *dev);
 	void (*set_multicast_list)(struct net_device *dev);
-	void (*adjust_link)(struct net_device *dev);
 	void (*restart)(struct net_device *dev);
 	void (*stop)(struct net_device *dev);
 	void (*napi_clear_event)(struct net_device *dev);
-- 
2.45.2

Re: [PATCH net-next 3/6] net: ethernet: fs_enet: drop the .adjust_link custom fs_ops

[Christophe Leroy]

Le 28/08/2024 à 11:50, Maxime Chevallier a écrit :
> There's no in-tree user for the fs_ops .adjust_link() function, so we
> can always use the generic one in fe_enet-main.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> ---
>   drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c | 7 +------
>   drivers/net/ethernet/freescale/fs_enet/fs_enet.h      | 1 -
>   2 files changed, 1 insertion(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> index 2b48a2a5e32d..caca81b3ccb6 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
> @@ -649,12 +649,7 @@ static void fs_adjust_link(struct net_device *dev)
>   	unsigned long flags;
>   
>   	spin_lock_irqsave(&fep->lock, flags);
> -
> -	if (fep->ops->adjust_link)
> -		fep->ops->adjust_link(dev);
> -	else
> -		generic_adjust_link(dev);
> -
> +	generic_adjust_link(dev);
>   	spin_unlock_irqrestore(&fep->lock, flags);
>   }
>   
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> index 21c07ac05225..abe4dc97e52a 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> @@ -77,7 +77,6 @@ struct fs_ops {
>   	void (*free_bd)(struct net_device *dev);
>   	void (*cleanup_data)(struct net_device *dev);
>   	void (*set_multicast_list)(struct net_device *dev);
> -	void (*adjust_link)(struct net_device *dev);
>   	void (*restart)(struct net_device *dev);
>   	void (*stop)(struct net_device *dev);
>   	void (*napi_clear_event)(struct net_device *dev);

[PATCH net-next 4/6] net: ethernet: fs_enet: drop unused phy_info and mii_if_info

[Maxime Chevallier]

There's no user of the struct phy_info, the 'phy' field and the
mii_if_info in the fs_enet driver, probably dating back when phylib
wasn't as widely used.  Drop these from the driver code.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/freescale/fs_enet/fs_enet.h | 11 -----------
 1 file changed, 11 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
index abe4dc97e52a..781f506c933c 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
@@ -92,14 +92,6 @@ struct fs_ops {
 	void (*tx_restart)(struct net_device *dev);
 };
 
-struct phy_info {
-	unsigned int id;
-	const char *name;
-	void (*startup) (struct net_device * dev);
-	void (*shutdown) (struct net_device * dev);
-	void (*ack_int) (struct net_device * dev);
-};
-
 /* The FEC stores dest/src/type, data, and checksum for receive packets.
  */
 #define MAX_MTU 1508		/* Allow fullsized pppoe packets over VLAN */
@@ -153,10 +145,7 @@ struct fs_enet_private {
 	cbd_t __iomem *cur_rx;
 	cbd_t __iomem *cur_tx;
 	int tx_free;
-	const struct phy_info *phy;
 	u32 msg_enable;
-	struct mii_if_info mii_if;
-	unsigned int last_mii_status;
 	int interrupt;
 
 	int oldduplex, oldspeed, oldlink;	/* current settings */
-- 
2.45.2

Re: [PATCH net-next 4/6] net: ethernet: fs_enet: drop unused phy_info and mii_if_info

[Christophe Leroy]

Le 28/08/2024 à 11:51, Maxime Chevallier a écrit :
> There's no user of the struct phy_info, the 'phy' field and the
> mii_if_info in the fs_enet driver, probably dating back when phylib
> wasn't as widely used.  Drop these from the driver code.

Seems like they haven't been used since commit 5b4b8454344a ("[PATCH] 
FS_ENET: use PAL for mii management")

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>

> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
> ---
>   drivers/net/ethernet/freescale/fs_enet/fs_enet.h | 11 -----------
>   1 file changed, 11 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> index abe4dc97e52a..781f506c933c 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> +++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
> @@ -92,14 +92,6 @@ struct fs_ops {
>   	void (*tx_restart)(struct net_device *dev);
>   };
>   
> -struct phy_info {
> -	unsigned int id;
> -	const char *name;
> -	void (*startup) (struct net_device * dev);
> -	void (*shutdown) (struct net_device * dev);
> -	void (*ack_int) (struct net_device * dev);
> -};
> -
>   /* The FEC stores dest/src/type, data, and checksum for receive packets.
>    */
>   #define MAX_MTU 1508		/* Allow fullsized pppoe packets over VLAN */
> @@ -153,10 +145,7 @@ struct fs_enet_private {
>   	cbd_t __iomem *cur_rx;
>   	cbd_t __iomem *cur_tx;
>   	int tx_free;
> -	const struct phy_info *phy;
>   	u32 msg_enable;
> -	struct mii_if_info mii_if;
> -	unsigned int last_mii_status;
>   	int interrupt;
>   
>   	int oldduplex, oldspeed, oldlink;	/* current settings */

[PATCH net-next 5/6] net: ethernet: fs_enet: fcc: use macros for speed and duplex values

[Maxime Chevallier]

The PHY speed and duplex should be manipulated using the SPEED_XXX and
DUPLEX_XXX macros available. Use it in the fcc, fec and scc MAC for fs_enet.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 drivers/net/ethernet/freescale/fs_enet/mac-fcc.c | 4 ++--
 drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +-
 drivers/net/ethernet/freescale/fs_enet/mac-scc.c | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index add062928d99..056909156b4f 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -361,7 +361,7 @@ static void restart(struct net_device *dev)
 
 	/* adjust to speed (for RMII mode) */
 	if (fpi->use_rmii) {
-		if (dev->phydev->speed == 100)
+		if (dev->phydev->speed == SPEED_100)
 			C8(fcccp, fcc_gfemr, 0x20);
 		else
 			S8(fcccp, fcc_gfemr, 0x20);
@@ -387,7 +387,7 @@ static void restart(struct net_device *dev)
 		S32(fccp, fcc_fpsmr, FCC_PSMR_RMII);
 
 	/* adjust to duplex mode */
-	if (dev->phydev->duplex)
+	if (dev->phydev->duplex == DUPLEX_FULL)
 		S32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
 	else
 		C32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
index f75acb3b358f..855ee9e3f042 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
@@ -309,7 +309,7 @@ static void restart(struct net_device *dev)
 	/*
 	 * adjust to duplex mode
 	 */
-	if (dev->phydev->duplex) {
+	if (dev->phydev->duplex == DUPLEX_FULL) {
 		FC(fecp, r_cntrl, FEC_RCNTRL_DRT);
 		FS(fecp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD enable */
 	} else {
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
index 29ba0048396b..9e5e29312c27 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
@@ -338,7 +338,7 @@ static void restart(struct net_device *dev)
 	W16(sccp, scc_psmr, SCC_PSMR_ENCRC | SCC_PSMR_NIB22);
 
 	/* Set full duplex mode if needed */
-	if (dev->phydev->duplex)
+	if (dev->phydev->duplex == DUPLEX_FULL)
 		S16(sccp, scc_psmr, SCC_PSMR_LPB | SCC_PSMR_FDE);
 
 	/* Restore multicast and promiscuous settings */
-- 
2.45.2

Re: [PATCH net-next 5/6] net: ethernet: fs_enet: fcc: use macros for speed and duplex values

[Christophe Leroy]

Le 28/08/2024 à 11:51, Maxime Chevallier a écrit :
> The PHY speed and duplex should be manipulated using the SPEED_XXX and
> DUPLEX_XXX macros available. Use it in the fcc, fec and scc MAC for fs_enet.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Reviewed-by: Christophe Leroy <christophe.leroy@csgroup.eu>


> ---
>   drivers/net/ethernet/freescale/fs_enet/mac-fcc.c | 4 ++--
>   drivers/net/ethernet/freescale/fs_enet/mac-fec.c | 2 +-
>   drivers/net/ethernet/freescale/fs_enet/mac-scc.c | 2 +-
>   3 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> index add062928d99..056909156b4f 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
> @@ -361,7 +361,7 @@ static void restart(struct net_device *dev)
>   
>   	/* adjust to speed (for RMII mode) */
>   	if (fpi->use_rmii) {
> -		if (dev->phydev->speed == 100)
> +		if (dev->phydev->speed == SPEED_100)
>   			C8(fcccp, fcc_gfemr, 0x20);
>   		else
>   			S8(fcccp, fcc_gfemr, 0x20);
> @@ -387,7 +387,7 @@ static void restart(struct net_device *dev)
>   		S32(fccp, fcc_fpsmr, FCC_PSMR_RMII);
>   
>   	/* adjust to duplex mode */
> -	if (dev->phydev->duplex)
> +	if (dev->phydev->duplex == DUPLEX_FULL)
>   		S32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
>   	else
>   		C32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> index f75acb3b358f..855ee9e3f042 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
> @@ -309,7 +309,7 @@ static void restart(struct net_device *dev)
>   	/*
>   	 * adjust to duplex mode
>   	 */
> -	if (dev->phydev->duplex) {
> +	if (dev->phydev->duplex == DUPLEX_FULL) {
>   		FC(fecp, r_cntrl, FEC_RCNTRL_DRT);
>   		FS(fecp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD enable */
>   	} else {
> diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> index 29ba0048396b..9e5e29312c27 100644
> --- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> +++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
> @@ -338,7 +338,7 @@ static void restart(struct net_device *dev)
>   	W16(sccp, scc_psmr, SCC_PSMR_ENCRC | SCC_PSMR_NIB22);
>   
>   	/* Set full duplex mode if needed */
> -	if (dev->phydev->duplex)
> +	if (dev->phydev->duplex == DUPLEX_FULL)
>   		S16(sccp, scc_psmr, SCC_PSMR_LPB | SCC_PSMR_FDE);
>   
>   	/* Restore multicast and promiscuous settings */

[PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Maxime Chevallier]

fs_enet is a quite old but still used Ethernet driver found on some NXP
devices. It has support for 10/100 Mbps ethernet, with half and full
duplex. Some variants of it can use RMII, while other integrations are
MII-only.

Add phylink support, thus removing custom fixed-link hanldling.

This also allows removing some internal flags such as the use_rmii flag.

Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>
---
 .../net/ethernet/freescale/fs_enet/Kconfig    |   2 +-
 .../ethernet/freescale/fs_enet/fs_enet-main.c | 203 +++++++++---------
 .../net/ethernet/freescale/fs_enet/fs_enet.h  |  12 +-
 .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  11 +-
 .../net/ethernet/freescale/fs_enet/mac-fec.c  |   9 +-
 .../net/ethernet/freescale/fs_enet/mac-scc.c  |   5 +-
 6 files changed, 120 insertions(+), 122 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fs_enet/Kconfig b/drivers/net/ethernet/freescale/fs_enet/Kconfig
index 7f20840fde07..57013bf14d7c 100644
--- a/drivers/net/ethernet/freescale/fs_enet/Kconfig
+++ b/drivers/net/ethernet/freescale/fs_enet/Kconfig
@@ -3,7 +3,7 @@ config FS_ENET
 	tristate "Freescale Ethernet Driver"
 	depends on NET_VENDOR_FREESCALE && (CPM1 || CPM2 || PPC_MPC512x)
 	select MII
-	select PHYLIB
+	select PHYLINK
 
 config FS_ENET_MPC5121_FEC
 	def_bool y if (FS_ENET && PPC_MPC512x)
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
index caca81b3ccb6..89ad9697f225 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet-main.c
@@ -32,11 +32,13 @@
 #include <linux/fs.h>
 #include <linux/platform_device.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/property.h>
 #include <linux/of.h>
 #include <linux/of_mdio.h>
 #include <linux/of_net.h>
 #include <linux/pgtable.h>
+#include <linux/rtnetlink.h>
 
 #include <linux/vmalloc.h>
 #include <asm/irq.h>
@@ -69,6 +71,16 @@ static void fs_set_multicast_list(struct net_device *dev)
 	(*fep->ops->set_multicast_list)(dev);
 }
 
+static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	if (!netif_running(dev))
+		return -EINVAL;
+
+	return phylink_mii_ioctl(fep->phylink, ifr, cmd);
+}
+
 static void skb_align(struct sk_buff *skb, int align)
 {
 	int off = ((unsigned long)skb->data) & (align - 1);
@@ -582,15 +594,12 @@ static void fs_timeout_work(struct work_struct *work)
 
 	dev->stats.tx_errors++;
 
-	spin_lock_irqsave(&fep->lock, flags);
+	rtnl_lock();
+	phylink_stop(fep->phylink);
+	phylink_start(fep->phylink);
+	rtnl_unlock();
 
-	if (dev->flags & IFF_UP) {
-		phy_stop(dev->phydev);
-		(*fep->ops->stop)(dev);
-		(*fep->ops->restart)(dev);
-	}
-
-	phy_start(dev->phydev);
+	spin_lock_irqsave(&fep->lock, flags);
 	wake = fep->tx_free >= MAX_SKB_FRAGS &&
 	       !(CBDR_SC(fep->cur_tx) & BD_ENET_TX_READY);
 	spin_unlock_irqrestore(&fep->lock, flags);
@@ -606,74 +615,37 @@ static void fs_timeout(struct net_device *dev, unsigned int txqueue)
 	schedule_work(&fep->timeout_work);
 }
 
-/* generic link-change handler - should be sufficient for most cases */
-static void generic_adjust_link(struct  net_device *dev)
+static void fs_mac_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 fs_enet_private *fep = netdev_priv(dev);
-	struct phy_device *phydev = dev->phydev;
-	int new_state = 0;
-
-	if (phydev->link) {
-		/* adjust to duplex mode */
-		if (phydev->duplex != fep->oldduplex) {
-			new_state = 1;
-			fep->oldduplex = phydev->duplex;
-		}
-
-		if (phydev->speed != fep->oldspeed) {
-			new_state = 1;
-			fep->oldspeed = phydev->speed;
-		}
-
-		if (!fep->oldlink) {
-			new_state = 1;
-			fep->oldlink = 1;
-		}
-
-		if (new_state)
-			fep->ops->restart(dev);
-	} else if (fep->oldlink) {
-		new_state = 1;
-		fep->oldlink = 0;
-		fep->oldspeed = 0;
-		fep->oldduplex = -1;
-	}
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct fs_enet_private *fep = netdev_priv(ndev);
+	unsigned long flags;
 
-	if (new_state && netif_msg_link(fep))
-		phy_print_status(phydev);
+	spin_lock_irqsave(&fep->lock, flags);
+	fep->ops->restart(ndev, interface, speed, duplex);
+	spin_unlock_irqrestore(&fep->lock, flags);
 }
 
-static void fs_adjust_link(struct net_device *dev)
+static void fs_mac_link_down(struct phylink_config *config,
+			     unsigned int mode, phy_interface_t interface)
 {
-	struct fs_enet_private *fep = netdev_priv(dev);
+	struct net_device *ndev = to_net_dev(config->dev);
+	struct fs_enet_private *fep = netdev_priv(ndev);
 	unsigned long flags;
 
 	spin_lock_irqsave(&fep->lock, flags);
-	generic_adjust_link(dev);
+	fep->ops->stop(ndev);
 	spin_unlock_irqrestore(&fep->lock, flags);
 }
 
-static int fs_init_phy(struct net_device *dev)
+static void fs_mac_config(struct phylink_config *config, unsigned int mode,
+			  const struct phylink_link_state *state)
 {
-	struct fs_enet_private *fep = netdev_priv(dev);
-	struct phy_device *phydev;
-	phy_interface_t iface;
-
-	fep->oldlink = 0;
-	fep->oldspeed = 0;
-	fep->oldduplex = -1;
-
-	iface = fep->fpi->use_rmii ?
-		PHY_INTERFACE_MODE_RMII : PHY_INTERFACE_MODE_MII;
-
-	phydev = of_phy_connect(dev, fep->fpi->phy_node, &fs_adjust_link, 0,
-				iface);
-	if (!phydev) {
-		dev_err(&dev->dev, "Could not attach to PHY\n");
-		return -ENODEV;
-	}
-
-	return 0;
+	/* Nothing to do */
 }
 
 static int fs_enet_open(struct net_device *dev)
@@ -698,13 +670,13 @@ static int fs_enet_open(struct net_device *dev)
 		return -EINVAL;
 	}
 
-	err = fs_init_phy(dev);
+	err = phylink_of_phy_connect(fep->phylink, fep->dev->of_node, 0);
 	if (err) {
 		free_irq(fep->interrupt, dev);
 		napi_disable(&fep->napi);
 		return err;
 	}
-	phy_start(dev->phydev);
+	phylink_start(fep->phylink);
 
 	netif_start_queue(dev);
 
@@ -717,19 +689,18 @@ static int fs_enet_close(struct net_device *dev)
 	unsigned long flags;
 
 	netif_stop_queue(dev);
-	netif_carrier_off(dev);
 	napi_disable(&fep->napi);
 	cancel_work_sync(&fep->timeout_work);
-	phy_stop(dev->phydev);
+	phylink_stop(fep->phylink);
 
 	spin_lock_irqsave(&fep->lock, flags);
 	spin_lock(&fep->tx_lock);
 	(*fep->ops->stop)(dev);
 	spin_unlock(&fep->tx_lock);
 	spin_unlock_irqrestore(&fep->lock, flags);
+	phylink_disconnect_phy(fep->phylink);
 
 	/* release any irqs */
-	phy_disconnect(dev->phydev);
 	free_irq(fep->interrupt, dev);
 
 	return 0;
@@ -817,6 +788,22 @@ static int fs_set_tunable(struct net_device *dev,
 	return ret;
 }
 
+static int fs_ethtool_set_link_ksettings(struct net_device *dev,
+					 const struct ethtool_link_ksettings *cmd)
+{
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	return phylink_ethtool_ksettings_set(fep->phylink, cmd);
+}
+
+static int fs_ethtool_get_link_ksettings(struct net_device *dev,
+					 struct ethtool_link_ksettings *cmd)
+{
+	struct fs_enet_private *fep = netdev_priv(dev);
+
+	return phylink_ethtool_ksettings_get(fep->phylink, cmd);
+}
+
 static const struct ethtool_ops fs_ethtool_ops = {
 	.get_drvinfo = fs_get_drvinfo,
 	.get_regs_len = fs_get_regs_len,
@@ -826,8 +813,8 @@ static const struct ethtool_ops fs_ethtool_ops = {
 	.set_msglevel = fs_set_msglevel,
 	.get_regs = fs_get_regs,
 	.get_ts_info = ethtool_op_get_ts_info,
-	.get_link_ksettings = phy_ethtool_get_link_ksettings,
-	.set_link_ksettings = phy_ethtool_set_link_ksettings,
+	.get_link_ksettings = fs_ethtool_get_link_ksettings,
+	.set_link_ksettings = fs_ethtool_set_link_ksettings,
 	.get_tunable = fs_get_tunable,
 	.set_tunable = fs_set_tunable,
 };
@@ -844,7 +831,7 @@ static const struct net_device_ops fs_enet_netdev_ops = {
 	.ndo_start_xmit		= fs_enet_start_xmit,
 	.ndo_tx_timeout		= fs_timeout,
 	.ndo_set_rx_mode	= fs_set_multicast_list,
-	.ndo_eth_ioctl		= phy_do_ioctl_running,
+	.ndo_eth_ioctl		= fs_eth_ioctl,
 	.ndo_validate_addr	= eth_validate_addr,
 	.ndo_set_mac_address	= eth_mac_addr,
 #ifdef CONFIG_NET_POLL_CONTROLLER
@@ -852,14 +839,21 @@ static const struct net_device_ops fs_enet_netdev_ops = {
 #endif
 };
 
+static const struct phylink_mac_ops fs_enet_phylink_mac_ops = {
+	.mac_config = fs_mac_config,
+	.mac_link_down = fs_mac_link_down,
+	.mac_link_up = fs_mac_link_up,
+};
+
 static int fs_enet_probe(struct platform_device *ofdev)
 {
-	int err, privsize, len, ret = -ENODEV;
-	const char *phy_connection_type;
+	int privsize, len, ret = -ENODEV;
 	struct fs_platform_info *fpi;
 	struct fs_enet_private *fep;
+	phy_interface_t phy_mode;
 	const struct fs_ops *ops;
 	struct net_device *ndev;
+	struct phylink *phylink;
 	const u32 *data;
 	struct clk *clk;
 
@@ -879,29 +873,18 @@ static int fs_enet_probe(struct platform_device *ofdev)
 		fpi->cp_command = *data;
 	}
 
+	ret = of_get_phy_mode(ofdev->dev.of_node, &phy_mode);
+	if (ret) {
+		/* For compatibility, if the mode isn't specified in DT,
+		 * assume MII
+		 */
+		phy_mode = PHY_INTERFACE_MODE_MII;
+	}
+
 	fpi->rx_ring = RX_RING_SIZE;
 	fpi->tx_ring = TX_RING_SIZE;
 	fpi->rx_copybreak = 240;
 	fpi->napi_weight = 17;
-	fpi->phy_node = of_parse_phandle(ofdev->dev.of_node, "phy-handle", 0);
-	if (!fpi->phy_node && of_phy_is_fixed_link(ofdev->dev.of_node)) {
-		err = of_phy_register_fixed_link(ofdev->dev.of_node);
-		if (err)
-			goto out_free_fpi;
-
-		/* In the case of a fixed PHY, the DT node associated
-		 * to the PHY is the Ethernet MAC DT node.
-		 */
-		fpi->phy_node = of_node_get(ofdev->dev.of_node);
-	}
-
-	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec")) {
-		phy_connection_type = of_get_property(ofdev->dev.of_node,
-						      "phy-connection-type",
-						      NULL);
-		if (phy_connection_type && !strcmp("rmii", phy_connection_type))
-			fpi->use_rmii = 1;
-	}
 
 	/* make clock lookup non-fatal (the driver is shared among platforms),
 	 * but require enable to succeed when a clock was specified/found,
@@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
 	if (!IS_ERR(clk)) {
 		ret = clk_prepare_enable(clk);
 		if (ret)
-			goto out_deregister_fixed_link;
+			goto out_phylink;
 
 		fpi->clk_per = clk;
 	}
@@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
 	fep->fpi = fpi;
 	fep->ops = ops;
 
+	fep->phylink_config.dev = &ndev->dev;
+	fep->phylink_config.type = PHYLINK_NETDEV;
+	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
+
+	__set_bit(PHY_INTERFACE_MODE_MII,
+		  fep->phylink_config.supported_interfaces);
+
+	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
+		__set_bit(PHY_INTERFACE_MODE_RMII,
+			  fep->phylink_config.supported_interfaces);
+
+	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
+				 phy_mode, &fs_enet_phylink_mac_ops);
+	if (IS_ERR(phylink)) {
+		ret = PTR_ERR(phylink);
+		goto out_free_fpi;
+	}
+
+	fep->phylink = phylink;
+
 	ret = fep->ops->setup_data(ndev);
 	if (ret)
 		goto out_free_dev;
@@ -968,8 +971,6 @@ static int fs_enet_probe(struct platform_device *ofdev)
 
 	ndev->ethtool_ops = &fs_ethtool_ops;
 
-	netif_carrier_off(ndev);
-
 	ndev->features |= NETIF_F_SG;
 
 	ret = register_netdev(ndev);
@@ -988,10 +989,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
 	free_netdev(ndev);
 out_put:
 	clk_disable_unprepare(fpi->clk_per);
-out_deregister_fixed_link:
-	of_node_put(fpi->phy_node);
-	if (of_phy_is_fixed_link(ofdev->dev.of_node))
-		of_phy_deregister_fixed_link(ofdev->dev.of_node);
+out_phylink:
+	phylink_destroy(fep->phylink);
 out_free_fpi:
 	kfree(fpi);
 	return ret;
@@ -1007,10 +1006,8 @@ static void fs_enet_remove(struct platform_device *ofdev)
 	fep->ops->free_bd(ndev);
 	fep->ops->cleanup_data(ndev);
 	dev_set_drvdata(fep->dev, NULL);
-	of_node_put(fep->fpi->phy_node);
 	clk_disable_unprepare(fep->fpi->clk_per);
-	if (of_phy_is_fixed_link(ofdev->dev.of_node))
-		of_phy_deregister_fixed_link(ofdev->dev.of_node);
+	phylink_destroy(fep->phylink);
 	free_netdev(ndev);
 }
 
diff --git a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
index 781f506c933c..2c436cdd4626 100644
--- a/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
+++ b/drivers/net/ethernet/freescale/fs_enet/fs_enet.h
@@ -8,6 +8,7 @@
 #include <linux/types.h>
 #include <linux/list.h>
 #include <linux/phy.h>
+#include <linux/phylink.h>
 #include <linux/dma-mapping.h>
 
 #ifdef CONFIG_CPM1
@@ -77,7 +78,8 @@ struct fs_ops {
 	void (*free_bd)(struct net_device *dev);
 	void (*cleanup_data)(struct net_device *dev);
 	void (*set_multicast_list)(struct net_device *dev);
-	void (*restart)(struct net_device *dev);
+	void (*restart)(struct net_device *dev, phy_interface_t interface,
+			int speed, int duplex);
 	void (*stop)(struct net_device *dev);
 	void (*napi_clear_event)(struct net_device *dev);
 	void (*napi_enable)(struct net_device *dev);
@@ -113,14 +115,10 @@ struct fs_platform_info {
 
 	u32 dpram_offset;
 
-	struct device_node *phy_node;
-
 	int rx_ring, tx_ring;	/* number of buffers on rx	*/
 	int rx_copybreak;	/* limit we copy small frames	*/
 	int napi_weight;	/* NAPI weight			*/
 
-	int use_rmii;		/* use RMII mode		*/
-
 	struct clk *clk_per;	/* 'per' clock for register access */
 };
 
@@ -146,10 +144,10 @@ struct fs_enet_private {
 	cbd_t __iomem *cur_tx;
 	int tx_free;
 	u32 msg_enable;
+	struct phylink *phylink;
+	struct phylink_config phylink_config;
 	int interrupt;
 
-	int oldduplex, oldspeed, oldlink;	/* current settings */
-
 	/* event masks */
 	u32 ev_napi;		/* mask of NAPI events */
 	u32 ev;			/* event mask          */
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
index 056909156b4f..3cb88fd91eaf 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fcc.c
@@ -236,7 +236,8 @@ static void set_multicast_list(struct net_device *dev)
 		set_promiscuous_mode(dev);
 }
 
-static void restart(struct net_device *dev)
+static void restart(struct net_device *dev, phy_interface_t interface,
+		    int speed, int duplex)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 	const struct fs_platform_info *fpi = fep->fpi;
@@ -360,8 +361,8 @@ static void restart(struct net_device *dev)
 	fs_init_bds(dev);
 
 	/* adjust to speed (for RMII mode) */
-	if (fpi->use_rmii) {
-		if (dev->phydev->speed == SPEED_100)
+	if (interface == PHY_INTERFACE_MODE_RMII) {
+		if (speed == SPEED_100)
 			C8(fcccp, fcc_gfemr, 0x20);
 		else
 			S8(fcccp, fcc_gfemr, 0x20);
@@ -383,11 +384,11 @@ static void restart(struct net_device *dev)
 
 	W32(fccp, fcc_fpsmr, FCC_PSMR_ENCRC);
 
-	if (fpi->use_rmii)
+	if (interface == PHY_INTERFACE_MODE_RMII)
 		S32(fccp, fcc_fpsmr, FCC_PSMR_RMII);
 
 	/* adjust to duplex mode */
-	if (dev->phydev->duplex == DUPLEX_FULL)
+	if (duplex == DUPLEX_FULL)
 		S32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
 	else
 		C32(fccp, fcc_fpsmr, FCC_PSMR_FDE | FCC_PSMR_LPB);
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
index 855ee9e3f042..9442847efa13 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-fec.c
@@ -221,7 +221,8 @@ static void set_multicast_list(struct net_device *dev)
 		set_promiscuous_mode(dev);
 }
 
-static void restart(struct net_device *dev)
+static void restart(struct net_device *dev, phy_interface_t interface,
+		    int speed, int duplex)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 	struct fec __iomem *fecp = fep->fec.fecp;
@@ -303,13 +304,13 @@ static void restart(struct net_device *dev)
 	 * Only set MII/RMII mode - do not touch maximum frame length
 	 * configured before.
 	 */
-	FS(fecp, r_cntrl, fpi->use_rmii ?
-			FEC_RCNTRL_RMII_MODE : FEC_RCNTRL_MII_MODE);
+	FS(fecp, r_cntrl, interface == PHY_INTERFACE_MODE_RMII ?
+			  FEC_RCNTRL_RMII_MODE : FEC_RCNTRL_MII_MODE);
 #endif
 	/*
 	 * adjust to duplex mode
 	 */
-	if (dev->phydev->duplex == DUPLEX_FULL) {
+	if (duplex == DUPLEX_FULL) {
 		FC(fecp, r_cntrl, FEC_RCNTRL_DRT);
 		FS(fecp, x_cntrl, FEC_TCNTRL_FDEN);	/* FD enable */
 	} else {
diff --git a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
index 9e5e29312c27..846aafff6951 100644
--- a/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
+++ b/drivers/net/ethernet/freescale/fs_enet/mac-scc.c
@@ -227,7 +227,8 @@ static void set_multicast_list(struct net_device *dev)
  * change.  This only happens when switching between half and full
  * duplex.
  */
-static void restart(struct net_device *dev)
+static void restart(struct net_device *dev, phy_interface_t interface,
+		    int speed, int duplex)
 {
 	struct fs_enet_private *fep = netdev_priv(dev);
 	scc_t __iomem *sccp = fep->scc.sccp;
@@ -338,7 +339,7 @@ static void restart(struct net_device *dev)
 	W16(sccp, scc_psmr, SCC_PSMR_ENCRC | SCC_PSMR_NIB22);
 
 	/* Set full duplex mode if needed */
-	if (dev->phydev->duplex == DUPLEX_FULL)
+	if (duplex == DUPLEX_FULL)
 		S16(sccp, scc_psmr, SCC_PSMR_LPB | SCC_PSMR_FDE);
 
 	/* Restore multicast and promiscuous settings */
-- 
2.45.2

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Russell King (Oracle)]

On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> +	struct fs_enet_private *fep = netdev_priv(dev);
> +
> +	if (!netif_running(dev))
> +		return -EINVAL;

Why do you need this check?

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

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

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Maxime Chevallier]

Hi Russell,

On Wed, 28 Aug 2024 11:38:31 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> > +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > +{
> > +	struct fs_enet_private *fep = netdev_priv(dev);
> > +
> > +	if (!netif_running(dev))
> > +		return -EINVAL;  
> 
> Why do you need this check?
> 

I included it as the original ioctl was phy_do_ioctl_running(), which
includes that check.

Is this check irrelevant with phylink ? I could only find macb and
xilinx_axienet that do the same check in their ioctl.

I can't tell you why that check is there in the first place in that
driver, a quick grep search leads back from a major driver rework in
2011, at which point the check was already there...

Regards,

Maxime

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Russell King (Oracle)]

On Wed, Aug 28, 2024 at 01:44:13PM +0200, Maxime Chevallier wrote:
> Hi Russell,
> 
> On Wed, 28 Aug 2024 11:38:31 +0100
> "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> 
> > On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> > > +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > > +{
> > > +	struct fs_enet_private *fep = netdev_priv(dev);
> > > +
> > > +	if (!netif_running(dev))
> > > +		return -EINVAL;  
> > 
> > Why do you need this check?
> > 
> 
> I included it as the original ioctl was phy_do_ioctl_running(), which
> includes that check.
> 
> Is this check irrelevant with phylink ? I could only find macb and
> xilinx_axienet that do the same check in their ioctl.
> 
> I can't tell you why that check is there in the first place in that
> driver, a quick grep search leads back from a major driver rework in
> 2011, at which point the check was already there...

int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
{
	if (pl->phydev) {
		... do PHY based access / pass on to phylib ...
	} else {
		... for reads:
		...  return emulated fixed-phy state if in fixed mode
		...  return emulated inband state if in inband mode
		... for writes:
		...  ignore writes in fixed and inband modes
		... otherwise return -EOPNOTSUPP
	}
}

So, if a driver decides to connect the PHY during probe, the PHY will
always be accessible.

If a driver decides to connect the PHY during .ndo_open, the PHY will
only be accessible while the netdev is open, otherwise -EOPNOTSUPP
will be returned.

What do ethernet drivers return when they're not open or not having a
PHY? The answer is pretty random error codes.

drivers/net/ethernet/renesas/ravb_main.c-       if (!netif_running(ndev))
drivers/net/ethernet/renesas/ravb_main.c-               return -EINVAL;
drivers/net/ethernet/renesas/ravb_main.c-
drivers/net/ethernet/renesas/ravb_main.c-       if (!phydev)
drivers/net/ethernet/renesas/ravb_main.c-               return -ENODEV;
...
drivers/net/ethernet/renesas/ravb_main.c:       return phy_mii_ioctl(phydev, req, cmd);

drivers/net/ethernet/renesas/rswitch.c- if (!netif_running(ndev))
drivers/net/ethernet/renesas/rswitch.c-         return -EINVAL;

drivers/net/ethernet/8390/ax88796.c-    if (!netif_running(dev))
drivers/net/ethernet/8390/ax88796.c-            return -EINVAL;
drivers/net/ethernet/8390/ax88796.c-
drivers/net/ethernet/8390/ax88796.c-    if (!phy_dev)
drivers/net/ethernet/8390/ax88796.c-            return -ENODEV;

drivers/net/ethernet/marvell/mv643xx_eth.c-     if (!dev->phydev)
drivers/net/ethernet/marvell/mv643xx_eth.c-             return -ENOTSUPP;

drivers/net/ethernet/xilinx/xilinx_emaclite.c-  if (!dev->phydev || !netif_running(dev))
drivers/net/ethernet/xilinx/xilinx_emaclite.c-          return -EINVAL;

drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c-     if (!(netif_running(netdev)))
drivers/net/ethernet/mellanox/mlxbf_gige/mlxbf_gige_main.c-             return -EINVAL;

drivers/net/ethernet/actions/owl-emac.c-        if (!netif_running(netdev))
drivers/net/ethernet/actions/owl-emac.c-                return -EINVAL;

drivers/net/ethernet/engleder/tsnep_main.c-     if (!netif_running(netdev))
drivers/net/ethernet/engleder/tsnep_main.c-             return -EINVAL;

drivers/net/ethernet/ti/davinci_emac.c- if (!(netif_running(ndev)))
drivers/net/ethernet/ti/davinci_emac.c-         return -EINVAL;
drivers/net/ethernet/ti/davinci_emac.c- if (ndev->phydev)
drivers/net/ethernet/ti/davinci_emac.c:         return phy_mii_ioctl(ndev->phydev, ifrq, cmd);
drivers/net/ethernet/ti/davinci_emac.c- else
drivers/net/ethernet/ti/davinci_emac.c-         return -EOPNOTSUPP;

drivers/net/ethernet/ti/netcp_ethss.c-  if (phy)
drivers/net/ethernet/ti/netcp_ethss.c:          return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/netcp_ethss.c-
drivers/net/ethernet/ti/netcp_ethss.c-  return -EOPNOTSUPP;

drivers/net/ethernet/ti/cpsw_priv.c-    if (phy)
drivers/net/ethernet/ti/cpsw_priv.c:            return phy_mii_ioctl(phy, req, cmd);
drivers/net/ethernet/ti/cpsw_priv.c-
drivers/net/ethernet/ti/cpsw_priv.c-    return -EOPNOTSUPP;

drivers/net/ethernet/xscale/ixp4xx_eth.c-       if (!netif_running(dev))
drivers/net/ethernet/xscale/ixp4xx_eth.c-               return -EINVAL;

drivers/net/ethernet/freescale/gianfar.c-       if (!netif_running(dev))
drivers/net/ethernet/freescale/gianfar.c-               return -EINVAL;
...
drivers/net/ethernet/freescale/gianfar.c-       if (!phydev)
drivers/net/ethernet/freescale/gianfar.c-               return -ENODEV;
drivers/net/ethernet/freescale/gianfar.c-
drivers/net/ethernet/freescale/gianfar.c:       return phy_mii_ioctl(phydev, rq, cmd);

drivers/net/ethernet/freescale/ucc_geth.c-      if (!netif_running(dev))
drivers/net/ethernet/freescale/ucc_geth.c-              return -EINVAL;
drivers/net/ethernet/freescale/ucc_geth.c-
drivers/net/ethernet/freescale/ucc_geth.c-      if (!ugeth->phydev)
drivers/net/ethernet/freescale/ucc_geth.c-              return -ENODEV;

drivers/net/ethernet/mediatek/mtk_star_emac.c-  if (!netif_running(ndev))
drivers/net/ethernet/mediatek/mtk_star_emac.c-          return -EINVAL;

drivers/net/ethernet/microchip/lan743x_main.c-  if (!netif_running(netdev))
drivers/net/ethernet/microchip/lan743x_main.c-          return -EINVAL;

drivers/net/ethernet/ethoc.c-   if (!netif_running(dev))
drivers/net/ethernet/ethoc.c-           return -EINVAL;

drivers/net/ethernet/broadcom/b44.c-    int err = -EINVAL;
drivers/net/ethernet/broadcom/b44.c-
drivers/net/ethernet/broadcom/b44.c-    if (!netif_running(dev))
drivers/net/ethernet/broadcom/b44.c-            goto out;

drivers/net/ethernet/broadcom/sb1250-mac.c-     if (!netif_running(dev) || !sc->phy_dev)
drivers/net/ethernet/broadcom/sb1250-mac.c-             return -EINVAL;

drivers/net/usb/smsc95xx.c-     if (!netif_running(netdev))
drivers/net/usb/smsc95xx.c-             return -EINVAL;

Of 28 drivers that call phy_mii_ioctl():
 - 17 drivers return EINVAL if !netif_running().
 - 11 drivers do not check netif_running().
and if there's no phydev:
 - 4 drivers return ENODEV
 - 3 drivers return EOPNOTSUPP (plus all drivers converted to phylink)
 - 2 drivers return EINVAL
 - 1 driver returns ENOTSUPP

Phylib itself doesn't want NULL passed to phy_mii_ioctl(), so its up to
the driver to trap this if its calling phy_mii_ioctl(). However, phylib
also provides phy_do_ioctl() for hooking directly into .ndo_eth_ioctl,
which is:

int phy_do_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
{
        if (!dev->phydev)
                return -ENODEV;

        return phy_mii_ioctl(dev->phydev, ifr, cmd);
}

then there's (as you point out) phy_do_ioctl_running(), which is:

int phy_do_ioctl_running(struct net_device *dev, struct ifreq *ifr, int cmd)
{
        if (!netif_running(dev))
                return -ENODEV;

        return phy_do_ioctl(dev, ifr, cmd);
}

and this returns ENODEV if the netif isn't running, not EINVAL which
the majority of net drivers that manually check are doing. Maybe phylib
has the error code wrong?

In any case, I think it's pretty clear that there's no single "right"
error code for these cases, everyone just puts up on a piece of paper
with a donkey the names of various error codes, and while blindfolded
sticks a pin in to find the "right" error code to use! So, I don't see
any point in debating what is the "One True Right Error Code" for these
conditions.

Now, the question is... why do drivers do this netif_running() check?
Is it because "other drivers do" or is it because there's a valid
reason. There's no way to know, no one ever documents why the check
is there - and based on your response, it's "because other drivers do".

Without looking at the history, I wouldn't make any assumption about
using phy_do_ioctl_running() - that could be the result of a drive-by
cleanup patch converting code to use that helper.

At this point... this email has eaten up a substantial amount of time,
and I can't spend anymore time in front of the screen today so that's
the end of my contributions for today. Sorry.

-- 
*** please note that I probably will only be occasionally responsive
*** for an unknown period of time due to recent eye surgery making
*** reading quite difficult.

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

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Maxime Chevallier]

Hello Russell,

On Wed, 28 Aug 2024 14:54:57 +0100
"Russell King (Oracle)" <linux@armlinux.org.uk> wrote:

> On Wed, Aug 28, 2024 at 01:44:13PM +0200, Maxime Chevallier wrote:
> > Hi Russell,
> > 
> > On Wed, 28 Aug 2024 11:38:31 +0100
> > "Russell King (Oracle)" <linux@armlinux.org.uk> wrote:
> >   
> > > On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:  
> > > > +static int fs_eth_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> > > > +{
> > > > +	struct fs_enet_private *fep = netdev_priv(dev);
> > > > +
> > > > +	if (!netif_running(dev))
> > > > +		return -EINVAL;    
> > > 
> > > Why do you need this check?
> > >   
> > 
> > I included it as the original ioctl was phy_do_ioctl_running(), which
> > includes that check.
> > 
> > Is this check irrelevant with phylink ? I could only find macb and
> > xilinx_axienet that do the same check in their ioctl.
> > 
> > I can't tell you why that check is there in the first place in that
> > driver, a quick grep search leads back from a major driver rework in
> > 2011, at which point the check was already there...  
> 
> int phylink_mii_ioctl(struct phylink *pl, struct ifreq *ifr, int cmd)
> {
> 	if (pl->phydev) {
> 		... do PHY based access / pass on to phylib ...
> 	} else {
> 		... for reads:
> 		...  return emulated fixed-phy state if in fixed mode
> 		...  return emulated inband state if in inband mode
> 		... for writes:
> 		...  ignore writes in fixed and inband modes
> 		... otherwise return -EOPNOTSUPP
> 	}
> }
> 
> So, if a driver decides to connect the PHY during probe, the PHY will
> always be accessible.
> 
> If a driver decides to connect the PHY during .ndo_open, the PHY will
> only be accessible while the netdev is open, otherwise -EOPNOTSUPP
> will be returned.

That makes sense, so there's no point keeping that check then.

I'll update the patch, thanks for this clarification.

[...]

> At this point... this email has eaten up a substantial amount of time,
> and I can't spend anymore time in front of the screen today so that's
> the end of my contributions for today. Sorry.

I've been in the same rabbit-hole today debating in my head whether or
not to add this check, I'm sorry that I dragged you in there... With
what you stressed-out, I have a good enough justification to drop the
check in the current patch.

As for the current situation with the ioctl return codes, there indeed
room for lots of improvements. For drivers that simply forward the
ioctl to phylib/phylink, I think we could pretty easily add some
consistency on the return code, provided we agree on the proper one to
return.

Thanks for your time,

Maxime 

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Simon Horman]

On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> fs_enet is a quite old but still used Ethernet driver found on some NXP
> devices. It has support for 10/100 Mbps ethernet, with half and full
> duplex. Some variants of it can use RMII, while other integrations are
> MII-only.
> 
> Add phylink support, thus removing custom fixed-link hanldling.
> 
> This also allows removing some internal flags such as the use_rmii flag.
> 
> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Hi Maxime,

Some minor issues from my side: not a full review by any stretch of
the imagination.

...

> @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	if (!IS_ERR(clk)) {
>  		ret = clk_prepare_enable(clk);
>  		if (ret)
> -			goto out_deregister_fixed_link;
> +			goto out_phylink;
>  
>  		fpi->clk_per = clk;
>  	}

This goto will result in a dereference of fep.
But fep is not initialised until the following line,
which appears a little below this hunk.

	fep = netdev_priv(ndev);

This goto will also result in the function returning without
releasing clk.

Both flagged by Smatch.

> @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	fep->fpi = fpi;
>  	fep->ops = ops;
>  
> +	fep->phylink_config.dev = &ndev->dev;
> +	fep->phylink_config.type = PHYLINK_NETDEV;
> +	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> +
> +	__set_bit(PHY_INTERFACE_MODE_MII,
> +		  fep->phylink_config.supported_interfaces);
> +
> +	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> +		__set_bit(PHY_INTERFACE_MODE_RMII,
> +			  fep->phylink_config.supported_interfaces);
> +
> +	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> +				 phy_mode, &fs_enet_phylink_mac_ops);
> +	if (IS_ERR(phylink)) {
> +		ret = PTR_ERR(phylink);
> +		goto out_free_fpi;

This also appears to leak clk, as well as ndev.

I didn't look for other cases.

> +	}
> +
> +	fep->phylink = phylink;
> +
>  	ret = fep->ops->setup_data(ndev);
>  	if (ret)
>  		goto out_free_dev;
> @@ -968,8 +971,6 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  
>  	ndev->ethtool_ops = &fs_ethtool_ops;
>  
> -	netif_carrier_off(ndev);
> -
>  	ndev->features |= NETIF_F_SG;
>  
>  	ret = register_netdev(ndev);
> @@ -988,10 +989,8 @@ static int fs_enet_probe(struct platform_device *ofdev)
>  	free_netdev(ndev);
>  out_put:
>  	clk_disable_unprepare(fpi->clk_per);
> -out_deregister_fixed_link:
> -	of_node_put(fpi->phy_node);
> -	if (of_phy_is_fixed_link(ofdev->dev.of_node))
> -		of_phy_deregister_fixed_link(ofdev->dev.of_node);
> +out_phylink:
> +	phylink_destroy(fep->phylink);
>  out_free_fpi:
>  	kfree(fpi);
>  	return ret;

...

Re: [PATCH net-next 6/6] net: ethernet: fs_enet: phylink conversion

[Maxime Chevallier]

Hello Simon,

On Wed, 28 Aug 2024 17:32:24 +0100
Simon Horman <horms@kernel.org> wrote:

> On Wed, Aug 28, 2024 at 11:51:02AM +0200, Maxime Chevallier wrote:
> > fs_enet is a quite old but still used Ethernet driver found on some NXP
> > devices. It has support for 10/100 Mbps ethernet, with half and full
> > duplex. Some variants of it can use RMII, while other integrations are
> > MII-only.
> > 
> > Add phylink support, thus removing custom fixed-link hanldling.
> > 
> > This also allows removing some internal flags such as the use_rmii flag.
> > 
> > Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
> 
> Hi Maxime,
> 
> Some minor issues from my side: not a full review by any stretch of
> the imagination.
> 
> ...
> 
> > @@ -911,7 +894,7 @@ static int fs_enet_probe(struct platform_device *ofdev)
> >  	if (!IS_ERR(clk)) {
> >  		ret = clk_prepare_enable(clk);
> >  		if (ret)
> > -			goto out_deregister_fixed_link;
> > +			goto out_phylink;
> >  
> >  		fpi->clk_per = clk;
> >  	}  
> 
> This goto will result in a dereference of fep.
> But fep is not initialised until the following line,
> which appears a little below this hunk.
> 
> 	fep = netdev_priv(ndev);

Nice catch, the goto should rather go to out_free_fpi.

> 
> This goto will also result in the function returning without
> releasing clk.

ah yes, it's never disabled_unprepared, the phylink cleanup label is at
the wrong spot. I'll include a patch in the next iteration to make use
of devm_clk_get_enabled(), that should simplify all of that.

> Both flagged by Smatch.
> 
> > @@ -936,6 +919,26 @@ static int fs_enet_probe(struct platform_device *ofdev)
> >  	fep->fpi = fpi;
> >  	fep->ops = ops;
> >  
> > +	fep->phylink_config.dev = &ndev->dev;
> > +	fep->phylink_config.type = PHYLINK_NETDEV;
> > +	fep->phylink_config.mac_capabilities = MAC_10 | MAC_100;
> > +
> > +	__set_bit(PHY_INTERFACE_MODE_MII,
> > +		  fep->phylink_config.supported_interfaces);
> > +
> > +	if (of_device_is_compatible(ofdev->dev.of_node, "fsl,mpc5125-fec"))
> > +		__set_bit(PHY_INTERFACE_MODE_RMII,
> > +			  fep->phylink_config.supported_interfaces);
> > +
> > +	phylink = phylink_create(&fep->phylink_config, dev_fwnode(fep->dev),
> > +				 phy_mode, &fs_enet_phylink_mac_ops);
> > +	if (IS_ERR(phylink)) {
> > +		ret = PTR_ERR(phylink);
> > +		goto out_free_fpi;  
> 
> This also appears to leak clk, as well as ndev.

Thanks, will be addressed in V2.

> I didn't look for other cases.

I'll go over the cleanup path, thanks for checking this !

Thanks,

Maxime

Re: [PATCH net-next 0/6] net: ethernet: fs_enet: Cleanup and phylink conversion

[Christophe Leroy]

Le 28/08/2024 à 11:50, Maxime Chevallier a écrit :
> This series aims at improving the fs_enet code and port it's PHY
> handling from direct phylib access to using phylink instead.
> 
> Although this driver is quite old, there are still some users out there,
> running an upstream kernel. The development I'm doing is on an MPC885
> device, which uses fs_enet, as well as a MPC866-based device.
> 
> The main motivation for that work is to eventually support ethernet interfaces
> that have more than one PHY attached to the MAC upstream, for which
> phylink might be a pre-requisite. That work isn't submitted yet, and the
> final solution might not even require phylink.
> 
> Regardless, I do believe that this series is relevant, as it does some
> cleanup to the driver, and having it use phylink brings some nice
> improvements as it simplifies the DT parsing, fixed-link handling and
> removes code in that driver that predates even phylib itself.
> 
> The series is structured in the following way :
> 
> - Patches 1 and 2 are cosmetic changes. The former converts the source
>    to SPDX, while the latter has fs_enet-main.c pass checkpatch. Patch 2 is
>    really not mandatory in this series, and I understand that this isn't
>    the easiest or most pleasant patch to review. OTOH, this allows
>    getting a clean checkpatch output for the main part of the driver.
> 
> - Patches 3, 4 and 5 drop some leftovers from back when the driver didn't
>    use phylib, and brings the use of phylib macros.
> 
> - Patch 6 is the actual phylink port, which also cleans the bits of code
>    that become irrelevant when using phylink.
> 
> Testing was done on an MPC866 and MPC885, any test on other platforms
> that use fs_enet are more than welcome.
> 
> Thanks,
> 
> Maxime
> 
> Maxime Chevallier (6):
>    net: ethernet: fs_enet: convert to SPDX
>    net: ethernet: fs_enet: cosmetic cleanups
>    net: ethernet: fs_enet: drop the .adjust_link custom fs_ops
>    net: ethernet: fs_enet: drop unused phy_info and mii_if_info
>    net: ethernet: fs_enet: fcc: use macros for speed and duplex values
>    net: ethernet: fs_enet: phylink conversion

For the series,

Acked-by: Christophe Leroy <christophe.leroy@csgroup.eu> # LINUX FOR 
POWERPC EMBEDDED PPC8XX AND PPC83XX

> 
>   .../net/ethernet/freescale/fs_enet/Kconfig    |   2 +-
>   .../ethernet/freescale/fs_enet/fs_enet-main.c | 421 ++++++++----------
>   .../net/ethernet/freescale/fs_enet/fs_enet.h  |  24 +-
>   .../net/ethernet/freescale/fs_enet/mac-fcc.c  |  16 +-
>   .../net/ethernet/freescale/fs_enet/mac-fec.c  |  14 +-
>   .../net/ethernet/freescale/fs_enet/mac-scc.c  |  10 +-
>   .../ethernet/freescale/fs_enet/mii-bitbang.c  |   5 +-
>   .../net/ethernet/freescale/fs_enet/mii-fec.c  |   5 +-
>   8 files changed, 209 insertions(+), 288 deletions(-)
>