Re: [PATCH v6 3/6] clk: spacemit: set up reset auxiliary devices

[Alex Elder <elder@riscstar.com>]

On 5/8/25 3:04 PM, Alex Elder wrote:
> On 5/7/25 11:46 PM, Haylen Chu wrote:
>> On Tue, May 06, 2025 at 04:06:34PM -0500, Alex Elder wrote:
>>> Add a new reset_name field to the spacemit_ccu_data structure.  If it is
>>> non-null, the CCU implements a reset controller, and the name will be
>>> used as the name for the auxiliary device that implements it.
>>>
>>> Define a new type to hold an auxiliary device as well as the regmap
>>> pointer that will be needed by CCU reset controllers.  Set up code to
>>> initialize and add an auxiliary device for any CCU that implements reset
>>> functionality.
>>>
>>> Make it optional for a CCU to implement a clock controller.  This
>>> doesn't apply to any of the existing CCUs but will for some new ones
>>> that will be added soon.
>>>
>>> Signed-off-by: Alex Elder <elder@riscstar.com>
>>> ---
>>>   drivers/clk/spacemit/ccu-k1.c | 85 +++++++++++++++++++++++++++++++----
>>>   include/soc/spacemit/ccu_k1.h | 12 +++++
>>>   2 files changed, 89 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/clk/spacemit/ccu-k1.c b/drivers/clk/spacemit/ 
>>> ccu-k1.c
>>> index 9545cfe60b92b..6b1845e899e5f 100644
>>> --- a/drivers/clk/spacemit/ccu-k1.c
>>> +++ b/drivers/clk/spacemit/ccu-k1.c
>>
>> ...
>>
>>> +static void spacemit_cadev_release(struct device *dev)
>>> +{
>>> +    struct auxiliary_device *adev = to_auxiliary_dev(dev);
>>> +
>>> +    kfree(to_spacemit_ccu_adev(adev));
>>> +}
>>
>> spacemit_ccu_adev structures are allocated with devm_kzalloc() in
>> spacemit_ccu_reset_register(), which means its lifetime is bound to the
>> driver and it'll be automatically released after driver removal; won't
>> there be a possibility of double-free? I think the release callback
>> could be simply dropped.
> 
> You are correct.  And unfortunately I didn't include the fix
> for this in the patches I just posted, because somehow this
> message was not included with the group in my mail program.
> 
> I'm going to send v8 after I fix this and verify it again.

To be clear, the fix is to use kzalloc(), rather than calling
devm_kzalloc() with the parent device as first argument.

I'll also include <linux/slab.h> to avoid the warning
reported by the kernel test robot.

					-Alex

> 
>                      -Alex
> 
> 
>> ...
>>
>>> +static int spacemit_ccu_reset_register(struct device *dev,
>>> +                       struct regmap *regmap,
>>> +                       const char *reset_name)
>>> +{
>>> +    struct spacemit_ccu_adev *cadev;
>>> +    struct auxiliary_device *adev;
>>> +    static u32 next_id;
>>> +    int ret;
>>> +
>>> +    /* Nothing to do if the CCU does not implement a reset 
>>> controller */
>>> +    if (!reset_name)
>>> +        return 0;
>>> +
>>> +    cadev = devm_kzalloc(dev, sizeof(*cadev), GFP_KERNEL);
>>
>> Here spacemit_ccu_adev is allocated.
>>
>>> +    if (!cadev)
>>> +        return -ENOMEM;
>>> +    cadev->regmap = regmap;
>>> +
>>> +    adev = &cadev->adev;
>>> +    adev->name = reset_name;
>>> +    adev->dev.parent = dev;
>>> +    adev->dev.release = spacemit_cadev_release;
>>> +    adev->dev.of_node = dev->of_node;
>>> +    adev->id = next_id++;
>>> +
>>> +    ret = auxiliary_device_init(adev);
>>> +    if (ret)
>>> +        return ret;
>>> +
>>> +    ret = auxiliary_device_add(adev);
>>> +    if (ret) {
>>> +        auxiliary_device_uninit(adev);
>>> +        return ret;
>>> +    }
>>> +
>>> +    return devm_add_action_or_reset(dev, spacemit_adev_unregister, 
>>> adev);
>>> +}
>>> +
>>
>> Best regards,
>> Haylen Chu
>