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
| ||
|
Date: Thu, 12 Jan 2012 17:19:23 -0800 From: Saravana Kannan <skannan@...eaurora.org> To: Rob Herring <robherring2@...il.com> CC: "Turquette, Mike" <mturquette@...com>, andrew@...n.ch, linaro-dev@...ts.linaro.org, eric.miao@...aro.org, grant.likely@...retlab.ca, amit.kucheria@...aro.org, jeremy.kerr@...onical.com, linux@....linux.org.uk, sboyd@...cinc.com, magnus.damm@...il.com, dsaxena@...aro.org, linux-arm-kernel@...ts.infradead.org, arnd.bergmann@...aro.org, patches@...aro.org, Thomas Gleixner <tglx@...utronix.de>, linux-omap@...r.kernel.org, richard.zhao@...aro.org, shawn.guo@...escale.com, paul@...an.com, linus.walleij@...ricsson.com, broonie@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org, Colin Cross <ccross@...gle.com>, Richard Zhao <richard.zhao@...escale.com>, skannan@...cinc.com Subject: Re: [PATCH v4 3/6] clk: introduce the common clock framework On 01/12/2012 04:48 PM, Rob Herring wrote: > On 01/12/2012 06:04 PM, Saravana Kannan wrote: >> On 01/04/2012 08:07 PM, Turquette, Mike wrote: >>> On Wed, Jan 4, 2012 at 6:11 PM, Rob Herring<robherring2@...il.com> >>> wrote: >>>> On 01/04/2012 07:01 PM, Turquette, Mike wrote: >>>>> On Wed, Jan 4, 2012 at 6:32 AM, Rob Herring<robherring2@...il.com> >>>>> wrote: >>>>>> On 01/03/2012 08:15 PM, Richard Zhao wrote: >>>>>>> On Fri, Dec 16, 2011 at 04:45:48PM -0800, Turquette, Mike wrote: >>>>>>>> On Wed, Dec 14, 2011 at 5:18 AM, Thomas >>>>>>>> Gleixner<tglx@...utronix.de> wrote: >>>>>>>>> On Tue, 13 Dec 2011, Mike Turquette wrote: >>>>>> >>>>>> snip >>>>>> >>>>>>>>>> +/** >>>>>>>>>> + * clk_init - initialize the data structures in a struct clk >>>>>>>>>> + * @dev: device initializing this clk, placeholder for now >>>>>>>>>> + * @clk: clk being initialized >>>>>>>>>> + * >>>>>>>>>> + * Initializes the lists in struct clk, queries the hardware >>>>>>>>>> for the >>>>>>>>>> + * parent and rate and sets them both. Adds the clk to the >>>>>>>>>> sysfs tree >>>>>>>>>> + * topology. >>>>>>>>>> + * >>>>>>>>>> + * Caller must populate clk->name and clk->flags before calling >>>>>>>>> >>>>>>>>> I'm not too happy about this construct. That leaves struct clk >>>>>>>>> and its >>>>>>>>> members exposed to the world instead of making it a real opaque >>>>>>>>> cookie. I know from my own painful experience, that this will >>>>>>>>> lead to >>>>>>>>> random fiddling in that data structure in drivers and arch code >>>>>>>>> just >>>>>>>>> because the core code has a shortcoming. >>>>>>>>> >>>>>>>>> Why can't we make struct clk a real cookie and confine the data >>>>>>>>> structure to the core code ? >>>>>>>>> >>>>>>>>> That would change the init call to something like: >>>>>>>>> >>>>>>>>> struct clk *clk_init(struct device *dev, const struct clk_hw *hw, >>>>>>>>> struct clk *parent) >>>>>>>>> >>>>>>>>> And have: >>>>>>>>> struct clk_hw { >>>>>>>>> struct clk_hw_ops *ops; >>>>>>>>> const char *name; >>>>>>>>> unsigned long flags; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> Implementers can do: >>>>>>>>> struct my_clk_hw { >>>>>>>>> struct clk_hw hw; >>>>>>>>> mydata; >>>>>>>>> }; >>>>>>>>> >>>>>>>>> And then change the clk ops callbacks to take struct clk_hw * as an >>>>>>>>> argument. >>>>>>> We have to define static clocks before we adopt DT binding. >>>>>>> If clk is opaque and allocate memory in clk core, it'll make hard >>>>>>> to define static clocks. And register/init will pass a long parameter >>>>>>> list. >>>>>> >>>>>> DT is not a prerequisite for having dynamically created clocks. You >>>>>> can >>>>>> make clock init dynamic without DT. >>>>> >>>>> Agreed. >>>>> >>>>>> What data goes in struct clk vs. struct clk_hw could change over time. >>>>>> So perhaps we can start with some data in clk_hw and plan to move >>>>>> it to >>>>>> struct clk later. Even if almost everything ends up in clk_hw >>>>>> initially, >>>>>> at least the structure is in place to have common, core-only data >>>>>> separate from platform data. >>>>> >>>>> What is the point of this? >>>> >>>> To have a way forward. It would be nice to have a clk infrastructure >>>> before I retire... >>> >>> Haha, agreed. >>> >>>> >>>>> The original clk_hw was defined simply as: >>>>> >>>>> struct clk_hw { >>>>> struct clk *clk; >>>>> }; >>>>> >>>>> It's only purpose in life was as a handle for navigation between the >>>>> opaque struct clk and the hardware-specific struct my_clk_hw. struct >>>>> clk_hw is defined in clk.h and everyone can see it. If we're suddenly >>>>> OK putting clk data in this structure then why bother with an opaque >>>>> struct clk at all? >>>>> >>>>>> What is the actual data you need to be static and accessible to the >>>>>> platform code? A ptr to parent clk is the main thing I've seen for >>>>>> static initialization. So make the parent ptr be struct clk_hw* and >>>>>> allow the platforms to access. >>>>> >>>>> To answer your question on what data we're trying to expose: platform >>>>> code commonly needs the parent pointer and the clk rate (and by >>>>> extension, the rate of the parent). For debug/error prints it is also >>>>> nice to have the clk name. Generic clk flags are also conceivably >>>>> something that platform code might want. >>>> >>>> I agree with the need to have the parent and flags from a static init >>>> perspective. There's not really a good reason the others can't be >>>> accessed thru accessors though. >>>> >>>>> I'd like to spin the question around: if we're OK exposing some stuff >>>>> (in your example above, the parent pointer), then what clk data are >>>>> you trying to hide? >>>> >>>> Well, everything from drivers which the current clk implementations do >>>> hide. Catching abuse in with drivers coming in from all different trees >>>> and lists will be impossible. >>>> >>>> For platform code it is more fuzzy. I don't think platform code should >>>> be allowed to muck with prepare/enable counts for example. >>> >>> So there is a clear dichotomy: drivers shouldn't be exposed to any of >>> it and platform code should be exposed to some of it. >>> >>> How about a drivers/clk/clk-private.h which will define struct clk and >>> must only be included by clk drivers in drivers/clk/*? >>> >>> This establishes a bright line between those things which are allowed >>> to know the details of struct clk and those that are not: namely that >>> clk drivers in drivers/clk/ may use '#include "clk-private.h"'. >>> Obviously struct clk is opaque to the rest of the kernel (in the same >>> way it has been prior to the common clk patches) and there is no need >>> for struct clk_hw anymore. Also helper functions are no longer needed >>> for clock driver code, which I think *is* a manageable set of code to >>> review. Also clk drivers must live in drivers/clk/ for this to work >>> (without a big ugly path in someone's #include directive somewhere). >> >> While the original clk_hw suggestion was well intentioned, it just >> forces too many unnecessary dereferences and indirection. It also >> prevents static init of some fields as others have mentioned. Overall, >> it made the MSM clock code a mess when I tried to convert it to the >> common clock framework during Linaro Connect Q4 2011. >> >> The current off-tree MSM clock code uses a very similar approach to what >> the original patches that Jeremy sent out did. When Mike sent out the >> patches that removed clk_hw, the MSM code was much clearer and easier to >> convert to the common clock framework. >> >> The clk-private.h suggestion by Mike is reasonable seems like a good >> compromise. It support the idea of not letting the world peek into the >> clock struct (I want this too) while letting the clock driver use the >> struct without jumping through hoops. It has my vote (not the whole >> patch series, but the idea of having clk driver/framework specific stuff >> in clk-private.h). I really hope we move ahead with this. >> > > I'm fine with this approach. We're certainly no worse off as platforms > today have full access. However, it not me that has to be convinced. Sorry for not being clear. My previous mail was a general comment to the community and not directed specifically at you Rob. > My suggestion was to build into the data structures at least the option > to have core only and core+platform data. Maybe the core only part is > mostly empty at first. This at least shows some intent to hide some of > the data. Which fields give you pain not having access to them? So far > this mainly seems to be parent and rate. It's been a while, but if I'm not mistaken, it was messy to statically initialize the parent field and to tie up the clock specific struct (say, clk_fixed) to the generic clock struct (clk) without having to define each of them separately. With the clk-private.h approach, I could do something like this: static struct fixed_clk cxo_clk = { .reg = 0x12345678 .c = { .dbg_name = "cxo_clk", .ops = &clk_ops_cxo, CLK_INIT(cxo_clk.c), }, } And without it, it would look something like: struct clk _cxo_clk; static struct msm_fixed_clk cxo_clk = { .reg = 0x12345678 .c.clk = &_cxo_clk, }; struct clk _cxo_clk = { .name = "cxo_clk", .ops = &clk_ops_cxo, .hw = &cxo_clk.c }; As you can see, I now have to give a name for a struct that I don't really care about after init and pollute the name space. It's also clumsy since both the structs try to reference each other and I have to use forward declaration for every single clock I try to statically initialize. I will also need to do several pointer derefs instead of using container_of(), etc which is more efficient and less confusing than multiple levels of pointer deref. Thanks, Saravana -- Sent by an employee of the Qualcomm Innovation Center, Inc. The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum. -- 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