Re: [PATCH v6 2/2] clk: microchip: Add driver for Microchip PolarFire SoC

[<Conor.Dooley@microchip.com>]

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);
>> +}