Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

> On (04/19/15 14:09), David Miller wrote:
>
> > On (04/18/15 21:23), Guenter Roeck wrote:
> >> lib/built-in.o:(.discard+0x1): multiple definition of
> >> `__pcpu_unique_iommu_pool_hash'
> >> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
> >> .. I get a similar failure in the
> >> powerpc:allmodconfig build
    :
> 
> Maybe ping the powerpc folks becuase if they can do a quick
> conversion, this change isn't necessary.

linuxppc-dev,

The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
is in reference to the issue that Guenter Roeck
identified. The problem is that we have a 
  static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
in both lib/iommu-common.c as well as arch/powerpc/kernel/iommu.c,
and as Guenter correctly pointed out,

" DEFINE_PER_CPU translates to DEFINE_PER_CPU_SECTION, which in turn is
  defined as

  #define DEFINE_PER_CPU_SECTION(type, name, sec)                         \
        __PCPU_DUMMY_ATTRS char __pcpu_scope_##name;                    \
	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
  -->	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak                 \
					        __typeof__(type) name

  if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
  The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
  global variable"

This results in a build error for allmodconfig on powerpc, due to 
a multiple definition of __pcpu_unique_iommu_pool_hash.

Given that the goal is to use the functions in lib/iommu-common.c
as the Generic IOMMU pooled allocator that will be a superset of
functionality in arch/powerpc/kernel/iommu.c, DaveM suggested that I 
ping you to see if you can do a quick conversion to handle this
duplicate defintion, rather than rename iommu_pool_hash in iommu-common.c.

Would that be possible?

--Sowmini

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Sun, 2015-04-19 at 14:36 -0400, Sowmini Varadhan wrote:
> > On (04/19/15 14:09), David Miller wrote:
> >
> > > On (04/18/15 21:23), Guenter Roeck wrote:
> > >> lib/built-in.o:(.discard+0x1): multiple definition of
> > >> `__pcpu_unique_iommu_pool_hash'
> > >> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
> > >> .. I get a similar failure in the
> > >> powerpc:allmodconfig build
>     :
> > 
> > Maybe ping the powerpc folks becuase if they can do a quick
> > conversion, this change isn't necessary.
> 
> linuxppc-dev,
> 
> The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
> is in reference to the issue that Guenter Roeck
> identified. The problem is that we have a 
>   static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

It's static ..

>   if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
>   The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
>   global variable"

OK, so why doesn't CONFIG_DEBUG_FORCE_WEAK_PER_CPU depend on s390 and/or alpha?

Someone needs to be doing s390/alpha builds with that enabled anyway, because
otherwise a clash between generic code and s390/alpha won't be caught.

Or if that's too hard we can rename the powerpc version, but it seems silly to
rename a powerpc variable to deal with a debug option that is only useful for
s390/alpha.

cheers

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

Hi Michael,

On 04/19/2015 08:01 PM, Michael Ellerman wrote:
> On Sun, 2015-04-19 at 14:36 -0400, Sowmini Varadhan wrote:
>>> On (04/19/15 14:09), David Miller wrote:
>>>
>>>> On (04/18/15 21:23), Guenter Roeck wrote:
>>>>> lib/built-in.o:(.discard+0x1): multiple definition of
>>>>> `__pcpu_unique_iommu_pool_hash'
>>>>> arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here
>>>>> .. I get a similar failure in the
>>>>> powerpc:allmodconfig build
>>      :
>>>
>>> Maybe ping the powerpc folks becuase if they can do a quick
>>> conversion, this change isn't necessary.
>>
>> linuxppc-dev,
>>
>> The disussion above [http://www.spinics.net/lists/sparclinux/msg13835.html]
>> is in reference to the issue that Guenter Roeck
>> identified. The problem is that we have a
>>    static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
>
> It's static ..
>
Not if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured.

>>    if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
>>    The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
>>    global variable"
>
> OK, so why doesn't CONFIG_DEBUG_FORCE_WEAK_PER_CPU depend on s390 and/or alpha?
>
The idea is to ensure that per cpu variable names are unique, even if static,
because that is what is needed for s390 and alpha.

> Someone needs to be doing s390/alpha builds with that enabled anyway, because
> otherwise a clash between generic code and s390/alpha won't be caught.
>
> Or if that's too hard we can rename the powerpc version, but it seems silly to
> rename a powerpc variable to deal with a debug option that is only useful for
> s390/alpha.
>

The debug option is intended for all _other_ architectures, to ensure that
changes made for those don't break alpha/s390 builds. alpha/s390 have
ARCH_NEEDS_WEAK_PER_CPU and don't need the debug option.

Sowmini's patch would change the variable name in the lib/ code. But that was
not the question here. The question was if the powerpc code could be changed
to use the generic iommu code instead of using the powerpc specific code.

Thanks,
Guenter

Re: Fwd: Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Sun, 2015-04-19 at 22:17 -0700, Guenter Roeck wrote:
> Hi Michael,

Hi Guenter,

> On 04/19/2015 08:01 PM, Michael Ellerman wrote:
> 
> > Someone needs to be doing s390/alpha builds with that enabled anyway, because
> > otherwise a clash between generic code and s390/alpha won't be caught.
> >
> > Or if that's too hard we can rename the powerpc version, but it seems silly to
> > rename a powerpc variable to deal with a debug option that is only useful for
> > s390/alpha.
> 
> The debug option is intended for all _other_ architectures, to ensure that
> changes made for those don't break alpha/s390 builds. alpha/s390 have
> ARCH_NEEDS_WEAK_PER_CPU and don't need the debug option.

But that can't actually work. If you introduce a percpu variable in generic
code that has the same name as a variable in the s390/alpha code, the only way
you can detect that is by building for s390/alpha.

Sure it might catch two variables in generic code that have the same name, but
that will get caught as soon as someone does a s390/alpha build anyway - which
is at least every night for linux-next.

More likely it catches cases like this, which is variables in code that will
never build for s390/alpha. So it just seems like a PITA to me.

> Sowmini's patch would change the variable name in the lib/ code. But that was
> not the question here. The question was if the powerpc code could be changed
> to use the generic iommu code instead of using the powerpc specific code.

Not for this merge window sorry, we need to test it properly.

cheers

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Sun, 19 Apr 2015 22:17:21 -0700

> The debug option is intended for all _other_ architectures, to
> ensure that changes made for those don't break alpha/s390
> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> debug option.

Ironically this would not create a build failure for the architectures
where this matters, because only powerpc has the like named percpu
symbol.

So it's not really meeting the stated objective in this case.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Sun, 19 Apr 2015 22:17:21 -0700
> 
> > The debug option is intended for all _other_ architectures, to
> > ensure that changes made for those don't break alpha/s390
> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> > debug option.
> 
> Ironically this would not create a build failure for the architectures
> where this matters, because only powerpc has the like named percpu
> symbol.
> 
> So it's not really meeting the stated objective in this case.

Yes, that is correct; it can only find problems in non-architecture
code, and on the downside produces false positives and thus build errors
like this one.

Which makes the fix a bit philosophical. Rename iommu_pool_hash in
iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
iommu_pool_hash, but that is just me. Ultimately, I don't really
care one way or another, as long as the problem gets fixed.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Guenter Roeck <linux@roeck-us.net>
Date: Mon, 20 Apr 2015 09:44:31 -0700

> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Sun, 19 Apr 2015 22:17:21 -0700
>> 
>> > The debug option is intended for all _other_ architectures, to
>> > ensure that changes made for those don't break alpha/s390
>> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
>> > debug option.
>> 
>> Ironically this would not create a build failure for the architectures
>> where this matters, because only powerpc has the like named percpu
>> symbol.
>> 
>> So it's not really meeting the stated objective in this case.
> 
> Yes, that is correct; it can only find problems in non-architecture
> code, and on the downside produces false positives and thus build errors
> like this one.
> 
> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> iommu_pool_hash, but that is just me. Ultimately, I don't really
> care one way or another, as long as the problem gets fixed.

If nightly builds of s390 and alpha, the two platforms where this
matters, are being done as reported in this thread, then I really
don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

But I guess that's a more involved longer-term discussion and I guess
I'll apply Sowmini's patches for now.

Thanks.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
> From: Guenter Roeck <linux@roeck-us.net>
> Date: Mon, 20 Apr 2015 09:44:31 -0700
> 
> > On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Sun, 19 Apr 2015 22:17:21 -0700
> >> 
> >> > The debug option is intended for all _other_ architectures, to
> >> > ensure that changes made for those don't break alpha/s390
> >> > builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> >> > debug option.
> >> 
> >> Ironically this would not create a build failure for the architectures
> >> where this matters, because only powerpc has the like named percpu
> >> symbol.
> >> 
> >> So it's not really meeting the stated objective in this case.
> > 
> > Yes, that is correct; it can only find problems in non-architecture
> > code, and on the downside produces false positives and thus build errors
> > like this one.
> > 
> > Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> > iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> > iommu_pool_hash, but that is just me. Ultimately, I don't really
> > care one way or another, as long as the problem gets fixed.
> 
> If nightly builds of s390 and alpha, the two platforms where this
> matters, are being done as reported in this thread, then I really
> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

We do an s390 allmodconfig for every linux-next release:

  http://kisskb.ellerman.id.au/kisskb/target/573/

And also for Linus' tree:

  http://kisskb.ellerman.id.au/kisskb/target/568/

We don't have alpha allmodconfig enabled, though we could, but we do build the
defconfig:

  http://kisskb.ellerman.id.au/kisskb/target/2499/
  http://kisskb.ellerman.id.au/kisskb/target/2494/

So I think that should be sufficient to catch any percpus that are introduced
in generic code with the same name as s390/alpha variables.


> But I guess that's a more involved longer-term discussion and I guess
> I'll apply Sowmini's patches for now.

Yeah I guess it is. Thanks for merging the fix.

cheers

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On 04/20/2015 06:54 PM, Michael Ellerman wrote:
> On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
>> From: Guenter Roeck <linux@roeck-us.net>
>> Date: Mon, 20 Apr 2015 09:44:31 -0700
>>
>>> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
>>>> From: Guenter Roeck <linux@roeck-us.net>
>>>> Date: Sun, 19 Apr 2015 22:17:21 -0700
>>>>
>>>>> The debug option is intended for all _other_ architectures, to
>>>>> ensure that changes made for those don't break alpha/s390
>>>>> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
>>>>> debug option.
>>>>
>>>> Ironically this would not create a build failure for the architectures
>>>> where this matters, because only powerpc has the like named percpu
>>>> symbol.
>>>>
>>>> So it's not really meeting the stated objective in this case.
>>>
>>> Yes, that is correct; it can only find problems in non-architecture
>>> code, and on the downside produces false positives and thus build errors
>>> like this one.
>>>
>>> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
>>> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
>>> iommu_pool_hash, but that is just me. Ultimately, I don't really
>>> care one way or another, as long as the problem gets fixed.
>>
>> If nightly builds of s390 and alpha, the two platforms where this
>> matters, are being done as reported in this thread, then I really
>> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.

Me not either, but, as you say, that is a different discussion.

>
> We do an s390 allmodconfig for every linux-next release:
>
>    http://kisskb.ellerman.id.au/kisskb/target/573/
>
> And also for Linus' tree:
>
>    http://kisskb.ellerman.id.au/kisskb/target/568/
>
> We don't have alpha allmodconfig enabled, though we could, but we do build the
> defconfig:
>
>    http://kisskb.ellerman.id.au/kisskb/target/2499/
>    http://kisskb.ellerman.id.au/kisskb/target/2494/
>
I cover alpha:allmodconfig in my builds for -next, mainline, as well as all
kernel.org stable releases and release candidates. This discussion is a good
argument for enabling s390:allmodconfig as well.

> So I think that should be sufficient to catch any percpus that are introduced
> in generic code with the same name as s390/alpha variables.
>
Yes, but unfortunately only after the fact, though I don't see a means
to avoid that.

>
>> But I guess that's a more involved longer-term discussion and I guess
>> I'll apply Sowmini's patches for now.
>
Thanks!

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Michael Ellerman]

On Mon, 2015-04-20 at 19:32 -0700, Guenter Roeck wrote:
> On 04/20/2015 06:54 PM, Michael Ellerman wrote:
> > On Mon, 2015-04-20 at 12:50 -0400, David Miller wrote:
> >> From: Guenter Roeck <linux@roeck-us.net>
> >> Date: Mon, 20 Apr 2015 09:44:31 -0700
> >>
> >>> On Mon, Apr 20, 2015 at 12:25:19PM -0400, David Miller wrote:
> >>>> From: Guenter Roeck <linux@roeck-us.net>
> >>>> Date: Sun, 19 Apr 2015 22:17:21 -0700
> >>>>
> >>>>> The debug option is intended for all _other_ architectures, to
> >>>>> ensure that changes made for those don't break alpha/s390
> >>>>> builds. alpha/s390 have ARCH_NEEDS_WEAK_PER_CPU and don't need the
> >>>>> debug option.
> >>>>
> >>>> Ironically this would not create a build failure for the architectures
> >>>> where this matters, because only powerpc has the like named percpu
> >>>> symbol.
> >>>>
> >>>> So it's not really meeting the stated objective in this case.
> >>>
> >>> Yes, that is correct; it can only find problems in non-architecture
> >>> code, and on the downside produces false positives and thus build errors
> >>> like this one.
> >>>
> >>> Which makes the fix a bit philosophical. Rename iommu_pool_hash in
> >>> iommu-common, or drop DEBUG_FORCE_WEAK_PER_CPU. I would rename
> >>> iommu_pool_hash, but that is just me. Ultimately, I don't really
> >>> care one way or another, as long as the problem gets fixed.
> >>
> >> If nightly builds of s390 and alpha, the two platforms where this
> >> matters, are being done as reported in this thread, then I really
> >> don't see the value in DEBUG_FORCE_WEAK_PER_CPU.
> 
> Me not either, but, as you say, that is a different discussion.
> 
> >
> > We do an s390 allmodconfig for every linux-next release:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/573/
> >
> > And also for Linus' tree:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/568/
> >
> > We don't have alpha allmodconfig enabled, though we could, but we do build the
> > defconfig:
> >
> >    http://kisskb.ellerman.id.au/kisskb/target/2499/
> >    http://kisskb.ellerman.id.au/kisskb/target/2494/
>
> I cover alpha:allmodconfig in my builds for -next, mainline, as well as all
> kernel.org stable releases and release candidates. This discussion is a good
> argument for enabling s390:allmodconfig as well.
> 
> > So I think that should be sufficient to catch any percpus that are introduced
> > in generic code with the same name as s390/alpha variables.
> 
> Yes, but unfortunately only after the fact, though I don't see a means
> to avoid that.

Yeah after the merge into linux-next, which I think is probably good enough for
something like this.

cheers

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sun, 19 Apr 2015 06:51:57 -0400

> On (04/18/15 21:23), Guenter Roeck wrote:
>> 
>> I spoke too early. Now I get a similar failure in the
>> powerpc:allmodconfig build
>> (which previously failed for a different reason).
> 
> I think this duplicate symbol is genuine.. there's a definition
> in arch/powerpc/kernel/iommu.c. To avoid conflicting with that,
> the one in lib/iommu-common.c can be renamed.  I guess we missed
> this during review, and this went under the radar when I did
> all my testing on sparc.
> 
> I'll put a patch together for this today..

Maybe ping the powerpc folks becuase if they can do a quick
conversion, this change isn't necessary.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

On (04/18/15 21:23), Guenter Roeck wrote:
> 
> I spoke too early. Now I get a similar failure in the
> powerpc:allmodconfig build
> (which previously failed for a different reason).

I think this duplicate symbol is genuine.. there's a definition
in arch/powerpc/kernel/iommu.c. To avoid conflicting with that,
the one in lib/iommu-common.c can be renamed.  I guess we missed
this during review, and this went under the radar when I did
all my testing on sparc.

I'll put a patch together for this today..

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On 04/18/2015 09:13 PM, Guenter Roeck wrote:
> On 04/18/2015 02:41 PM, David Miller wrote:
>> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>> Date: Sat, 18 Apr 2015 15:55:14 -0400
>>
>>>>
>>>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>>>>
>>>> Can you take a look?
>>>
>>> The patches look right now. These are the commit-ids I checked
>>>
>>>    ff7d37a502022149655c18035b99a53391be0383
>>>    bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>>>    0ae53ed15d9b87b883b593a9884957cfa4fc2480
>>
>> Thanks for checking, I'll get this to Linus ASAP.
>>
> Latest upstream (v4.0-8110-g64fb1d0) passes my tests.
>

I spoke too early. Now I get a similar failure in the powerpc:allmodconfig build
(which previously failed for a different reason).

lib/built-in.o:(.discard+0x1): multiple definition of `__pcpu_unique_iommu_pool_hash'
arch/powerpc/kernel/built-in.o:(.discard+0x18): first defined here

And, yes, there are still two instances of iommu_pool_hash.

arch/powerpc/kernel/iommu.c:static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
lib/iommu-common.c:static       DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On 04/18/2015 02:41 PM, David Miller wrote:
> From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
> Date: Sat, 18 Apr 2015 15:55:14 -0400
>
>>>
>>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>>>
>>> Can you take a look?
>>
>> The patches look right now. These are the commit-ids I checked
>>
>>    ff7d37a502022149655c18035b99a53391be0383
>>    bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>>    0ae53ed15d9b87b883b593a9884957cfa4fc2480
>
> Thanks for checking, I'll get this to Linus ASAP.
>
Latest upstream (v4.0-8110-g64fb1d0) passes my tests.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 15:55:14 -0400

>> 
>> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
>> 
>> Can you take a look?
> 
> The patches look right now. These are the commit-ids I checked
> 
>   ff7d37a502022149655c18035b99a53391be0383
>   bb620c3d3925aec0ed4f21010c86df08ec18a8c7
>   0ae53ed15d9b87b883b593a9884957cfa4fc2480

Thanks for checking, I'll get this to Linus ASAP.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

> 
> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
> 
> Can you take a look?

The patches look right now. These are the commit-ids I checked

  ff7d37a502022149655c18035b99a53391be0383
  bb620c3d3925aec0ed4f21010c86df08ec18a8c7
  0ae53ed15d9b87b883b593a9884957cfa4fc2480

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

On (04/18/15 15:40), David Miller wrote:
> 
> Sowmini, I think I sorted this out in the 'sparc' GIT tree.
> 
> Can you take a look?
> 

checking it right now.. give me a few minutes..

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Sat, 18 Apr 2015 15:27:53 -0400 (EDT)

> Dammit, somehow I applied V4 :-/
> 
> Sorry about that.  I'll try to sort this out.

Sowmini, I think I sorted this out in the 'sparc' GIT tree.

Can you take a look?

Thanks!

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 14:38:44 -0400

> On (04/18/15 11:28), Guenter Roeck wrote:
>> 
>> Some merge gone wrong, maybe ? I tried to revert f1600e549b94
>> and apply http://patchwork.ozlabs.org/patch/459803/ instead.
> 
> That patch is a part-2 of a 3-part patch set. In order, this should
> have been v10: applied as:
> 
>    http://patchwork.ozlabs.org/patch/459804/
>    http://patchwork.ozlabs.org/patch/459803/
>    http://patchwork.ozlabs.org/patch/459802/
> 
> The mail thread for the 3-part patch set was here
>    http://www.spinics.net/lists/sparclinux/msg13785.html
> 
> I noticed that the existing dates on the iommu commits were also odd:
> 
>   commit 671d773297969bebb1732e1cdc1ec03aa53c6be2
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:37 2015 -0400
> 
> 
>   commit f1600e549b948a32ad7672e069b2915314637ae3
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:36 2015 -0400
>   
>   commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
>   Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
>   Date:   Thu Mar 12 20:02:35 2015 -0400
> 
> We have had many discussions of this patch set since March 12,
> and the correct version should have been v10, dated something
> after Apr 9, and acked by Benjamin Herrenschmidt.

Dammit, somehow I applied V4 :-/

Sorry about that.  I'll try to sort this out.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[David Miller]

From: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Date: Sat, 18 Apr 2015 08:05:10 -0400

> But when I clone 
>   git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
> and do a git show of the commit-id above, I see deltas that
> dont make sense (they seem to be from a patchset from somewhere
> in the middle of the review chain from the thread). 
> 
> What am I missing? 

I am pretty sure I applied V10 of your IOMMU submission.

http://patchwork.ozlabs.org/patch/459804/
http://patchwork.ozlabs.org/patch/459803/
http://patchwork.ozlabs.org/patch/459802/

and follow-on parisc build fix:

http://patchwork.ozlabs.org/patch/461905/

if I somehow botched this up, wow that's impressive :-)

If you can sort out what I did wrong and send me a relative
fixup patch, I'd really appreciate it.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

On (04/18/15 11:28), Guenter Roeck wrote:
> 
> Some merge gone wrong, maybe ? I tried to revert f1600e549b94
> and apply http://patchwork.ozlabs.org/patch/459803/ instead.

That patch is a part-2 of a 3-part patch set. In order, this should
have been v10: applied as:

   http://patchwork.ozlabs.org/patch/459804/
   http://patchwork.ozlabs.org/patch/459803/
   http://patchwork.ozlabs.org/patch/459802/

The mail thread for the 3-part patch set was here
   http://www.spinics.net/lists/sparclinux/msg13785.html

I noticed that the existing dates on the iommu commits were also odd:

  commit 671d773297969bebb1732e1cdc1ec03aa53c6be2
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:37 2015 -0400


  commit f1600e549b948a32ad7672e069b2915314637ae3
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:36 2015 -0400
  
  commit 10b88a4b17d31a7409494b179dcb76e7ab2fcaea
  Author: Sowmini Varadhan <sowmini.varadhan@oracle.com>
  Date:   Thu Mar 12 20:02:35 2015 -0400

We have had many discussions of this patch set since March 12,
and the correct version should have been v10, dated something
after Apr 9, and acked by Benjamin Herrenschmidt.

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

On 04/18/2015 05:05 AM, Sowmini Varadhan wrote:
> On (04/17/15 22:47), Guenter Roeck wrote:
>> The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
>> use scalable lib/iommu-common.c functions"), which introduces
>>
>> static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);
>
> I have to confess that I'm a little confused about what happened here..
>
> The specific patch (2/3) above should have come from this submission
>    http://www.spinics.net/lists/sparclinux/msg13786.html
> aka
>    http://patchwork.ozlabs.org/patch/459803/
>
> This does not add any additional defines for iommu_pool_hash.
>
> It also does not have any references to fields like page_table_map_base:
> instead, these were switched over to things like:
> -	*dma_addrp = (iommu->page_table_map_base +
> +	*dma_addrp = (iommu->tbl.table_map_base +
>   		      ((iopte - iommu->page_table) << IO_PAGE_SHIFT));
>
>
> But when I clone
>    git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
> and do a git show of the commit-id above, I see deltas that
> dont make sense (they seem to be from a patchset from somewhere
> in the middle of the review chain from the thread).
>
> What am I missing?
>

Some merge gone wrong, maybe ? I tried to revert f1600e549b94
and apply http://patchwork.ozlabs.org/patch/459803/ instead.
The result doesn't compile, so there may have been some other
changes in the same area.

Guenter

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

On (04/17/15 22:47), Guenter Roeck wrote:
> The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
> use scalable lib/iommu-common.c functions"), which introduces 
> 
> static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

I have to confess that I'm a little confused about what happened here..

The specific patch (2/3) above should have come from this submission
  http://www.spinics.net/lists/sparclinux/msg13786.html
aka 
  http://patchwork.ozlabs.org/patch/459803/

This does not add any additional defines for iommu_pool_hash.

It also does not have any references to fields like page_table_map_base:
instead, these were switched over to things like:
-	*dma_addrp = (iommu->page_table_map_base +
+	*dma_addrp = (iommu->tbl.table_map_base +
 		      ((iopte - iommu->page_table) << IO_PAGE_SHIFT));


But when I clone 
  git://git.kernel.org/pub/scm/linux/kernel/git/davem/sparc.git
and do a git show of the commit-id above, I see deltas that
dont make sense (they seem to be from a patchset from somewhere
in the middle of the review chain from the thread). 

What am I missing? 

--Sowmini

Re: sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Sowmini Varadhan]

On (04/17/15 22:47), Guenter Roeck wrote:
> #define DEFINE_PER_CPU_SECTION(type, name, sec)                         \
>         __PCPU_DUMMY_ATTRS char __pcpu_scope_##name;                    \
> 	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
> -->	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
> 	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
> 	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak                 \
> 					        __typeof__(type) name
> 
> if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
> The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
> global variable, which explains the problem (and makes me wonder what the
> 'static' keyword in front of DEFINE_PER_CPU is supposed to accomplish).
> 
> I thought about fixing the problem by renaming one of the variables, but
> I am not sure if that is what is intended. Specifically, I am not sure if
> the variables are supposed to be different, as it looks like, or if they
> are supposed to be the same.

In this particular case, the intention is to have this variable only
be static to lib/iommu-common.c which tries to generalize the code
in arch/powerpc/kernel/iommu.c. So it would be fine to rename one
of those variables, but I'm not sure you can safely do that by
changing the macro above, as that would impact other uses of this.

> In case it is relevant, I use gcc version 4.6.3 for my build test.

Let me reproduce your error on my test machine and try to put
a fix together..

--Sowmini

sparc64: Build failure due to commit f1600e549b94 (sparc: Make sparc64 use scalable lib/iommu-common.c functions)

[Guenter Roeck]

Hi all,

I see the following build failure when compiling sparc64:allmodconfig
in the upstream kernel (v4.0-7820-g04b7fe6a4a23).

arch/sparc/kernel/pci_sun4v.o:(.discard+0x1): multiple definition of `__pcpu_unique_iommu_pool_hash'
arch/sparc/kernel/iommu.o:(.discard+0x0): first defined here
make[2]: *** [arch/sparc/kernel/built-in.o] Error 1
make[1]: *** [arch/sparc/kernel] Error 2

The problem is caused by commit f1600e549b94 ("sparc: Make sparc64
use scalable lib/iommu-common.c functions"), which introduces 

static DEFINE_PER_CPU(unsigned int, iommu_pool_hash);

in both files.

DEFINE_PER_CPU translates to DEFINE_PER_CPU_SECTION, which in turn is
defined as

#define DEFINE_PER_CPU_SECTION(type, name, sec)                         \
        __PCPU_DUMMY_ATTRS char __pcpu_scope_##name;                    \
	extern __PCPU_DUMMY_ATTRS char __pcpu_unique_##name;		\
-->	__PCPU_DUMMY_ATTRS char __pcpu_unique_##name;			\
	extern __PCPU_ATTRS(sec) __typeof__(type) name;			\
	__PCPU_ATTRS(sec) PER_CPU_DEF_ATTRIBUTES __weak                 \
					        __typeof__(type) name

if CONFIG_DEBUG_FORCE_WEAK_PER_CPU is configured, which is the case here.
The marked line above shows that __pcpu_unique_iommu_pool_hash is declared as
global variable, which explains the problem (and makes me wonder what the
'static' keyword in front of DEFINE_PER_CPU is supposed to accomplish).

I thought about fixing the problem by renaming one of the variables, but
I am not sure if that is what is intended. Specifically, I am not sure if
the variables are supposed to be different, as it looks like, or if they
are supposed to be the same.

In case it is relevant, I use gcc version 4.6.3 for my build test.

Guenter