MUX could choose parent accordingly when set_rate. Add clk_rate_fops to debug whether mux choose correct parent
Example: cat clk_summary clock enable_cnt prepare_cnt rate --------------------------------------------------------------------- osc24mhz 3 3 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000 sfc_mux 0 0 200000000 sfc 0 0 200000000
cat osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate 200000000
echo 24000000 > osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate cat clk_summary clock enable_cnt prepare_cnt rate --------------------------------------------------------------------- osc24mhz 3 3 24000000 sfc_mux 0 0 24000000 sfc 0 0 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000
cat osc24mhz/sfc_mux/sfc/clk_rate 24000000
Signed-off-by: Zhangfei Gao zhangfei.gao@linaro.org --- drivers/clk/clk.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 373cd54..327b698 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -226,6 +226,33 @@ static const struct file_operations clk_dump_fops = { .release = single_release, };
+static int clk_rate_fops_get(void *data, u64 *rate) +{ + struct clk *clk = data; + + *rate = clk->rate; + + return 0; +}; + +static int clk_rate_fops_set(void *data, u64 rate) +{ + struct clk *clk = data; + int ret = 0; + + ret = clk_prepare_enable(clk); + if (ret) + goto out; + clk_set_rate(clk, rate); + clk_disable_unprepare(clk); + +out: + return ret; +}; + +DEFINE_SIMPLE_ATTRIBUTE(clk_rate_fops, clk_rate_fops_get, + clk_rate_fops_set, "%llu\n"); + /* caller must hold prepare_lock */ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry) { @@ -243,8 +270,8 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry)
clk->dentry = d;
- d = debugfs_create_u32("clk_rate", S_IRUGO, clk->dentry, - (u32 *)&clk->rate); + d = debugfs_create_file("clk_rate", S_IWUSR | S_IRUGO, clk->dentry, + clk, &clk_rate_fops); if (!d) goto err_out;
Quoting Zhangfei Gao (2013-08-15 21:03:28)
MUX could choose parent accordingly when set_rate. Add clk_rate_fops to debug whether mux choose correct parent
Example: cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000 sfc_mux 0 0 200000000 sfc 0 0 200000000
cat osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate 200000000
echo 24000000 > osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 sfc_mux 0 0 24000000 sfc 0 0 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000
cat osc24mhz/sfc_mux/sfc/clk_rate 24000000
Signed-off-by: Zhangfei Gao zhangfei.gao@linaro.org
NACK. This patch just makes it possible to call clk_set_rate on every clock from userspace. I don't support that operation, it just leads to tons of horrible hacks where closed userspace crapplications manage hardware directly.
Also I'm not sure what this patch has to do with the description in the changelog. How does setting the rate of every clock help debug mux clocks? You can already see a mux clocks parent and rate from the existing debugs info.
Regards, Mike
drivers/clk/clk.c | 31 +++++++++++++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c index 373cd54..327b698 100644 --- a/drivers/clk/clk.c +++ b/drivers/clk/clk.c @@ -226,6 +226,33 @@ static const struct file_operations clk_dump_fops = { .release = single_release, }; +static int clk_rate_fops_get(void *data, u64 *rate) +{
struct clk *clk = data;*rate = clk->rate;return 0;+};
+static int clk_rate_fops_set(void *data, u64 rate) +{
struct clk *clk = data;int ret = 0;ret = clk_prepare_enable(clk);if (ret)goto out;clk_set_rate(clk, rate);clk_disable_unprepare(clk);+out:
return ret;+};
+DEFINE_SIMPLE_ATTRIBUTE(clk_rate_fops, clk_rate_fops_get,
clk_rate_fops_set, "%llu\n");/* caller must hold prepare_lock */ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry) { @@ -243,8 +270,8 @@ static int clk_debug_create_one(struct clk *clk, struct dentry *pdentry) clk->dentry = d;
d = debugfs_create_u32("clk_rate", S_IRUGO, clk->dentry,(u32 *)&clk->rate);
d = debugfs_create_file("clk_rate", S_IWUSR | S_IRUGO, clk->dentry,clk, &clk_rate_fops); if (!d) goto err_out;1.7.9.5
On 16/08/13 06:30, Mike Turquette wrote:
Quoting Zhangfei Gao (2013-08-15 21:03:28)
MUX could choose parent accordingly when set_rate. Add clk_rate_fops to debug whether mux choose correct parent
Example: cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000 sfc_mux 0 0 200000000 sfc 0 0 200000000
cat osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate 200000000
echo 24000000 > osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 sfc_mux 0 0 24000000 sfc 0 0 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000
cat osc24mhz/sfc_mux/sfc/clk_rate 24000000
Signed-off-by: Zhangfei Gao zhangfei.gao@linaro.org
NACK. This patch just makes it possible to call clk_set_rate on every clock from userspace. I don't support that operation, it just leads to tons of horrible hacks where closed userspace crapplications manage hardware directly.
Also I'm not sure what this patch has to do with the description in the changelog. How does setting the rate of every clock help debug mux clocks? You can already see a mux clocks parent and rate from the existing debugs info.
Hi Mike,
I can this patch having value for debugging/testing any set_rate code (e.g. a PLL), not just remuxing. I think it's much more convenient to be able to just echo a few different numbers (or even a whole range from a script) into a debugfs file and see the result than to have to put temporary code in the kernel for everything you want to test. If it results in better tested clock code it might not be so bad.
Regarding misuse, it is in debugfs and requires the common clock debugfs interface to be turned on. You could go one step further and make it read only by default (so one must also chmod it - assuming debugfs supports that), or #ifdef DEBUG it so that one must explicitly put a #define DEBUG in the file, and it'd still be useful (though perhaps less obvious unless you're already looking through the code). What do you think?
Cheers James
Quoting James Hogan (2013-08-16 01:26:38)
On 16/08/13 06:30, Mike Turquette wrote:
Quoting Zhangfei Gao (2013-08-15 21:03:28)
MUX could choose parent accordingly when set_rate. Add clk_rate_fops to debug whether mux choose correct parent
Example: cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000 sfc_mux 0 0 200000000 sfc 0 0 200000000
cat osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate 200000000
echo 24000000 > osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 sfc_mux 0 0 24000000 sfc 0 0 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000
cat osc24mhz/sfc_mux/sfc/clk_rate 24000000
Signed-off-by: Zhangfei Gao zhangfei.gao@linaro.org
NACK. This patch just makes it possible to call clk_set_rate on every clock from userspace. I don't support that operation, it just leads to tons of horrible hacks where closed userspace crapplications manage hardware directly.
Also I'm not sure what this patch has to do with the description in the changelog. How does setting the rate of every clock help debug mux clocks? You can already see a mux clocks parent and rate from the existing debugs info.
Hi Mike,
I can this patch having value for debugging/testing any set_rate code (e.g. a PLL), not just remuxing. I think it's much more convenient to be able to just echo a few different numbers (or even a whole range from a script) into a debugfs file and see the result than to have to put temporary code in the kernel for everything you want to test. If it results in better tested clock code it might not be so bad.
Regarding misuse, it is in debugfs and requires the common clock debugfs interface to be turned on.
Lots of Android smartphones ship with debugfs enabled and I'm sure that those phones will have the clk debug stuff enabled too.
You could go one step further and make it read only by default (so one must also chmod it - assuming debugfs supports that),
I still do not think this is a strong enough deterrent. Assuming what I said above is true about enabling clk debugfs options in production devices, all that this requires is root access on a device (which isn't so hard to get) and people can damage their device by running a PLL at 4GHz or something.
or #ifdef DEBUG it so that one must explicitly put a #define DEBUG in the file, and it'd still be useful (though perhaps less obvious unless you're already looking through the code). What do you think?
I'm OK with this. This basically means that to use this feature you need to recompile the kernel, which rules out most attack vectors. And if a mfg left this enabled in a production device then they deserve to have some ruined products.
Zhangfei,
Can you respin this patch and protect these new debugfs entries with an '#ifdef DEBUG'?
Thanks, Mike
Cheers James
On 13-08-16 01:30 PM, Mike Turquette wrote:
Quoting Zhangfei Gao (2013-08-15 21:03:28)
MUX could choose parent accordingly when set_rate. Add clk_rate_fops to debug whether mux choose correct parent
Example: cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000 sfc_mux 0 0 200000000 sfc 0 0 200000000
cat osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate 200000000
echo 24000000 > osc24mhz/bpll/bpll_fout3/sfc_mux/sfc/clk_rate cat clk_summary clock enable_cnt prepare_cnt rate
osc24mhz 3 3 24000000 sfc_mux 0 0 24000000 sfc 0 0 24000000 bpll 1 1 4294967295 bpll_fout3 0 0 200000000
cat osc24mhz/sfc_mux/sfc/clk_rate 24000000
Signed-off-by: Zhangfei Gao zhangfei.gao@linaro.org
NACK. This patch just makes it possible to call clk_set_rate on every clock from userspace. I don't support that operation, it just leads to tons of horrible hacks where closed userspace crapplications manage hardware directly.
Also I'm not sure what this patch has to do with the description in the changelog. How does setting the rate of every clock help debug mux clocks? You can already see a mux clocks parent and rate from the existing debugs info.
Regards, Mike
Thanks Mike for the education.
We use this to debug whether mux is really choose correct parent. And even dump the register to see whether config correct or not.
For example, By default, sfc is 200M, with mux's parent is 200M output bpll_fout3. Then we change sfc clock to 24M, and to see whether mux parent change to 24M output osc24mhz.
Otherwise, we have to debug mux choose correct parent from specific driver. There may other method.
Yes, your concern is right, it opens hacking.
Thanks.
linaro-kernel@lists.linaro.org
-
James Hogan -
Mike Turquette -
zhangfei -
Zhangfei Gao