why usr/include/linux/if_frad.h is required

why usr/include/linux/if_frad.h is required

[Jaswinder Singh Rajput]

For if_frad.h I am getting two warnings with 'make headers_check':
  usr/include/linux/if_frad.h:29: leaks CONFIG_DLCI to userspace where it is not valid
  usr/include/linux/if_frad.h:129: leaks CONFIG_DLCI to userspace where it is not valid

Almost all if_frad.h is covered with #if defined(CONFIG_DLCI) || defined(CONFIG_DLCI_MODULE) 

If I remove this portion so only this what we get in userspace:

#ifndef _FRAD_H_
#define _FRAD_H_

#include <linux/if.h>

#endif

So I am surprising what is the point of this ? 

Can I change its name to is_fraud ;-)

--
JSR

Re: why usr/include/linux/if_frad.h is required

[Jaswinder Singh Rajput]

CC: LKML

On Tue, 2009-01-20 at 20:23 +0530, Jaswinder Singh Rajput wrote:
> For if_frad.h I am getting two warnings with 'make headers_check':
>   usr/include/linux/if_frad.h:29: leaks CONFIG_DLCI to userspace where it is not valid
>   usr/include/linux/if_frad.h:129: leaks CONFIG_DLCI to userspace where it is not valid
> 
> Almost all if_frad.h is covered with #if defined(CONFIG_DLCI) || defined(CONFIG_DLCI_MODULE) 
> 
> If I remove this portion so only this what we get in userspace:
> 
> #ifndef _FRAD_H_
> #define _FRAD_H_
> 
> #include <linux/if.h>
> 
> #endif
> 
> So I am surprising what is the point of this ? 
> 
> Can I change its name to is_fraud ;-)
> 
> --
> JSR

Re: why usr/include/linux/if_frad.h is required

[Arnd Bergmann]

On Tuesday 20 January 2009, Jaswinder Singh Rajput wrote:
> CC: LKML
> 
> On Tue, 2009-01-20 at 20:23 +0530, Jaswinder Singh Rajput wrote:
> > For if_frad.h I am getting two warnings with 'make headers_check':
> >   usr/include/linux/if_frad.h:29: leaks CONFIG_DLCI to userspace where it is not valid
> >   usr/include/linux/if_frad.h:129: leaks CONFIG_DLCI to userspace where it is not valid
> > 
> > Almost all if_frad.h is covered with #if defined(CONFIG_DLCI) || defined(CONFIG_DLCI_MODULE) 
> > 
> > If I remove this portion so only this what we get in userspace:
> > 
> > #ifndef _FRAD_H_
> > #define _FRAD_H_
> > 
> > #include <linux/if.h>
> > 
> > #endif
> > 
> > So I am surprising what is the point of this ? 
> > 
> > Can I change its name to is_fraud ;-)

The CONFIG_DLCI check in there is clearly in error, the definition in there
is a user interface. The obvious solution is to drop the #if / #endif pair
without a replacement.

	Arnd <><

Re: why usr/include/linux/if_frad.h is required

[Krzysztof Halasa]

Arnd Bergmann <arnd@arndb.de> writes:

> The CONFIG_DLCI check in there is clearly in error, the definition in there
> is a user interface. The obvious solution is to drop the #if / #endif pair
> without a replacement.

I think so.
OTOH I'd be really surprised if anyone on the planet still used this
code/driver. IIRC and AFAICS it is (sdla.c coupled with dlci.c) the
first driver for the old Sangoma ISA sync serial cards (S502 - S508,
i.e. those based on Z80CPU doing HDLC and other things in firmware).


There was also a second(?) driver for those cards (official from
Sangoma), but:

commit 8db60bcf3021921e2d10d158641792d640e52fe8
Author: Adrian Bunk <bunk@stusta.de>
Date:   Tue Apr 11 17:28:33 2006 -0700

    [WAN]: Remove broken and unmaintained Sangoma drivers.
    
    The in-kernel Sangoma drivers are both not compiling and marked as BROKEN
    since at least kernel 2.6.0.
    
    Sangoma offers out-of-tree drivers, and David Mandelstam told me Sangoma
    does no longer maintain the in-kernel drivers and prefers to provide them
    as a separate installation package.
    
    This patch therefore removes these drivers.


They claim support for "All S-Series Legacy Cards" with out-of-tree
drivers available from www.sangoma.com.
-- 
Krzysztof Halasa

Re: why usr/include/linux/if_frad.h is required

[Jaswinder Singh Rajput]

On Tue, 2009-01-20 at 20:53 +0100, Krzysztof Halasa wrote:
> Arnd Bergmann <arnd@arndb.de> writes:
> 
> > The CONFIG_DLCI check in there is clearly in error, the definition in there
> > is a user interface. The obvious solution is to drop the #if / #endif pair
> > without a replacement.
> 
> I think so.
> OTOH I'd be really surprised if anyone on the planet still used this
> code/driver. IIRC and AFAICS it is (sdla.c coupled with dlci.c) the
> first driver for the old Sangoma ISA sync serial cards (S502 - S508,
> i.e. those based on Z80CPU doing HDLC and other things in firmware).
> 
> 

usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
and no body is complaining about it so it means no body is using it.

So should we need to drop #if / #endif pair or the whole file from
userspace.

--
JSR

Re: why usr/include/linux/if_frad.h is required

[Krzysztof Halasa]

Jaswinder Singh Rajput <jaswinder@kernel.org> writes:

> usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
> and no body is complaining about it so it means no body is using it.
>
> So should we need to drop #if / #endif pair or the whole file from
> userspace.

I think the file. "Empty file exported to userspace", long unused. We
can also have it back there, and it being not exported is an indicator
that it's not used. I guess the #ifdef __KERNEL__ can be removed, too.
-- 
Krzysztof Halasa

Re: why usr/include/linux/if_frad.h is required

[Jaswinder Singh Rajput]

On Wed, Jan 21, 2009 at 6:36 AM, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> Jaswinder Singh Rajput <jaswinder@kernel.org> writes:
>
>> usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
>> and no body is complaining about it so it means no body is using it.
>>
>> So should we need to drop #if / #endif pair or the whole file from
>> userspace.
>
> I think the file. "Empty file exported to userspace", long unused. We
> can also have it back there, and it being not exported is an indicator
> that it's not used. I guess the #ifdef __KERNEL__ can be removed, too.

I will definately define #ifdef __KERNEL__ but I am also curious after
defining it there is no point of making empty
usr/include/linux/if_frad.h

--
JSR

Re: why usr/include/linux/if_frad.h is required

[Sam Ravnborg]

On Wed, Jan 21, 2009 at 10:20:17AM +0530, Jaswinder Singh Rajput wrote:
> On Wed, Jan 21, 2009 at 6:36 AM, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > Jaswinder Singh Rajput <jaswinder@kernel.org> writes:
> >
> >> usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
> >> and no body is complaining about it so it means no body is using it.
> >>
> >> So should we need to drop #if / #endif pair or the whole file from
> >> userspace.
> >
> > I think the file. "Empty file exported to userspace", long unused. We
> > can also have it back there, and it being not exported is an indicator
> > that it's not used. I guess the #ifdef __KERNEL__ can be removed, too.
> 
> I will definately define #ifdef __KERNEL__ but I am also curious after
> defining it there is no point of making empty
> usr/include/linux/if_frad.h

Googling a bit did not turn up _any_ non-kernel hits that has relevance.
So based on the information given in this thread I strongly
suggest to drop the export of this header.

	Sam

Re: why usr/include/linux/if_frad.h is required

[David Miller]

From: Sam Ravnborg <sam@ravnborg.org>
Date: Wed, 21 Jan 2009 06:33:10 +0100

> On Wed, Jan 21, 2009 at 10:20:17AM +0530, Jaswinder Singh Rajput wrote:
> > On Wed, Jan 21, 2009 at 6:36 AM, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > > Jaswinder Singh Rajput <jaswinder@kernel.org> writes:
> > >
> > >> usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
> > >> and no body is complaining about it so it means no body is using it.
> > >>
> > >> So should we need to drop #if / #endif pair or the whole file from
> > >> userspace.
> > >
> > > I think the file. "Empty file exported to userspace", long unused. We
> > > can also have it back there, and it being not exported is an indicator
> > > that it's not used. I guess the #ifdef __KERNEL__ can be removed, too.
> > 
> > I will definately define #ifdef __KERNEL__ but I am also curious after
> > defining it there is no point of making empty
> > usr/include/linux/if_frad.h
> 
> Googling a bit did not turn up _any_ non-kernel hits that has relevance.
> So based on the information given in this thread I strongly
> suggest to drop the export of this header.

Sure, but on the other hand this makes all of the userland APIs
essentially inaccessible and undefined.

I bet Sangoma's internal tools reference this stuff.

Re: why usr/include/linux/if_frad.h is required

[Stephen Hemminger]

On Tue, 20 Jan 2009 22:15:04 -0800 (PST)
David Miller <davem@davemloft.net> wrote:

> From: Sam Ravnborg <sam@ravnborg.org>
> Date: Wed, 21 Jan 2009 06:33:10 +0100
> 
> > On Wed, Jan 21, 2009 at 10:20:17AM +0530, Jaswinder Singh Rajput wrote:
> > > On Wed, Jan 21, 2009 at 6:36 AM, Krzysztof Halasa <khc@pm.waw.pl> wrote:
> > > > Jaswinder Singh Rajput <jaswinder@kernel.org> writes:
> > > >
> > > >> usr/include/linux/if_frad.h is covered with CONFIG_DLCI from many years
> > > >> and no body is complaining about it so it means no body is using it.
> > > >>
> > > >> So should we need to drop #if / #endif pair or the whole file from
> > > >> userspace.
> > > >
> > > > I think the file. "Empty file exported to userspace", long unused. We
> > > > can also have it back there, and it being not exported is an indicator
> > > > that it's not used. I guess the #ifdef __KERNEL__ can be removed, too.
> > > 
> > > I will definately define #ifdef __KERNEL__ but I am also curious after
> > > defining it there is no point of making empty
> > > usr/include/linux/if_frad.h
> > 
> > Googling a bit did not turn up _any_ non-kernel hits that has relevance.
> > So based on the information given in this thread I strongly
> > suggest to drop the export of this header.
> 
> Sure, but on the other hand this makes all of the userland APIs
> essentially inaccessible and undefined.
> 
> I bet Sangoma's internal tools reference this stuff.
> --
> 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

There are no references if_frad.h in the version of Sangoma (out of tree) code
that we use in Vyatta. 

Re: why usr/include/linux/if_frad.h is required

[David Miller]

From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 21 Jan 2009 17:38:38 +1100

> There are no references if_frad.h in the version of Sangoma (out of
> tree) code that we use in Vyatta.

frad-0.20:src/dlcicfg.c

It includes linux/if_frad.h and also performs the DLCI_SET_CONF
ioctl.

Enough evidence to show we need to keep this header in the
export set.  Let's just kill the bogus CONFIG_* ifdef
tests in there.

Re: why usr/include/linux/if_frad.h is required

[Sam Ravnborg]

On Wed, Jan 21, 2009 at 03:37:12PM -0800, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 21 Jan 2009 17:38:38 +1100
> 
> > There are no references if_frad.h in the version of Sangoma (out of
> > tree) code that we use in Vyatta.
> 
> frad-0.20:src/dlcicfg.c
> 
> It includes linux/if_frad.h and also performs the DLCI_SET_CONF
> ioctl.
> 
> Enough evidence to show we need to keep this header in the
> export set.  Let's just kill the bogus CONFIG_* ifdef
> tests in there.

Thanks for investigating David.

	Sam

Re: why usr/include/linux/if_frad.h is required

[Jaswinder Singh Rajput]

On Wed, 2009-01-21 at 15:37 -0800, David Miller wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 21 Jan 2009 17:38:38 +1100
> 
> > There are no references if_frad.h in the version of Sangoma (out of
> > tree) code that we use in Vyatta.
> 
> frad-0.20:src/dlcicfg.c
> 
> It includes linux/if_frad.h and also performs the DLCI_SET_CONF
> ioctl.
> 
> Enough evidence to show we need to keep this header in the
> export set.  Let's just kill the bogus CONFIG_* ifdef
> tests in there.

After killing CONFIG_DLCI and CONFIG_DLCI_MODULE, it is also killing the
kernel:

  CC      net/socket.o
In file included from net/socket.c:75:
include/linux/if_frad.h:170: error: ‘CONFIG_DLCI_MAX’ undeclared here (not in a function)
make[1]: *** [net/socket.o] Error 1
make: *** [net] Error 2

because these things still depends on CONFIG_DLCI:

struct frad_local
{
   struct net_device_stats stats;

   /* devices which this FRAD is slaved to */
   struct net_device     *master[CONFIG_DLCI_MAX];
   short             dlci[CONFIG_DLCI_MAX];

Are you still sure we do not need CONFIG_DLCI.

--
JSR

Re: why usr/include/linux/if_frad.h is required

[Krzysztof Halasa]

Jaswinder Singh Rajput <jaswinder@kernel.org> writes:

> After killing CONFIG_DLCI and CONFIG_DLCI_MODULE, it is also killing the
> kernel:
>
>   CC      net/socket.o
> In file included from net/socket.c:75:
> include/linux/if_frad.h:170: error: ‘CONFIG_DLCI_MAX’ undeclared here (not in a function)

Perhaps this would do (untested).
Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

diff --git a/include/linux/if_frad.h b/include/linux/if_frad.h
index 5c34240..60e16a5 100644
--- a/include/linux/if_frad.h
+++ b/include/linux/if_frad.h
@@ -26,8 +26,6 @@
 
 #include <linux/if.h>
 
-#if defined(CONFIG_DLCI) || defined(CONFIG_DLCI_MODULE)
-
 /* Structures and constants associated with the DLCI device driver */
 
 struct dlci_add
@@ -127,6 +125,8 @@ struct frad_conf
 
 #ifdef __KERNEL__
 
+#if defined(CONFIG_DLCI) || defined(CONFIG_DLCI_MODULE)
+
 /* these are the fields of an RFC 1490 header */
 struct frhdr
 {
@@ -190,12 +190,10 @@ struct frad_local
    int               buffer;		/* current buffer for S508 firmware */
 };
 
-#endif /* __KERNEL__ */
-
 #endif /* CONFIG_DLCI || CONFIG_DLCI_MODULE */
 
-#ifdef __KERNEL__
 extern void dlci_ioctl_set(int (*hook)(unsigned int, void __user *));
-#endif
+
+#endif /* __KERNEL__ */
 
 #endif

Re: why usr/include/linux/if_frad.h is required

[David Miller]

From: Krzysztof Halasa <khc@pm.waw.pl>
Date: Sat, 24 Jan 2009 16:32:55 +0100

> Jaswinder Singh Rajput <jaswinder@kernel.org> writes:
> 
> > After killing CONFIG_DLCI and CONFIG_DLCI_MODULE, it is also killing the
> > kernel:
> >
> >   CC      net/socket.o
> > In file included from net/socket.c:75:
> > include/linux/if_frad.h:170: error: ‘CONFIG_DLCI_MAX’ undeclared here (not in a function)
> 
> Perhaps this would do (untested).
> Signed-off-by: Krzysztof Hałasa <khc@pm.waw.pl>

This looks good, applied, thanks everyone.