Hi Geert,
Just going to ack the other items you raised for now, but curious about
this one:
On 06/12/2021 15:53, Geert Uytterhoeven wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>> +static void mpfs_clk_unregister_cfg(struct device *dev, struct clk_hw *hw)
>> +{
>> + struct mpfs_cfg_hw_clock *cfg_hw = to_mpfs_cfg_clk(hw);
>> +
>> + devm_clk_hw_unregister(dev, hw);
>> + kfree(cfg_hw);
>
> This is freeing a part of the big array allocated with devm_kzalloc()?
I took a look at this, and I don't think it is freeing the devm
allocated array.
To me, what is actually being freed is an element in the array of
structs passed to mpfs_clk_register_cfgs in the probe function.
However, this struct is statically defined - so its elements shouldn't
be freed at all?
drivers/clk/clk-bm1880.c has the same behaviour in the unregister
function, where it calls kfree on the elements of a static array of clk
structs. So if my understanding is correct it would need fixing there too.
Cheers,
Conor.
>> +}
>> +
>> +static struct clk_hw *mpfs_clk_register_cfg(struct device *dev,
>> + struct mpfs_cfg_hw_clock *cfg_hw,
>> + void __iomem *sys_base)
>> +{
>> + struct clk_hw *hw;
>> + int err;
>> +
>> + cfg_hw->sys_base = sys_base;
>> + cfg_hw->lock = &mpfs_clk_lock;
>> +
>> + hw = &cfg_hw->hw;
>> + err = devm_clk_hw_register(dev, hw);
>> + if (err)
>> + return ERR_PTR(err);
>> +
>> + return hw;
>> +}
>> +
>> +static int mpfs_clk_register_cfgs(struct device *dev, struct mpfs_cfg_hw_clock *cfg_hws,
>> + int num_clks, struct mpfs_clock_data *data)
>
> unsigned int num_clks
>
>> +{
>> + struct clk_hw *hw;
>> + void __iomem *sys_base = data->base;
>> + unsigned int i, id;
>> +
>> + for (i = 0; i < num_clks; i++) {
>> + struct mpfs_cfg_hw_clock *cfg_hw = &cfg_hws[i];
>> +
>> + hw = mpfs_clk_register_cfg(dev, cfg_hw, sys_base);
>> + if (IS_ERR(hw)) {
>> + dev_err(dev, "%s: failed to register clock %s\n", __func__,
>
> I guess the __func__ can be dropped, as the clock name is unique?
>
>> + cfg_hw->cfg.name);
>> + goto err_clk;
>> + }
>> +
>> + id = cfg_hws[i].cfg.id;
>> + data->hw_data.hws[id] = hw;
>> + }
>> +
>> + return 0;
>> +
>> +err_clk:
>> + while (i--)
>> + mpfs_clk_unregister_cfg(dev, data->hw_data.hws[cfg_hws[i].cfg.id]);
>> +
>> + return PTR_ERR(hw);
>> +}
