[PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[Antonio Borneo]

The builds of arch/sh are failing in linux-next with:

drivers/net/ethernet/via/via-rhine.c:1098:2: error: implicit declaration
of function 'pci_iomap' [-Werror=implicit-function-declaration]
drivers/net/ethernet/via/via-rhine.c:1119:2: error: implicit declaration
of function 'pci_iounmap' [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
make[5]: *** [drivers/net/ethernet/via/via-rhine.o] Error 1

The logic in include/asm-generic/{io,iomap,pci_iomap}.h
requires GENERIC_PCI_IOMAP to inline empty pci_iomap/pci_iounmap
when PCI is not selected.

Add GENERIC_PCI_IOMAP in the dependency list for VIA_RHINE and
VIA_VELOCITY.

Reported-by: Paul Gortmaker <paul.gortmaker@windriver.com>
Signed-off-by: Antonio Borneo <borneo.antonio@gmail.com>
---
 drivers/net/ethernet/via/Kconfig | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/via/Kconfig b/drivers/net/ethernet/via/Kconfig
index 9bf3ff1..96c527c 100644
--- a/drivers/net/ethernet/via/Kconfig
+++ b/drivers/net/ethernet/via/Kconfig
@@ -19,7 +19,7 @@ if NET_VENDOR_VIA
 
 config VIA_RHINE
 	tristate "VIA Rhine support"
-	depends on (PCI || OF_IRQ)
+	depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))
 	depends on HAS_DMA
 	select CRC32
 	select MII
@@ -44,7 +44,7 @@ config VIA_RHINE_MMIO
 
 config VIA_VELOCITY
 	tristate "VIA Velocity support"
-	depends on (PCI || (OF_ADDRESS && OF_IRQ))
+	depends on (PCI || (OF_ADDRESS && OF_IRQ && GENERIC_PCI_IOMAP))
 	depends on HAS_DMA
 	select CRC32
 	select CRC_CCITT
-- 
2.4.6

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[David Miller]

From: Antonio Borneo <borneo.antonio@gmail.com>
Date: Thu, 23 Jul 2015 00:34:14 +0800

> -	depends on (PCI || OF_IRQ)
> +	depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))

Having GENERIC_PCI_IOMAP withut PCI makes absolutely no sense to me.

I can't see why a platform would provide GENERIC_PCI_IOMAP when it
does not have PCI enabled.

You need to explain this.

When PCI is not enabled, you should not be using any interface
pci_*() whatsoever.  Especially such a core interface as pci_iomap().

ALTERNATIVELY, NOP versions of pci_iomap() and friends should be
provided when PCI is disabled, which just return errors and
non-success statuses.

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[Antonio Borneo]

On Thu, Jul 23, 2015 at 2:08 AM, David Miller <davem@davemloft.net> wrote:
> From: Antonio Borneo <borneo.antonio@gmail.com>
> Date: Thu, 23 Jul 2015 00:34:14 +0800
>
>> -     depends on (PCI || OF_IRQ)
>> +     depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))
>
> Having GENERIC_PCI_IOMAP withut PCI makes absolutely no sense to me.
>
> I can't see why a platform would provide GENERIC_PCI_IOMAP when it
> does not have PCI enabled.

My understanding for GENERIC_PCI_IOMAP is to enable providing empty
pci_iomap/pci_iounmap.
Digging in git log, arch that used to provide such empty instances
moved to a centralized implementation by using GENERIC_PCI_IOMAP.
For me such empty functions should be available by default when PCI is
not set, but this is not the case today.

So there are cases that set GENERIC_PCI_IOMAP without setting PCI. E.g.
make ARCH=mips workpad_defconfig
make ARCH=microblaze allnoconfig

Moreover, in lib/Kconfig GENERIC_PCI_IOMAP is selected when
GENERIC_IOMAP is set, without dependencies from PCI.

You can also check
https://lkml.org/lkml/2013/9/11/76
already upstream in commit
78857614104a26cdada4c53eea104752042bf5a1

Anyway, no other Kconfig uses GENERIC_PCI_IOMAP within conditionals,
which makes this patch quite unusual.

If you think this is not the way to go, we can consider to revert both
0f8b6cea1f3271ccc37dd2847b39e235461e0ced
b7d3282a245f44286b88e436b1d08c518cdf4569
and take some more time for a different solution.

Regards,
Antonio

> You need to explain this.
>
> When PCI is not enabled, you should not be using any interface
> pci_*() whatsoever.  Especially such a core interface as pci_iomap().
>
> ALTERNATIVELY, NOP versions of pci_iomap() and friends should be
> provided when PCI is disabled, which just return errors and
> non-success statuses.

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[Alexey Charkov]

2015-07-23 4:35 GMT+03:00 Antonio Borneo <borneo.antonio@gmail.com>:
> On Thu, Jul 23, 2015 at 2:08 AM, David Miller <davem@davemloft.net> wrote:
>> From: Antonio Borneo <borneo.antonio@gmail.com>
>> Date: Thu, 23 Jul 2015 00:34:14 +0800
>>
>>> -     depends on (PCI || OF_IRQ)
>>> +     depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))
>>
>> Having GENERIC_PCI_IOMAP withut PCI makes absolutely no sense to me.
>>
>> I can't see why a platform would provide GENERIC_PCI_IOMAP when it
>> does not have PCI enabled.
>
> My understanding for GENERIC_PCI_IOMAP is to enable providing empty
> pci_iomap/pci_iounmap.

There is also a non-empty implementation that calls to ioremap/iounmap
(compiled in when PCI=y). So it's rather an option that every
architecture should use if they don't have any special requirements
(which seems to be true about SH, too). Maybe a name like
"MY_ARCH_DOESNT_NEED_ANYTHING_SPECIAL_FROM_PCI_IOMAP" instead of
GENERIC_PCI_IOMAP would be more reflective of its current semantics,
but it doesn't look very pretty anyway :)

> Digging in git log, arch that used to provide such empty instances
> moved to a centralized implementation by using GENERIC_PCI_IOMAP.
> For me such empty functions should be available by default when PCI is
> not set, but this is not the case today.

If we trust the commit message in
97a29d59fc222b36bac3ee3a8ae994f65bf7ffdf
it looks like architectures that use their own PCI mapping functions
would provide them unconditionally, so we shouldn't unconditionally
include stubs in the generic header to avoid double definition.

I think the way to fix this most consistently with what other arches
do would be to move "select GENERIC_PCI_IOMAP" from arch/sh/Kconfig
outside of "config PCI" (towards line 40 or so in the same file). This
will ensure that all NOP stubs are applied correctly to avoid such
compile-time errors.

Happy to provide a 2-line patch to do the above, but I don't have any
SH hardware to test.

Best,
Alexey

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[David Miller]

From: Antonio Borneo <borneo.antonio@gmail.com>
Date: Thu, 23 Jul 2015 09:35:51 +0800

> Digging in git log, arch that used to provide such empty instances
> moved to a centralized implementation by using GENERIC_PCI_IOMAP.
> For me such empty functions should be available by default when PCI is
> not set, but this is not the case today.

Then that is what we should fix instead of these hackish per-driver
patches.

Make it so that these fallback implementations always are present
when PCI is not set.

[PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[Sudip Mukherjee]

The builds of allmodconfig of avr32 is failing with:

drivers/net/ethernet/via/via-rhine.c:1098:2: error: implicit declaration
of function 'pci_iomap' [-Werror=implicit-function-declaration]
drivers/net/ethernet/via/via-rhine.c:1119:2: error: implicit declaration
of function 'pci_iounmap' [-Werror=implicit-function-declaration]

The generic empty pci_iomap and pci_iounmap is used only if CONFIG_PCI
is not defined and CONFIG_GENERIC_PCI_IOMAP is defined.

Add GENERIC_PCI_IOMAP in the dependency list for VIA_RHINE as we are
getting build failure when CONFIG_PCI and CONFIG_GENERIC_PCI_IOMAP both
are not defined.

Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
---

Failed build log is at:
https://travis-ci.org/sudipm-mukherjee/parport/jobs/81127188

An exactly similar patch has been posted earlier and relevant discussion
is at:
https://patchwork.ozlabs.org/patch/498722/

I posted a patch to include empty pci_iomap, pci_iounmap in avr32. That
discussion is at:
https://lkml.org/lkml/2015/9/19/100

 drivers/net/ethernet/via/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/via/Kconfig b/drivers/net/ethernet/via/Kconfig
index 2f1264b..17831e5 100644
--- a/drivers/net/ethernet/via/Kconfig
+++ b/drivers/net/ethernet/via/Kconfig
@@ -17,7 +17,7 @@ if NET_VENDOR_VIA
 
 config VIA_RHINE
 	tristate "VIA Rhine support"
-	depends on (PCI || OF_IRQ)
+	depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))
 	depends on HAS_DMA
 	select CRC32
 	select MII
-- 
1.9.1

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[Sergei Shtylyov]

Hello.

On 9/24/2015 1:16 PM, Sudip Mukherjee wrote:

> The builds of allmodconfig of avr32 is failing with:
>
> drivers/net/ethernet/via/via-rhine.c:1098:2: error: implicit declaration
> of function 'pci_iomap' [-Werror=implicit-function-declaration]
> drivers/net/ethernet/via/via-rhine.c:1119:2: error: implicit declaration
> of function 'pci_iounmap' [-Werror=implicit-function-declaration]
>
> The generic empty pci_iomap and pci_iounmap is used only if CONFIG_PCI
> is not defined and CONFIG_GENERIC_PCI_IOMAP is defined.
>
> Add GENERIC_PCI_IOMAP in the dependency list for VIA_RHINE as we are
> getting build failure when CONFIG_PCI and CONFIG_GENERIC_PCI_IOMAP both
> are not defined.
>
> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
> ---
>
> Failed build log is at:
> https://travis-ci.org/sudipm-mukherjee/parport/jobs/81127188
>
> An exactly similar patch has been posted earlier and relevant discussion
> is at:
> https://patchwork.ozlabs.org/patch/498722/
>
> I posted a patch to include empty pci_iomap, pci_iounmap in avr32. That
> discussion is at:
> https://lkml.org/lkml/2015/9/19/100
>
>   drivers/net/ethernet/via/Kconfig | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/via/Kconfig b/drivers/net/ethernet/via/Kconfig
> index 2f1264b..17831e5 100644
> --- a/drivers/net/ethernet/via/Kconfig
> +++ b/drivers/net/ethernet/via/Kconfig
> @@ -17,7 +17,7 @@ if NET_VENDOR_VIA
>
>   config VIA_RHINE
>   	tristate "VIA Rhine support"
> -	depends on (PCI || OF_IRQ)
> +	depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))

   Could you remove outer braces, while at it?

[...]

MBR, Sergei

Re: [PATCH] net: via/Kconfig: GENERIC_PCI_IOMAP required if PCI not selected

[David Miller]

From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Fri, 25 Sep 2015 21:03:52 +0300

> Hello.
> 
> On 9/24/2015 1:16 PM, Sudip Mukherjee wrote:
> 
>> The builds of allmodconfig of avr32 is failing with:
>>
>> drivers/net/ethernet/via/via-rhine.c:1098:2: error: implicit
>> declaration
>> of function 'pci_iomap' [-Werror=implicit-function-declaration]
>> drivers/net/ethernet/via/via-rhine.c:1119:2: error: implicit
>> declaration
>> of function 'pci_iounmap' [-Werror=implicit-function-declaration]
>>
>> The generic empty pci_iomap and pci_iounmap is used only if CONFIG_PCI
>> is not defined and CONFIG_GENERIC_PCI_IOMAP is defined.
>>
>> Add GENERIC_PCI_IOMAP in the dependency list for VIA_RHINE as we are
>> getting build failure when CONFIG_PCI and CONFIG_GENERIC_PCI_IOMAP
>> both
>> are not defined.
>>
>> Signed-off-by: Sudip Mukherjee <sudip@vectorindia.org>
 ...
>> @@ -17,7 +17,7 @@ if NET_VENDOR_VIA
>>
>>   config VIA_RHINE
>>   	tristate "VIA Rhine support"
>> -	depends on (PCI || OF_IRQ)
>> +	depends on (PCI || (OF_IRQ && GENERIC_PCI_IOMAP))
> 
>   Could you remove outer braces, while at it?

I took care of this while applying this patch, thanks everyone.