Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH 5/5] soc/fsl/qe: qe.c: fold qe_get_num_of_snums into
 qe_snums_init
To:     Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Qiang Zhao <qiang.zhao@nxp.com>, Li Yang <leoyang.li@nxp.com>
Cc:     Valentin Longchamp <valentin.longchamp@keymile.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Scott Wood <oss@buserror.net>,
        Rasmus Villemoes <Rasmus.Villemoes@prevas.se>,
        Rob Herring <robh+dt@kernel.org>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>
References: <20190430133615.25721-1-rasmus.villemoes@prevas.dk>
 <20190430133615.25721-6-rasmus.villemoes@prevas.dk>
From:   Christophe Leroy <christophe.leroy@c-s.fr>
Message-ID: <35a50a5b-061c-236a-16ff-0389bf34d736@c-s.fr>
Date:   Tue, 30 Apr 2019 19:27:13 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.6.1
MIME-Version: 1.0
In-Reply-To: <20190430133615.25721-6-rasmus.villemoes@prevas.dk>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: fr
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


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)
>   {
>