Thread (39 messages) 39 messages, 7 authors, 2019-06-04

Re: [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into qe_snums_init

From: Christophe Leroy <hidden>
Date: 2019-04-30 17:27:18
Also in: linux-arm-kernel, lkml


Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit :
quoted hunk ↗ jump to hunk
The comment "No QE ever has fewer than 28 SNUMs" is false; e.g. the
MPC8309 has 14. The code path returning -EINVAL is also a recipe for
instant disaster, since the caller (qe_snums_init) uncritically
assigns the return value to the unsigned qe_num_of_snum, and would
thus proceed to attempt to copy 4GB from snum_init_46[] to the snum[]
array.

So fold the handling of the legacy fsl,qe-num-snums into
qe_snums_init, and make sure we do not end up using the snum_init_46
array in cases other than the two where we know it makes sense.

Signed-off-by: Rasmus Villemoes <redacted>
---
  drivers/net/ethernet/freescale/ucc_geth.c |  2 +-
  drivers/soc/fsl/qe/qe.c                   | 54 +++++++----------------
  include/soc/fsl/qe/qe.h                   |  2 +-
  3 files changed, 19 insertions(+), 39 deletions(-)
diff --git a/drivers/net/ethernet/freescale/ucc_geth.c b/drivers/net/ethernet/freescale/ucc_geth.c
index eb3e65e8868f..5748eb8464d0 100644
--- a/drivers/net/ethernet/freescale/ucc_geth.c
+++ b/drivers/net/ethernet/freescale/ucc_geth.c
@@ -3837,7 +3837,7 @@ static int ucc_geth_probe(struct platform_device* ofdev)
  		}
  
  	if (max_speed == SPEED_1000) {
-		unsigned int snums = qe_get_num_of_snums();
+		unsigned int snums = qe_num_of_snum;
  
  		/* configure muram FIFOs for gigabit operation */
  		ug_info->uf_info.urfs = UCC_GETH_URFS_GIGA_INIT;
diff --git a/drivers/soc/fsl/qe/qe.c b/drivers/soc/fsl/qe/qe.c
index af3c2b2b268f..8c3b3c62d81b 100644
--- a/drivers/soc/fsl/qe/qe.c
+++ b/drivers/soc/fsl/qe/qe.c
@@ -52,7 +52,8 @@ EXPORT_SYMBOL(qe_immr);
  
  static u8 snums[QE_NUM_OF_SNUM];	/* Dynamically allocated SNUMs */
  static DECLARE_BITMAP(snum_state, QE_NUM_OF_SNUM);
-static unsigned int qe_num_of_snum;
+unsigned int qe_num_of_snum;
+EXPORT_SYMBOL(qe_num_of_snum);
By exporting the object you allow other drivers to modify it. Is that 
really what we want ?

Why not keep qe_get_num_of_snums() as a helper that simply returns 
qe_num_of_snum ?
quoted hunk ↗ jump to hunk
  
  static phys_addr_t qebase = -1;
  
@@ -308,26 +309,34 @@ static void qe_snums_init(void)
  	int i;
  
  	bitmap_zero(snum_state, QE_NUM_OF_SNUM);
+	qe_num_of_snum = 28; /* The default number of snum for threads is 28 */
  	qe = qe_get_device_node();
  	if (qe) {
  		i = of_property_read_variable_u8_array(qe, "fsl,qe-snums",
  						       snums, 1, QE_NUM_OF_SNUM);
-		of_node_put(qe);
  		if (i > 0) {
+			of_node_put(qe);
  			qe_num_of_snum = i;
  			return;
  		}
+		/*
+		 * Fall back to legacy binding of using the value of
+		 * fsl,qe-num-snums to choose one of the static arrays
+		 * above.
+		 */
+		of_property_read_u32(qe, "fsl,qe-num-snums", &qe_num_of_snum);
+		of_node_put(qe);
  	}
  
-	qe_num_of_snum = qe_get_num_of_snums();
-
  	if (qe_num_of_snum == 76)
  		snum_init = snum_init_76;
-	else
+	else if (qe_num_of_snum == 28 || qe_num_of_snum == 46)
  		snum_init = snum_init_46;
-
-	for (i = 0; i < qe_num_of_snum; i++)
-		snums[i] = snum_init[i];
+	else {
+		pr_err("QE: unsupported value of fsl,qe-num-snums: %u\n", qe_num_of_snum);
+		return;
+	}
The first leg of the if/else must have {} too when the second leg has them.
quoted hunk ↗ jump to hunk
+	memcpy(snums, snum_init, qe_num_of_snum);
  }
  
  int qe_get_snum(void)
@@ -645,35 +654,6 @@ unsigned int qe_get_num_of_risc(void)
  }
  EXPORT_SYMBOL(qe_get_num_of_risc);
  
-unsigned int qe_get_num_of_snums(void)
I think this function should remain and just return num_of_snums, see my 
other comment above.

Christophe

quoted hunk ↗ jump to hunk
-{
-	struct device_node *qe;
-	int size;
-	unsigned int num_of_snums;
-	const u32 *prop;
-
-	num_of_snums = 28; /* The default number of snum for threads is 28 */
-	qe = qe_get_device_node();
-	if (!qe)
-		return num_of_snums;
-
-	prop = of_get_property(qe, "fsl,qe-num-snums", &size);
-	if (prop && size == sizeof(*prop)) {
-		num_of_snums = *prop;
-		if ((num_of_snums < 28) || (num_of_snums > QE_NUM_OF_SNUM)) {
-			/* No QE ever has fewer than 28 SNUMs */
-			pr_err("QE: number of snum is invalid\n");
-			of_node_put(qe);
-			return -EINVAL;
-		}
-	}
-
-	of_node_put(qe);
-
-	return num_of_snums;
-}
-EXPORT_SYMBOL(qe_get_num_of_snums);
-
  static int __init qe_init(void)
  {
  	struct device_node *np;
diff --git a/include/soc/fsl/qe/qe.h b/include/soc/fsl/qe/qe.h
index b3d1aff5e8ad..af5739850bf4 100644
--- a/include/soc/fsl/qe/qe.h
+++ b/include/soc/fsl/qe/qe.h
@@ -212,7 +212,7 @@ int qe_setbrg(enum qe_clock brg, unsigned int rate, unsigned int multiplier);
  int qe_get_snum(void);
  void qe_put_snum(u8 snum);
  unsigned int qe_get_num_of_risc(void);
-unsigned int qe_get_num_of_snums(void);
+extern unsigned int qe_num_of_snum;
  
  static inline int qe_alive_during_sleep(void)
  {
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help