Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 6405 bytes --]

Hello Stephen,

thank you very much for your review and your comments.

On Mon, 12 Dec 2005 09:41:38 -0800, you wrote:

> Networking drivers belong on the netdev@vger.kernel.org list

The Gigaset driver isn't in fact a networking driver; but if you think
it's appropriate to CC the netdev list anyway then we'll be glad to do
so when we'll resubmit it with the modifications prompted by the
comments we received so far.

> One of the principles of drivers, which is often overlooked
> is to use existing infrastructure for debugging and interfaces and not invent
> driver specific versions.

Quite so. We're happy to use existing infrastructure wherever
available. Thanks for pointing out some of it we had overlooked.

[Following quotes edited for brevity]

>> --- linux-2.6.14/drivers/isdn/gigaset/gigaset.h	1970-01-01 01:00:00.000000000 +0100
>> +++ linux-2.6.14-gig/drivers/isdn/gigaset/gigaset.h	2005-12-11 13:21:42.000000000 +0100

>> +enum debuglevel { /* up to 24 bits (atomic_t) */
>> +	DEBUG_REG	  = 0x0002, /* serial port I/O register operations */
>> +	DEBUG_OPEN	  = 0x0004, /* open/close serial port */
>> +	DEBUG_INTR	  = 0x0008, /* interrupt processing */
>> +	DEBUG_INTR_DUMP   = 0x0010, /* Activating hexdump debug output on interrupt
>> +				      requests, not available as run-time option */
>> +	DEBUG_CMD	  = 0x00020, /* sent/received LL commands */
>> +	DEBUG_STREAM	  = 0x00040, /* application data stream I/O events */
>> +	DEBUG_STREAM_DUMP = 0x00080, /* application data stream content */
>> +	DEBUG_LLDATA	  = 0x00100, /* sent/received LL data */
>> +	DEBUG_INTR_0	  = 0x00200, /* serial port output interrupt processing */
>> +	DEBUG_DRIVER	  = 0x00400, /* driver structure */
>> +	DEBUG_HDLC	  = 0x00800, /* M10x HDLC processing */
>> +	DEBUG_WRITE	  = 0x01000, /* M105 data write */
>> +	DEBUG_TRANSCMD    = 0x02000, /*AT-COMMANDS+RESPONSES*/
>> +	DEBUG_MCMD        = 0x04000, /*COMMANDS THAT ARE SENT VERY OFTEN*/
>> +	DEBUG_INIT	  = 0x08000, /* (de)allocation+initialization of data structures */
>> +	DEBUG_LOCK	  = 0x10000, /* semaphore operations */
>> +	DEBUG_OUTPUT	  = 0x20000, /* output to device */
>> +	DEBUG_ISO         = 0x40000, /* isochronous transfers */
>> +	DEBUG_IF	  = 0x80000, /* character device operations */
>> +	DEBUG_USBREQ	  = 0x100000, /* USB communication (except payload data) */
>> +	DEBUG_LOCKCMD     = 0x200000, /* AT commands and responses when MS_LOCKED */
>> +
>> +	DEBUG_ANY	  = 0x3fffff, /* print message if any of the others is activated */
>> +};
>
> Use existing netdevice message flag values?

Unfortunately these don't fit our needs, as we are not dealing with a
network device, but with an ISDN device.

>> +/* define our own syslog macros which add our module name to the message */
>> +/* The space before the comma in ", ##" is needed by gcc 2.95 */
>> +#undef info
>> +#define info(format, arg...) printk(KERN_INFO "%s: " format "\n", THIS_MODULE ? THIS_MODULE->name : "gigaset_hw" , ## arg)
>> +
>> +#undef notice
>> +#define notice(format, arg...) printk(KERN_NOTICE "%s: " format "\n", THIS_MODULE ? THIS_MODULE->name : "gigaset_hw" , ## arg)
>> +
>> +#undef warn
>> +#define warn(format, arg...) printk(KERN_WARNING "%s: " format "\n", THIS_MODULE ? THIS_MODULE->name : "gigaset_hw" , ## arg)
>> +
>> +#undef err
>> +#define err(format, arg...) printk(KERN_ERR "%s: " format "\n", THIS_MODULE ? THIS_MODULE->name : "gigaset_hw" , ## arg)
>> +
>> +#undef dbg
>
> Gack. Don't reinvent more debug infrastructure. If you still need this stuff
> use the existing macros in kernel.h

I couldn't find any existing macros of that kind in kernel.h, so I
assume you are referring to the ones in usb.h.

The usb.h versions of info(), warn() and err() macros prepend the
entire source file path to each message, which isn't appropriate for
our purpose of informing users or administrators (as opposed to kernel
hackers). The module name is much more informative for that audience.
Also, a notice() macro is missing entirely. In that situation, rather
than reinventing the whole set of macros with new names, or cluttering
our code by repeatedly writing them out in full, we decided on
modifying the existing macros as the most readable solution.

We will however rephrase the comment preceding these definitions in
order to clarify our reasons for doing this.

>> +struct proc_atomic {

This will be elliminated by the move to sysfs.

>> +struct at_state_t {
>> +	struct at_state_t       *next, *prev;
>
> Use list.h

Thanks for the hint. Will do.

>> +/* ===========================================================================
>> + *  Functions implemented in asyncdata.c
>> + */
>> +
>> +//FIXME this should not be included by the base driver
>
> Then fix it before submitting??

Um, no. As we couldn't find a fix which wasn't worse than the problem,
we've decided to just remove that FIXME note.

Generally, however, if all FIXMEs must be fixed before a driver may be
submitted then we'll have to continue maintaining our driver outside the
kernel for quite some time yet. IMHO such a requirement would clearly
contradict the spirit of Documentation/stable_api_nonsense.txt.

Of course, removing all FIXME notes would be a matter of a simple
editor command. But I do not think the community would be well served
by that. ;-)

>> --- linux-2.6.14/drivers/isdn/gigaset/interface.c	1970-01-01 01:00:00.000000000 +0100
>> +++ linux-2.6.14-gig/drivers/isdn/gigaset/interface.c	2005-12-11 13:21:42.000000000 +0100

>> +static struct tty_operations if_ops = {
>> +	.open =			if_open,
>> +	.close =		if_close,
>> +	.ioctl =		if_ioctl,
>> +	.write =		if_write,
>> +	.write_room =		if_write_room,
>> +	.chars_in_buffer =	if_chars_in_buffer,
>> +	.set_termios =		if_set_termios,
>> +	.throttle =		if_throttle,
>> +	.unthrottle =		if_unthrottle,
>> +#if 0
>> +	.break_ctl =		serial_break,
>> +#endif
>> +	.tiocmget =		if_tiocmget,
>> +	.tiocmset =		if_tiocmset,
>> +};
>
> Missing .owner = THIS_MODULE

It appears that in the kernel versions I have been working with,
struct tty_operations does not have a member "owner".

Thanks again for your time and effort. Your comments are appreciated.

Regards
Tilman

PS: Please keep me CCed on any replies.

--
Tilman Schmidt                    E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Lee Revell]

On Mon, 2005-12-19 at 17:38 +0100, Tilman Schmidt wrote:
> Unfortunately these don't fit our needs, as we are not dealing with a
> network device, but with an ISDN device.

Um, isn't that what the N in ISDN stands for?

I guess what you mean is that although ISDN devices are obviously
networking devices, the kernel uses a separate subsystem for ISDN?

Lee

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Arjan van de Ven]

On Mon, 2005-12-19 at 12:01 -0500, Lee Revell wrote:
> On Mon, 2005-12-19 at 17:38 +0100, Tilman Schmidt wrote:
> > Unfortunately these don't fit our needs, as we are not dealing with a
> > network device, but with an ISDN device.
> 
> Um, isn't that what the N in ISDN stands for?
> 
> I guess what you mean is that although ISDN devices are obviously
> networking devices, the kernel uses a separate subsystem for 

that's not obvious. They're telephony like devices. they do voice and
data. They're not ethernet or even remotely like that. You can do ppp
over it tho... 

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 1399 bytes --]

On 2005-12-19 18:01, Lee Revell wrote:
> On Mon, 2005-12-19 at 17:38 +0100, Tilman Schmidt wrote:
> 
>>Unfortunately these don't fit our needs, as we are not dealing with a
>>network device, but with an ISDN device.
> 
> Um, isn't that what the N in ISDN stands for?

While the ISDN is indeed called a network, devices connecting a computer
to it are nevertheless not commonly referred to as network devices.

> I guess what you mean is that although ISDN devices are obviously
> networking devices, the kernel uses a separate subsystem for ISDN?

There's more to it than that. The notion of a "network" is a rather
broad one, including such diverse phenomena as Ethernet, ISDN, TV cable
or even roads or TV stations. The notion of a "network device", on the
other hand, is a quite specific one, at least in the computer world, and
it certainly doesn't include ISDN TAs.

In fact, the operation of an ISDN device is much closer to a modem or
even an answering machine than to that prototypical network device which
is the Ethernet card. This is of course the reason why the Linux kernel
puts them in a subsystem of their own. Making them net_device-s just
wouldn't work.

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Stephen Hemminger]

On Mon, 19 Dec 2005 20:22:42 +0100
Tilman Schmidt <tilman@imap.cc> wrote:

> On 2005-12-19 18:01, Lee Revell wrote:
> > On Mon, 2005-12-19 at 17:38 +0100, Tilman Schmidt wrote:
> > 
> >>Unfortunately these don't fit our needs, as we are not dealing with a
> >>network device, but with an ISDN device.
> > 
> > Um, isn't that what the N in ISDN stands for?
> 
> While the ISDN is indeed called a network, devices connecting a computer
> to it are nevertheless not commonly referred to as network devices.
> 
> > I guess what you mean is that although ISDN devices are obviously
> > networking devices, the kernel uses a separate subsystem for ISDN?
> 
> There's more to it than that. The notion of a "network" is a rather
> broad one, including such diverse phenomena as Ethernet, ISDN, TV cable
> or even roads or TV stations. The notion of a "network device", on the
> other hand, is a quite specific one, at least in the computer world, and
> it certainly doesn't include ISDN TAs.
> 
> In fact, the operation of an ISDN device is much closer to a modem or
> even an answering machine than to that prototypical network device which
> is the Ethernet card. This is of course the reason why the Linux kernel
> puts them in a subsystem of their own. Making them net_device-s just
> wouldn't work.
> 


My definition is simple. Any device driver that exports a netdevice
interface needs to be reviewed on netdev to make sure the assumptions
about network device semantics are being followed.

-- 
Stephen Hemminger <shemminger@osdl.org>
OSDL http://developer.osdl.org/~shemminger

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Tilman Schmidt]

[-- Attachment #1: Type: text/plain, Size: 554 bytes --]

On 19.12.2005 22:30, Stephen Hemminger wrote:

> My definition is simple. Any device driver that exports a netdevice
> interface needs to be reviewed on netdev to make sure the assumptions
> about network device semantics are being followed.

I agree with that definition. And based on that, the Gigaset driver is
not a network device driver.

-- 
Tilman Schmidt                          E-Mail: tilman@imap.cc
Bonn, Germany
Diese Nachricht besteht zu 100% aus wiederverwerteten Bits.
Ungeöffnet mindestens haltbar bis: (siehe Rückseite)


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 250 bytes --]

[PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Hansjoerg Lipp]

The following patches add drivers for the Siemens Gigaset 3070 family of
ISDN DECT PABXes connected via USB, either directly or over a DECT link
using a Gigaset M105 or compatible DECT data adapter. The devices are
integrated as ISDN adapters within the isdn4linux framework, supporting
incoming and outgoing voice and data connections, and also as tty
devices providing access to device specific AT commands.

Supported devices include models 3070, 3075, 4170, 4175, SX205, SX255,
and SX353 from the Siemens Gigaset product family, as well as the
technically identical models 45isdn and 721X from the Deutsche Telekom
Sinus series. Supported DECT adapters are the Gigaset M105 data and the
technically identical Gigaset USB Adapter DECT, Sinus 45 data 2, and
Sinus 721 data (but not the Gigaset M34 and Sinus 702 data which
advertise themselves as CDC-ACM devices).

These drivers have been developed over the last four years within the
SourceForge project http://sourceforge.net/projects/gigaset307x/. They
are being used successfully in several installations for dial-in
Internet access and for voice call switching with Asterisk.

This is our second attempt at submitting these drivers, taking into
account the comments we received to our first submission on 2005-12-11.

The patch set adds three kernel modules:

- a common module "gigaset" encapsulating the common logic for
  controlling the PABX and the interfaces to userspace and the
  isdn4linux subsystem.

- a connection-specific module "bas_gigaset" which handles
  communication with the PABX over a direct USB connection.

- a connection-specific module "usb_gigaset" which does the same
  for a DECT connection using the Gigaset M105 USB DECT adapter.

We also have a module "ser_gigaset" which supports the Gigaset M101
RS232 DECT adapter, but we didn't judge it fit for inclusion in the
kernel, as it does direct programming of a i8250 serial port. It
should probably be rewritten as a serial line discipline but so far we
lack the neccessary knowledge about writing a line discipline for that.

The drivers have been working with kernel releases 2.2 and 2.4 as well
as 2.6, and although we took efforts to remove the compatibility code
for this submission, it probably still shows in places. Please make
allowances.

The patch has been split into 9 parts to comply to size limits.
All of the parts are designed to be applied together.

The patches are based on kernel release 2.6.16-rc2, in particular with
respect to the recent tty changes. If it is more appropriate to base it
on 2.6.15 at this time please let us know.

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Andrew Morton]

Hansjoerg Lipp <hjlipp@web.de> wrote:
>
> The patch has been split into 9 parts to comply to size limits.
>  All of the parts are designed to be applied together.

This means that the patches should go into git in a single commit, so that
we don't have a non-compiling tree at any step.   That's not a problem.

[PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Hansjoerg Lipp]

The SourceForge project http://sourceforge.net/projects/gigaset307x/
has, over the last four years, developed Linux support for the Siemens
Gigaset 3070/3075/4170/4175/SX205/SX255 family of ISDN DECT PABXes,
connected to a PC either directly via USB or over a DECT link using
the M101/M105 DECT data adapters.
The devices are integrated as ISDN adapters within the isdn4linux
framework, as well as providing access to device specific commands
through a tty device.

After much encouragement from the USB and isdn4linux maintainers, we
have decided to submit our drivers for inclusion in the kernel source
tree.

The patch set that follows adds three kernel modules:

- a common module "gigaset" encapsulating the common logic for
  controlling the PABX and the interfaces to userspace and the
  isdn4linux subsystem.

- a connection-specific module "bas_gigaset" which handles
  communication with the PABX over a direct USB connection.

- a connection-specific module "usb_gigaset" which does the same
  for a DECT connection using the Gigaset M105 USB DECT adapter.

We also have a module "ser_gigaset" which supports the Gigaset M101
RS232 DECT adapter, but we didn't judge it fit for inclusion in the
kernel, as it does direct programming of a i8250 serial port. It
should probably be rewritten as a serial line discipline but so far we
lack the neccessary knowledge about writing a line discipline for that.

The drivers have been working with kernel releases 2.2 and 2.4 as well
as 2.6, and although we took efforts to remove the compatibility code
for this submission, it probably still shows in places. Please make
allowances.

The patch has been split into 9 parts to comply to size limits.
All of the parts are designed to be applied together.

Re: [PATCH 0/9] isdn4linux: add drivers for Siemens Gigaset ISDN DECT PABX

[Stephen Hemminger]

Networking drivers belong on the netdev@vger.kernel.org list