Re: [PATCH 2/2] clk: Move init fields from clk to clk_hw

[Saravana Kannan]

On 04/11/2012 01:17 PM, Turquette, Mike wrote:
> On Wed, Apr 11, 2012 at 12:57 PM, Saravana Kannan
> <skannan@xxxxxxxxxxxxxx>  wrote:
> > On 04/11/2012 10:59 AM, Turquette, Mike wrote:
> > > On Thu, Apr 5, 2012 at 6:30 PM, Saravana Kannan<skannan@xxxxxxxxxxxxxx>
> > > My only concern with this series is that platform clock code will
> > > access struct clk_hw members without the appropriate lock held (namely
> > > prepare_lock).  I'm a bit worried that there might be a case where
> > > some clock code access clk_hw->name when clk_hw has somehow been
> > > destroyed/altered (e.g. if clk_put every finally gets implemented...).
> > >   Besides clk_put are there any real instances where this might happen?
> >
> > This problem is true for any struct/field marked as __init_data. So, I don't
> > think this is a real problem. If someone is stupid enough to mark their data
> > as __init_data and access them later, then there is not much we can do.
> > Also, I believe the compiler throws out some warning when you try to access
> > __init_data from non-init code.
>
> I'm referring to platform clock code accessing clk_hw->name or
> clk_hw->parent_names AFTER the core code has copied the initialization
> data.  So this isn't an __initdata problem as much as it is a design
> issue with exposing some members of struct clk_hw.  I think that this
> is basically OK since the benefits of not having to use so many
> helpers functions in platform clock code is obvious and if a platform
> gets bitten by this then they will learn the hard way to honor the
> locking semantics outlined in Documentation/clk.txt

That's what I meant too. The platform code and the static init data will 
most likely be written by the same dev. So, hopefully they know what 
they are doing.

> > > I'll be pushing my fixes branch out to the list soon.  Do you want to
> > > rebase this change on top of it taking into account the __initdata
> > > bits?
> >
> > I thought it might be easier for you to base your changes on top of my
> > patch. But I can try to rebase mine on top of your changes. Hopefully your
> > fixes aren't crazy big/complex.
>
> I don't think the fixes are a big deal, especially in the core code.
>
> > This is a busy week for me at work. I will try to send a patch in a day or
> > two.
>
> OK.  I might take a swing at it myself if I have time.

I still need to make the changes to add the copying part. So, don't 
bother with taking a swing at it. If you want to help out, help with 
rebasing it on top of your fixes would be appreciated. But based on what 
you said, it should be hard.

TLDR: Just wait for me to send out the patches. :)

-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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/