| 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: <4B99C0BB.7080900@st.com>
Date: Fri, 12 Mar 2010 09:49:07 +0530
From: Viresh KUMAR <viresh.kumar@...com>
To: Linus Walleij <linus.ml.walleij@...il.com>
Cc: linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
armando.visconti@...com, amit.goel@...com, shiraz.hashim@...com,
vipin.kumar@...com, rajeev-dlh.kumar@...com, deepak.sikri@...com,
ashish.priyadarshi@...com
Subject: Re: [PATCH 05/11] ST SPEAr: Added clock framework for SPEAr platform
and machines
On 3/11/2010 12:30 PM, Linus Walleij wrote:
> 2010/3/3 Viresh KUMAR <viresh.kumar@...com>:
>
>> +{
>> + unsigned int val;
>> +
>> + if (!clk->en_reg)
>> + return -EFAULT;
>> +
>> + val = readl(clk->en_reg);
>> + if (unlikely(clk->flags & RESET_TO_ENABLE))
>> + val &= ~(1 << clk->en_reg_bit);
>> + else
>> + val |= 1 << clk->en_reg_bit;
>> + writel(val, clk->en_reg);
>
> I don't understand one bit of this. What happens if the RESET_TO_ENABLE
> flag is set for the clock? The exact same bit is &-masked and then
> immediately |:ed to 1 again. Then it is written to the register. Practical
> effect: absolutely none.
>
> Is there a writel(val, clk->en_reg); missing from the unlikely() execution
> path?
Already explained by shiraz.
>
>> +
>> + return 0;
>> +}
>> +
>> +static void generic_clk_disable(struct clk *clk)
>> +{
>> + unsigned int val;
>> +
>> + if (!clk->en_reg)
>> + return;
>> +
>> + val = readl(clk->en_reg);
>> + if (unlikely(clk->flags & RESET_TO_ENABLE))
>> + val |= 1 << clk->en_reg_bit;
>> + else
>> + val &= ~(1 << clk->en_reg_bit);
>
> Same issue here...
>
>> +
>> + writel(val, clk->en_reg);
>> +}
>> +
>> +/* generic clk ops */
>> +static struct clkops generic_clkops = {
>> + .enable = generic_clk_enable,
>> + .disable = generic_clk_disable,
>> +};
>> +
>> +/*
>> + * clk_enable - inform the system when the clock source should be running.
>> + * @clk: clock source
>> + *
>> + * If the clock can not be enabled/disabled, this should return success.
>> + *
>> + * Returns success (0) or negative errno.
>> + */
>> +int clk_enable(struct clk *clk)
>> +{
>> + unsigned long flags;
>> + int ret = 0;
>> +
>> + if (!clk || IS_ERR(clk))
>> + return -EFAULT;
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (clk->usage_count++ == 0) {
>
> Isnt if (++clk.>usage_count == 1) easier to understand, or is it just me?
> BTW doing this:
> clk->usage_count++;
> if (clk->usage_count == 1)
> will not use more memory, the compiler optimize this, so choose the
> version you think is most readable. If you think this is very readable, by
> all means keep it.
>
I will simplify it, to make it more readable.
>> + if (clk->ops && clk->ops->enable)
>> + ret = clk->ops->enable(clk);
>> + }
>> + spin_unlock_irqrestore(&clocks_lock, flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL(clk_enable);
>> +
>> +/*
>> + * clk_disable - inform the system when the clock source is no longer required.
>> + * @clk: clock source
>> + *
>> + * Inform the system that a clock source is no longer required by
>> + * a driver and may be shut down.
>> + *
>> + * Implementation detail: if the clock source is shared between
>> + * multiple drivers, clk_enable() calls must be balanced by the
>> + * same number of clk_disable() calls for the clock source to be
>> + * disabled.
>> + */
>> +void clk_disable(struct clk *clk)
>> +{
>> + unsigned long flags;
>> +
>> + if (!clk || IS_ERR(clk))
>> + return;
>> +
>> + WARN_ON(clk->usage_count == 0);
>> +
>> + spin_lock_irqsave(&clocks_lock, flags);
>> + if (--clk->usage_count == 0) {
>
> Same readability issue here.
I will simplify it, to make it more readable.
>
>> + if (!clk || IS_ERR(clk) || !parent || IS_ERR(parent))
>> + return -EFAULT;
>> + if (clk->usage_count == 0)
>> + return -EBUSY;
>
> Why will the clk_set_parent() call fail if there are *no* users of the clock?
> Should it not be the other way around? Or what am I misunderstanding here?
>
My mistake. should be !=.
>> + if (!clk->pclk_sel)
>> + return -EPERM;
>> + if (clk->pclk == parent)
>> + return 0;
>> +
>> + for (i = 0; i < clk->pclk_sel->pclk_count; i++) {
>> + if (clk->pclk_sel->pclk_info[i].pclk == parent) {
>> + found = 1;
>> + break;
>> + }
>> + }
>> +
>> + if (!found)
>> + return -EPERM;
>
> What about -ENODEV or so? (I don't know what people typically
> use for clocks that don't exist.)
Will correct it to return correct error.
>
>> + */
>> +void recalc_root_clocks(void)
>> +{
>> + propagate_rate(&root_clks);
>> +}
>
> I understand (I think) how speed change can propagate through the clocks.
> However I think it will be hard to notify users that the clock rate has changed,
> because there is nothing in the clk framework that supports that. If you have
> drivers with dynamic clocks, do you have any plans on how you will
> notify drivers?
>
> OMAP uses CPUfreq but that is really about the CPU. As it happens, all
> their clk:s always change frequency at the same operating points as the
> CPU. So they can have pre/post calls from CPUfreq in their code, but
> this will not work with things like PrimeCells where other users of the cell
> may not have operating points correlated with CPU operating points.
>
> (I'm not requesting you to solve this problem, more to be aware of it.)
>
already answered by shiraz.
>> diff --git a/arch/arm/plat-spear/include/plat/clkdev.h b/arch/arm/plat-spear/include/plat/clkdev.h
>> +/* clk values */
>> +#define KHZ (1000)
>> +#define MHZ (1000000)
>
> This looks far to generic to be hidden in some weird special architecture.
> And I *think* the preferred way to encode frequencies in the kernel is raw
> Hertz measure with all the extra zeroes.
>
> Doing
> .foo = MHZ *48;
>
> Is a bit awkward, don't you think it's better to do:
> #define MHZ(f) f * 1000000
> .foo = MHZ(48);
>
> If you absolutely want to do this, I would suggest to add the KHZ and MHZ
> macros to some global kernel file but I honestly cannot say which one.
> Perhaps inlcude/linux/clk.h?
>
I will better remove them.
regards,
viresh kumar.
--
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