Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1478647 > unrolled thread

Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-07 23:50 +0200
    Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 00:30 +0200
      Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:20 +0200
        Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 17:00 +0200
          Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 17:10 +0200
    Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Guenter Roeck <linux@roeck-us.net> - 2016-09-08 00:40 +0200
      Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver Arnd Bergmann <arnd@arndb.de> - 2016-09-08 10:20 +0200
        Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver James Morse <james.morse@arm.com> - 2016-09-08 12:50 +0200
          Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver AKASHI Takahiro <takahiro.akashi@linaro.org> - 2016-09-09 05:20 +0200
            Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver James Morse <james.morse@arm.com> - 2016-09-09 11:40 +0200

#1478647 — Re: [PATCH v3 2/3] hwmon: xgene: Add hwmon driver

On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> +               ctx->comm_base_addr = cppc_ss->base_address;
> +               if (ctx->comm_base_addr) {
> +                       ctx->pcc_comm_addr =
> +                                       acpi_os_ioremap(ctx->comm_base_addr,
> +                                                       cppc_ss->length);
> 

This causes the arm64 allmodconfig build to fail now, according to
kernelci:

      1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!

Should this perhaps call ioremap() or memremap() instead?

	Arnd

[toc] | [next] | [standalone]

#1478666

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               ctx->comm_base_addr = cppc_ss->base_address;
> > +               if (ctx->comm_base_addr) {
> > +                       ctx->pcc_comm_addr =
> > +                                       acpi_os_ioremap(ctx->comm_base_addr,
> > +                                                       cppc_ss->length);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
How do you even get there ? arm64:allmodconfig fails for me in -next with

drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’

Guenter

[toc] | [prev] | [next] | [standalone]

#1478907

On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               ctx->comm_base_addr = cppc_ss->base_address;
> > > +               if (ctx->comm_base_addr) {
> > > +                       ctx->pcc_comm_addr =
> > > +                                       acpi_os_ioremap(ctx->comm_base_addr,
> > > +                                                       cppc_ss->length);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> How do you even get there ? arm64:allmodconfig fails for me in -next with
> 
> drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
> drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’

That was fixed in yesterday's linux-next.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1479302

On 09/08/2016 01:15 AM, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:27:54 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               ctx->comm_base_addr = cppc_ss->base_address;
>>>> +               if (ctx->comm_base_addr) {
>>>> +                       ctx->pcc_comm_addr =
>>>> +                                       acpi_os_ioremap(ctx->comm_base_addr,
>>>> +                                                       cppc_ss->length);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>> How do you even get there ? arm64:allmodconfig fails for me in -next with
>>
>> drivers/pwm/pwm-berlin.c: In function ‘berlin_pwm_suspend’:
>> drivers/pwm/pwm-berlin.c:245:35: error: ‘struct berlin_pwm_chip’ has no member named ‘chips’
>
> That was fixed in yesterday's linux-next.
>

Maybe, but now I get

Building arm:allmodconfig ... failed
--------------
Error log:
crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'

Building arm64:allmodconfig ... failed
--------------
Error log:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ‘kvm_timer_hyp_init’:
arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ‘out_free’ defined but not used

Guenter

[toc] | [prev] | [next] | [standalone]

#1479313

On Thursday, September 8, 2016 7:55:44 AM CEST Guenter Roeck wrote:
> Maybe, but now I get
> 
> Building arm:allmodconfig ... failed
> --------------
> Error log:
> crypto/crypto_engine.c: In function 'crypto_transfer_hash_request':
> crypto/crypto_engine.c:234:3: error: implicit declaration of function 'queue_kthread_work'
> 
> Building arm64:allmodconfig ... failed
> --------------
> Error log:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c: In function ‘kvm_timer_hyp_init’:
> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:457:1: warning: label ‘out_free’ defined but not used

Right, I sent fixes for both. I think we had a clean build yesterday,
probably tomorrow those will both be fixed.

The time between -rc5 and -rc7 seems to be the worst for build testing ;-)

	Arnd

[toc] | [prev] | [next] | [standalone]

#1478668

On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > +               ctx->comm_base_addr = cppc_ss->base_address;
> > +               if (ctx->comm_base_addr) {
> > +                       ctx->pcc_comm_addr =
> > +                                       acpi_os_ioremap(ctx->comm_base_addr,
> > +                                                       cppc_ss->length);
> > 
> 
> This causes the arm64 allmodconfig build to fail now, according to
> kernelci:
> 
>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> 
> Should this perhaps call ioremap() or memremap() instead?
> 
Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
mark reserved memblock regions explicitly in iomem") starts using a function
in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
is declared as __init_memblock, which makes me really uncomfortable.
If acpi_os_ioremap() must not be used by modules, and possibly only during
early (?) initialization, maybe its declaration should state those limitations ?

Thanks,
Guenter

[toc] | [prev] | [next] | [standalone]

#1478904

On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> > On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> > > +               ctx->comm_base_addr = cppc_ss->base_address;
> > > +               if (ctx->comm_base_addr) {
> > > +                       ctx->pcc_comm_addr =
> > > +                                       acpi_os_ioremap(ctx->comm_base_addr,
> > > +                                                       cppc_ss->length);
> > > 
> > 
> > This causes the arm64 allmodconfig build to fail now, according to
> > kernelci:
> > 
> >       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> > 
> > Should this perhaps call ioremap() or memremap() instead?
> > 
> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> mark reserved memblock regions explicitly in iomem") starts using a function
> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> is declared as __init_memblock, which makes me really uncomfortable.
> If acpi_os_ioremap() must not be used by modules, and possibly only during
> early (?) initialization, maybe its declaration should state those limitations ?

Ah, I didn't notice that. I guess both patches were correct individually and
got added to linux-next around the same time but caused allmodconfig to blow up
when used together.

Adding everyone who was involved in the memblock patch to Cc here, maybe one
of them has an idea what the correct fix is. There are only two other drivers
using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
that drivers are not actually supposed to use this symbol. Making
acpi_os_ioremap() an exported function in arm64 would also work.

	Arnd

[toc] | [prev] | [next] | [standalone]

#1479076

Hi,

On 08/09/16 09:14, Arnd Bergmann wrote:
> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>> +               ctx->comm_base_addr = cppc_ss->base_address;
>>>> +               if (ctx->comm_base_addr) {
>>>> +                       ctx->pcc_comm_addr =
>>>> +                                       acpi_os_ioremap(ctx->comm_base_addr,
>>>> +                                                       cppc_ss->length);
>>>>
>>>
>>> This causes the arm64 allmodconfig build to fail now, according to
>>> kernelci:
>>>
>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>
>>> Should this perhaps call ioremap() or memremap() instead?
>>>
>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>> mark reserved memblock regions explicitly in iomem") starts using a function
>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>> is declared as __init_memblock, which makes me really uncomfortable.
>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>> early (?) initialization, maybe its declaration should state those limitations ?
> 
> Ah, I didn't notice that. I guess both patches were correct individually and
> got added to linux-next around the same time but caused allmodconfig to blow up
> when used together.
> 
> Adding everyone who was involved in the memblock patch to Cc here, maybe one
> of them has an idea what the correct fix is. There are only two other drivers
> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> that drivers are not actually supposed to use this symbol. Making
> acpi_os_ioremap() an exported function in arm64 would also work.

You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
already EXPORT_SYMBOL_GPL().

(I'm still waiting for allmodconfig on linux-next to finish building)


Thanks,

James

[toc] | [prev] | [next] | [standalone]

#1479628

On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
> Hi,
> 
> On 08/09/16 09:14, Arnd Bergmann wrote:
> > On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
> >> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
> >>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
> >>>> +               ctx->comm_base_addr = cppc_ss->base_address;
> >>>> +               if (ctx->comm_base_addr) {
> >>>> +                       ctx->pcc_comm_addr =
> >>>> +                                       acpi_os_ioremap(ctx->comm_base_addr,
> >>>> +                                                       cppc_ss->length);
> >>>>
> >>>
> >>> This causes the arm64 allmodconfig build to fail now, according to
> >>> kernelci:
> >>>
> >>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
> >>>
> >>> Should this perhaps call ioremap() or memremap() instead?
> >>>
> >> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
> >> mark reserved memblock regions explicitly in iomem") starts using a function
> >> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
> >> is declared as __init_memblock, which makes me really uncomfortable.
> >> If acpi_os_ioremap() must not be used by modules, and possibly only during
> >> early (?) initialization, maybe its declaration should state those limitations ?
> > 
> > Ah, I didn't notice that. I guess both patches were correct individually and
> > got added to linux-next around the same time but caused allmodconfig to blow up
> > when used together.
> > 
> > Adding everyone who was involved in the memblock patch to Cc here, maybe one
> > of them has an idea what the correct fix is. There are only two other drivers
> > using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
> > that drivers are not actually supposed to use this symbol. Making
> > acpi_os_ioremap() an exported function in arm64 would also work.
> 
> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
> already EXPORT_SYMBOL_GPL().

acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.

The problem is that, as memblock_is_memory() is declared as __init,
we cannot build any drivers which call acpi_os_ioremap() as modules.

As far as this specific issue is concerned, if we make a change like:

===8<===
--- a/arch/arm64/kernel/setup.c
+++ b/arch/arm64/kernel/setup.c
@@ -207,7 +207,7 @@ static void __init request_standard_resources(void)
                res = alloc_bootmem_low(sizeof(*res));
                if (memblock_is_nomap(region)) {
                        res->name  = "reserved";
-                       res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
+                       res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
                } else {
                        res->name  = "System RAM";
                        res->flags = IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
===>8===

and revert the following hunk from the commit:

===8<===
--- a/arch/arm64/include/asm/acpi.h
+++ b/arch/arm64/include/asm/acpi.h
@@ -12,7 +12,7 @@
 #ifndef _ASM_ACPI_H
 #define _ASM_ACPI_H
 
-#include <linux/mm.h>
+#include <linux/memblock.h>
 #include <linux/psci.h>
 
 #include <asm/cputype.h>
@@ -32,7 +32,11 @@
 static inline void __iomem *acpi_os_ioremap(acpi_physical_address phys,
                                            acpi_size size)
 {
-       if (!page_is_ram(phys >> PAGE_SHIFT))
+       /*
+        * EFI's reserve_regions() call adds memory with the WB attribute
+        * to memblock via early_init_dt_add_memory_arch().
+        */
+       if (!memblock_is_memory(phys))
                return ioremap(phys, size);
 
        return ioremap_cache(phys, size);
diff --git a/arch/arm64/kernel/setup.c b/arch/arm64/kernel/setup.c
===>8===

The build error will be gone. (and still kdump should work.)

But I don't know how we should distinguish IORESOURCE_MEM and
IORESOURCE_SYSTEM_RAM.

Thanks,
-Takahiro AKASHI

> (I'm still waiting for allmodconfig on linux-next to finish building)
> 
> 
> Thanks,
> 
> James
> 
> 

[toc] | [prev] | [next] | [standalone]

#1479799

Hi,

On 09/09/16 04:18, AKASHI Takahiro wrote:
> On Thu, Sep 08, 2016 at 11:47:59AM +0100, James Morse wrote:
>> On 08/09/16 09:14, Arnd Bergmann wrote:
>>> On Wednesday, September 7, 2016 3:37:05 PM CEST Guenter Roeck wrote:
>>>> On Wed, Sep 07, 2016 at 11:41:44PM +0200, Arnd Bergmann wrote:
>>>>> On Thursday, July 21, 2016 1:55:56 PM CEST Hoan Tran wrote:
>>>>>> +               ctx->comm_base_addr = cppc_ss->base_address;
>>>>>> +               if (ctx->comm_base_addr) {
>>>>>> +                       ctx->pcc_comm_addr =
>>>>>> +                                       acpi_os_ioremap(ctx->comm_base_addr,
>>>>>> +                                                       cppc_ss->length);
>>>>>>
>>>>>
>>>>> This causes the arm64 allmodconfig build to fail now, according to
>>>>> kernelci:
>>>>>
>>>>>       1  ERROR: "memblock_is_memory" [drivers/hwmon/xgene-hwmon.ko] undefined!
>>>>>
>>>>> Should this perhaps call ioremap() or memremap() instead?
>>>>>
>>>> Hmmm ... almost sounds to me like blaming the messenger. e7cd190385d1 ("arm64:
>>>> mark reserved memblock regions explicitly in iomem") starts using a function
>>>> in acpi_os_ioremap() which is not exported. On top of that, memblock_is_memory()
>>>> is declared as __init_memblock, which makes me really uncomfortable.
>>>> If acpi_os_ioremap() must not be used by modules, and possibly only during
>>>> early (?) initialization, maybe its declaration should state those limitations ?
>>>
>>> Ah, I didn't notice that. I guess both patches were correct individually and
>>> got added to linux-next around the same time but caused allmodconfig to blow up
>>> when used together.
>>>
>>> Adding everyone who was involved in the memblock patch to Cc here, maybe one
>>> of them has an idea what the correct fix is. There are only two other drivers
>>> using acpi_os_ioremap() and one of them is x86-specific, so it's still likely
>>> that drivers are not actually supposed to use this symbol. Making
>>> acpi_os_ioremap() an exported function in arm64 would also work.
>>
>> You could use acpi_os_map_iomem()/acpi_os_unmap_iomem() from acpi/acpi_io.h.
>> If there isn't an existing mapping these end up in acpi_os_ioremap(), and are
>> already EXPORT_SYMBOL_GPL().
> 
> acpi_os_ioremap() is re-defined in arm64/include/asm/acpi.h.
> 
> The problem is that, as memblock_is_memory() is declared as __init,

__init_memblock ...

... as is memblock_is_map_memory(), which we call from pfn_valid() which is
EXPORT_SYMBOL()'d
and used from modules, (e.g. mac80211.ko). So something fishy is going on...

From include/linux/memblock.h:
> #ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> #define __init_memblock __meminit
> #define __initdata_memblock __meminitdata
> #else
> #define __init_memblock
> #define __initdata_memblock
> #endif

arm64 doesn't define ARCH_DISCARD_MEMBLOCK, so we always keep these symbols.
If we didn't, pfn_valid() would break too.


Thanks,

James

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web