Patchwork isdn: hisax: netjet requires VIRT_TO_BUS

login
register
mail settings
Submitter Arnd Bergmann
Date March 14, 2013, 9:56 p.m.
Message ID <1363298204-8014-4-git-send-email-arnd@arndb.de>
Download mbox | patch
Permalink /patch/227809/
State Accepted
Delegated to: David Miller
Headers show

Comments

Arnd Bergmann - March 14, 2013, 9:56 p.m.
Disabling CONFIG_VIRT_TO_BUS on ARM showed that the hisax netjet
driver depends on this deprecated functionality but is not
marked so in Kconfig.

Rather than adding ARM to the already long list of architectures
that this driver is broken on, this patch adds 'depends on
VIRT_TO_BUS' and removes the dependency on !SPARC, which is
also implied by that.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Cc: Karsten Keil <isdn@linux-pingi.de>
Cc: netdev@vger.kernel.org
---
Please apply for 3.9 or 3.10, this one is not urgent

 drivers/isdn/hisax/Kconfig | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
Geert Uytterhoeven - March 15, 2013, 6:26 a.m.
On Thu, Mar 14, 2013 at 10:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> Disabling CONFIG_VIRT_TO_BUS on ARM showed that the hisax netjet
> driver depends on this deprecated functionality but is not
> marked so in Kconfig.
>
> Rather than adding ARM to the already long list of architectures
> that this driver is broken on, this patch adds 'depends on
> VIRT_TO_BUS' and removes the dependency on !SPARC, which is
> also implied by that.

IIRC, the real "arch" dependency for this driver is !BIG_ENDIAN, but
unfortunately
we don't have a generic Kconfig symbol for that.
Perhaps we may want to introduce that?
Of course we prefer to make drivers work on all endianness instead...

> --- a/drivers/isdn/hisax/Kconfig
> +++ b/drivers/isdn/hisax/Kconfig
> @@ -237,7 +237,8 @@ config HISAX_MIC
>
>  config HISAX_NETJET
>         bool "NETjet card"
> -       depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
> +       depends on PCI && (BROKEN || !(PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
> +       depends on VIRT_TO_BUS
>         help
>           This enables HiSax support for the NetJet from Traverse
>           Technologies.
> @@ -248,7 +249,8 @@ config HISAX_NETJET
>
>  config HISAX_NETJET_U
>         bool "NETspider U card"
> -       depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
> +       depends on PCI && (BROKEN || !(PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
> +       depends on VIRT_TO_BUS

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 15, 2013, 10:15 a.m.
On Friday 15 March 2013, Geert Uytterhoeven wrote:
> On Thu, Mar 14, 2013 at 10:56 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> > Disabling CONFIG_VIRT_TO_BUS on ARM showed that the hisax netjet
> > driver depends on this deprecated functionality but is not
> > marked so in Kconfig.
> >
> > Rather than adding ARM to the already long list of architectures
> > that this driver is broken on, this patch adds 'depends on
> > VIRT_TO_BUS' and removes the dependency on !SPARC, which is
> > also implied by that.
> 
> IIRC, the real "arch" dependency for this driver is !BIG_ENDIAN, but
> unfortunately
> we don't have a generic Kconfig symbol for that.
> Perhaps we may want to introduce that?
> Of course we prefer to make drivers work on all endianness instead...

CONFIG_VIRT_TO_BUS is certainly also a dependency, since it fails
to build without that. I think we can address the two problems separately.

David Howells brought up the topic of endian checks recently, noting
that the way we define __BIG_ENDIAN and __LITTLE_ENDIAN in the kernel
is incompatible to how do it in user space, and not much better either.

A few architectures that are bi-endian (arc, arm, c6x, mips, sh) already
have CONFIG_CPU_IS_LITTLE_ENDIAN and CONFIG_CPU_IS_BIG_ENDIAN. I guess
it would be a good idea to provide these on all architectures.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 15, 2013, 12:16 p.m.
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 15 Mar 2013 10:15:00 +0000

> David Howells brought up the topic of endian checks recently, noting
> that the way we define __BIG_ENDIAN and __LITTLE_ENDIAN in the kernel
> is incompatible to how do it in user space, and not much better either.
> 
> A few architectures that are bi-endian (arc, arm, c6x, mips, sh) already
> have CONFIG_CPU_IS_LITTLE_ENDIAN and CONFIG_CPU_IS_BIG_ENDIAN. I guess
> it would be a good idea to provide these on all architectures.

I do not want to see us add such a Kconfig dependency knob.

Then the real tendency will exist to make new drivers little-endian
only, refuse to fix endian-broken old drivers, etc.

Which means that allmodconfig on my architecture will have build
coverage on less code, which is really the only thing that matters
for me.  I want all drivers that could be effected by my changes
to be compile testable on as many platforms as possible.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 15, 2013, 12:29 p.m.
On Friday 15 March 2013, David Miller wrote:
> I do not want to see us add such a Kconfig dependency knob.
> 
> Then the real tendency will exist to make new drivers little-endian
> only, refuse to fix endian-broken old drivers, etc.
> 
> Which means that allmodconfig on my architecture will have build
> coverage on less code, which is really the only thing that matters
> for me.  I want all drivers that could be effected by my changes
> to be compile testable on as many platforms as possible.

There are two separate issues here. The first one that David Howells
brought up was ill-defined __BIG_ENDIAN/__LITTLE_ENDIAN macros.
Using CONFIG_CPU_IS_BIG_ENDIAN/CONFIG_CPU_IS_LITTLE_ENDIAN in the code
is what Linus suggested as a replacement, although I see little
incentive to do mass conversion there, it would be mainly for new
code.

The other issue is the Kconfig logic where Geert would replace
all the instances of "depends on BROKEN || !(SPARC || PPC || PARISC ||
M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA &&
!CPU_LITTLE_ENDIAN))" with something that actually reflects what
we mean. I think it would be a nice cleanup, but I can also understand
your hesitation there.

Do you object to both uses of that symbol, or just to using it in order
to disable drivers in Kconfig? I don't care too much right now, since
nothing is actually broken at the moment, I would just like to get
the VIRT_TO_BUS patch merged so people can also build allmodconfig
on my architecture ;-)

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 15, 2013, 12:33 p.m.
From: Arnd Bergmann <arnd@arndb.de>
Date: Fri, 15 Mar 2013 12:29:52 +0000

> On Friday 15 March 2013, David Miller wrote:
>> I do not want to see us add such a Kconfig dependency knob.
>> 
>> Then the real tendency will exist to make new drivers little-endian
>> only, refuse to fix endian-broken old drivers, etc.
>> 
>> Which means that allmodconfig on my architecture will have build
>> coverage on less code, which is really the only thing that matters
>> for me.  I want all drivers that could be effected by my changes
>> to be compile testable on as many platforms as possible.
> 
> There are two separate issues here. The first one that David Howells
> brought up was ill-defined __BIG_ENDIAN/__LITTLE_ENDIAN macros.
> Using CONFIG_CPU_IS_BIG_ENDIAN/CONFIG_CPU_IS_LITTLE_ENDIAN in the code
> is what Linus suggested as a replacement, although I see little
> incentive to do mass conversion there, it would be mainly for new
> code.
> 
> The other issue is the Kconfig logic where Geert would replace
> all the instances of "depends on BROKEN || !(SPARC || PPC || PARISC ||
> M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA &&
> !CPU_LITTLE_ENDIAN))" with something that actually reflects what
> we mean. I think it would be a nice cleanup, but I can also understand
> your hesitation there.
> 
> Do you object to both uses of that symbol, or just to using it in order
> to disable drivers in Kconfig? I don't care too much right now, since
> nothing is actually broken at the moment, I would just like to get
> the VIRT_TO_BUS patch merged so people can also build allmodconfig
> on my architecture ;-)

The first usage seems reason, but the temtation is going to be quite
strong to misuse to block out drivers when there is "no value" in
spending time necessary to simply make them endian clean instead.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arnd Bergmann - March 15, 2013, 1:41 p.m.
On Friday 15 March 2013, David Miller wrote:
> The first usage seems reason, but the temtation is going to be quite
> strong to misuse to block out drivers when there is "no value" in
> spending time necessary to simply make them endian clean instead.

Right. I'll let someone else start that discussion then if they
really want to see that patch.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller - March 17, 2013, 4:01 p.m.
From: Arnd Bergmann <arnd@arndb.de>
Date: Thu, 14 Mar 2013 22:56:41 +0100

> Disabling CONFIG_VIRT_TO_BUS on ARM showed that the hisax netjet
> driver depends on this deprecated functionality but is not
> marked so in Kconfig.
> 
> Rather than adding ARM to the already long list of architectures
> that this driver is broken on, this patch adds 'depends on
> VIRT_TO_BUS' and removes the dependency on !SPARC, which is
> also implied by that.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/isdn/hisax/Kconfig b/drivers/isdn/hisax/Kconfig
index 5313c9e..d9edcc9 100644
--- a/drivers/isdn/hisax/Kconfig
+++ b/drivers/isdn/hisax/Kconfig
@@ -237,7 +237,8 @@  config HISAX_MIC
 
 config HISAX_NETJET
 	bool "NETjet card"
-	depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
+	depends on PCI && (BROKEN || !(PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
+	depends on VIRT_TO_BUS
 	help
 	  This enables HiSax support for the NetJet from Traverse
 	  Technologies.
@@ -248,7 +249,8 @@  config HISAX_NETJET
 
 config HISAX_NETJET_U
 	bool "NETspider U card"
-	depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
+	depends on PCI && (BROKEN || !(PPC || PARISC || M68K || (MIPS && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && !CPU_LITTLE_ENDIAN)))
+	depends on VIRT_TO_BUS
 	help
 	  This enables HiSax support for the Netspider U interface ISDN card
 	  from Traverse Technologies.