Re: [PATCH v7 5/7] OPP: Add sanity checks in _read_opp_key()

[Matthias Kaehlcke]

On Fri, Apr 24, 2020 at 06:54:02PM +0300, Georgi Djakov wrote:
> When we read the OPP keys, it would be nice to do some sanity checks
> of the values we get from DT and see if they match with the information
> that is populated in the OPP table. Let's pass a pointer of the table,
> so that we can do some validation.
> 
> Signed-off-by: Georgi Djakov <georgi.djakov@xxxxxxxxxx>
> ---
> v7:
> New patch.
> 
>  drivers/opp/of.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/opp/of.c b/drivers/opp/of.c
> index 978e445b0cdb..2b590fe2e69a 100644
> --- a/drivers/opp/of.c
> +++ b/drivers/opp/of.c
> @@ -574,8 +574,8 @@ void dev_pm_opp_of_remove_table(struct device *dev)
>  }
>  EXPORT_SYMBOL_GPL(dev_pm_opp_of_remove_table);
>  
> -static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
> -			 bool *rate_not_available)
> +static int _read_opp_key(struct dev_pm_opp *new_opp, struct opp_table *table,
> +			 struct device_node *np, bool *rate_not_available)
>  {
>  	struct property *peak, *avg;
>  	u32 *peak_bw, *avg_bw;
> @@ -603,6 +603,12 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
>  		 * opp-avg-kBps = <path1_value path2_value>;
>  		 */
>  		count = peak->length / sizeof(u32);
> +		if (table->path_count != count) {
> +			pr_err("%s: Mismatch between opp-peak-kBps and paths (%d %d)\n",
> +			       __func__, count, table->path_count);
> +			return -EINVAL;
> +		}
> +
>  		peak_bw = kmalloc_array(count, sizeof(*peak_bw), GFP_KERNEL);
>  		if (!peak_bw)
>  			return -ENOMEM;
> @@ -624,6 +630,13 @@ static int _read_opp_key(struct dev_pm_opp *new_opp, struct device_node *np,
>  	avg = of_find_property(np, "opp-avg-kBps", NULL);
>  	if (peak && avg) {
>  		count = avg->length / sizeof(u32);
> +		if (table->path_count != count) {
> +			pr_err("%s: Mismatch between opp-avg-kBps and paths (%d %d)\n",
> +			       __func__, count, table->path_count);
> +			ret = -EINVAL;
> +			goto free_peak_bw;
> +		}
> +
>  		avg_bw = kmalloc_array(count, sizeof(*avg_bw), GFP_KERNEL);
>  		if (!avg_bw) {
>  			ret = -ENOMEM;
> @@ -695,7 +708,7 @@ static struct dev_pm_opp *_opp_add_static_v2(struct opp_table *opp_table,
>  	if (!new_opp)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = _read_opp_key(new_opp, np, &rate_not_available);
> +	ret = _read_opp_key(new_opp, opp_table, np, &rate_not_available);
>  	if (ret < 0) {
>  		if (!opp_table->is_genpd)
>  			dev_err(dev, "%s: opp key field not found\n", __func__);

Reviewed-by: Matthias Kaehlcke <mka@xxxxxxxxxxxx>