| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20130823215838.8231.21635@quantum>
Date: Fri, 23 Aug 2013 14:58:38 -0700
From: Mike Turquette <mturquette@...aro.org>
To: Sylwester Nawrocki <s.nawrocki@...sung.com>,
linux-arm-kernel@...ts.infradead.org
Cc: linux@....linux.org.uk, jiada_wang@...tor.com,
robherring2@...il.com, grant.likely@...aro.org, broonie@...nel.org,
vapier@...too.org, ralf@...ux-mips.org, kyungmin.park@...sung.com,
shawn.guo@...aro.org, sebastian.hesselbarth@...il.com,
LW@...O-electronics.de, t.figa@...sung.com, g.liakhovetski@....de,
laurent.pinchart@...asonboard.com, linux-kernel@...r.kernel.org,
uclinux-dist-devel@...ckfin.uclinux.org, linux-mips@...ux-mips.org,
linux-sh@...r.kernel.org,
Sylwester Nawrocki <s.nawrocki@...sung.com>
Subject: Re: [PATCH v2 2/4] clk: implement clk_unregister
Quoting Sylwester Nawrocki (2013-08-20 10:34:21)
> clk_unregister() is currently not implemented and it is required when
> a clock provider module needs to be unloaded.
>
> Normally the clock supplier module is prevented to be unloaded by
> taking reference on the module in clk_get().
>
> For cases when the clock supplier module deinitializes despite the
> consumers of its clocks holding a reference on the module, e.g. when
> the driver is unbound through "unbind" sysfs attribute, there are
> empty clock ops added. These ops are assigned temporarily to struct
> clk and used until all consumers release the clock, to avoid invoking
> callbacks from the module which just got removed.
>
> Signed-off-by: Jiada Wang <jiada_wang@...tor.com>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@...sung.com>
This change looks good to me. One minor nitpick below.
> ---
> Changes since RFC v1:
> - renamed clk_dummy_* to clk_nodrv_*.
>
> Changes since v3 of the original patch [1]:
> - reparent all children to the orphan list instead of leaving
> the clock unregistered when it has child clocks,
> - removed unnecessary prerequisite checks in clk_debug_unregister(),
> - struct clk is now being freed only when the last clock consumer
> calls clk_put(),
> - empty clock ops are used after clk_unregister() has been called
> until all references to the clock are released and the clock
> object is freed.
>
> [1] http://www.spinics.net/lists/arm-kernel/msg247548.html
> ---
> drivers/clk/clk.c | 123 +++++++++++++++++++++++++++++++++++++++++--
> include/linux/clk-private.h | 2 +
> 2 files changed, 122 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 0e0eb31..549fcb7 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -342,6 +342,21 @@ out:
> return ret;
> }
>
> + /**
> + * clk_debug_unregister - remove a clk node from the debugfs clk tree
> + * @clk: the clk being removed from the debugfs clk tree
> + *
> + * Dynamically removes a clk and all it's children clk nodes from the
> + * debugfs clk tree if clk->dentry points to debugfs created by
> + * clk_debug_register in __clk_init.
> + *
> + * Caller must hold prepare_lock.
> + */
> +static void clk_debug_unregister(struct clk *clk)
> +{
> + debugfs_remove_recursive(clk->dentry);
> +}
> +
> /**
> * clk_debug_reparent - reparent clk node in the debugfs clk tree
> * @clk: the clk being reparented
> @@ -432,6 +447,9 @@ static inline int clk_debug_register(struct clk *clk) { return 0; }
> static inline void clk_debug_reparent(struct clk *clk, struct clk *new_parent)
> {
> }
> +static inline void clk_debug_unregister(struct clk *clk)
> +{
> +}
> #endif
>
> /* caller must hold prepare_lock */
> @@ -1656,6 +1674,7 @@ int __clk_init(struct device *dev, struct clk *clk)
>
> clk_debug_register(clk);
>
> + kref_init(&clk->ref);
> out:
> clk_prepare_unlock();
>
> @@ -1785,13 +1804,106 @@ fail_out:
> }
> EXPORT_SYMBOL_GPL(clk_register);
>
> +/*
> + * Free memory allocated for a clock.
> + * Caller must hold prepare_lock.
> + */
> +static void __clk_release(struct kref *ref)
> +{
> + struct clk *clk = container_of(ref, struct clk, ref);
> + int i = clk->num_parents;
> +
> + kfree(clk->parents);
> + while (--i >= 0)
> + kfree(clk->parent_names[i]);
> +
> + kfree(clk->parent_names);
> + kfree(clk->name);
> + kfree(clk);
> +}
> +
> +/*
> + * 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_nodrv_prepare_enable(struct clk_hw *hw)
> +{
> + return -ENXIO;
> +}
> +
> +static void clk_nodrv_disable_unprepare(struct clk_hw *hw)
> +{
> + WARN_ON(1);
Ideally we shouldn't get here, but if we do I guess it could be very
noisy. How about WARN_ONCE?
After you address Russell's comments in patch #1 I will be happy to take
this series.
Thanks,
Mike
> +}
> +
> +static int clk_nodrv_set_rate(struct clk_hw *hw, unsigned long rate,
> + unsigned long parent_rate)
> +{
> + return -ENXIO;
> +}
> +
> +static int clk_nodrv_set_parent(struct clk_hw *hw, u8 index)
> +{
> + return -ENXIO;
> +}
> +
> +static const struct clk_ops clk_nodrv_ops = {
> + .enable = clk_nodrv_prepare_enable,
> + .disable = clk_nodrv_disable_unprepare,
> + .prepare = clk_nodrv_prepare_enable,
> + .unprepare = clk_nodrv_disable_unprepare,
> + .set_rate = clk_nodrv_set_rate,
> + .set_parent = clk_nodrv_set_parent,
> +};
> +
> /**
> * 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_nodrv_ops) {
> + pr_err("%s: unregistered clock: %s\n", __func__, clk->name);
> + goto out;
> + }
> + /*
> + * Assign empty clock ops for consumers that might still hold
> + * a reference to this clock.
> + */
> + flags = clk_enable_lock();
> + clk->ops = &clk_nodrv_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);
> + }
> +
> + 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)
> @@ -1862,6 +1974,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);
> @@ -1871,6 +1984,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);
> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
> index b7c0b58..36c1fc8 100644
> --- a/include/linux/clk-private.h
> +++ b/include/linux/clk-private.h
> @@ -12,6 +12,7 @@
> #define __LINUX_CLK_PRIVATE_H
>
> #include <linux/clk-provider.h>
> +#include <linux/kref.h>
> #include <linux/list.h>
>
> /*
> @@ -47,6 +48,7 @@ struct clk {
> #ifdef CONFIG_COMMON_CLK_DEBUG
> struct dentry *dentry;
> #endif
> + struct kref ref;
> };
>
> /*
> --
> 1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists