[PATCH net-next v2 00/17] ucc_geth improvements

[PATCH net-next v2 00/17] ucc_geth improvements

[Rasmus Villemoes]

This is a resend of some improvements to the ucc_geth driver that was
previously sent together with bug fixes, which have by now been
applied.

Li Yang, if you don't speak up, I'm going to assume you're fine with
2,3,4 being taken through the net tree?

v2: rebase to net/master; address minor style issues; don't introduce
a use-after-free in patch "don't statically allocate eight
ucc_geth_info".

Rasmus Villemoes (17):
  ethernet: ucc_geth: remove unused read of temoder field
  soc: fsl: qe: make cpm_muram_offset take a const void* argument
  soc: fsl: qe: store muram_vbase as a void pointer instead of u8
  soc: fsl: qe: add cpm_muram_free_addr() helper
  ethernet: ucc_geth: use qe_muram_free_addr()
  ethernet: ucc_geth: remove unnecessary memset_io() calls
  ethernet: ucc_geth: replace kmalloc+memset by kzalloc
  ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct
    ucc_geth_private
  ethernet: ucc_geth: factor out parsing of {rx,tx}-clock{,-name}
    properties
  ethernet: ucc_geth: constify ugeth_primary_info
  ethernet: ucc_geth: don't statically allocate eight ucc_geth_info
  ethernet: ucc_geth: use UCC_GETH_{RX,TX}_BD_RING_ALIGNMENT macros
    directly
  ethernet: ucc_geth: remove bd_mem_part and all associated code
  ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()
  ethernet: ucc_geth: add helper to replace repeated switch statements
  ethernet: ucc_geth: inform the compiler that numQueues is always 1
  ethernet: ucc_geth: simplify rx/tx allocations

 drivers/net/ethernet/freescale/ucc_geth.c | 549 ++++++++--------------
 drivers/net/ethernet/freescale/ucc_geth.h |   6 -
 drivers/soc/fsl/qe/qe_common.c            |  20 +-
 include/soc/fsl/qe/qe.h                   |  15 +-
 include/soc/fsl/qe/ucc_fast.h             |   1 -
 5 files changed, 209 insertions(+), 382 deletions(-)

-- 
2.23.0

[PATCH net-next v2 01/17] ethernet: ucc_geth: remove unused read of temoder field

[Rasmus Villemoes]

In theory, such a read-after-write might be required by the hardware,
but nothing in the data sheet suggests that to be the case. The name
test also suggests that it's some debug leftover.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 6d853f018d53..d4b775870f4e 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2359,7 +2359,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	u32 init_enet_pram_offset, cecr_subblock, command;
 	u32 ifstat, i, j, size, l2qt, l3qt;
 	u16 temoder = UCC_GETH_TEMODER_INIT;
-	u16 test;
 	u8 function_code = 0;
 	u8 __iomem *endOfRing;
 	u8 numThreadsRxNumerical, numThreadsTxNumerical;
@@ -2667,8 +2666,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	temoder |= ((ug_info->numQueuesTx - 1) << TEMODER_NUM_OF_QUEUES_SHIFT);
 	out_be16(&ugeth->p_tx_glbl_pram->temoder, temoder);
 
-	test = in_be16(&ugeth->p_tx_glbl_pram->temoder);
-
 	/* Function code register value to be used later */
 	function_code = UCC_BMR_BO_BE | UCC_BMR_GBL;
 	/* Required for QE */
-- 
2.23.0

[PATCH net-next v2 02/17] soc: fsl: qe: make cpm_muram_offset take a const void* argument

[Rasmus Villemoes]

Allow passing const-qualified pointers without requiring a cast in the
caller.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/soc/fsl/qe/qe_common.c | 2 +-
 include/soc/fsl/qe/qe.h        | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
index 75075591f630..0fbdc965c4cb 100644
--- a/drivers/soc/fsl/qe/qe_common.c
+++ b/drivers/soc/fsl/qe/qe_common.c
@@ -223,7 +223,7 @@ void __iomem *cpm_muram_addr(unsigned long offset)
 }
 EXPORT_SYMBOL(cpm_muram_addr);
 
-unsigned long cpm_muram_offset(void __iomem *addr)
+unsigned long cpm_muram_offset(const void __iomem *addr)
 {
 	return addr - (void __iomem *)muram_vbase;
 }
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index 3feddfec9f87..8ee3747433c0 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -102,7 +102,7 @@ s32 cpm_muram_alloc(unsigned long size, unsigned long align);
 void cpm_muram_free(s32 offset);
 s32 cpm_muram_alloc_fixed(unsigned long offset, unsigned long size);
 void __iomem *cpm_muram_addr(unsigned long offset);
-unsigned long cpm_muram_offset(void __iomem *addr);
+unsigned long cpm_muram_offset(const void __iomem *addr);
 dma_addr_t cpm_muram_dma(void __iomem *addr);
 #else
 static inline s32 cpm_muram_alloc(unsigned long size,
@@ -126,7 +126,7 @@ static inline void __iomem *cpm_muram_addr(unsigned long offset)
 	return NULL;
 }
 
-static inline unsigned long cpm_muram_offset(void __iomem *addr)
+static inline unsigned long cpm_muram_offset(const void __iomem *addr)
 {
 	return -ENOSYS;
 }
-- 
2.23.0

RE: [PATCH net-next v2 02/17] soc: fsl: qe: make cpm_muram_offset take a const void* argument

[Leo Li]

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: Tuesday, January 19, 2021 9:08 AM
> To: netdev@vger.kernel.org
> Cc: Leo Li <leoyang.li@nxp.com>; David S . Miller <davem@davemloft.net>;
> Qiang Zhao <qiang.zhao@nxp.com>; Andrew Lunn <andrew@lunn.ch>;
> Christophe Leroy <christophe.leroy@csgroup.eu>; Jakub Kicinski
> <kuba@kernel.org>; jocke@infinera.com <joakim.tjernlund@infinera.com>;
> Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: [PATCH net-next v2 02/17] soc: fsl: qe: make cpm_muram_offset
> take a const void* argument
> 
> Allow passing const-qualified pointers without requiring a cast in the
> caller.

Acked-by: Li Yang <leoyang.li@nxp.com>

> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>  drivers/soc/fsl/qe/qe_common.c | 2 +-
>  include/soc/fsl/qe/qe.h        | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/fsl/qe/qe_common.c
> b/drivers/soc/fsl/qe/qe_common.c
> index 75075591f630..0fbdc965c4cb 100644
> --- a/drivers/soc/fsl/qe/qe_common.c
> +++ b/drivers/soc/fsl/qe/qe_common.c
> @@ -223,7 +223,7 @@ void __iomem *cpm_muram_addr(unsigned long
> offset)
>  }
>  EXPORT_SYMBOL(cpm_muram_addr);
> 
> -unsigned long cpm_muram_offset(void __iomem *addr)
> +unsigned long cpm_muram_offset(const void __iomem *addr)
>  {
>  	return addr - (void __iomem *)muram_vbase;
>  }
> diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
> index 3feddfec9f87..8ee3747433c0 100644
> --- a/include/soc/fsl/qe/qe.h
> +++ b/include/soc/fsl/qe/qe.h
> @@ -102,7 +102,7 @@ s32 cpm_muram_alloc(unsigned long size, unsigned
> long align);
>  void cpm_muram_free(s32 offset);
>  s32 cpm_muram_alloc_fixed(unsigned long offset, unsigned long size);
>  void __iomem *cpm_muram_addr(unsigned long offset);
> -unsigned long cpm_muram_offset(void __iomem *addr);
> +unsigned long cpm_muram_offset(const void __iomem *addr);
>  dma_addr_t cpm_muram_dma(void __iomem *addr);
>  #else
>  static inline s32 cpm_muram_alloc(unsigned long size,
> @@ -126,7 +126,7 @@ static inline void __iomem
> *cpm_muram_addr(unsigned long offset)
>  	return NULL;
>  }
> 
> -static inline unsigned long cpm_muram_offset(void __iomem *addr)
> +static inline unsigned long cpm_muram_offset(const void __iomem *addr)
>  {
>  	return -ENOSYS;
>  }
> --
> 2.23.0

[PATCH net-next v2 03/17] soc: fsl: qe: store muram_vbase as a void pointer instead of u8

[Rasmus Villemoes]

The two functions cpm_muram_offset() and cpm_muram_dma() both need a
cast currently, one casts muram_vbase to do the pointer arithmetic on
void pointers, the other casts the passed-in address u8*.

It's simpler and more consistent to just always use void* and drop all
the casting.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/soc/fsl/qe/qe_common.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
index 0fbdc965c4cb..303cc2f5eb4a 100644
--- a/drivers/soc/fsl/qe/qe_common.c
+++ b/drivers/soc/fsl/qe/qe_common.c
@@ -27,7 +27,7 @@
 
 static struct gen_pool *muram_pool;
 static spinlock_t cpm_muram_lock;
-static u8 __iomem *muram_vbase;
+static void __iomem *muram_vbase;
 static phys_addr_t muram_pbase;
 
 struct muram_block {
@@ -225,7 +225,7 @@ EXPORT_SYMBOL(cpm_muram_addr);
 
 unsigned long cpm_muram_offset(const void __iomem *addr)
 {
-	return addr - (void __iomem *)muram_vbase;
+	return addr - muram_vbase;
 }
 EXPORT_SYMBOL(cpm_muram_offset);
 
@@ -235,6 +235,6 @@ EXPORT_SYMBOL(cpm_muram_offset);
  */
 dma_addr_t cpm_muram_dma(void __iomem *addr)
 {
-	return muram_pbase + ((u8 __iomem *)addr - muram_vbase);
+	return muram_pbase + (addr - muram_vbase);
 }
 EXPORT_SYMBOL(cpm_muram_dma);
-- 
2.23.0

Re: [PATCH net-next v2 03/17] soc: fsl: qe: store muram_vbase as a void pointer instead of u8

[Li Yang]

On Tue, Jan 19, 2021 at 9:16 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> The two functions cpm_muram_offset() and cpm_muram_dma() both need a
> cast currently, one casts muram_vbase to do the pointer arithmetic on
> void pointers, the other casts the passed-in address u8*.
>
> It's simpler and more consistent to just always use void* and drop all
> the casting.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Acked-by: Li Yang <leoyang.li@nxp.com>

> ---
>  drivers/soc/fsl/qe/qe_common.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
> index 0fbdc965c4cb..303cc2f5eb4a 100644
> --- a/drivers/soc/fsl/qe/qe_common.c
> +++ b/drivers/soc/fsl/qe/qe_common.c
> @@ -27,7 +27,7 @@
>
>  static struct gen_pool *muram_pool;
>  static spinlock_t cpm_muram_lock;
> -static u8 __iomem *muram_vbase;
> +static void __iomem *muram_vbase;
>  static phys_addr_t muram_pbase;
>
>  struct muram_block {
> @@ -225,7 +225,7 @@ EXPORT_SYMBOL(cpm_muram_addr);
>
>  unsigned long cpm_muram_offset(const void __iomem *addr)
>  {
> -       return addr - (void __iomem *)muram_vbase;
> +       return addr - muram_vbase;
>  }
>  EXPORT_SYMBOL(cpm_muram_offset);
>
> @@ -235,6 +235,6 @@ EXPORT_SYMBOL(cpm_muram_offset);
>   */
>  dma_addr_t cpm_muram_dma(void __iomem *addr)
>  {
> -       return muram_pbase + ((u8 __iomem *)addr - muram_vbase);
> +       return muram_pbase + (addr - muram_vbase);
>  }
>  EXPORT_SYMBOL(cpm_muram_dma);
> --
> 2.23.0
>

[PATCH net-next v2 04/17] soc: fsl: qe: add cpm_muram_free_addr() helper

[Rasmus Villemoes]

Add a helper that takes a virtual address rather than the muram
offset. This will be used in a couple of places to avoid having to
store both the offset and the virtual address, as well as removing
NULL checks from the callers.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/soc/fsl/qe/qe_common.c | 12 ++++++++++++
 include/soc/fsl/qe/qe.h        |  5 +++++
 2 files changed, 17 insertions(+)

diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
index 303cc2f5eb4a..448ef7f5321a 100644
--- a/drivers/soc/fsl/qe/qe_common.c
+++ b/drivers/soc/fsl/qe/qe_common.c
@@ -238,3 +238,15 @@ dma_addr_t cpm_muram_dma(void __iomem *addr)
 	return muram_pbase + (addr - muram_vbase);
 }
 EXPORT_SYMBOL(cpm_muram_dma);
+
+/*
+ * As cpm_muram_free, but takes the virtual address rather than the
+ * muram offset.
+ */
+void cpm_muram_free_addr(const void __iomem *addr)
+{
+	if (!addr)
+		return;
+	cpm_muram_free(cpm_muram_offset(addr));
+}
+EXPORT_SYMBOL(cpm_muram_free_addr);
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index 8ee3747433c0..66f1afc393d1 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -104,6 +104,7 @@ s32 cpm_muram_alloc_fixed(unsigned long offset, unsigned long size);
 void __iomem *cpm_muram_addr(unsigned long offset);
 unsigned long cpm_muram_offset(const void __iomem *addr);
 dma_addr_t cpm_muram_dma(void __iomem *addr);
+void cpm_muram_free_addr(const void __iomem *addr);
 #else
 static inline s32 cpm_muram_alloc(unsigned long size,
 				  unsigned long align)
@@ -135,6 +136,9 @@ static inline dma_addr_t cpm_muram_dma(void __iomem *addr)
 {
 	return 0;
 }
+static inline void cpm_muram_free_addr(const void __iomem *addr)
+{
+}
 #endif /* defined(CONFIG_CPM) || defined(CONFIG_QUICC_ENGINE) */
 
 /* QE PIO */
@@ -239,6 +243,7 @@ static inline int qe_alive_during_sleep(void)
 #define qe_muram_addr cpm_muram_addr
 #define qe_muram_offset cpm_muram_offset
 #define qe_muram_dma cpm_muram_dma
+#define qe_muram_free_addr cpm_muram_free_addr
 
 #ifdef CONFIG_PPC32
 #define qe_iowrite8(val, addr)     out_8(addr, val)
-- 
2.23.0

Re: [PATCH net-next v2 04/17] soc: fsl: qe: add cpm_muram_free_addr() helper

[Li Yang]

On Tue, Jan 19, 2021 at 9:21 AM Rasmus Villemoes
<rasmus.villemoes@prevas.dk> wrote:
>
> Add a helper that takes a virtual address rather than the muram
> offset. This will be used in a couple of places to avoid having to
> store both the offset and the virtual address, as well as removing
> NULL checks from the callers.
>
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>

Acked-by: Li Yang <leoyang.li@nxp.com>

> ---
>  drivers/soc/fsl/qe/qe_common.c | 12 ++++++++++++
>  include/soc/fsl/qe/qe.h        |  5 +++++
>  2 files changed, 17 insertions(+)
>
> diff --git a/drivers/soc/fsl/qe/qe_common.c b/drivers/soc/fsl/qe/qe_common.c
> index 303cc2f5eb4a..448ef7f5321a 100644
> --- a/drivers/soc/fsl/qe/qe_common.c
> +++ b/drivers/soc/fsl/qe/qe_common.c
> @@ -238,3 +238,15 @@ dma_addr_t cpm_muram_dma(void __iomem *addr)
>         return muram_pbase + (addr - muram_vbase);
>  }
>  EXPORT_SYMBOL(cpm_muram_dma);
> +
> +/*
> + * As cpm_muram_free, but takes the virtual address rather than the
> + * muram offset.
> + */
> +void cpm_muram_free_addr(const void __iomem *addr)
> +{
> +       if (!addr)
> +               return;
> +       cpm_muram_free(cpm_muram_offset(addr));
> +}
> +EXPORT_SYMBOL(cpm_muram_free_addr);
> diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
> index 8ee3747433c0..66f1afc393d1 100644
> --- a/include/soc/fsl/qe/qe.h
> +++ b/include/soc/fsl/qe/qe.h
> @@ -104,6 +104,7 @@ s32 cpm_muram_alloc_fixed(unsigned long offset, unsigned long size);
>  void __iomem *cpm_muram_addr(unsigned long offset);
>  unsigned long cpm_muram_offset(const void __iomem *addr);
>  dma_addr_t cpm_muram_dma(void __iomem *addr);
> +void cpm_muram_free_addr(const void __iomem *addr);
>  #else
>  static inline s32 cpm_muram_alloc(unsigned long size,
>                                   unsigned long align)
> @@ -135,6 +136,9 @@ static inline dma_addr_t cpm_muram_dma(void __iomem *addr)
>  {
>         return 0;
>  }
> +static inline void cpm_muram_free_addr(const void __iomem *addr)
> +{
> +}
>  #endif /* defined(CONFIG_CPM) || defined(CONFIG_QUICC_ENGINE) */
>
>  /* QE PIO */
> @@ -239,6 +243,7 @@ static inline int qe_alive_during_sleep(void)
>  #define qe_muram_addr cpm_muram_addr
>  #define qe_muram_offset cpm_muram_offset
>  #define qe_muram_dma cpm_muram_dma
> +#define qe_muram_free_addr cpm_muram_free_addr
>
>  #ifdef CONFIG_PPC32
>  #define qe_iowrite8(val, addr)     out_8(addr, val)
> --
> 2.23.0
>

[PATCH net-next v2 05/17] ethernet: ucc_geth: use qe_muram_free_addr()

[Rasmus Villemoes]

This removes the explicit NULL checks, and allows us to stop storing
at least some of the _offset values separately.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 77 ++++++++++-------------
 1 file changed, 33 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index d4b775870f4e..14c58667992e 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1921,50 +1921,39 @@ static void ucc_geth_memclean(struct ucc_geth_private *ugeth)
 		ugeth->uccf = NULL;
 	}
 
-	if (ugeth->p_thread_data_tx) {
-		qe_muram_free(ugeth->thread_dat_tx_offset);
-		ugeth->p_thread_data_tx = NULL;
-	}
-	if (ugeth->p_thread_data_rx) {
-		qe_muram_free(ugeth->thread_dat_rx_offset);
-		ugeth->p_thread_data_rx = NULL;
-	}
-	if (ugeth->p_exf_glbl_param) {
-		qe_muram_free(ugeth->exf_glbl_param_offset);
-		ugeth->p_exf_glbl_param = NULL;
-	}
-	if (ugeth->p_rx_glbl_pram) {
-		qe_muram_free(ugeth->rx_glbl_pram_offset);
-		ugeth->p_rx_glbl_pram = NULL;
-	}
-	if (ugeth->p_tx_glbl_pram) {
-		qe_muram_free(ugeth->tx_glbl_pram_offset);
-		ugeth->p_tx_glbl_pram = NULL;
-	}
-	if (ugeth->p_send_q_mem_reg) {
-		qe_muram_free(ugeth->send_q_mem_reg_offset);
-		ugeth->p_send_q_mem_reg = NULL;
-	}
-	if (ugeth->p_scheduler) {
-		qe_muram_free(ugeth->scheduler_offset);
-		ugeth->p_scheduler = NULL;
-	}
-	if (ugeth->p_tx_fw_statistics_pram) {
-		qe_muram_free(ugeth->tx_fw_statistics_pram_offset);
-		ugeth->p_tx_fw_statistics_pram = NULL;
-	}
-	if (ugeth->p_rx_fw_statistics_pram) {
-		qe_muram_free(ugeth->rx_fw_statistics_pram_offset);
-		ugeth->p_rx_fw_statistics_pram = NULL;
-	}
-	if (ugeth->p_rx_irq_coalescing_tbl) {
-		qe_muram_free(ugeth->rx_irq_coalescing_tbl_offset);
-		ugeth->p_rx_irq_coalescing_tbl = NULL;
-	}
-	if (ugeth->p_rx_bd_qs_tbl) {
-		qe_muram_free(ugeth->rx_bd_qs_tbl_offset);
-		ugeth->p_rx_bd_qs_tbl = NULL;
-	}
+	qe_muram_free_addr(ugeth->p_thread_data_tx);
+	ugeth->p_thread_data_tx = NULL;
+
+	qe_muram_free_addr(ugeth->p_thread_data_rx);
+	ugeth->p_thread_data_rx = NULL;
+
+	qe_muram_free_addr(ugeth->p_exf_glbl_param);
+	ugeth->p_exf_glbl_param = NULL;
+
+	qe_muram_free_addr(ugeth->p_rx_glbl_pram);
+	ugeth->p_rx_glbl_pram = NULL;
+
+	qe_muram_free_addr(ugeth->p_tx_glbl_pram);
+	ugeth->p_tx_glbl_pram = NULL;
+
+	qe_muram_free_addr(ugeth->p_send_q_mem_reg);
+	ugeth->p_send_q_mem_reg = NULL;
+
+	qe_muram_free_addr(ugeth->p_scheduler);
+	ugeth->p_scheduler = NULL;
+
+	qe_muram_free_addr(ugeth->p_tx_fw_statistics_pram);
+	ugeth->p_tx_fw_statistics_pram = NULL;
+
+	qe_muram_free_addr(ugeth->p_rx_fw_statistics_pram);
+	ugeth->p_rx_fw_statistics_pram = NULL;
+
+	qe_muram_free_addr(ugeth->p_rx_irq_coalescing_tbl);
+	ugeth->p_rx_irq_coalescing_tbl = NULL;
+
+	qe_muram_free_addr(ugeth->p_rx_bd_qs_tbl);
+	ugeth->p_rx_bd_qs_tbl = NULL;
+
 	if (ugeth->p_init_enet_param_shadow) {
 		return_init_enet_entries(ugeth,
 					 &(ugeth->p_init_enet_param_shadow->
-- 
2.23.0

[PATCH net-next v2 06/17] ethernet: ucc_geth: remove unnecessary memset_io() calls

[Rasmus Villemoes]

These buffers have all just been handed out from qe_muram_alloc(), aka
cpm_muram_alloc(), and the helper cpm_muram_alloc_common() already
does

        memset_io(cpm_muram_addr(start), 0, size);

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 14c58667992e..be997b559577 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2506,9 +2506,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	ugeth->p_tx_glbl_pram =
 	    (struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth->
 							tx_glbl_pram_offset);
-	/* Zero out p_tx_glbl_pram */
-	memset_io((void __iomem *)ugeth->p_tx_glbl_pram, 0, sizeof(struct ucc_geth_tx_global_pram));
-
 	/* Fill global PRAM */
 
 	/* TQPTR */
@@ -2596,8 +2593,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 							   scheduler_offset);
 		out_be32(&ugeth->p_tx_glbl_pram->schedulerbasepointer,
 			 ugeth->scheduler_offset);
-		/* Zero out p_scheduler */
-		memset_io((void __iomem *)ugeth->p_scheduler, 0, sizeof(struct ucc_geth_scheduler));
 
 		/* Set values in scheduler */
 		out_be32(&ugeth->p_scheduler->mblinterval,
@@ -2640,9 +2635,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 		ugeth->p_tx_fw_statistics_pram =
 		    (struct ucc_geth_tx_firmware_statistics_pram __iomem *)
 		    qe_muram_addr(ugeth->tx_fw_statistics_pram_offset);
-		/* Zero out p_tx_fw_statistics_pram */
-		memset_io((void __iomem *)ugeth->p_tx_fw_statistics_pram,
-		       0, sizeof(struct ucc_geth_tx_firmware_statistics_pram));
 	}
 
 	/* temoder */
@@ -2675,9 +2667,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	ugeth->p_rx_glbl_pram =
 	    (struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth->
 							rx_glbl_pram_offset);
-	/* Zero out p_rx_glbl_pram */
-	memset_io((void __iomem *)ugeth->p_rx_glbl_pram, 0, sizeof(struct ucc_geth_rx_global_pram));
-
 	/* Fill global PRAM */
 
 	/* RQPTR */
@@ -2715,9 +2704,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 		ugeth->p_rx_fw_statistics_pram =
 		    (struct ucc_geth_rx_firmware_statistics_pram __iomem *)
 		    qe_muram_addr(ugeth->rx_fw_statistics_pram_offset);
-		/* Zero out p_rx_fw_statistics_pram */
-		memset_io((void __iomem *)ugeth->p_rx_fw_statistics_pram, 0,
-		       sizeof(struct ucc_geth_rx_firmware_statistics_pram));
 	}
 
 	/* intCoalescingPtr */
@@ -2803,11 +2789,6 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	    (struct ucc_geth_rx_bd_queues_entry __iomem *) qe_muram_addr(ugeth->
 				    rx_bd_qs_tbl_offset);
 	out_be32(&ugeth->p_rx_glbl_pram->rbdqptr, ugeth->rx_bd_qs_tbl_offset);
-	/* Zero out p_rx_bd_qs_tbl */
-	memset_io((void __iomem *)ugeth->p_rx_bd_qs_tbl,
-	       0,
-	       ug_info->numQueuesRx * (sizeof(struct ucc_geth_rx_bd_queues_entry) +
-				       sizeof(struct ucc_geth_rx_prefetched_bds)));
 
 	/* Setup the table */
 	/* Assume BD rings are already established */
-- 
2.23.0

[PATCH net-next v2 07/17] ethernet: ucc_geth: replace kmalloc+memset by kzalloc

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index be997b559577..74ee2ed2fbbb 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2904,14 +2904,11 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	 * allocated resources can be released when the channel is freed.
 	 */
 	if (!(ugeth->p_init_enet_param_shadow =
-	      kmalloc(sizeof(struct ucc_geth_init_pram), GFP_KERNEL))) {
+	      kzalloc(sizeof(struct ucc_geth_init_pram), GFP_KERNEL))) {
 		if (netif_msg_ifup(ugeth))
 			pr_err("Can not allocate memory for p_UccInitEnetParamShadows\n");
 		return -ENOMEM;
 	}
-	/* Zero out *p_init_enet_param_shadow */
-	memset((char *)ugeth->p_init_enet_param_shadow,
-	       0, sizeof(struct ucc_geth_init_pram));
 
 	/* Fill shadow InitEnet command parameter structure */
 
-- 
2.23.0

[PATCH net-next v2 08/17] ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct ucc_geth_private

[Rasmus Villemoes]

These fields are only used within ucc_geth_startup(), so they might as
well be local variables in that function rather than being stashed in
struct ucc_geth_private.

Aside from making that struct a tiny bit smaller, it also shortens
some lines (getting rid of pointless casts while here), and fixes the
problems with using IS_ERR_VALUE() on a u32 as explained in commit
800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc()
to s32").

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------
 drivers/net/ethernet/freescale/ucc_geth.h |  2 --
 2 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 74ee2ed2fbbb..75466489bf9a 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	u8 function_code = 0;
 	u8 __iomem *endOfRing;
 	u8 numThreadsRxNumerical, numThreadsTxNumerical;
+	s32 rx_glbl_pram_offset, tx_glbl_pram_offset;
 
 	ugeth_vdbg("%s: IN", __func__);
 	uccf = ugeth->uccf;
@@ -2495,17 +2496,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	 */
 	/* Tx global PRAM */
 	/* Allocate global tx parameter RAM page */
-	ugeth->tx_glbl_pram_offset =
+	tx_glbl_pram_offset =
 	    qe_muram_alloc(sizeof(struct ucc_geth_tx_global_pram),
 			   UCC_GETH_TX_GLOBAL_PRAM_ALIGNMENT);
-	if (IS_ERR_VALUE(ugeth->tx_glbl_pram_offset)) {
+	if (tx_glbl_pram_offset < 0) {
 		if (netif_msg_ifup(ugeth))
 			pr_err("Can not allocate DPRAM memory for p_tx_glbl_pram\n");
 		return -ENOMEM;
 	}
-	ugeth->p_tx_glbl_pram =
-	    (struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth->
-							tx_glbl_pram_offset);
+	ugeth->p_tx_glbl_pram = qe_muram_addr(tx_glbl_pram_offset);
 	/* Fill global PRAM */
 
 	/* TQPTR */
@@ -2656,17 +2655,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 
 	/* Rx global PRAM */
 	/* Allocate global rx parameter RAM page */
-	ugeth->rx_glbl_pram_offset =
+	rx_glbl_pram_offset =
 	    qe_muram_alloc(sizeof(struct ucc_geth_rx_global_pram),
 			   UCC_GETH_RX_GLOBAL_PRAM_ALIGNMENT);
-	if (IS_ERR_VALUE(ugeth->rx_glbl_pram_offset)) {
+	if (rx_glbl_pram_offset < 0) {
 		if (netif_msg_ifup(ugeth))
 			pr_err("Can not allocate DPRAM memory for p_rx_glbl_pram\n");
 		return -ENOMEM;
 	}
-	ugeth->p_rx_glbl_pram =
-	    (struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth->
-							rx_glbl_pram_offset);
+	ugeth->p_rx_glbl_pram = qe_muram_addr(rx_glbl_pram_offset);
 	/* Fill global PRAM */
 
 	/* RQPTR */
@@ -2928,7 +2925,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	    ((u32) ug_info->numThreadsTx) << ENET_INIT_PARAM_TGF_SHIFT;
 
 	ugeth->p_init_enet_param_shadow->rgftgfrxglobal |=
-	    ugeth->rx_glbl_pram_offset | ug_info->riscRx;
+	    rx_glbl_pram_offset | ug_info->riscRx;
 	if ((ug_info->largestexternallookupkeysize !=
 	     QE_FLTR_LARGEST_EXTERNAL_TABLE_LOOKUP_KEY_SIZE_NONE) &&
 	    (ug_info->largestexternallookupkeysize !=
@@ -2966,7 +2963,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	}
 
 	ugeth->p_init_enet_param_shadow->txglobal =
-	    ugeth->tx_glbl_pram_offset | ug_info->riscTx;
+	    tx_glbl_pram_offset | ug_info->riscTx;
 	if ((ret_val =
 	     fill_init_enet_entries(ugeth,
 				    &(ugeth->p_init_enet_param_shadow->
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
index c80bed2c995c..be47fa8ced15 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -1166,9 +1166,7 @@ struct ucc_geth_private {
 	struct ucc_geth_exf_global_pram __iomem *p_exf_glbl_param;
 	u32 exf_glbl_param_offset;
 	struct ucc_geth_rx_global_pram __iomem *p_rx_glbl_pram;
-	u32 rx_glbl_pram_offset;
 	struct ucc_geth_tx_global_pram __iomem *p_tx_glbl_pram;
-	u32 tx_glbl_pram_offset;
 	struct ucc_geth_send_queue_mem_region __iomem *p_send_q_mem_reg;
 	u32 send_q_mem_reg_offset;
 	struct ucc_geth_thread_data_tx __iomem *p_thread_data_tx;
-- 
2.23.0

Re: [PATCH net-next v2 08/17] ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct ucc_geth_private

[Christophe Leroy]

Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
> These fields are only used within ucc_geth_startup(), so they might as
> well be local variables in that function rather than being stashed in
> struct ucc_geth_private.
> 
> Aside from making that struct a tiny bit smaller, it also shortens
> some lines (getting rid of pointless casts while here), and fixes the
> problems with using IS_ERR_VALUE() on a u32 as explained in commit
> 800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc()
> to s32").
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------
>   drivers/net/ethernet/freescale/ucc_geth.h |  2 --
>   2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index 74ee2ed2fbbb..75466489bf9a 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   	u8 function_code = 0;
>   	u8 __iomem *endOfRing;
>   	u8 numThreadsRxNumerical, numThreadsTxNumerical;
> +	s32 rx_glbl_pram_offset, tx_glbl_pram_offset;

That's still a quite long name for a local variable. Kernel Codying Style says:


LOCAL variable names should be short, and to the point. If you have some random integer loop 
counter, it should probably be called i. Calling it loop_counter is non-productive, if there is no 
chance of it being mis-understood. Similarly, tmp can be just about any type of variable that is 
used to hold a temporary value.

If you are afraid to mix up your local variable names, you have another problem, which is called the 
function-growth-hormone-imbalance syndrome. See chapter 6 (Functions).

>   
>   	ugeth_vdbg("%s: IN", __func__);
>   	uccf = ugeth->uccf;
> @@ -2495,17 +2496,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   	 */
>   	/* Tx global PRAM */
>   	/* Allocate global tx parameter RAM page */
> -	ugeth->tx_glbl_pram_offset =
> +	tx_glbl_pram_offset =
>   	    qe_muram_alloc(sizeof(struct ucc_geth_tx_global_pram),
>   			   UCC_GETH_TX_GLOBAL_PRAM_ALIGNMENT);
> -	if (IS_ERR_VALUE(ugeth->tx_glbl_pram_offset)) {
> +	if (tx_glbl_pram_offset < 0) {
>   		if (netif_msg_ifup(ugeth))
>   			pr_err("Can not allocate DPRAM memory for p_tx_glbl_pram\n");
>   		return -ENOMEM;
>   	}
> -	ugeth->p_tx_glbl_pram =
> -	    (struct ucc_geth_tx_global_pram __iomem *) qe_muram_addr(ugeth->
> -							tx_glbl_pram_offset);
> +	ugeth->p_tx_glbl_pram = qe_muram_addr(tx_glbl_pram_offset);
>   	/* Fill global PRAM */
>   
>   	/* TQPTR */
> @@ -2656,17 +2655,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   
>   	/* Rx global PRAM */
>   	/* Allocate global rx parameter RAM page */
> -	ugeth->rx_glbl_pram_offset =
> +	rx_glbl_pram_offset =
>   	    qe_muram_alloc(sizeof(struct ucc_geth_rx_global_pram),
>   			   UCC_GETH_RX_GLOBAL_PRAM_ALIGNMENT);
> -	if (IS_ERR_VALUE(ugeth->rx_glbl_pram_offset)) {
> +	if (rx_glbl_pram_offset < 0) {
>   		if (netif_msg_ifup(ugeth))
>   			pr_err("Can not allocate DPRAM memory for p_rx_glbl_pram\n");
>   		return -ENOMEM;
>   	}
> -	ugeth->p_rx_glbl_pram =
> -	    (struct ucc_geth_rx_global_pram __iomem *) qe_muram_addr(ugeth->
> -							rx_glbl_pram_offset);
> +	ugeth->p_rx_glbl_pram = qe_muram_addr(rx_glbl_pram_offset);
>   	/* Fill global PRAM */
>   
>   	/* RQPTR */
> @@ -2928,7 +2925,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   	    ((u32) ug_info->numThreadsTx) << ENET_INIT_PARAM_TGF_SHIFT;
>   
>   	ugeth->p_init_enet_param_shadow->rgftgfrxglobal |=
> -	    ugeth->rx_glbl_pram_offset | ug_info->riscRx;
> +	    rx_glbl_pram_offset | ug_info->riscRx;
>   	if ((ug_info->largestexternallookupkeysize !=
>   	     QE_FLTR_LARGEST_EXTERNAL_TABLE_LOOKUP_KEY_SIZE_NONE) &&
>   	    (ug_info->largestexternallookupkeysize !=
> @@ -2966,7 +2963,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   	}
>   
>   	ugeth->p_init_enet_param_shadow->txglobal =
> -	    ugeth->tx_glbl_pram_offset | ug_info->riscTx;
> +	    tx_glbl_pram_offset | ug_info->riscTx;
>   	if ((ret_val =
>   	     fill_init_enet_entries(ugeth,
>   				    &(ugeth->p_init_enet_param_shadow->
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
> index c80bed2c995c..be47fa8ced15 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.h
> +++ b/drivers/net/ethernet/freescale/ucc_geth.h
> @@ -1166,9 +1166,7 @@ struct ucc_geth_private {
>   	struct ucc_geth_exf_global_pram __iomem *p_exf_glbl_param;
>   	u32 exf_glbl_param_offset;
>   	struct ucc_geth_rx_global_pram __iomem *p_rx_glbl_pram;
> -	u32 rx_glbl_pram_offset;
>   	struct ucc_geth_tx_global_pram __iomem *p_tx_glbl_pram;
> -	u32 tx_glbl_pram_offset;
>   	struct ucc_geth_send_queue_mem_region __iomem *p_send_q_mem_reg;
>   	u32 send_q_mem_reg_offset;
>   	struct ucc_geth_thread_data_tx __iomem *p_thread_data_tx;
> 

Re: [PATCH net-next v2 08/17] ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct ucc_geth_private

[Rasmus Villemoes]

On 20/01/2021 07.57, Christophe Leroy wrote:
> 
> 
> Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
>> These fields are only used within ucc_geth_startup(), so they might as
>> well be local variables in that function rather than being stashed in
>> struct ucc_geth_private.
>>
>> Aside from making that struct a tiny bit smaller, it also shortens
>> some lines (getting rid of pointless casts while here), and fixes the
>> problems with using IS_ERR_VALUE() on a u32 as explained in commit
>> 800cd6fb76f0 ("soc: fsl: qe: change return type of cpm_muram_alloc()
>> to s32").
>>
>> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
>> ---
>>   drivers/net/ethernet/freescale/ucc_geth.c | 21 +++++++++------------
>>   drivers/net/ethernet/freescale/ucc_geth.h |  2 --
>>   2 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c
>> b/drivers/net/ethernet/freescale/ucc_geth.c
>> index 74ee2ed2fbbb..75466489bf9a 100644
>> --- a/drivers/net/ethernet/freescale/ucc_geth.c
>> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
>> @@ -2351,6 +2351,7 @@ static int ucc_geth_startup(struct
>> ucc_geth_private *ugeth)
>>       u8 function_code = 0;
>>       u8 __iomem *endOfRing;
>>       u8 numThreadsRxNumerical, numThreadsTxNumerical;
>> +    s32 rx_glbl_pram_offset, tx_glbl_pram_offset;
> 
> That's still a quite long name for a local variable. 

True, but I wanted to keep this mechanical and easy to verify. If
somebody wants to clean up the local variable names
(numThreads[RT]xNumerical also stand out), that can be done later.

Rasmus

[PATCH net-next v2 09/17] ethernet: ucc_geth: factor out parsing of {rx,tx}-clock{,-name} properties

[Rasmus Villemoes]

Reduce the code duplication a bit by moving the parsing of
rx-clock-name and the fallback handling to a helper function.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 80 ++++++++++-------------
 1 file changed, 36 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 75466489bf9a..75d1fb049698 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3646,6 +3646,36 @@ static const struct net_device_ops ucc_geth_netdev_ops = {
 #endif
 };
 
+static int ucc_geth_parse_clock(struct device_node *np, const char *which,
+				enum qe_clock *out)
+{
+	const char *sprop;
+	char buf[24];
+
+	snprintf(buf, sizeof(buf), "%s-clock-name", which);
+	sprop = of_get_property(np, buf, NULL);
+	if (sprop) {
+		*out = qe_clock_source(sprop);
+	} else {
+		u32 val;
+
+		snprintf(buf, sizeof(buf), "%s-clock", which);
+		if (of_property_read_u32(np, buf, &val)) {
+			/* If both *-clock-name and *-clock are missing,
+			 * we want to tell people to use *-clock-name.
+			 */
+			pr_err("missing %s-clock-name property\n", buf);
+			return -EINVAL;
+		}
+		*out = val;
+	}
+	if (*out < QE_CLK_NONE || *out > QE_CLK24) {
+		pr_err("invalid %s property\n", buf);
+		return -EINVAL;
+	}
+	return 0;
+}
+
 static int ucc_geth_probe(struct platform_device* ofdev)
 {
 	struct device *device = &ofdev->dev;
@@ -3656,7 +3686,6 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 	struct resource res;
 	int err, ucc_num, max_speed = 0;
 	const unsigned int *prop;
-	const char *sprop;
 	const void *mac_addr;
 	phy_interface_t phy_interface;
 	static const int enet_to_speed[] = {
@@ -3695,49 +3724,12 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 
 	ug_info->uf_info.ucc_num = ucc_num;
 
-	sprop = of_get_property(np, "rx-clock-name", NULL);
-	if (sprop) {
-		ug_info->uf_info.rx_clock = qe_clock_source(sprop);
-		if ((ug_info->uf_info.rx_clock < QE_CLK_NONE) ||
-		    (ug_info->uf_info.rx_clock > QE_CLK24)) {
-			pr_err("invalid rx-clock-name property\n");
-			return -EINVAL;
-		}
-	} else {
-		prop = of_get_property(np, "rx-clock", NULL);
-		if (!prop) {
-			/* If both rx-clock-name and rx-clock are missing,
-			   we want to tell people to use rx-clock-name. */
-			pr_err("missing rx-clock-name property\n");
-			return -EINVAL;
-		}
-		if ((*prop < QE_CLK_NONE) || (*prop > QE_CLK24)) {
-			pr_err("invalid rx-clock property\n");
-			return -EINVAL;
-		}
-		ug_info->uf_info.rx_clock = *prop;
-	}
-
-	sprop = of_get_property(np, "tx-clock-name", NULL);
-	if (sprop) {
-		ug_info->uf_info.tx_clock = qe_clock_source(sprop);
-		if ((ug_info->uf_info.tx_clock < QE_CLK_NONE) ||
-		    (ug_info->uf_info.tx_clock > QE_CLK24)) {
-			pr_err("invalid tx-clock-name property\n");
-			return -EINVAL;
-		}
-	} else {
-		prop = of_get_property(np, "tx-clock", NULL);
-		if (!prop) {
-			pr_err("missing tx-clock-name property\n");
-			return -EINVAL;
-		}
-		if ((*prop < QE_CLK_NONE) || (*prop > QE_CLK24)) {
-			pr_err("invalid tx-clock property\n");
-			return -EINVAL;
-		}
-		ug_info->uf_info.tx_clock = *prop;
-	}
+	err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock);
+	if (err)
+		return err;
+	err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock);
+	if (err)
+		return err;
 
 	err = of_address_to_resource(np, 0, &res);
 	if (err)
-- 
2.23.0

[PATCH net-next v2 10/17] ethernet: ucc_geth: constify ugeth_primary_info

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 75d1fb049698..65ef7ae38912 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -70,7 +70,7 @@ static struct {
 module_param_named(debug, debug.msg_enable, int, 0);
 MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 0xffff=all)");
 
-static struct ucc_geth_info ugeth_primary_info = {
+static const struct ucc_geth_info ugeth_primary_info = {
 	.uf_info = {
 		    .bd_mem_part = MEM_PART_SYSTEM,
 		    .rtsm = UCC_FAST_SEND_IDLES_BETWEEN_FRAMES,
-- 
2.23.0

[PATCH net-next v2 11/17] ethernet: ucc_geth: don't statically allocate eight ucc_geth_info

[Rasmus Villemoes]

struct ucc_geth_info is somewhat large, and on systems with only one
or two UCC instances, that just wastes a few KB of memory. So
allocate and populate a chunk of memory at probe time instead of
initializing them all during driver init.

Note that the existing "ug_info == NULL" check was dead code, as the
address of some static array element can obviously never be NULL.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++--------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 65ef7ae38912..67b93d60243e 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = {
 	.riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2,
 };
 
-static struct ucc_geth_info ugeth_info[8];
-
 #ifdef DEBUG
 static void mem_disp(u8 *addr, int size)
 {
@@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 	if ((ucc_num < 0) || (ucc_num > 7))
 		return -ENODEV;
 
-	ug_info = &ugeth_info[ucc_num];
-	if (ug_info == NULL) {
-		if (netif_msg_probe(&debug))
-			pr_err("[%d] Missing additional data!\n", ucc_num);
-		return -ENODEV;
-	}
+	ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
+	if (ug_info == NULL)
+		return -ENOMEM;
+	memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info));
 
 	ug_info->uf_info.ucc_num = ucc_num;
 
 	err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock);
 	if (err)
-		return err;
+		goto err_free_info;
 	err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock);
 	if (err)
-		return err;
+		goto err_free_info;
 
 	err = of_address_to_resource(np, 0, &res);
 	if (err)
-		return -EINVAL;
+		goto err_free_info;
 
 	ug_info->uf_info.regs = res.start;
 	ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);
@@ -3746,7 +3742,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 		 */
 		err = of_phy_register_fixed_link(np);
 		if (err)
-			return err;
+			goto err_free_info;
 		ug_info->phy_node = of_node_get(np);
 	}
 
@@ -3877,6 +3873,8 @@ static int ucc_geth_probe(struct platform_device* ofdev)
 		of_phy_deregister_fixed_link(np);
 	of_node_put(ug_info->tbi_node);
 	of_node_put(ug_info->phy_node);
+err_free_info:
+	kfree(ug_info);
 
 	return err;
 }
@@ -3893,6 +3891,7 @@ static int ucc_geth_remove(struct platform_device* ofdev)
 		of_phy_deregister_fixed_link(np);
 	of_node_put(ugeth->ug_info->tbi_node);
 	of_node_put(ugeth->ug_info->phy_node);
+	kfree(ugeth->ug_info);
 	free_netdev(dev);
 
 	return 0;
@@ -3921,17 +3920,10 @@ static struct platform_driver ucc_geth_driver = {
 
 static int __init ucc_geth_init(void)
 {
-	int i, ret;
-
 	if (netif_msg_drv(&debug))
 		pr_info(DRV_DESC "\n");
-	for (i = 0; i < 8; i++)
-		memcpy(&(ugeth_info[i]), &ugeth_primary_info,
-		       sizeof(ugeth_primary_info));
-
-	ret = platform_driver_register(&ucc_geth_driver);
 
-	return ret;
+	return platform_driver_register(&ucc_geth_driver);
 }
 
 static void __exit ucc_geth_exit(void)
-- 
2.23.0

Re: [PATCH net-next v2 11/17] ethernet: ucc_geth: don't statically allocate eight ucc_geth_info

[Christophe Leroy]

Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
> struct ucc_geth_info is somewhat large, and on systems with only one
> or two UCC instances, that just wastes a few KB of memory. So
> allocate and populate a chunk of memory at probe time instead of
> initializing them all during driver init.
> 
> Note that the existing "ug_info == NULL" check was dead code, as the
> address of some static array element can obviously never be NULL.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/ethernet/freescale/ucc_geth.c | 32 +++++++++--------------
>   1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index 65ef7ae38912..67b93d60243e 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -157,8 +157,6 @@ static const struct ucc_geth_info ugeth_primary_info = {
>   	.riscRx = QE_RISC_ALLOCATION_RISC1_AND_RISC2,
>   };
>   
> -static struct ucc_geth_info ugeth_info[8];
> -
>   #ifdef DEBUG
>   static void mem_disp(u8 *addr, int size)
>   {
> @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct platform_device* ofdev)
>   	if ((ucc_num < 0) || (ucc_num > 7))
>   		return -ENODEV;
>   
> -	ug_info = &ugeth_info[ucc_num];
> -	if (ug_info == NULL) {
> -		if (netif_msg_probe(&debug))
> -			pr_err("[%d] Missing additional data!\n", ucc_num);
> -		return -ENODEV;
> -	}
> +	ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);

What about using devm_kmalloc() and avoid those kfree and associated goto ?


> +	if (ug_info == NULL)
> +		return -ENOMEM;
> +	memcpy(ug_info, &ugeth_primary_info, sizeof(*ug_info));
>   
>   	ug_info->uf_info.ucc_num = ucc_num;
>   
>   	err = ucc_geth_parse_clock(np, "rx", &ug_info->uf_info.rx_clock);
>   	if (err)
> -		return err;
> +		goto err_free_info;
>   	err = ucc_geth_parse_clock(np, "tx", &ug_info->uf_info.tx_clock);
>   	if (err)
> -		return err;
> +		goto err_free_info;
>   
>   	err = of_address_to_resource(np, 0, &res);
>   	if (err)
> -		return -EINVAL;
> +		goto err_free_info;
>   
>   	ug_info->uf_info.regs = res.start;
>   	ug_info->uf_info.irq = irq_of_parse_and_map(np, 0);

Re: [PATCH net-next v2 11/17] ethernet: ucc_geth: don't statically allocate eight ucc_geth_info

[Rasmus Villemoes]

On 20/01/2021 08.02, Christophe Leroy wrote:
>
>> @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct
>> platform_device* ofdev)
>>       if ((ucc_num < 0) || (ucc_num > 7))
>>           return -ENODEV;
>>   -    ug_info = &ugeth_info[ucc_num];
>> -    if (ug_info == NULL) {
>> -        if (netif_msg_probe(&debug))
>> -            pr_err("[%d] Missing additional data!\n", ucc_num);
>> -        return -ENODEV;
>> -    }
>> +    ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
> 
> What about using devm_kmalloc() and avoid those kfree and associated goto ?

I already replied to that: I'd rather not mix kmalloc() and
devm_kmalloc() as that makes it much harder to reason about the order in
which stuff gets deallocated. But sure, if you insist.

Rasmus

Re: [PATCH net-next v2 11/17] ethernet: ucc_geth: don't statically allocate eight ucc_geth_info

[Christophe Leroy]

Le 20/01/2021 à 12:25, Rasmus Villemoes a écrit :
> On 20/01/2021 08.02, Christophe Leroy wrote:
>>
>>> @@ -3715,25 +3713,23 @@ static int ucc_geth_probe(struct
>>> platform_device* ofdev)
>>>        if ((ucc_num < 0) || (ucc_num > 7))
>>>            return -ENODEV;
>>>    -    ug_info = &ugeth_info[ucc_num];
>>> -    if (ug_info == NULL) {
>>> -        if (netif_msg_probe(&debug))
>>> -            pr_err("[%d] Missing additional data!\n", ucc_num);
>>> -        return -ENODEV;
>>> -    }
>>> +    ug_info = kmalloc(sizeof(*ug_info), GFP_KERNEL);
>>
>> What about using devm_kmalloc() and avoid those kfree and associated goto ?
> 
> I already replied to that: I'd rather not mix kmalloc() and
> devm_kmalloc() as that makes it much harder to reason about the order in
> which stuff gets deallocated. But sure, if you insist.
> 

I didn't remember I already did the same comment, sorry.

Christophe

[PATCH net-next v2 12/17] ethernet: ucc_geth: use UCC_GETH_{RX,TX}_BD_RING_ALIGNMENT macros directly

[Rasmus Villemoes]

These macros both have the value 32, there's no point first
initializing align to a lower value.

If anything, one could throw in a
BUILD_BUG_ON(UCC_GETH_TX_BD_RING_ALIGNMENT < 4), but it's not worth it
- lots of code depends on named constants having sensible values.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 67b93d60243e..2369a5ede680 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2196,9 +2196,8 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 		    UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
 			length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
 		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
-			u32 align = 4;
-			if (UCC_GETH_TX_BD_RING_ALIGNMENT > 4)
-				align = UCC_GETH_TX_BD_RING_ALIGNMENT;
+			u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
+
 			ugeth->tx_bd_ring_offset[j] =
 				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
 
@@ -2274,9 +2273,8 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 	for (j = 0; j < ug_info->numQueuesRx; j++) {
 		length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
 		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
-			u32 align = 4;
-			if (UCC_GETH_RX_BD_RING_ALIGNMENT > 4)
-				align = UCC_GETH_RX_BD_RING_ALIGNMENT;
+			u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
+
 			ugeth->rx_bd_ring_offset[j] =
 				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
 			if (ugeth->rx_bd_ring_offset[j] != 0)
-- 
2.23.0

[PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem_part and all associated code

[Rasmus Villemoes]

The bd_mem_part member of ucc_geth_info always has the value
MEM_PART_SYSTEM, and AFAICT, there has never been any code setting it
to any other value. Moreover, muram is a somewhat precious resource,
so there's no point using that when normal memory serves just as well.

Apart from removing a lot of dead code, this is also motivated by
wanting to clean up the "store result from kmalloc() in a u32" mess.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 108 ++++++----------------
 include/soc/fsl/qe/qe.h                   |   6 --
 include/soc/fsl/qe/ucc_fast.h             |   1 -
 3 files changed, 29 insertions(+), 86 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 2369a5ede680..1e9d2f3f47a3 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -72,7 +72,6 @@ MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 0xffff=all)");
 
 static const struct ucc_geth_info ugeth_primary_info = {
 	.uf_info = {
-		    .bd_mem_part = MEM_PART_SYSTEM,
 		    .rtsm = UCC_FAST_SEND_IDLES_BETWEEN_FRAMES,
 		    .max_rx_buf_length = 1536,
 		    /* adjusted at startup if max-speed 1000 */
@@ -1854,12 +1853,7 @@ static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
 
 			kfree(ugeth->rx_skbuff[i]);
 
-			if (ugeth->ug_info->uf_info.bd_mem_part ==
-			    MEM_PART_SYSTEM)
-				kfree((void *)ugeth->rx_bd_ring_offset[i]);
-			else if (ugeth->ug_info->uf_info.bd_mem_part ==
-				 MEM_PART_MURAM)
-				qe_muram_free(ugeth->rx_bd_ring_offset[i]);
+			kfree((void *)ugeth->rx_bd_ring_offset[i]);
 			ugeth->p_rx_bd_ring[i] = NULL;
 		}
 	}
@@ -1897,12 +1891,7 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
 		kfree(ugeth->tx_skbuff[i]);
 
 		if (ugeth->p_tx_bd_ring[i]) {
-			if (ugeth->ug_info->uf_info.bd_mem_part ==
-			    MEM_PART_SYSTEM)
-				kfree((void *)ugeth->tx_bd_ring_offset[i]);
-			else if (ugeth->ug_info->uf_info.bd_mem_part ==
-				 MEM_PART_MURAM)
-				qe_muram_free(ugeth->tx_bd_ring_offset[i]);
+			kfree((void *)ugeth->tx_bd_ring_offset[i]);
 			ugeth->p_tx_bd_ring[i] = NULL;
 		}
 	}
@@ -2060,13 +2049,6 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 	ug_info = ugeth->ug_info;
 	uf_info = &ug_info->uf_info;
 
-	if (!((uf_info->bd_mem_part == MEM_PART_SYSTEM) ||
-	      (uf_info->bd_mem_part == MEM_PART_MURAM))) {
-		if (netif_msg_probe(ugeth))
-			pr_err("Bad memory partition value\n");
-		return -EINVAL;
-	}
-
 	/* Rx BD lengths */
 	for (i = 0; i < ug_info->numQueuesRx; i++) {
 		if ((ug_info->bdRingLenRx[i] < UCC_GETH_RX_BD_RING_SIZE_MIN) ||
@@ -2186,6 +2168,8 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 
 	/* Allocate Tx bds */
 	for (j = 0; j < ug_info->numQueuesTx; j++) {
+		u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
+
 		/* Allocate in multiple of
 		   UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT,
 		   according to spec */
@@ -2195,25 +2179,15 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 		if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
 		    UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
 			length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
-		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
-			u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
-
-			ugeth->tx_bd_ring_offset[j] =
-				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
-
-			if (ugeth->tx_bd_ring_offset[j] != 0)
-				ugeth->p_tx_bd_ring[j] =
-					(u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
-					align) & ~(align - 1));
-		} else if (uf_info->bd_mem_part == MEM_PART_MURAM) {
-			ugeth->tx_bd_ring_offset[j] =
-			    qe_muram_alloc(length,
-					   UCC_GETH_TX_BD_RING_ALIGNMENT);
-			if (!IS_ERR_VALUE(ugeth->tx_bd_ring_offset[j]))
-				ugeth->p_tx_bd_ring[j] =
-				    (u8 __iomem *) qe_muram_addr(ugeth->
-							 tx_bd_ring_offset[j]);
-		}
+
+		ugeth->tx_bd_ring_offset[j] =
+			(u32) kmalloc((u32) (length + align), GFP_KERNEL);
+
+		if (ugeth->tx_bd_ring_offset[j] != 0)
+			ugeth->p_tx_bd_ring[j] =
+				(u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
+						align) & ~(align - 1));
+
 		if (!ugeth->p_tx_bd_ring[j]) {
 			if (netif_msg_ifup(ugeth))
 				pr_err("Can not allocate memory for Tx bd rings\n");
@@ -2271,25 +2245,16 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 
 	/* Allocate Rx bds */
 	for (j = 0; j < ug_info->numQueuesRx; j++) {
+		u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
+
 		length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
-		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
-			u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
-
-			ugeth->rx_bd_ring_offset[j] =
-				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
-			if (ugeth->rx_bd_ring_offset[j] != 0)
-				ugeth->p_rx_bd_ring[j] =
-					(u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
-					align) & ~(align - 1));
-		} else if (uf_info->bd_mem_part == MEM_PART_MURAM) {
-			ugeth->rx_bd_ring_offset[j] =
-			    qe_muram_alloc(length,
-					   UCC_GETH_RX_BD_RING_ALIGNMENT);
-			if (!IS_ERR_VALUE(ugeth->rx_bd_ring_offset[j]))
-				ugeth->p_rx_bd_ring[j] =
-				    (u8 __iomem *) qe_muram_addr(ugeth->
-							 rx_bd_ring_offset[j]);
-		}
+		ugeth->rx_bd_ring_offset[j] =
+			(u32) kmalloc((u32) (length + align), GFP_KERNEL);
+		if (ugeth->rx_bd_ring_offset[j] != 0)
+			ugeth->p_rx_bd_ring[j] =
+				(u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
+						align) & ~(align - 1));
+
 		if (!ugeth->p_rx_bd_ring[j]) {
 			if (netif_msg_ifup(ugeth))
 				pr_err("Can not allocate memory for Rx bd rings\n");
@@ -2554,20 +2519,11 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 		endOfRing =
 		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
 					      1) * sizeof(struct qe_bd);
-		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-				 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
-				 last_bd_completed_address,
-				 (u32) virt_to_phys(endOfRing));
-		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
-			   MEM_PART_MURAM) {
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-				 (u32)qe_muram_dma(ugeth->p_tx_bd_ring[i]));
-			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
-				 last_bd_completed_address,
-				 (u32)qe_muram_dma(endOfRing));
-		}
+		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
+			 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
+		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
+			 last_bd_completed_address,
+			 (u32) virt_to_phys(endOfRing));
 	}
 
 	/* schedulerbasepointer */
@@ -2786,14 +2742,8 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	/* Setup the table */
 	/* Assume BD rings are already established */
 	for (i = 0; i < ug_info->numQueuesRx; i++) {
-		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
-			out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
-				 (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
-		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
-			   MEM_PART_MURAM) {
-			out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
-				 (u32)qe_muram_dma(ugeth->p_rx_bd_ring[i]));
-		}
+		out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
+			 (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
 		/* rest of fields handled by QE */
 	}
 
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index 66f1afc393d1..4925a1b59dc9 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -27,12 +27,6 @@
 #define QE_NUM_OF_BRGS	16
 #define QE_NUM_OF_PORTS	1024
 
-/* Memory partitions
-*/
-#define MEM_PART_SYSTEM		0
-#define MEM_PART_SECONDARY	1
-#define MEM_PART_MURAM		2
-
 /* Clocks and BRGs */
 enum qe_clock {
 	QE_CLK_NONE = 0,
diff --git a/include/soc/fsl/qe/ucc_fast.h b/include/soc/fsl/qe/ucc_fast.h
index dc4e79468094..9696a5b9b5d1 100644
--- a/include/soc/fsl/qe/ucc_fast.h
+++ b/include/soc/fsl/qe/ucc_fast.h
@@ -146,7 +146,6 @@ struct ucc_fast_info {
 	resource_size_t regs;
 	int irq;
 	u32 uccm_mask;
-	int bd_mem_part;
 	int brkpt_support;
 	int grant_support;
 	int tsa;
-- 
2.23.0

Re: [PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem_part and all associated code

[Christophe Leroy]

Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
> The bd_mem_part member of ucc_geth_info always has the value
> MEM_PART_SYSTEM, and AFAICT, there has never been any code setting it
> to any other value. Moreover, muram is a somewhat precious resource,
> so there's no point using that when normal memory serves just as well.
> 
> Apart from removing a lot of dead code, this is also motivated by
> wanting to clean up the "store result from kmalloc() in a u32" mess.
> 
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/ethernet/freescale/ucc_geth.c | 108 ++++++----------------
>   include/soc/fsl/qe/qe.h                   |   6 --
>   include/soc/fsl/qe/ucc_fast.h             |   1 -
>   3 files changed, 29 insertions(+), 86 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index 2369a5ede680..1e9d2f3f47a3 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -72,7 +72,6 @@ MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 0xffff=all)");
>   
>   static const struct ucc_geth_info ugeth_primary_info = {
>   	.uf_info = {
> -		    .bd_mem_part = MEM_PART_SYSTEM,
>   		    .rtsm = UCC_FAST_SEND_IDLES_BETWEEN_FRAMES,
>   		    .max_rx_buf_length = 1536,
>   		    /* adjusted at startup if max-speed 1000 */
> @@ -1854,12 +1853,7 @@ static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
>   
>   			kfree(ugeth->rx_skbuff[i]);
>   
> -			if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			    MEM_PART_SYSTEM)
> -				kfree((void *)ugeth->rx_bd_ring_offset[i]);
> -			else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -				 MEM_PART_MURAM)
> -				qe_muram_free(ugeth->rx_bd_ring_offset[i]);
> +			kfree((void *)ugeth->rx_bd_ring_offset[i]);
>   			ugeth->p_rx_bd_ring[i] = NULL;
>   		}
>   	}
> @@ -1897,12 +1891,7 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
>   		kfree(ugeth->tx_skbuff[i]);
>   
>   		if (ugeth->p_tx_bd_ring[i]) {
> -			if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			    MEM_PART_SYSTEM)
> -				kfree((void *)ugeth->tx_bd_ring_offset[i]);
> -			else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -				 MEM_PART_MURAM)
> -				qe_muram_free(ugeth->tx_bd_ring_offset[i]);
> +			kfree((void *)ugeth->tx_bd_ring_offset[i]);
>   			ugeth->p_tx_bd_ring[i] = NULL;
>   		}
>   	}
> @@ -2060,13 +2049,6 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
>   	ug_info = ugeth->ug_info;
>   	uf_info = &ug_info->uf_info;
>   
> -	if (!((uf_info->bd_mem_part == MEM_PART_SYSTEM) ||
> -	      (uf_info->bd_mem_part == MEM_PART_MURAM))) {
> -		if (netif_msg_probe(ugeth))
> -			pr_err("Bad memory partition value\n");
> -		return -EINVAL;
> -	}
> -
>   	/* Rx BD lengths */
>   	for (i = 0; i < ug_info->numQueuesRx; i++) {
>   		if ((ug_info->bdRingLenRx[i] < UCC_GETH_RX_BD_RING_SIZE_MIN) ||
> @@ -2186,6 +2168,8 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
>   
>   	/* Allocate Tx bds */
>   	for (j = 0; j < ug_info->numQueuesTx; j++) {
> +		u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
> +
>   		/* Allocate in multiple of
>   		   UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT,
>   		   according to spec */
> @@ -2195,25 +2179,15 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
>   		if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
>   		    UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
>   			length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
> -		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
> -			u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
> -
> -			ugeth->tx_bd_ring_offset[j] =
> -				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
> -
> -			if (ugeth->tx_bd_ring_offset[j] != 0)
> -				ugeth->p_tx_bd_ring[j] =
> -					(u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
> -					align) & ~(align - 1));
> -		} else if (uf_info->bd_mem_part == MEM_PART_MURAM) {
> -			ugeth->tx_bd_ring_offset[j] =
> -			    qe_muram_alloc(length,
> -					   UCC_GETH_TX_BD_RING_ALIGNMENT);
> -			if (!IS_ERR_VALUE(ugeth->tx_bd_ring_offset[j]))
> -				ugeth->p_tx_bd_ring[j] =
> -				    (u8 __iomem *) qe_muram_addr(ugeth->
> -							 tx_bd_ring_offset[j]);
> -		}
> +
> +		ugeth->tx_bd_ring_offset[j] =
> +			(u32) kmalloc((u32) (length + align), GFP_KERNEL);

Can't this fit on a single ? Nowadays, max allowed line length is 100 chars.

> +
> +		if (ugeth->tx_bd_ring_offset[j] != 0)
> +			ugeth->p_tx_bd_ring[j] =
> +				(u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
> +						align) & ~(align - 1));

Can we get the above fit on only 2 lines ?

> +
>   		if (!ugeth->p_tx_bd_ring[j]) {
>   			if (netif_msg_ifup(ugeth))
>   				pr_err("Can not allocate memory for Tx bd rings\n");
> @@ -2271,25 +2245,16 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
>   
>   	/* Allocate Rx bds */
>   	for (j = 0; j < ug_info->numQueuesRx; j++) {
> +		u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
> +
>   		length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
> -		if (uf_info->bd_mem_part == MEM_PART_SYSTEM) {
> -			u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
> -
> -			ugeth->rx_bd_ring_offset[j] =
> -				(u32) kmalloc((u32) (length + align), GFP_KERNEL);
> -			if (ugeth->rx_bd_ring_offset[j] != 0)
> -				ugeth->p_rx_bd_ring[j] =
> -					(u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
> -					align) & ~(align - 1));
> -		} else if (uf_info->bd_mem_part == MEM_PART_MURAM) {
> -			ugeth->rx_bd_ring_offset[j] =
> -			    qe_muram_alloc(length,
> -					   UCC_GETH_RX_BD_RING_ALIGNMENT);
> -			if (!IS_ERR_VALUE(ugeth->rx_bd_ring_offset[j]))
> -				ugeth->p_rx_bd_ring[j] =
> -				    (u8 __iomem *) qe_muram_addr(ugeth->
> -							 rx_bd_ring_offset[j]);
> -		}
> +		ugeth->rx_bd_ring_offset[j] =
> +			(u32) kmalloc((u32) (length + align), GFP_KERNEL);

Same.

> +		if (ugeth->rx_bd_ring_offset[j] != 0)
> +			ugeth->p_rx_bd_ring[j] =
> +				(u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
> +						align) & ~(align - 1));

Same.

> +
>   		if (!ugeth->p_rx_bd_ring[j]) {
>   			if (netif_msg_ifup(ugeth))
>   				pr_err("Can not allocate memory for Rx bd rings\n");
> @@ -2554,20 +2519,11 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   		endOfRing =
>   		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
>   					      1) * sizeof(struct qe_bd);
> -		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> -				 last_bd_completed_address,
> -				 (u32) virt_to_phys(endOfRing));
> -		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			   MEM_PART_MURAM) {
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> -				 (u32)qe_muram_dma(ugeth->p_tx_bd_ring[i]));
> -			out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> -				 last_bd_completed_address,
> -				 (u32)qe_muram_dma(endOfRing));
> -		}
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
> +			 (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
> +		out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
> +			 last_bd_completed_address,

Same.


> +			 (u32) virt_to_phys(endOfRing));
>   	}
>   
>   	/* schedulerbasepointer */
> @@ -2786,14 +2742,8 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
>   	/* Setup the table */
>   	/* Assume BD rings are already established */
>   	for (i = 0; i < ug_info->numQueuesRx; i++) {
> -		if (ugeth->ug_info->uf_info.bd_mem_part == MEM_PART_SYSTEM) {
> -			out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
> -				 (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
> -		} else if (ugeth->ug_info->uf_info.bd_mem_part ==
> -			   MEM_PART_MURAM) {
> -			out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
> -				 (u32)qe_muram_dma(ugeth->p_rx_bd_ring[i]));
> -		}
> +		out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
> +			 (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
>   		/* rest of fields handled by QE */
>   	}
>   
> diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
> index 66f1afc393d1..4925a1b59dc9 100644
> --- a/include/soc/fsl/qe/qe.h
> +++ b/include/soc/fsl/qe/qe.h
> @@ -27,12 +27,6 @@
>   #define QE_NUM_OF_BRGS	16
>   #define QE_NUM_OF_PORTS	1024
>   
> -/* Memory partitions
> -*/
> -#define MEM_PART_SYSTEM		0
> -#define MEM_PART_SECONDARY	1
> -#define MEM_PART_MURAM		2
> -
>   /* Clocks and BRGs */
>   enum qe_clock {
>   	QE_CLK_NONE = 0,
> diff --git a/include/soc/fsl/qe/ucc_fast.h b/include/soc/fsl/qe/ucc_fast.h
> index dc4e79468094..9696a5b9b5d1 100644
> --- a/include/soc/fsl/qe/ucc_fast.h
> +++ b/include/soc/fsl/qe/ucc_fast.h
> @@ -146,7 +146,6 @@ struct ucc_fast_info {
>   	resource_size_t regs;
>   	int irq;
>   	u32 uccm_mask;
> -	int bd_mem_part;
>   	int brkpt_support;
>   	int grant_support;
>   	int tsa;
> 

Re: [PATCH net-next v2 13/17] ethernet: ucc_geth: remove bd_mem_part and all associated code

[Rasmus Villemoes]

On 20/01/2021 08.17, Christophe Leroy wrote:
> 
> Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
>> The bd_mem_part member of ucc_geth_info always has the value
>> MEM_PART_SYSTEM, and AFAICT, there has never been any code setting it
>> to any other value. Moreover, muram is a somewhat precious resource,
>> so there's no point using that when normal memory serves just as well.
>>
>> Apart from removing a lot of dead code, this is also motivated by
>> wanting to clean up the "store result from kmalloc() in a u32" mess.
>>
>> @@ -2195,25 +2179,15 @@ static int ucc_geth_alloc_tx(struct
>> ucc_geth_private *ugeth)
>>           if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
>>               UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
>>               length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
>> -        }
>> +
>> +        ugeth->tx_bd_ring_offset[j] =
>> +            (u32) kmalloc((u32) (length + align), GFP_KERNEL);
> 
> Can't this fit on a single ? Nowadays, max allowed line length is 100
> chars.
> 
>> +
>> +        if (ugeth->tx_bd_ring_offset[j] != 0)
>> +            ugeth->p_tx_bd_ring[j] =
>> +                (u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
>> +                        align) & ~(align - 1));
> 
> Can we get the above fit on only 2 lines ?
> 
>> +
>> -        }
>> +        ugeth->rx_bd_ring_offset[j] =
>> +            (u32) kmalloc((u32) (length + align), GFP_KERNEL);
> 
> Same.

This is all deliberate: Verifying that this patch merely removes the
dead branch (and thus outdenting the always-taken branch) is easily done
by using "git show -w". That shows hunks like

@@ -2554,20 +2519,11 @@ static int ucc_geth_startup(struct
ucc_geth_private *ugeth)
                endOfRing =
                    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
                                              1) * sizeof(struct qe_bd);
-               if (ugeth->ug_info->uf_info.bd_mem_part ==
MEM_PART_SYSTEM) {
                out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
                         (u32) virt_to_phys(ugeth->p_tx_bd_ring[i]));
                out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
                         last_bd_completed_address,
                         (u32) virt_to_phys(endOfRing));
-               } else if (ugeth->ug_info->uf_info.bd_mem_part ==
-                          MEM_PART_MURAM) {
-
out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].bd_ring_base,
-                                (u32)qe_muram_dma(ugeth->p_tx_bd_ring[i]));
-                       out_be32(&ugeth->p_send_q_mem_reg->sqqd[i].
-                                last_bd_completed_address,
-                                (u32)qe_muram_dma(endOfRing));
-               }
        }

So I didn't want to rewrap any of the lines.

Rasmus

[PATCH net-next v2 14/17] ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()

[Rasmus Villemoes]

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 1e9d2f3f47a3..621a9e3e4b65 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -2203,8 +2203,8 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 	for (j = 0; j < ug_info->numQueuesTx; j++) {
 		/* Setup the skbuff rings */
 		ugeth->tx_skbuff[j] =
-			kmalloc_array(ugeth->ug_info->bdRingLenTx[j],
-				      sizeof(struct sk_buff *), GFP_KERNEL);
+			kcalloc(ugeth->ug_info->bdRingLenTx[j],
+				sizeof(struct sk_buff *), GFP_KERNEL);
 
 		if (ugeth->tx_skbuff[j] == NULL) {
 			if (netif_msg_ifup(ugeth))
@@ -2212,9 +2212,6 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 			return -ENOMEM;
 		}
 
-		for (i = 0; i < ugeth->ug_info->bdRingLenTx[j]; i++)
-			ugeth->tx_skbuff[j][i] = NULL;
-
 		ugeth->skb_curtx[j] = ugeth->skb_dirtytx[j] = 0;
 		bd = ugeth->confBd[j] = ugeth->txBd[j] = ugeth->p_tx_bd_ring[j];
 		for (i = 0; i < ug_info->bdRingLenTx[j]; i++) {
@@ -2266,8 +2263,8 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 	for (j = 0; j < ug_info->numQueuesRx; j++) {
 		/* Setup the skbuff rings */
 		ugeth->rx_skbuff[j] =
-			kmalloc_array(ugeth->ug_info->bdRingLenRx[j],
-				      sizeof(struct sk_buff *), GFP_KERNEL);
+			kcalloc(ugeth->ug_info->bdRingLenRx[j],
+				sizeof(struct sk_buff *), GFP_KERNEL);
 
 		if (ugeth->rx_skbuff[j] == NULL) {
 			if (netif_msg_ifup(ugeth))
@@ -2275,9 +2272,6 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 			return -ENOMEM;
 		}
 
-		for (i = 0; i < ugeth->ug_info->bdRingLenRx[j]; i++)
-			ugeth->rx_skbuff[j][i] = NULL;
-
 		ugeth->skb_currx[j] = 0;
 		bd = ugeth->rxBd[j] = ugeth->p_rx_bd_ring[j];
 		for (i = 0; i < ug_info->bdRingLenRx[j]; i++) {
-- 
2.23.0

Re: [PATCH net-next v2 14/17] ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()

[Christophe Leroy]

Can you detail a bit the change ?

At least tell that the loop was a zeroising loop and that kcalloc() already zeroises the allocated 
memory ?

Le 19/01/2021 à 16:07, Rasmus Villemoes a écrit :
> Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> ---
>   drivers/net/ethernet/freescale/ucc_geth.c | 14 ++++----------
>   1 file changed, 4 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
> index 1e9d2f3f47a3..621a9e3e4b65 100644
> --- a/drivers/net/ethernet/freescale/ucc_geth.c
> +++ b/drivers/net/ethernet/freescale/ucc_geth.c
> @@ -2203,8 +2203,8 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
>   	for (j = 0; j < ug_info->numQueuesTx; j++) {
>   		/* Setup the skbuff rings */
>   		ugeth->tx_skbuff[j] =
> -			kmalloc_array(ugeth->ug_info->bdRingLenTx[j],
> -				      sizeof(struct sk_buff *), GFP_KERNEL);
> +			kcalloc(ugeth->ug_info->bdRingLenTx[j],
> +				sizeof(struct sk_buff *), GFP_KERNEL);
>   
>   		if (ugeth->tx_skbuff[j] == NULL) {
>   			if (netif_msg_ifup(ugeth))
> @@ -2212,9 +2212,6 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
>   			return -ENOMEM;
>   		}
>   
> -		for (i = 0; i < ugeth->ug_info->bdRingLenTx[j]; i++)
> -			ugeth->tx_skbuff[j][i] = NULL;
> -
>   		ugeth->skb_curtx[j] = ugeth->skb_dirtytx[j] = 0;
>   		bd = ugeth->confBd[j] = ugeth->txBd[j] = ugeth->p_tx_bd_ring[j];
>   		for (i = 0; i < ug_info->bdRingLenTx[j]; i++) {
> @@ -2266,8 +2263,8 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
>   	for (j = 0; j < ug_info->numQueuesRx; j++) {
>   		/* Setup the skbuff rings */
>   		ugeth->rx_skbuff[j] =
> -			kmalloc_array(ugeth->ug_info->bdRingLenRx[j],
> -				      sizeof(struct sk_buff *), GFP_KERNEL);
> +			kcalloc(ugeth->ug_info->bdRingLenRx[j],
> +				sizeof(struct sk_buff *), GFP_KERNEL);
>   
>   		if (ugeth->rx_skbuff[j] == NULL) {
>   			if (netif_msg_ifup(ugeth))
> @@ -2275,9 +2272,6 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
>   			return -ENOMEM;
>   		}
>   
> -		for (i = 0; i < ugeth->ug_info->bdRingLenRx[j]; i++)
> -			ugeth->rx_skbuff[j][i] = NULL;
> -
>   		ugeth->skb_currx[j] = 0;
>   		bd = ugeth->rxBd[j] = ugeth->p_rx_bd_ring[j];
>   		for (i = 0; i < ug_info->bdRingLenRx[j]; i++) {
> 

[PATCH net-next v2 15/17] ethernet: ucc_geth: add helper to replace repeated switch statements

[Rasmus Villemoes]

The translation from the ucc_geth_num_of_threads enum value to the
actual count can be written somewhat more compactly with a small
lookup table, allowing us to replace the four switch statements.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 100 +++++-----------------
 1 file changed, 22 insertions(+), 78 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 621a9e3e4b65..960b19fc4fb8 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -70,6 +70,20 @@ static struct {
 module_param_named(debug, debug.msg_enable, int, 0);
 MODULE_PARM_DESC(debug, "Debug verbosity level (0=none, ..., 0xffff=all)");
 
+static int ucc_geth_thread_count(enum ucc_geth_num_of_threads idx)
+{
+	static const u8 count[] = {
+		[UCC_GETH_NUM_OF_THREADS_1] = 1,
+		[UCC_GETH_NUM_OF_THREADS_2] = 2,
+		[UCC_GETH_NUM_OF_THREADS_4] = 4,
+		[UCC_GETH_NUM_OF_THREADS_6] = 6,
+		[UCC_GETH_NUM_OF_THREADS_8] = 8,
+	};
+	if (idx >= ARRAY_SIZE(count))
+		return 0;
+	return count[idx];
+}
+
 static const struct ucc_geth_info ugeth_primary_info = {
 	.uf_info = {
 		    .rtsm = UCC_FAST_SEND_IDLES_BETWEEN_FRAMES,
@@ -668,32 +682,12 @@ static void dump_regs(struct ucc_geth_private *ugeth)
 		in_be32(&ugeth->ug_regs->scam));
 
 	if (ugeth->p_thread_data_tx) {
-		int numThreadsTxNumerical;
-		switch (ugeth->ug_info->numThreadsTx) {
-		case UCC_GETH_NUM_OF_THREADS_1:
-			numThreadsTxNumerical = 1;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_2:
-			numThreadsTxNumerical = 2;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_4:
-			numThreadsTxNumerical = 4;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_6:
-			numThreadsTxNumerical = 6;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_8:
-			numThreadsTxNumerical = 8;
-			break;
-		default:
-			numThreadsTxNumerical = 0;
-			break;
-		}
+		int count = ucc_geth_thread_count(ugeth->ug_info->numThreadsTx);
 
 		pr_info("Thread data TXs:\n");
 		pr_info("Base address: 0x%08x\n",
 			(u32)ugeth->p_thread_data_tx);
-		for (i = 0; i < numThreadsTxNumerical; i++) {
+		for (i = 0; i < count; i++) {
 			pr_info("Thread data TX[%d]:\n", i);
 			pr_info("Base address: 0x%08x\n",
 				(u32)&ugeth->p_thread_data_tx[i]);
@@ -702,32 +696,12 @@ static void dump_regs(struct ucc_geth_private *ugeth)
 		}
 	}
 	if (ugeth->p_thread_data_rx) {
-		int numThreadsRxNumerical;
-		switch (ugeth->ug_info->numThreadsRx) {
-		case UCC_GETH_NUM_OF_THREADS_1:
-			numThreadsRxNumerical = 1;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_2:
-			numThreadsRxNumerical = 2;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_4:
-			numThreadsRxNumerical = 4;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_6:
-			numThreadsRxNumerical = 6;
-			break;
-		case UCC_GETH_NUM_OF_THREADS_8:
-			numThreadsRxNumerical = 8;
-			break;
-		default:
-			numThreadsRxNumerical = 0;
-			break;
-		}
+		int count = ucc_geth_thread_count(ugeth->ug_info->numThreadsRx);
 
 		pr_info("Thread data RX:\n");
 		pr_info("Base address: 0x%08x\n",
 			(u32)ugeth->p_thread_data_rx);
-		for (i = 0; i < numThreadsRxNumerical; i++) {
+		for (i = 0; i < count; i++) {
 			pr_info("Thread data RX[%d]:\n", i);
 			pr_info("Base address: 0x%08x\n",
 				(u32)&ugeth->p_thread_data_rx[i]);
@@ -2315,45 +2289,15 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	uf_regs = uccf->uf_regs;
 	ug_regs = ugeth->ug_regs;
 
-	switch (ug_info->numThreadsRx) {
-	case UCC_GETH_NUM_OF_THREADS_1:
-		numThreadsRxNumerical = 1;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_2:
-		numThreadsRxNumerical = 2;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_4:
-		numThreadsRxNumerical = 4;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_6:
-		numThreadsRxNumerical = 6;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_8:
-		numThreadsRxNumerical = 8;
-		break;
-	default:
+	numThreadsRxNumerical = ucc_geth_thread_count(ug_info->numThreadsRx);
+	if (!numThreadsRxNumerical) {
 		if (netif_msg_ifup(ugeth))
 			pr_err("Bad number of Rx threads value\n");
 		return -EINVAL;
 	}
 
-	switch (ug_info->numThreadsTx) {
-	case UCC_GETH_NUM_OF_THREADS_1:
-		numThreadsTxNumerical = 1;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_2:
-		numThreadsTxNumerical = 2;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_4:
-		numThreadsTxNumerical = 4;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_6:
-		numThreadsTxNumerical = 6;
-		break;
-	case UCC_GETH_NUM_OF_THREADS_8:
-		numThreadsTxNumerical = 8;
-		break;
-	default:
+	numThreadsTxNumerical = ucc_geth_thread_count(ug_info->numThreadsTx);
+	if (!numThreadsTxNumerical) {
 		if (netif_msg_ifup(ugeth))
 			pr_err("Bad number of Tx threads value\n");
 		return -EINVAL;
-- 
2.23.0

[PATCH net-next v2 16/17] ethernet: ucc_geth: inform the compiler that numQueues is always 1

[Rasmus Villemoes]

The numQueuesTx and numQueuesRx members of struct ucc_geth_info are
never set to anything but 1, and never have been. It's unclear how
well the code supporting multiple queues would work. Until somebody
wants to play with enabling that, help the compiler eliminate a lot of
dead code and loops that are not really loops by creating static
inline helpers. If and when the numQueuesTx/numQueuesRx fields are
re-introduced, it suffices to update those helper to return the
appropriate field.

This cuts the .text segment of ucc_geth.o by 8%.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 76 +++++++++++++----------
 drivers/net/ethernet/freescale/ucc_geth.h |  2 -
 2 files changed, 42 insertions(+), 36 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 960b19fc4fb8..9be1d4455a6b 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -84,6 +84,16 @@ static int ucc_geth_thread_count(enum ucc_geth_num_of_threads idx)
 	return count[idx];
 }
 
+static inline int ucc_geth_tx_queues(const struct ucc_geth_info *info)
+{
+	return 1;
+}
+
+static inline int ucc_geth_rx_queues(const struct ucc_geth_info *info)
+{
+	return 1;
+}
+
 static const struct ucc_geth_info ugeth_primary_info = {
 	.uf_info = {
 		    .rtsm = UCC_FAST_SEND_IDLES_BETWEEN_FRAMES,
@@ -103,8 +113,6 @@ static const struct ucc_geth_info ugeth_primary_info = {
 		    .tcrc = UCC_FAST_16_BIT_CRC,
 		    .synl = UCC_FAST_SYNC_LEN_NOT_USED,
 		    },
-	.numQueuesTx = 1,
-	.numQueuesRx = 1,
 	.extendedFilteringChainPointer = ((uint32_t) NULL),
 	.typeorlen = 3072 /*1536 */ ,
 	.nonBackToBackIfgPart1 = 0x40,
@@ -569,7 +577,7 @@ static void dump_bds(struct ucc_geth_private *ugeth)
 	int i;
 	int length;
 
-	for (i = 0; i < ugeth->ug_info->numQueuesTx; i++) {
+	for (i = 0; i < ucc_geth_tx_queues(ugeth->ug_info); i++) {
 		if (ugeth->p_tx_bd_ring[i]) {
 			length =
 			    (ugeth->ug_info->bdRingLenTx[i] *
@@ -578,7 +586,7 @@ static void dump_bds(struct ucc_geth_private *ugeth)
 			mem_disp(ugeth->p_tx_bd_ring[i], length);
 		}
 	}
-	for (i = 0; i < ugeth->ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ugeth->ug_info); i++) {
 		if (ugeth->p_rx_bd_ring[i]) {
 			length =
 			    (ugeth->ug_info->bdRingLenRx[i] *
@@ -876,7 +884,7 @@ static void dump_regs(struct ucc_geth_private *ugeth)
 	if (ugeth->p_send_q_mem_reg) {
 		pr_info("Send Q memory registers:\n");
 		pr_info("Base address: 0x%08x\n", (u32)ugeth->p_send_q_mem_reg);
-		for (i = 0; i < ugeth->ug_info->numQueuesTx; i++) {
+		for (i = 0; i < ucc_geth_tx_queues(ugeth->ug_info); i++) {
 			pr_info("SQQD[%d]:\n", i);
 			pr_info("Base address: 0x%08x\n",
 				(u32)&ugeth->p_send_q_mem_reg->sqqd[i]);
@@ -908,7 +916,7 @@ static void dump_regs(struct ucc_geth_private *ugeth)
 		pr_info("RX IRQ coalescing tables:\n");
 		pr_info("Base address: 0x%08x\n",
 			(u32)ugeth->p_rx_irq_coalescing_tbl);
-		for (i = 0; i < ugeth->ug_info->numQueuesRx; i++) {
+		for (i = 0; i < ucc_geth_rx_queues(ugeth->ug_info); i++) {
 			pr_info("RX IRQ coalescing table entry[%d]:\n", i);
 			pr_info("Base address: 0x%08x\n",
 				(u32)&ugeth->p_rx_irq_coalescing_tbl->
@@ -930,7 +938,7 @@ static void dump_regs(struct ucc_geth_private *ugeth)
 	if (ugeth->p_rx_bd_qs_tbl) {
 		pr_info("RX BD QS tables:\n");
 		pr_info("Base address: 0x%08x\n", (u32)ugeth->p_rx_bd_qs_tbl);
-		for (i = 0; i < ugeth->ug_info->numQueuesRx; i++) {
+		for (i = 0; i < ucc_geth_rx_queues(ugeth->ug_info); i++) {
 			pr_info("RX BD QS table[%d]:\n", i);
 			pr_info("Base address: 0x%08x\n",
 				(u32)&ugeth->p_rx_bd_qs_tbl[i]);
@@ -1806,7 +1814,7 @@ static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
 	ug_info = ugeth->ug_info;
 	uf_info = &ug_info->uf_info;
 
-	for (i = 0; i < ugeth->ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ugeth->ug_info); i++) {
 		if (ugeth->p_rx_bd_ring[i]) {
 			/* Return existing data buffers in ring */
 			bd = ugeth->p_rx_bd_ring[i];
@@ -1846,7 +1854,7 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
 	ug_info = ugeth->ug_info;
 	uf_info = &ug_info->uf_info;
 
-	for (i = 0; i < ugeth->ug_info->numQueuesTx; i++) {
+	for (i = 0; i < ucc_geth_tx_queues(ugeth->ug_info); i++) {
 		bd = ugeth->p_tx_bd_ring[i];
 		if (!bd)
 			continue;
@@ -2024,7 +2032,7 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 	uf_info = &ug_info->uf_info;
 
 	/* Rx BD lengths */
-	for (i = 0; i < ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++) {
 		if ((ug_info->bdRingLenRx[i] < UCC_GETH_RX_BD_RING_SIZE_MIN) ||
 		    (ug_info->bdRingLenRx[i] %
 		     UCC_GETH_RX_BD_RING_SIZE_ALIGNMENT)) {
@@ -2035,7 +2043,7 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 	}
 
 	/* Tx BD lengths */
-	for (i = 0; i < ug_info->numQueuesTx; i++) {
+	for (i = 0; i < ucc_geth_tx_queues(ug_info); i++) {
 		if (ug_info->bdRingLenTx[i] < UCC_GETH_TX_BD_RING_SIZE_MIN) {
 			if (netif_msg_probe(ugeth))
 				pr_err("Tx BD ring length must be no smaller than 2\n");
@@ -2052,14 +2060,14 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 	}
 
 	/* num Tx queues */
-	if (ug_info->numQueuesTx > NUM_TX_QUEUES) {
+	if (ucc_geth_tx_queues(ug_info) > NUM_TX_QUEUES) {
 		if (netif_msg_probe(ugeth))
 			pr_err("number of tx queues too large\n");
 		return -EINVAL;
 	}
 
 	/* num Rx queues */
-	if (ug_info->numQueuesRx > NUM_RX_QUEUES) {
+	if (ucc_geth_rx_queues(ug_info) > NUM_RX_QUEUES) {
 		if (netif_msg_probe(ugeth))
 			pr_err("number of rx queues too large\n");
 		return -EINVAL;
@@ -2067,7 +2075,7 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 
 	/* l2qt */
 	for (i = 0; i < UCC_GETH_VLAN_PRIORITY_MAX; i++) {
-		if (ug_info->l2qt[i] >= ug_info->numQueuesRx) {
+		if (ug_info->l2qt[i] >= ucc_geth_rx_queues(ug_info)) {
 			if (netif_msg_probe(ugeth))
 				pr_err("VLAN priority table entry must not be larger than number of Rx queues\n");
 			return -EINVAL;
@@ -2076,7 +2084,7 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 
 	/* l3qt */
 	for (i = 0; i < UCC_GETH_IP_PRIORITY_MAX; i++) {
-		if (ug_info->l3qt[i] >= ug_info->numQueuesRx) {
+		if (ug_info->l3qt[i] >= ucc_geth_rx_queues(ug_info)) {
 			if (netif_msg_probe(ugeth))
 				pr_err("IP priority table entry must not be larger than number of Rx queues\n");
 			return -EINVAL;
@@ -2099,10 +2107,10 @@ static int ucc_struct_init(struct ucc_geth_private *ugeth)
 
 	/* Generate uccm_mask for receive */
 	uf_info->uccm_mask = ug_info->eventRegMask & UCCE_OTHER;/* Errors */
-	for (i = 0; i < ug_info->numQueuesRx; i++)
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++)
 		uf_info->uccm_mask |= (UCC_GETH_UCCE_RXF0 << i);
 
-	for (i = 0; i < ug_info->numQueuesTx; i++)
+	for (i = 0; i < ucc_geth_tx_queues(ug_info); i++)
 		uf_info->uccm_mask |= (UCC_GETH_UCCE_TXB0 << i);
 	/* Initialize the general fast UCC block. */
 	if (ucc_fast_init(uf_info, &ugeth->uccf)) {
@@ -2141,7 +2149,7 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 	uf_info = &ug_info->uf_info;
 
 	/* Allocate Tx bds */
-	for (j = 0; j < ug_info->numQueuesTx; j++) {
+	for (j = 0; j < ucc_geth_tx_queues(ug_info); j++) {
 		u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
 
 		/* Allocate in multiple of
@@ -2174,7 +2182,7 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 	}
 
 	/* Init Tx bds */
-	for (j = 0; j < ug_info->numQueuesTx; j++) {
+	for (j = 0; j < ucc_geth_tx_queues(ug_info); j++) {
 		/* Setup the skbuff rings */
 		ugeth->tx_skbuff[j] =
 			kcalloc(ugeth->ug_info->bdRingLenTx[j],
@@ -2215,7 +2223,7 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 	uf_info = &ug_info->uf_info;
 
 	/* Allocate Rx bds */
-	for (j = 0; j < ug_info->numQueuesRx; j++) {
+	for (j = 0; j < ucc_geth_rx_queues(ug_info); j++) {
 		u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
 
 		length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
@@ -2234,7 +2242,7 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 	}
 
 	/* Init Rx bds */
-	for (j = 0; j < ug_info->numQueuesRx; j++) {
+	for (j = 0; j < ucc_geth_rx_queues(ug_info); j++) {
 		/* Setup the skbuff rings */
 		ugeth->rx_skbuff[j] =
 			kcalloc(ugeth->ug_info->bdRingLenRx[j],
@@ -2437,7 +2445,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	/* SQPTR */
 	/* Size varies with number of Tx queues */
 	ugeth->send_q_mem_reg_offset =
-	    qe_muram_alloc(ug_info->numQueuesTx *
+	    qe_muram_alloc(ucc_geth_tx_queues(ug_info) *
 			   sizeof(struct ucc_geth_send_queue_qd),
 			   UCC_GETH_SEND_QUEUE_QUEUE_DESCRIPTOR_ALIGNMENT);
 	if (IS_ERR_VALUE(ugeth->send_q_mem_reg_offset)) {
@@ -2453,7 +2461,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 
 	/* Setup the table */
 	/* Assume BD rings are already established */
-	for (i = 0; i < ug_info->numQueuesTx; i++) {
+	for (i = 0; i < ucc_geth_tx_queues(ug_info); i++) {
 		endOfRing =
 		    ugeth->p_tx_bd_ring[i] + (ug_info->bdRingLenTx[i] -
 					      1) * sizeof(struct qe_bd);
@@ -2466,7 +2474,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 
 	/* schedulerbasepointer */
 
-	if (ug_info->numQueuesTx > 1) {
+	if (ucc_geth_tx_queues(ug_info) > 1) {
 	/* scheduler exists only if more than 1 tx queue */
 		ugeth->scheduler_offset =
 		    qe_muram_alloc(sizeof(struct ucc_geth_scheduler),
@@ -2529,11 +2537,11 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	/* temoder */
 	/* Already has speed set */
 
-	if (ug_info->numQueuesTx > 1)
+	if (ucc_geth_tx_queues(ug_info) > 1)
 		temoder |= TEMODER_SCHEDULER_ENABLE;
 	if (ug_info->ipCheckSumGenerate)
 		temoder |= TEMODER_IP_CHECKSUM_GENERATE;
-	temoder |= ((ug_info->numQueuesTx - 1) << TEMODER_NUM_OF_QUEUES_SHIFT);
+	temoder |= ((ucc_geth_tx_queues(ug_info) - 1) << TEMODER_NUM_OF_QUEUES_SHIFT);
 	out_be16(&ugeth->p_tx_glbl_pram->temoder, temoder);
 
 	/* Function code register value to be used later */
@@ -2597,7 +2605,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 
 	/* Size varies with number of Rx queues */
 	ugeth->rx_irq_coalescing_tbl_offset =
-	    qe_muram_alloc(ug_info->numQueuesRx *
+	    qe_muram_alloc(ucc_geth_rx_queues(ug_info) *
 			   sizeof(struct ucc_geth_rx_interrupt_coalescing_entry)
 			   + 4, UCC_GETH_RX_INTERRUPT_COALESCING_ALIGNMENT);
 	if (IS_ERR_VALUE(ugeth->rx_irq_coalescing_tbl_offset)) {
@@ -2613,7 +2621,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 		 ugeth->rx_irq_coalescing_tbl_offset);
 
 	/* Fill interrupt coalescing table */
-	for (i = 0; i < ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++) {
 		out_be32(&ugeth->p_rx_irq_coalescing_tbl->coalescingentry[i].
 			 interruptcoalescingmaxvalue,
 			 ug_info->interruptcoalescingmaxvalue[i]);
@@ -2662,7 +2670,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	/* RBDQPTR */
 	/* Size varies with number of Rx queues */
 	ugeth->rx_bd_qs_tbl_offset =
-	    qe_muram_alloc(ug_info->numQueuesRx *
+	    qe_muram_alloc(ucc_geth_rx_queues(ug_info) *
 			   (sizeof(struct ucc_geth_rx_bd_queues_entry) +
 			    sizeof(struct ucc_geth_rx_prefetched_bds)),
 			   UCC_GETH_RX_BD_QUEUES_ALIGNMENT);
@@ -2679,7 +2687,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 
 	/* Setup the table */
 	/* Assume BD rings are already established */
-	for (i = 0; i < ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++) {
 		out_be32(&ugeth->p_rx_bd_qs_tbl[i].externalbdbaseptr,
 			 (u32) virt_to_phys(ugeth->p_rx_bd_ring[i]));
 		/* rest of fields handled by QE */
@@ -2702,7 +2710,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	    ug_info->
 	    vlanOperationNonTagged << REMODER_VLAN_OPERATION_NON_TAGGED_SHIFT;
 	remoder |= ug_info->rxQoSMode << REMODER_RX_QOS_MODE_SHIFT;
-	remoder |= ((ug_info->numQueuesRx - 1) << REMODER_NUM_OF_QUEUES_SHIFT);
+	remoder |= ((ucc_geth_rx_queues(ug_info) - 1) << REMODER_NUM_OF_QUEUES_SHIFT);
 	if (ug_info->ipCheckSumCheck)
 		remoder |= REMODER_IP_CHECKSUM_CHECK;
 	if (ug_info->ipAddressAlignment)
@@ -2861,7 +2869,7 @@ static int ucc_geth_startup(struct ucc_geth_private *ugeth)
 	}
 
 	/* Load Rx bds with buffers */
-	for (i = 0; i < ug_info->numQueuesRx; i++) {
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++) {
 		if ((ret_val = rx_bd_buffer_set(ugeth, (u8) i)) != 0) {
 			if (netif_msg_ifup(ugeth))
 				pr_err("Can not fill Rx bds with buffers\n");
@@ -3132,12 +3140,12 @@ static int ucc_geth_poll(struct napi_struct *napi, int budget)
 
 	/* Tx event processing */
 	spin_lock(&ugeth->lock);
-	for (i = 0; i < ug_info->numQueuesTx; i++)
+	for (i = 0; i < ucc_geth_tx_queues(ug_info); i++)
 		ucc_geth_tx(ugeth->ndev, i);
 	spin_unlock(&ugeth->lock);
 
 	howmany = 0;
-	for (i = 0; i < ug_info->numQueuesRx; i++)
+	for (i = 0; i < ucc_geth_rx_queues(ug_info); i++)
 		howmany += ucc_geth_rx(ugeth, i, budget - howmany);
 
 	if (howmany < budget) {
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
index be47fa8ced15..6539fed9cc22 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -1077,8 +1077,6 @@ struct ucc_geth_tad_params {
 /* GETH protocol initialization structure */
 struct ucc_geth_info {
 	struct ucc_fast_info uf_info;
-	u8 numQueuesTx;
-	u8 numQueuesRx;
 	int ipCheckSumCheck;
 	int ipCheckSumGenerate;
 	int rxExtendedFiltering;
-- 
2.23.0

[PATCH net-next v2 17/17] ethernet: ucc_geth: simplify rx/tx allocations

[Rasmus Villemoes]

Since kmalloc() is nowadays [1] guaranteed to return naturally
aligned (i.e., aligned to the size itself) memory for power-of-2
sizes, we don't need to over-allocate the align amount, compute an
aligned address within the allocation, and (for later freeing) also
storing the original pointer [2].

Instead, just round up the length we want to allocate to the alignment
requirements, then round that up to the next power of 2. In theory,
this could allocate up to about twice as much memory as we needed.  In
practice, (a) kmalloc() would in most cases anyway return a
power-of-2-sized allocation and (b) with the default values of the
bdRingLen[RT]x fields, the length is already itself a power of 2
greater than the alignment.

So we actually end up saving memory compared to the current
situtation (e.g. for tx, we currently allocate 128+32 bytes, which
kmalloc() likely rounds up to 192 or 256; with this patch, we just
allocate 128 bytes.) Also struct ucc_geth_private becomes a little
smaller.

[1] 59bb47985c1d ("mm, sl[aou]b: guarantee natural alignment for
kmalloc(power-of-two)")

[2] That storing was anyway done in a u32, which works on 32 bit
machines, but is not very elegant and certainly makes a reader of the
code pause for a while.

Signed-off-by: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
---
 drivers/net/ethernet/freescale/ucc_geth.c | 50 ++++++++---------------
 drivers/net/ethernet/freescale/ucc_geth.h |  2 -
 2 files changed, 17 insertions(+), 35 deletions(-)

diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index 9be1d4455a6b..ef4e2febeb5b 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -1835,7 +1835,7 @@ static void ucc_geth_free_rx(struct ucc_geth_private *ugeth)
 
 			kfree(ugeth->rx_skbuff[i]);
 
-			kfree((void *)ugeth->rx_bd_ring_offset[i]);
+			kfree(ugeth->p_rx_bd_ring[i]);
 			ugeth->p_rx_bd_ring[i] = NULL;
 		}
 	}
@@ -1872,10 +1872,8 @@ static void ucc_geth_free_tx(struct ucc_geth_private *ugeth)
 
 		kfree(ugeth->tx_skbuff[i]);
 
-		if (ugeth->p_tx_bd_ring[i]) {
-			kfree((void *)ugeth->tx_bd_ring_offset[i]);
-			ugeth->p_tx_bd_ring[i] = NULL;
-		}
+		kfree(ugeth->p_tx_bd_ring[i]);
+		ugeth->p_tx_bd_ring[i] = NULL;
 	}
 
 }
@@ -2150,25 +2148,15 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 
 	/* Allocate Tx bds */
 	for (j = 0; j < ucc_geth_tx_queues(ug_info); j++) {
-		u32 align = UCC_GETH_TX_BD_RING_ALIGNMENT;
-
-		/* Allocate in multiple of
-		   UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT,
-		   according to spec */
-		length = ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd))
-			  / UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
-		    * UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
-		if ((ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)) %
-		    UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT)
-			length += UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT;
-
-		ugeth->tx_bd_ring_offset[j] =
-			(u32) kmalloc((u32) (length + align), GFP_KERNEL);
-
-		if (ugeth->tx_bd_ring_offset[j] != 0)
-			ugeth->p_tx_bd_ring[j] =
-				(u8 __iomem *)((ugeth->tx_bd_ring_offset[j] +
-						align) & ~(align - 1));
+		u32 align = max(UCC_GETH_TX_BD_RING_ALIGNMENT,
+				UCC_GETH_TX_BD_RING_SIZE_MEMORY_ALIGNMENT);
+		u32 alloc;
+
+		length = ug_info->bdRingLenTx[j] * sizeof(struct qe_bd);
+		alloc = round_up(length, align);
+		alloc = roundup_pow_of_two(alloc);
+
+		ugeth->p_tx_bd_ring[j] = kmalloc(alloc, GFP_KERNEL);
 
 		if (!ugeth->p_tx_bd_ring[j]) {
 			if (netif_msg_ifup(ugeth))
@@ -2176,9 +2164,7 @@ static int ucc_geth_alloc_tx(struct ucc_geth_private *ugeth)
 			return -ENOMEM;
 		}
 		/* Zero unused end of bd ring, according to spec */
-		memset_io((void __iomem *)(ugeth->p_tx_bd_ring[j] +
-		       ug_info->bdRingLenTx[j] * sizeof(struct qe_bd)), 0,
-		       length - ug_info->bdRingLenTx[j] * sizeof(struct qe_bd));
+		memset(ugeth->p_tx_bd_ring[j] + length, 0, alloc - length);
 	}
 
 	/* Init Tx bds */
@@ -2225,15 +2211,13 @@ static int ucc_geth_alloc_rx(struct ucc_geth_private *ugeth)
 	/* Allocate Rx bds */
 	for (j = 0; j < ucc_geth_rx_queues(ug_info); j++) {
 		u32 align = UCC_GETH_RX_BD_RING_ALIGNMENT;
+		u32 alloc;
 
 		length = ug_info->bdRingLenRx[j] * sizeof(struct qe_bd);
-		ugeth->rx_bd_ring_offset[j] =
-			(u32) kmalloc((u32) (length + align), GFP_KERNEL);
-		if (ugeth->rx_bd_ring_offset[j] != 0)
-			ugeth->p_rx_bd_ring[j] =
-				(u8 __iomem *)((ugeth->rx_bd_ring_offset[j] +
-						align) & ~(align - 1));
+		alloc = round_up(length, align);
+		alloc = roundup_pow_of_two(alloc);
 
+		ugeth->p_rx_bd_ring[j] = kmalloc(alloc, GFP_KERNEL);
 		if (!ugeth->p_rx_bd_ring[j]) {
 			if (netif_msg_ifup(ugeth))
 				pr_err("Can not allocate memory for Rx bd rings\n");
diff --git a/drivers/net/ethernet/freescale/ucc_geth.h b/drivers/net/ethernet/freescale/ucc_geth.h
index 6539fed9cc22..ccc4ca1ae9b6 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.h
+++ b/drivers/net/ethernet/freescale/ucc_geth.h
@@ -1182,9 +1182,7 @@ struct ucc_geth_private {
 	struct ucc_geth_rx_bd_queues_entry __iomem *p_rx_bd_qs_tbl;
 	u32 rx_bd_qs_tbl_offset;
 	u8 __iomem *p_tx_bd_ring[NUM_TX_QUEUES];
-	u32 tx_bd_ring_offset[NUM_TX_QUEUES];
 	u8 __iomem *p_rx_bd_ring[NUM_RX_QUEUES];
-	u32 rx_bd_ring_offset[NUM_RX_QUEUES];
 	u8 __iomem *confBd[NUM_TX_QUEUES];
 	u8 __iomem *txBd[NUM_TX_QUEUES];
 	u8 __iomem *rxBd[NUM_RX_QUEUES];
-- 
2.23.0

RE: [PATCH net-next v2 00/17] ucc_geth improvements

[Leo Li]

> -----Original Message-----
> From: Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Sent: Tuesday, January 19, 2021 9:08 AM
> To: netdev@vger.kernel.org
> Cc: Leo Li <leoyang.li@nxp.com>; David S . Miller <davem@davemloft.net>;
> Qiang Zhao <qiang.zhao@nxp.com>; Andrew Lunn <andrew@lunn.ch>;
> Christophe Leroy <christophe.leroy@csgroup.eu>; Jakub Kicinski
> <kuba@kernel.org>; jocke@infinera.com <joakim.tjernlund@infinera.com>;
> Rasmus Villemoes <rasmus.villemoes@prevas.dk>
> Subject: [PATCH net-next v2 00/17] ucc_geth improvements
> 
> This is a resend of some improvements to the ucc_geth driver that was
> previously sent together with bug fixes, which have by now been applied.
> 
> Li Yang, if you don't speak up, I'm going to assume you're fine with
> 2,3,4 being taken through the net tree?

I'm fine with them going through the net tree.

> 
> v2: rebase to net/master; address minor style issues; don't introduce a use-
> after-free in patch "don't statically allocate eight ucc_geth_info".
> 
> Rasmus Villemoes (17):
>   ethernet: ucc_geth: remove unused read of temoder field
>   soc: fsl: qe: make cpm_muram_offset take a const void* argument
>   soc: fsl: qe: store muram_vbase as a void pointer instead of u8
>   soc: fsl: qe: add cpm_muram_free_addr() helper
>   ethernet: ucc_geth: use qe_muram_free_addr()
>   ethernet: ucc_geth: remove unnecessary memset_io() calls
>   ethernet: ucc_geth: replace kmalloc+memset by kzalloc
>   ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct
>     ucc_geth_private
>   ethernet: ucc_geth: factor out parsing of {rx,tx}-clock{,-name}
>     properties
>   ethernet: ucc_geth: constify ugeth_primary_info
>   ethernet: ucc_geth: don't statically allocate eight ucc_geth_info
>   ethernet: ucc_geth: use UCC_GETH_{RX,TX}_BD_RING_ALIGNMENT
> macros
>     directly
>   ethernet: ucc_geth: remove bd_mem_part and all associated code
>   ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()
>   ethernet: ucc_geth: add helper to replace repeated switch statements
>   ethernet: ucc_geth: inform the compiler that numQueues is always 1
>   ethernet: ucc_geth: simplify rx/tx allocations
> 
>  drivers/net/ethernet/freescale/ucc_geth.c | 549 ++++++++--------------
>  drivers/net/ethernet/freescale/ucc_geth.h |   6 -
>  drivers/soc/fsl/qe/qe_common.c            |  20 +-
>  include/soc/fsl/qe/qe.h                   |  15 +-
>  include/soc/fsl/qe/ucc_fast.h             |   1 -
>  5 files changed, 209 insertions(+), 382 deletions(-)
> 
> --
> 2.23.0

Re: [PATCH net-next v2 00/17] ucc_geth improvements

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Tue, 19 Jan 2021 16:07:45 +0100 you wrote:
> This is a resend of some improvements to the ucc_geth driver that was
> previously sent together with bug fixes, which have by now been
> applied.
> 
> Li Yang, if you don't speak up, I'm going to assume you're fine with
> 2,3,4 being taken through the net tree?
> 
> [...]

Here is the summary with links:
  - [net-next,v2,01/17] ethernet: ucc_geth: remove unused read of temoder field
    https://git.kernel.org/netdev/net-next/c/0a950ce029c8
  - [net-next,v2,02/17] soc: fsl: qe: make cpm_muram_offset take a const void* argument
    https://git.kernel.org/netdev/net-next/c/e8e507a8ac90
  - [net-next,v2,03/17] soc: fsl: qe: store muram_vbase as a void pointer instead of u8
    https://git.kernel.org/netdev/net-next/c/155ea0dc8dcb
  - [net-next,v2,04/17] soc: fsl: qe: add cpm_muram_free_addr() helper
    https://git.kernel.org/netdev/net-next/c/186b8daffb4e
  - [net-next,v2,05/17] ethernet: ucc_geth: use qe_muram_free_addr()
    https://git.kernel.org/netdev/net-next/c/03588e92c07f
  - [net-next,v2,06/17] ethernet: ucc_geth: remove unnecessary memset_io() calls
    https://git.kernel.org/netdev/net-next/c/0a71c415297f
  - [net-next,v2,07/17] ethernet: ucc_geth: replace kmalloc+memset by kzalloc
    https://git.kernel.org/netdev/net-next/c/830c8ddc66df
  - [net-next,v2,08/17] ethernet: ucc_geth: remove {rx,tx}_glbl_pram_offset from struct ucc_geth_private
    https://git.kernel.org/netdev/net-next/c/7d9fe90036f7
  - [net-next,v2,09/17] ethernet: ucc_geth: factor out parsing of {rx,tx}-clock{,-name} properties
    https://git.kernel.org/netdev/net-next/c/632e3f2d9922
  - [net-next,v2,10/17] ethernet: ucc_geth: constify ugeth_primary_info
    https://git.kernel.org/netdev/net-next/c/b0292e086bee
  - [net-next,v2,11/17] ethernet: ucc_geth: don't statically allocate eight ucc_geth_info
    https://git.kernel.org/netdev/net-next/c/baff4311c40d
  - [net-next,v2,12/17] ethernet: ucc_geth: use UCC_GETH_{RX,TX}_BD_RING_ALIGNMENT macros directly
    https://git.kernel.org/netdev/net-next/c/b29fafd3570b
  - [net-next,v2,13/17] ethernet: ucc_geth: remove bd_mem_part and all associated code
    https://git.kernel.org/netdev/net-next/c/64a99fe596f9
  - [net-next,v2,14/17] ethernet: ucc_geth: replace kmalloc_array()+for loop by kcalloc()
    https://git.kernel.org/netdev/net-next/c/33deb13c87e5
  - [net-next,v2,15/17] ethernet: ucc_geth: add helper to replace repeated switch statements
    https://git.kernel.org/netdev/net-next/c/634b5bd73187
  - [net-next,v2,16/17] ethernet: ucc_geth: inform the compiler that numQueues is always 1
    https://git.kernel.org/netdev/net-next/c/53f49d86ea21
  - [net-next,v2,17/17] ethernet: ucc_geth: simplify rx/tx allocations
    https://git.kernel.org/netdev/net-next/c/9b0dfef47553

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html