| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <35a50a5b-061c-236a-16ff-0389bf34d736@c-s.fr>
Date: Tue, 30 Apr 2019 19:27:13 +0200
From: Christophe Leroy <christophe.leroy@....fr>
To: Rasmus Villemoes <rasmus.villemoes@...vas.dk>,
Qiang Zhao <qiang.zhao@....com>, Li Yang <leoyang.li@....com>
Cc: Valentin Longchamp <valentin.longchamp@...mile.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Scott Wood <oss@...error.net>,
Rasmus Villemoes <Rasmus.Villemoes@...vas.se>,
Rob Herring <robh+dt@...nel.org>,
"linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into
qe_snums_init
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@...vas.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)
> {
>
Powered by blists - more mailing lists