[PATCH v2 02/11] clk: Check that ops of composite clock components, exist before calling

[Lukasz Majewski]

Hi Sean,

> clk_composite_ops was shared between all devices in the composite
> clock driver. If one clock had a feature (such as supporting
> set_parent) which another clock did not, it could call a null pointer
> dereference.
> 
> This patch does three things
> 1. It adds null-pointer checks to all composite clock functions.
> 2. It makes clk_composite_ops const and sets its functions at
> compile-time. 3. It adds some basic sanity checks to num_parents.
> 
> The combined effect of these changes is that any of mux, rate, or
> gate can be NULL, and composite clocks will still function normally.
> Previously, at least mux had to exist, since clk_composite_get_parent
> was used to determine the parent for clk_register.
> 

Thank you for those checks - up till now this code was tuned for i.MX
(and in-fact this SoC's clock tree).

However, I would like to first wait for the consensus regarding the
shape of composite clocks (and reply to this mail):

"clk: Always use the supplied struct clk"

> Signed-off-by: Sean Anderson <seanga2 at gmail.com>
> ---
>  drivers/clk/clk-composite.c | 57
> +++++++++++++++++++++++-------------- 1 file changed, 35
> insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/clk/clk-composite.c b/drivers/clk/clk-composite.c
> index d0f273d47f..ea9982fd57 100644
> --- a/drivers/clk/clk-composite.c
> +++ b/drivers/clk/clk-composite.c
> @@ -22,7 +22,10 @@ static u8 clk_composite_get_parent(struct clk *clk)
>  		(struct clk *)dev_get_clk_ptr(clk->dev) : clk);
>  	struct clk *mux = composite->mux;
>  
> -	return clk_mux_get_parent(mux);
> +	if (mux)
> +		return clk_mux_get_parent(mux);
> +	else
> +		return 0;
>  }
>  
>  static int clk_composite_set_parent(struct clk *clk, struct clk
> *parent) @@ -32,7 +35,10 @@ static int
> clk_composite_set_parent(struct clk *clk, struct clk *parent) const
> struct clk_ops *mux_ops = composite->mux_ops; struct clk *mux =
> composite->mux; 
> -	return mux_ops->set_parent(mux, parent);
> +	if (mux && mux_ops)
> +		return mux_ops->set_parent(mux, parent);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static unsigned long clk_composite_recalc_rate(struct clk *clk)
> @@ -42,7 +48,10 @@ static unsigned long
> clk_composite_recalc_rate(struct clk *clk) const struct clk_ops
> *rate_ops = composite->rate_ops; struct clk *rate = composite->rate;
>  
> -	return rate_ops->get_rate(rate);
> +	if (rate && rate_ops)
> +		return rate_ops->get_rate(rate);
> +	else
> +		return clk_get_parent_rate(clk);
>  }
>  
>  static ulong clk_composite_set_rate(struct clk *clk, unsigned long
> rate) @@ -52,7 +61,10 @@ static ulong clk_composite_set_rate(struct
> clk *clk, unsigned long rate) const struct clk_ops *rate_ops =
> composite->rate_ops; struct clk *clk_rate = composite->rate;
>  
> -	return rate_ops->set_rate(clk_rate, rate);
> +	if (rate && rate_ops)
> +		return rate_ops->set_rate(clk_rate, rate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static int clk_composite_enable(struct clk *clk)
> @@ -62,7 +74,10 @@ static int clk_composite_enable(struct clk *clk)
>  	const struct clk_ops *gate_ops = composite->gate_ops;
>  	struct clk *gate = composite->gate;
>  
> -	return gate_ops->enable(gate);
> +	if (gate && gate_ops)
> +		return gate_ops->enable(gate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
>  static int clk_composite_disable(struct clk *clk)
> @@ -72,15 +87,12 @@ static int clk_composite_disable(struct clk *clk)
>  	const struct clk_ops *gate_ops = composite->gate_ops;
>  	struct clk *gate = composite->gate;
>  
> -	gate_ops->disable(gate);
> -
> -	return 0;
> +	if (gate && gate_ops)
> +		return gate_ops->disable(gate);
> +	else
> +		return -ENOTSUPP;
>  }
>  
> -struct clk_ops clk_composite_ops = {
> -	/* This will be set according to clk_register_composite */
> -};
> -
>  struct clk *clk_register_composite(struct device *dev, const char
> *name, const char * const *parent_names,
>  				   int num_parents, struct clk *mux,
> @@ -94,7 +106,9 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, struct clk *clk;
>  	struct clk_composite *composite;
>  	int ret;
> -	struct clk_ops *composite_ops = &clk_composite_ops;
> +
> +	if (!num_parents || (num_parents != 1 && !mux))
> +		return ERR_PTR(-EINVAL);
>  
>  	composite = kzalloc(sizeof(*composite), GFP_KERNEL);
>  	if (!composite)
> @@ -103,8 +117,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, if (mux && mux_ops) {
>  		composite->mux = mux;
>  		composite->mux_ops = mux_ops;
> -		if (mux_ops->set_parent)
> -			composite_ops->set_parent =
> clk_composite_set_parent; mux->data = (ulong)composite;
>  	}
>  
> @@ -113,11 +125,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, clk = ERR_PTR(-EINVAL);
>  			goto err;
>  		}
> -		composite_ops->get_rate = clk_composite_recalc_rate;
> -
> -		/* .set_rate requires either .round_rate or
> .determine_rate */
> -		if (rate_ops->set_rate)
> -			composite_ops->set_rate =
> clk_composite_set_rate; 
>  		composite->rate = rate;
>  		composite->rate_ops = rate_ops;
> @@ -132,8 +139,6 @@ struct clk *clk_register_composite(struct device
> *dev, const char *name, 
>  		composite->gate = gate;
>  		composite->gate_ops = gate_ops;
> -		composite_ops->enable = clk_composite_enable;
> -		composite_ops->disable = clk_composite_disable;
>  		gate->data = (ulong)composite;
>  	}
>  
> @@ -160,6 +165,14 @@ err:
>  	return clk;
>  }
>  
> +static const struct clk_ops clk_composite_ops = {
> +	.set_parent = clk_composite_set_parent,
> +	.get_rate = clk_composite_recalc_rate,
> +	.set_rate = clk_composite_set_rate,
> +	.enable = clk_composite_enable,
> +	.disable = clk_composite_disable,
> +};
> +
>  U_BOOT_DRIVER(clk_composite) = {
>  	.name	= UBOOT_DM_CLK_COMPOSITE,
>  	.id	= UCLASS_CLK,




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200126/807181c9/attachment.sig>