| Message ID | 20190430133615.25721-6-rasmus.villemoes@prevas.dk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | soc/fsl/qe: cleanups and new DT binding | expand |
Le 30/04/2019 à 15:36, Rasmus Villemoes a écrit : > 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 <rasmus.villemoes@prevas.dk> > --- > 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 ? > > 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. > + 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 > -{ > - 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) > { >
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); 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; + } + 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) -{ - 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) {
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 <rasmus.villemoes@prevas.dk> --- 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(-)