Patchwork [1/2] powerpc: export ppc_tb_freq so that modules can reference it

login
register
mail settings
Submitter Timur Tabi
Date Sept. 17, 2010, 10:53 p.m.
Message ID <1284764008-19469-1-git-send-email-timur@freescale.com>
Download mbox | patch
Permalink /patch/65112/
State Superseded
Headers show

Comments

Timur Tabi - Sept. 17, 2010, 10:53 p.m.
Export the global variable 'ppc_tb_freq', so that modules (like the Book-E
watchdog driver) can use it.

Signed-off-by: Timur Tabi <timur@freescale.com>
---

This export is necessary for the Book-E watchdog driver to be compiled as a
module.  Since ppc_proc_freq is already exported, I figured it's okay for
ppc_tb_freq to be exported as well.

 arch/powerpc/kernel/time.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Josh Boyer - Sept. 18, 2010, 12:38 a.m.
On Fri, Sep 17, 2010 at 6:53 PM, Timur Tabi <timur@freescale.com> wrote:
> Export the global variable 'ppc_tb_freq', so that modules (like the Book-E
> watchdog driver) can use it.
>
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>
> This export is necessary for the Book-E watchdog driver to be compiled as a
> module.  Since ppc_proc_freq is already exported, I figured it's okay for
> ppc_tb_freq to be exported as well.
>
>  arch/powerpc/kernel/time.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
> index 8533b3b..49aa130 100644
> --- a/arch/powerpc/kernel/time.c
> +++ b/arch/powerpc/kernel/time.c
> @@ -163,6 +163,7 @@ static long timezone_offset;
>  unsigned long ppc_proc_freq;
>  EXPORT_SYMBOL(ppc_proc_freq);
>  unsigned long ppc_tb_freq;
> +EXPORT_SYMBOL(ppc_tb_freq);

EXPORT_SYMBOL_GPL probably, no?

josh
Timur Tabi - Sept. 18, 2010, 1:20 a.m.
On Fri, Sep 17, 2010 at 7:38 PM, Josh Boyer <jwboyer@gmail.com> wrote:

>>  unsigned long ppc_proc_freq;
>>  EXPORT_SYMBOL(ppc_proc_freq);
>>  unsigned long ppc_tb_freq;
>> +EXPORT_SYMBOL(ppc_tb_freq);
>
> EXPORT_SYMBOL_GPL probably, no?

I don't see any reason to limit it to GPL drivers.  Not only that, but
then we'll have this:

EXPORT_SYMBOL(ppc_proc_freq);
EXPORT_SYMBOL_GPL(ppc_tb_freq);

That just looks dumb.
Benjamin Herrenschmidt - Sept. 18, 2010, 3:14 a.m.
On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
> I don't see any reason to limit it to GPL drivers.  Not only that, but
> then we'll have this:

I do

> EXPORT_SYMBOL(ppc_proc_freq);
> EXPORT_SYMBOL_GPL(ppc_tb_freq);
> 
> That just looks dumb. 

Right, so send a patch to fix the first one too :-)

Cheers,
Ben.
Tabi Timur-B04825 - Sept. 18, 2010, 2:36 p.m.
On Sep 17, 2010, at 10:14 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:

> On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
>> I don't see any reason to limit it to GPL drivers.  Not only that, but
>> then we'll have this:
> 
> I do

Can you elaborate on that, or are you just going to pull rank on me?

> 
>> EXPORT_SYMBOL(ppc_proc_freq);
>> EXPORT_SYMBOL_GPL(ppc_tb_freq);
>> 
>> That just looks dumb. 
> 
> Right, so send a patch to fix the first one too :-)

Then why doesn't someone post a patch to change all EXPORT_SYMBOL to EXPORT_SYMBOL_GPL?  And why do we consider EXPORT_SYMBOL to be "broken"?

I'm not trying to be a troll, but I see a lot of inconsistency with respect to  EXPORT_SYMBOL and EXPORT_SYMBOL_GPL.  
>
Kumar Gala - Sept. 18, 2010, 3:34 p.m.
On Sep 18, 2010, at 9:36 AM, Tabi Timur-B04825 wrote:

> On Sep 17, 2010, at 10:14 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
> 
>> On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
>>> I don't see any reason to limit it to GPL drivers.  Not only that, but
>>> then we'll have this:
>> 
>> I do
> 
> Can you elaborate on that, or are you just going to pull rank on me?
> 
>> 
>>> EXPORT_SYMBOL(ppc_proc_freq);
>>> EXPORT_SYMBOL_GPL(ppc_tb_freq);
>>> 
>>> That just looks dumb. 
>> 
>> Right, so send a patch to fix the first one too :-)

I don't think either of these should be EXPORT_SYMBOL_GPL.  Why shouldn't a binary module be allowed to know these frequencies?  My view is why preclude anyone from using this how they want.  If they want to live in the gray area so be it.  Who am I to say they shouldn't have that choice.

> Then why doesn't someone post a patch to change all EXPORT_SYMBOL to EXPORT_SYMBOL_GPL?  And why do we consider EXPORT_SYMBOL to be "broken"?
> 
> I'm not trying to be a troll, but I see a lot of inconsistency with respect to  EXPORT_SYMBOL and EXPORT_SYMBOL_GPL.  

I'm in agreement with the lack of clarity, it seems to be developer whim.

- k
Vitaly Wool - Sept. 18, 2010, 3:52 p.m.
On Sat, Sep 18, 2010 at 5:34 PM, Kumar Gala <kumar.gala@freescale.com> wrote:

> I don't think either of these should be EXPORT_SYMBOL_GPL.  Why shouldn't a binary module be allowed to know these frequencies?  My view is why preclude anyone from using this how they want.  If they want to live in the gray area so be it.  Who am I to say they shouldn't have that choice.

W00t, binary modules again... No, please. No binary modules in kernelspace.

Thanks,
   Vitaly
Josh Boyer - Sept. 18, 2010, 4:56 p.m.
On Sat, Sep 18, 2010 at 11:34 AM, Kumar Gala <kumar.gala@freescale.com> wrote:
>
> On Sep 18, 2010, at 9:36 AM, Tabi Timur-B04825 wrote:
>
>> On Sep 17, 2010, at 10:14 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
>>
>>> On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
>>>> I don't see any reason to limit it to GPL drivers.  Not only that, but
>>>> then we'll have this:
>>>
>>> I do
>>
>> Can you elaborate on that, or are you just going to pull rank on me?
>>
>>>
>>>> EXPORT_SYMBOL(ppc_proc_freq);
>>>> EXPORT_SYMBOL_GPL(ppc_tb_freq);
>>>>
>>>> That just looks dumb.
>>>
>>> Right, so send a patch to fix the first one too :-)
>
> I don't think either of these should be EXPORT_SYMBOL_GPL.  Why shouldn't a binary module be allowed to know these frequencies?  My view is why preclude anyone from using this how they want.  If they want to live in the gray area so be it.  Who am I to say they shouldn't have that choice.
>

It is not, in my opinion, about what is technically possible and what
isn't.  The kernel is licensed under the GPL.  This is a Linux kernel
only symbol.  One would be hard pressed to claim they have a driver
that wasn't written for Linux that happens to need that symbol.  As a
member of the Linux kernel community, I find it important to encourage
the contribution of code back to the kernel, and this is one way to
help that.  This isn't BSD.

Besides, a developer is free to export it however they wish in their
own kernel tree.  They can deviate from mainline if they so choose.

josh
Tabi Timur-B04825 - Sept. 18, 2010, 5:36 p.m.
Josh Boyer wrote:
> It is not, in my opinion, about what is technically possible and what
> isn't.  The kernel is licensed under the GPL.  This is a Linux kernel
> only symbol.  One would be hard pressed to claim they have a driver
> that wasn't written for Linux that happens to need that symbol.  As a
> member of the Linux kernel community, I find it important to encourage
> the contribution of code back to the kernel, and this is one way to
> help that.  This isn't BSD.

Fine, but this goes back to my original question -- if this is how the 
community feels, then why hasn't someone posted a patch that converts all 
EXPORT_SYMBOL into EXPORT_SYMBOL_GPL?

Either we allow non-GPL drivers, or we don't.  If we don't, then we need to 
eliminate EXPORT_SYMBOL once and for all.  Otherwise, the message is 
hypocritical.
Josh Boyer - Sept. 18, 2010, 5:46 p.m.
On Sat, Sep 18, 2010 at 1:36 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> Josh Boyer wrote:
>> It is not, in my opinion, about what is technically possible and what
>> isn't.  The kernel is licensed under the GPL.  This is a Linux kernel
>> only symbol.  One would be hard pressed to claim they have a driver
>> that wasn't written for Linux that happens to need that symbol.  As a
>> member of the Linux kernel community, I find it important to encourage
>> the contribution of code back to the kernel, and this is one way to
>> help that.  This isn't BSD.
>
> Fine, but this goes back to my original question -- if this is how the
> community feels, then why hasn't someone posted a patch that converts all
> EXPORT_SYMBOL into EXPORT_SYMBOL_GPL?

Because of history in a lot of cases, and like all communities,
opinions vary.  I did say this was my opinion, not a mandate of some
sort.

> Either we allow non-GPL drivers, or we don't.  If we don't, then we need to
> eliminate EXPORT_SYMBOL once and for all.  Otherwise, the message is
> hypocritical.

I'd be all for it.  I don't think it is as black and white as that
though, as nothing rarely is.  (we can't even get all the code to
adhere to the < 80 column "rule" ).  I also don't think it is
necessarily hypocritical.  This is a new symbol being exported, not
one that has been exported for years.

josh
Tabi Timur-B04825 - Sept. 18, 2010, 5:55 p.m.
Josh Boyer wrote:
> This is a new symbol being exported, not
> one that has been exported for years.

Except that Ben says that I should change ppc_proc_freq from EXPORT_SYMBOL to 
EXPORT_SYMBOL_GPL as well.  In a sense, we're in a catch-22.  We have three 
choices:

1. We *arbitrarily* change ppc_proc_freq from EXPORT_SYMBOL to 
EXPORT_SYMBOL_GPL, so that the two symbols are exported the same way

2. We GPL-export only ppc_tb_freq and leave ppc_proc_freq as-is, but then it 
looks dumb.

3. We export ppc_tb_freq the same way we're exporting ppc_proc_freq, 
providing the most options and maintaining consistency.

I just don't see how options #1 or #2 are better than #3, and so far the only 
explanations I've heard are along the lines of "we just like it that way".

Obviously, Linus thinks it's okay to allow some non-GPL modules, otherwise he 
would have long ago changed all EXPORT_SYMBOLs to EXPORT_SYMBOL_GPL.  I'm 
just capitalizing on that mindset.
Josh Boyer - Sept. 18, 2010, 6:22 p.m.
On Sat, Sep 18, 2010 at 1:55 PM, Tabi Timur-B04825 <B04825@freescale.com> wrote:
> Josh Boyer wrote:
>> This is a new symbol being exported, not
>> one that has been exported for years.
>
> Except that Ben says that I should change ppc_proc_freq from EXPORT_SYMBOL
> to
> EXPORT_SYMBOL_GPL as well.  In a sense, we're in a catch-22.  We have three
> choices:
>
> 1. We *arbitrarily* change ppc_proc_freq from EXPORT_SYMBOL to
> EXPORT_SYMBOL_GPL, so that the two symbols are exported the same way
>
> 2. We GPL-export only ppc_tb_freq and leave ppc_proc_freq as-is, but then it
> looks dumb.

I dunno.  I don't think it looks dumb.  It could mean nothing more
than we were paying closer attention this time.

> 3. We export ppc_tb_freq the same way we're exporting ppc_proc_freq,
> providing the most options and maintaining consistency.
>
> I just don't see how options #1 or #2 are better than #3, and so far the
> only
> explanations I've heard are along the lines of "we just like it that way".

Now I think I've been a bit more detailed than that.  I at least
explained why I prefer it that way.  If you disagree, that's fine but
don't make me sound like some kind of petulant child.

> Obviously, Linus thinks it's okay to allow some non-GPL modules, otherwise
> he
> would have long ago changed all EXPORT_SYMBOLs to EXPORT_SYMBOL_GPL.  I'm
> just capitalizing on that mindset.

Capitalizing?  The patch you posted that uses this symbol is for a GPL
driver so you gain or lose nothing by having this symbol be
EXPORT_SYMBOL_GPL.  Are you somehow advocating and getting some sort
of gain by allowing non-GPL modules?  If so, I find that unfortunate.
If not, then I guess I don't understand what you mean by capitalizing.

josh
Kumar Gala - Sept. 18, 2010, 6:36 p.m.
On Sep 18, 2010, at 11:56 AM, Josh Boyer wrote:

> On Sat, Sep 18, 2010 at 11:34 AM, Kumar Gala <kumar.gala@freescale.com> wrote:
>> 
>> On Sep 18, 2010, at 9:36 AM, Tabi Timur-B04825 wrote:
>> 
>>> On Sep 17, 2010, at 10:14 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
>>> 
>>>> On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
>>>>> I don't see any reason to limit it to GPL drivers.  Not only that, but
>>>>> then we'll have this:
>>>> 
>>>> I do
>>> 
>>> Can you elaborate on that, or are you just going to pull rank on me?
>>> 
>>>> 
>>>>> EXPORT_SYMBOL(ppc_proc_freq);
>>>>> EXPORT_SYMBOL_GPL(ppc_tb_freq);
>>>>> 
>>>>> That just looks dumb.
>>>> 
>>>> Right, so send a patch to fix the first one too :-)
>> 
>> I don't think either of these should be EXPORT_SYMBOL_GPL.  Why shouldn't a binary module be allowed to know these frequencies?  My view is why preclude anyone from using this how they want.  If they want to live in the gray area so be it.  Who am I to say they shouldn't have that choice.
>> 
> 
> It is not, in my opinion, about what is technically possible and what
> isn't.  The kernel is licensed under the GPL.  This is a Linux kernel
> only symbol.  One would be hard pressed to claim they have a driver
> that wasn't written for Linux that happens to need that symbol.  As a
> member of the Linux kernel community, I find it important to encourage
> the contribution of code back to the kernel, and this is one way to
> help that.  This isn't BSD.
> 
> Besides, a developer is free to export it however they wish in their
> own kernel tree.  They can deviate from mainline if they so choose.

I'll buy this argument as a reason to make both EXPORT_SYMBOL_GPL.

- k
Benjamin Herrenschmidt - Sept. 19, 2010, 2:42 a.m.
On Sat, 2010-09-18 at 10:34 -0500, Kumar Gala wrote:
> On Sep 18, 2010, at 9:36 AM, Tabi Timur-B04825 wrote:
> 
> > On Sep 17, 2010, at 10:14 PM, "Benjamin Herrenschmidt" <benh@kernel.crashing.org> wrote:
> > 
> >> On Fri, 2010-09-17 at 20:20 -0500, Timur Tabi wrote:
> >>> I don't see any reason to limit it to GPL drivers.  Not only that, but
> >>> then we'll have this:
> >> 
> >> I do
> > 
> > Can you elaborate on that, or are you just going to pull rank on me?
> > 
> >> 
> >>> EXPORT_SYMBOL(ppc_proc_freq);
> >>> EXPORT_SYMBOL_GPL(ppc_tb_freq);
> >>> 
> >>> That just looks dumb. 
> >> 
> >> Right, so send a patch to fix the first one too :-)
> 
> I don't think either of these should be EXPORT_SYMBOL_GPL.  Why
> shouldn't a binary module be allowed to know these frequencies?  My
> view is why preclude anyone from using this how they want.  If they
> want to live in the gray area so be it.  Who am I to say they
> shouldn't have that choice.

Well, I'm all for making binary modules life as hard as possible just
for the sake of it :-)

Cheers,
Ben.
Scott Wood - Sept. 20, 2010, 6:51 p.m.
On Sat, 18 Sep 2010 14:22:12 -0400
Josh Boyer <jwboyer@gmail.com> wrote:

> Capitalizing?  The patch you posted that uses this symbol is for a GPL
> driver so you gain or lose nothing by having this symbol be
> EXPORT_SYMBOL_GPL.  Are you somehow advocating and getting some sort
> of gain by allowing non-GPL modules?  If so, I find that unfortunate.
> If not, then I guess I don't understand what you mean by capitalizing.

One can dislike DRM (even a very weak form such as this) without having
a particular desire to go outside the bounds of what it allows.

I thought EXPORT_SYMBOL_GPL was originally meant to indicate the
symbols whose use is likely to be indicitave of code that is, in some
copyright-meaningful way, derived from GPL code?  I have a hard time
seeing that being the case here.  If every symbol is made GPL-only,
then that just gives the binary-only people[1] more incentive to
circumvent the entire mechanism.  It loses its meaning.

Documentation/DocBook/kernel-hacking.tmpl says, "It implies that the
function is considered an internal implementation issue, and not really
an interface."

-Scott

[1] Plus anyone who might want to make a kernel module out of code
which is open source, but not under a license the GPLv2 is compatible
with.
Detlev Zundel - Sept. 21, 2010, 12:34 p.m.
Hi Scott,

> On Sat, 18 Sep 2010 14:22:12 -0400
> Josh Boyer <jwboyer@gmail.com> wrote:
>
>> Capitalizing?  The patch you posted that uses this symbol is for a GPL
>> driver so you gain or lose nothing by having this symbol be
>> EXPORT_SYMBOL_GPL.  Are you somehow advocating and getting some sort
>> of gain by allowing non-GPL modules?  If so, I find that unfortunate.
>> If not, then I guess I don't understand what you mean by capitalizing.
>
> One can dislike DRM (even a very weak form such as this) without having
> a particular desire to go outside the bounds of what it allows.
>
> I thought EXPORT_SYMBOL_GPL was originally meant to indicate the
> symbols whose use is likely to be indicitave of code that is, in some
> copyright-meaningful way, derived from GPL code?  

Google finds this, which coincides with what I remmber[1]:

  EXPORT_SYMBOL_GPL 
  
  Some kernel developers are unhappy with providing external interfaces to
  their code, only to see those interfaces being used by binary only
  modules. They view it as their work being appropriated. Whether you
  agree with that view or not is completely irrelevant, the person who
  owns the copyright decides how their work can be used.
  
  EXPORT_SYMBOL_GPL() allows for new interfaces to be marked as only
  available to modules with a GPL compatible license. This is independent
  of the kernel tainting, but obviously takes advantage of
  MODULE_LICENSE() strings.
  
  EXPORT_SYMBOL_GPL() may only be used for new exported symbols, Linus has
  spoken. I believe the phrase involved killer penguins with chainsaws for
  anybody who changed existing exported interfaces.

Cheers
  Detlev

[1] http://lkml.indiana.edu/hypermail/linux/kernel/0110.2/0369.html
  
--
DENX Software Engineering GmbH,      MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich,  Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-40 Fax: (+49)-8142-66989-80 Email: dzu@denx.de

Patch

diff --git a/arch/powerpc/kernel/time.c b/arch/powerpc/kernel/time.c
index 8533b3b..49aa130 100644
--- a/arch/powerpc/kernel/time.c
+++ b/arch/powerpc/kernel/time.c
@@ -163,6 +163,7 @@  static long timezone_offset;
 unsigned long ppc_proc_freq;
 EXPORT_SYMBOL(ppc_proc_freq);
 unsigned long ppc_tb_freq;
+EXPORT_SYMBOL(ppc_tb_freq);
 
 static DEFINE_PER_CPU(u64, last_jiffy);