Re: [PATCH RFC 2/2] clk: implement clk_unregister

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 08/16/2013 11:48 PM, Mike Turquette wrote:
> Quoting Sylwester Nawrocki (2013-08-06 08:51:57)
>> +/*
>> + * Empty clk_ops for unregistered clocks. These are used temporarily
>> + * after clk_unregister() was called on a clock and until last clock
>> + * consumer calls clk_put() and the struct clk object is freed.
>> + */
>> +static int clk_dummy_prepare_enable(struct clk_hw *hw)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static void clk_dummy_disable_unprepare(struct clk_hw *hw)
>> +{
>> +       WARN_ON(1);
>> +}
>> +
>> +static int clk_dummy_set_rate(struct clk_hw *hw, unsigned long rate,
>> +                                       unsigned long parent_rate)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static int clk_dummy_set_parent(struct clk_hw *hw, u8 index)
>> +{
>> +       return -ENXIO;
>> +}
>> +
>> +static const struct clk_ops clk_dummy_ops = {
>> +       .enable         = clk_dummy_prepare_enable,
>> +       .disable        = clk_dummy_disable_unprepare,
>> +       .prepare        = clk_dummy_prepare_enable,
>> +       .unprepare      = clk_dummy_disable_unprepare,
>> +       .set_rate       = clk_dummy_set_rate,
>> +       .set_parent     = clk_dummy_set_parent,
>> +};
> 
> Don't use "clk_dummy_*" here. The use of dummy often implies that
> operations will return success in the absence of actual hardware but
> these return an error, and rightly so. So maybe rename the functions and
> clk_ops instance to something like "clk_nodev_*" or "clk_missing_*"?

Hmm, this is more about a driver being removed rather than the device.
Then perhaps we could make it __clk_nodrv_* or clk_nodrv_* ?

>>  /**
>>   * clk_unregister - unregister a currently registered clock
>>   * @clk: clock to unregister
>> - *
>> - * Currently unimplemented.
>>   */
>> -void clk_unregister(struct clk *clk) {}
>> +void clk_unregister(struct clk *clk)
>> +{
>> +       unsigned long flags;
>> +
>> +       clk_prepare_lock();
>> +
>> +       if (!clk || IS_ERR(clk)) {
>> +               pr_err("%s: invalid clock: %p\n", __func__, clk);
>> +               goto out;
>> +       }
>> +
>> +       if (clk->ops == &clk_dummy_ops) {
>> +               pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
>> +               goto out;
>> +       }
>> +       /*
>> +        * Assign dummy clock ops for consumers that might still hold
>> +        * a reference to this clock.
>> +        */
>> +       flags = clk_enable_lock();
>> +       clk->ops = &clk_dummy_ops;
>> +       clk_enable_unlock(flags);
>> +
>> +       if (!hlist_empty(&clk->children)) {
>> +               struct clk *child;
>> +
>> +               /* Reparent all children to the orphan list. */
>> +               hlist_for_each_entry(child, &clk->children, child_node)
>> +                       clk_set_parent(child, NULL);
>> +       }
> 
> This looks pretty good. A remaining problem is re-loading the clock
> provider module will have string name conflicts with the old
> unregistered clocks (but not yet released) clocks during calls to
> __clk_lookup.

But the clock is being dropped from the clock tree immediately in this
function. After the hlist_del_init() call below the clock is not present
on any clocks list. Upon clock release only the memory allocated for
the clock is freed.

> The best solution would be to refactor clk.c to not use string name
> lookups but that is probably too big of an issue for the purpose of this
> series (but it will happen some day).
> 
> A short term solution would be to poison the clock's string name here.
> Reallocate the clk->name string with some poison data so that name
> conflicts don't occur. What do you think?

This shouldn't be necessary, for the reason described above. I've tested
multiple registrations when a clock was being referenced by a consumer
driver and it worked well.

I'm still a bit unsure about the kref reference counting, but I'd would
assume it is good to have. It prevents the kernel to crash in some
situations. Many other subsystems/drivers crash miserably when a driver
gets unbound using the sysfs "unbind" attribute. However, if it is assumed
that user space needs to keep track of respective resource references
and should know what it does when unbinding drivers, then we could probably
do without the kref. I'm seriously sceptical though about letting user
space to crash the kernel in fairly simple steps, it just doesn't sound
right.

> Regards,
> Mike
> 
>> +
>> +       clk_debug_unregister(clk);
>> +
>> +       hlist_del_init(&clk->child_node);
>> +
>> +       if (clk->prepare_count)
>> +               pr_warn("%s: unregistering prepared clock: %s\n",
>> +                                       __func__, clk->name);
>> +
>> +       kref_put(&clk->ref, __clk_release);
>> +out:
>> +       clk_prepare_unlock();
>> +}
>>  EXPORT_SYMBOL_GPL(clk_unregister);
>>
>>  static void devm_clk_release(struct device *dev, void *res)
>> @@ -1861,6 +1973,7 @@ int __clk_get(struct clk *clk)
>>         if (!try_module_get(clk->owner))
>>                 return 0;
>>
>> +       kref_get(&clk->ref);
>>         return 1;
>>  }
>>  EXPORT_SYMBOL(__clk_get);
>> @@ -1870,6 +1983,10 @@ void __clk_put(struct clk *clk)
>>         if (!clk || IS_ERR(clk))
>>                 return;
>>
>> +       clk_prepare_lock();
>> +       kref_put(&clk->ref, __clk_release);
>> +       clk_prepare_unlock();
>> +
>>         module_put(clk->owner);
>>  }
>>  EXPORT_SYMBOL(__clk_put);

--
Regards,
Sylwester