2005-09-04 11:05:42

by Giampaolo Tomassoni

[permalink] [raw]
Subject: [ATMSAR] Request for review - update #1

Dears,

thanks to Jiri Slaby who found a bug in the AAL0 handling of the ATMSAR module.

I attach a fixed version of the atmsar patch as a diff against the 2.6.13 kernel tree.

Now the sources are also fully-delethalized by avoiding any line wrap in the Linus' 80-column monitor and the danger due to whitespaces at end of lines is prevented.

However, I'm still hearing for your comments about the usefulness of an ATMSAR layer. I'm also trying hard to get in touch with Duncan Sands (SpeedTouch USB Driver maintainer) and Chas Williams (ATM maintainer) in order to get this patch reviewed and, eventually, committed.

How am I supposed to contact them? I had no reply to the mails I sent them...

Thanks for your interest in this,

-----------------------------------
Giampaolo Tomassoni - IT Consultant
Piazza VIII Aprile 1948, 4
I-53044 Chiusi (SI) - Italy
Ph: +39-0578-21100


Attachments:
patch-2.6.13+atmsar.diff (70.63 kB)

2005-09-04 12:03:01

by Francois Romieu

[permalink] [raw]
Subject: Re: [Linux-ATM-General] [ATMSAR] Request for review - update #1

Giampaolo Tomassoni <[email protected]> :
[...]
> However, I'm still hearing for your comments about the usefulness of an
> ATMSAR layer.

Afaik all but one pci ADSL modems are out of tree drivers and include various
level of proprietary code. If Duncan is not interested in changing its code,
the usefulness remains to be proven.

The codingstyle is broken. Please read again Documentation/CodingStyle,
remove the redundant typedef and the silly comments ("Reserve header space",
Encode packet into cells", ...).

- &page[strlen(page)] in atmProcRead sucks.
- "return" is not a function.
- consider 'goto' to handle the errors instead of deep nesting
- +const atmsar_aalops_t opsAALR = {
+ ATM_AAL0,
+ "raw",
-> use .foo = baz instead.

drivers/net/*c may give some hint.

--
Ueimor

2005-09-04 13:13:39

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

> -----Messaggio originale-----
> Da: [email protected]
> [mailto:[email protected]]Per conto di
> Francois Romieu
> Inviato: domenica 4 settembre 2005 14.01
> A: Giampaolo Tomassoni
> Cc: [email protected];
> [email protected]
> Oggetto: Re: [Linux-ATM-General] [ATMSAR] Request for review - update #1
>
>
> Giampaolo Tomassoni <[email protected]> :
> [...]
> > However, I'm still hearing for your comments about the usefulness of an
> > ATMSAR layer.
>
> Afaik all but one pci ADSL modems are out of tree drivers and
> include various
> level of proprietary code. If Duncan is not interested in
> changing its code,
> the usefulness remains to be proven.

Well, the idea is that more pci devices may appear, as adsl-enabled embedded systems will begin to appear in the market.

Also, I believe that adsl will carry much more services then just AAL5 for internet connection in the future. Even if the ATMSAR actually lacks of AAL1 and AAL2/3 capabilities, adding them in a single, specialized module is much easier than swimming in a usb+atm middle layer.

Finally, the fact that ATMSAR is device-unspecific makes it easier to maintain, I guess.


> The codingstyle is broken. Please read again Documentation/CodingStyle,

That's a matter of taste: even Linus burned the GNU coding style book...

However, if it is needed by the linux community, I shurely will fix it whenever the ATMSAR idea will get passed: I'm just gathering feedbacks like the previous one you expressed.


> remove the redundant typedef

Oh, you mean the "typedef enum _HECSTS ..." ?

You're right, thanks.


> and the silly comments ("Reserve
> header space",
> Encode packet into cells", ...).

I would prefer to explain better what the ATMSAR is doing there. So, I'll get your as a "clarify silly comments". Ok?


> - &page[strlen(page)] in atmProcRead sucks.

Why? It is preceded by an strcpy(page,...). A constant would be worse if someone changes the prefix string...

Or is a page (a pointer) + strlen(page) (an integer) preferred over a closed syntax?


> - "return" is not a function.

Not even for() or while(). But doesn't they look cute this way?


> - consider 'goto' to handle the errors instead of deep nesting

I prefer not using goto when not required to. Nesting is far more readable to my opinion. Compilers do work fine with both.

Anyway, which are the functions you are objecting?


> - +const atmsar_aalops_t opsAALR = {
> + ATM_AAL0,
> + "raw",
> -> use .foo = baz instead.

atmasr_aalops_t is not an exported structure (you'll find just an opaque definition in include/linux/atmsar.h), so it is not meant to be statically declared by device drivers. But I guess that the problem is readability, right?

Ok, I'm going to consider your hint in the next patch version.


> drivers/net/*c may give some hint.
>
> --
> Ueimor

Thank you for your help.

May I ask if this is just your own contribution or if you are in charge of something in the linux and/or linux-atm projects?

Regards,

-----------------------------------
Giampaolo Tomassoni - IT Consultant
Piazza VIII Aprile 1948, 4
I-53044 Chiusi (SI) - Italy
Ph: +39-0578-21100

2005-09-04 15:34:57

by Francois Romieu

[permalink] [raw]
Subject: Re: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

Giampaolo Tomassoni <[email protected]> :
[...]
> Well, the idea is that more pci devices may appear, as adsl-enabled
> embedded systems will begin to appear in the market.
>
> Also, I believe that adsl will carry much more services then just AAL5 for
> internet connection in the future.

I'd be happily surprized to see more documented ADSL PCI/USB device in the
near future. :o(

> Even if the ATMSAR actually lacks of AAL1 and AAL2/3 capabilities, adding
> them in a single, specialized module is much easier than swimming in a
> usb+atm middle layer.
>
> Finally, the fact that ATMSAR is device-unspecific makes it easier to
> maintain, I guess.

Ok. Your suggestion may have more impact if there is a patch to convert
the sole existing in-kernel driver to use this module.

[...]
> > The codingstyle is broken. Please read again Documentation/CodingStyle,
>
> That's a matter of taste: even Linus burned the GNU coding style book...

An uniform codingstyle is useful when people need to review code. Something
is wrong when a reviewer must uncipher a piece of code. You will find areas
in the kernel whose trends differ but a codingstyle from Mars is usually a
hint. So it is not _only_ a matter of taste.

> However, if it is needed by the linux community, I shurely will fix it
> whenever the ATMSAR idea will get passed: I'm just gathering feedbacks
> like the previous one you expressed.

You may have more feedback/review then. I only gave a cursory look at the
code.

[...]
> > remove the redundant typedef
>
> Oh, you mean the "typedef enum _HECSTS ..." ?

Rather the "typedef struct atmsar_dev atmsar_dev_t;" (yes, I know the "It
saves typing" argument). Maybe something could be done at the same time
regarding the need for the forward declarations.

[...]
> > and the silly comments ("Reserve
> > header space",
> > Encode packet into cells", ...).
>
> I would prefer to explain better what the ATMSAR is doing there. So, I'll
> get your as a "clarify silly comments". Ok?

s/what/why/

And no, documenting a call to skb_reserve is silly.

[...]
> > - &page[strlen(page)] in atmProcRead sucks.
>
> Why? It is preceded by an strcpy(page,...). A constant would be worse if
> someone changes the prefix string...

The value returned by sprintf and friends contains the needed offset, i.e.
buf += sprintf(buf, ...);.

[...]
> > - "return" is not a function.
>
> Not even for() or while(). But doesn't they look cute this way?

No.

for (), while (), return rc;

[...]
> > - consider 'goto' to handle the errors instead of deep nesting
>
> I prefer not using goto when not required to. Nesting is far more readable
> to my opinion.

OTOH, it makes ugly code to have it fit in a 80 columns console.

[...]
> Anyway, which are the functions you are objecting?

atmSend. Probably others.

If you can make the code look like existing in-kernel code (not fs/cifs
please) say network or ata driver code and you do not need goto, it's fine
too.

[...]
> > - +const atmsar_aalops_t opsAALR = {
> > + ATM_AAL0,
> > + "raw",
> > -> use .foo = baz instead.
>
> atmasr_aalops_t is not an exported structure (you'll find just an opaque
> definition in include/linux/atmsar.h), so it is not meant to be statically
> declared by device drivers. But I guess that the problem is readability,
> right?

struct foo zoy {
.bar = barbar,
.baz = bazbaz,
.quuz = ...
};

[...]
> May I ask if this is just your own contribution or if you are in charge of
> something in the linux and/or linux-atm projects?

/me scratches head

http://ww.google.com/search?hl=en&q=romieu+linux+cabal

--
Ueimor

2005-09-04 16:20:56

by Alistair John Strachan

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Sunday 04 September 2005 12:05, Giampaolo Tomassoni wrote:
> Dears,
>
> thanks to Jiri Slaby who found a bug in the AAL0 handling of the ATMSAR
> module.
>
> I attach a fixed version of the atmsar patch as a diff against the 2.6.13
> kernel tree.
>
[snip]

Just out of curiosity, is there ANY reason why this has to be done in the
kernel? The PPPoATM module for pppd implements (via linux-atm) a completely
userspace ATM decoder.. if anything, now redundant ATM stack code should be
REMOVED from Linux!

Most distributions (to my knowledge) supporting the speedtouch modem do so
using the method prescribed on speedtouch.sf.net; an entirely userspace
procedure. pppd does all the ATM magic.

Does this have real-world applications beyond the Speedtouch DSL modems? If
not, I propose adding this code to linux-atm, not the kernel, since most
users of USB speedtouch DSL modems will not be using the kernel's ATM.

--
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

2005-09-04 16:41:54

by Grzegorz Kulewski

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Sun, 4 Sep 2005, Alistair John Strachan wrote:

> On Sunday 04 September 2005 12:05, Giampaolo Tomassoni wrote:
>> Dears,
>>
>> thanks to Jiri Slaby who found a bug in the AAL0 handling of the ATMSAR
>> module.
>>
>> I attach a fixed version of the atmsar patch as a diff against the 2.6.13
>> kernel tree.
>>
> [snip]
>
> Just out of curiosity, is there ANY reason why this has to be done in the
> kernel? The PPPoATM module for pppd implements (via linux-atm) a completely
> userspace ATM decoder.. if anything, now redundant ATM stack code should be
> REMOVED from Linux!
>
> Most distributions (to my knowledge) supporting the speedtouch modem do so
> using the method prescribed on speedtouch.sf.net; an entirely userspace
> procedure. pppd does all the ATM magic.
>
> Does this have real-world applications beyond the Speedtouch DSL modems? If
> not, I propose adding this code to linux-atm, not the kernel, since most
> users of USB speedtouch DSL modems will not be using the kernel's ATM.

I am using SpeedTouch 330 modem with kernel driver (on Gentoo).

The instalation is currently (with firmware loader instead of modem_run)
very simple: USE="atm" emerge ppp, download firmware and place it in
/lib/firmware, compile the kernel with speedtch support.

I tried to use userspace driver some time ago but it wasn't working for me
so I gave up. I was using modem_run with kernel driver for long time to
load the firmware but there were many problems with it too (nearly every
kernel or modem_run upgrade was breaking something, modem_run was hanging
in D state in most unapropriate moments and so on).

Now I am using pure kernel driver and firmware loader and it works 100%
ok. There were no problems with it for long time. And I don't even want to
look at this userspace driver again.

Since Linux newer was (or is going to be) userspace-driver OS, maybe we
should leave it that way...


Grzegorz Kulewski

2005-09-04 16:54:47

by Alistair John Strachan

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Sunday 04 September 2005 17:41, Grzegorz Kulewski wrote:
> On Sun, 4 Sep 2005, Alistair John Strachan wrote:
> > On Sunday 04 September 2005 12:05, Giampaolo Tomassoni wrote:
> >> Dears,
> >>
> >> thanks to Jiri Slaby who found a bug in the AAL0 handling of the ATMSAR
> >> module.
> >>
> >> I attach a fixed version of the atmsar patch as a diff against the
> >> 2.6.13 kernel tree.
> >
> > [snip]
> >
> > Just out of curiosity, is there ANY reason why this has to be done in the
> > kernel? The PPPoATM module for pppd implements (via linux-atm) a
> > completely userspace ATM decoder.. if anything, now redundant ATM stack
> > code should be REMOVED from Linux!
> >
> > Most distributions (to my knowledge) supporting the speedtouch modem do
> > so using the method prescribed on speedtouch.sf.net; an entirely
> > userspace procedure. pppd does all the ATM magic.
> >
> > Does this have real-world applications beyond the Speedtouch DSL modems?
> > If not, I propose adding this code to linux-atm, not the kernel, since
> > most users of USB speedtouch DSL modems will not be using the kernel's
> > ATM.
>
> I am using SpeedTouch 330 modem with kernel driver (on Gentoo).
>
> The instalation is currently (with firmware loader instead of modem_run)
> very simple: USE="atm" emerge ppp, download firmware and place it in
> /lib/firmware, compile the kernel with speedtch support.

Compared to "place the firmware in /lib/firmware" on many other distros, this
sounds like a lot of work! The kernel speedtch provides no advantages to its
userspace alternative.

> I tried to use userspace driver some time ago but it wasn't working for me
> so I gave up. I was using modem_run with kernel driver for long time to
> load the firmware but there were many problems with it too (nearly every
> kernel or modem_run upgrade was breaking something, modem_run was hanging
> in D state in most unapropriate moments and so on).

This is not the case any longer.

> Now I am using pure kernel driver and firmware loader and it works 100%
> ok. There were no problems with it for long time. And I don't even want to
> look at this userspace driver again.

Conversely people (including myself) found the kernel implementation to be
buggy, and when userspace breaks, you can simply restart it.. when the kernel
breaks, you have to reboot.

> Since Linux newer was (or is going to be) userspace-driver OS, maybe we
> should leave it that way...

No.

What can be done in userspace, within valid performance constraints, usually
should be. This has always been the Linux kernel way.

The speedtouch modem is a USB device, and there are many existing userspace
"driver" implementations for USB devices. Including speedtouch.

I'm not necessarily saying this code shouldn't be in the kernel, I'd just be
interested to know why it has to be.

--
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

2005-09-04 16:56:15

by Jiri Slaby

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

Giampaolo Tomassoni napsal(a):

>I attach a fixed version of the atmsar patch as a diff against the 2.6.13 kernel tree.
>
>
static inline void dump_skb (char * prefix, unsigned int vc, struct
sk_buff * skb) {
what's this? 81+ chars on line
{ on a newline, please
' * ' --> ' *'

spin_lock_irqsave (&txq->lock, flags);
indent is tab (tab is as long as 8 chars), no 3, 4, 5 or ... spaces

and many, many others, please read CodingStyle in Documentation.

thanks,

--
Jiri Slaby http://www.fi.muni.cz/~xslaby
~\-/~ [email protected] ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

2005-09-04 17:22:28

by Grzegorz Kulewski

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Sun, 4 Sep 2005, Alistair John Strachan wrote:

> On Sunday 04 September 2005 17:41, Grzegorz Kulewski wrote:
>> On Sun, 4 Sep 2005, Alistair John Strachan wrote:
>>> On Sunday 04 September 2005 12:05, Giampaolo Tomassoni wrote:
>>>> Dears,
>>>>
>>>> thanks to Jiri Slaby who found a bug in the AAL0 handling of the ATMSAR
>>>> module.
>>>>
>>>> I attach a fixed version of the atmsar patch as a diff against the
>>>> 2.6.13 kernel tree.
>>>
>>> [snip]
>>>
>>> Just out of curiosity, is there ANY reason why this has to be done in the
>>> kernel? The PPPoATM module for pppd implements (via linux-atm) a
>>> completely userspace ATM decoder.. if anything, now redundant ATM stack
>>> code should be REMOVED from Linux!
>>>
>>> Most distributions (to my knowledge) supporting the speedtouch modem do
>>> so using the method prescribed on speedtouch.sf.net; an entirely
>>> userspace procedure. pppd does all the ATM magic.
>>>
>>> Does this have real-world applications beyond the Speedtouch DSL modems?
>>> If not, I propose adding this code to linux-atm, not the kernel, since
>>> most users of USB speedtouch DSL modems will not be using the kernel's
>>> ATM.
>>
>> I am using SpeedTouch 330 modem with kernel driver (on Gentoo).
>>
>> The instalation is currently (with firmware loader instead of modem_run)
>> very simple: USE="atm" emerge ppp, download firmware and place it in
>> /lib/firmware, compile the kernel with speedtch support.
>
> Compared to "place the firmware in /lib/firmware" on many other distros, this
> sounds like a lot of work! The kernel speedtch provides no advantages to its
> userspace alternative.

Are you saying that you do not need to install ppp and compile (or install
binary) kernel on other distros???


>> I tried to use userspace driver some time ago but it wasn't working for me
>> so I gave up. I was using modem_run with kernel driver for long time to
>> load the firmware but there were many problems with it too (nearly every
>> kernel or modem_run upgrade was breaking something, modem_run was hanging
>> in D state in most unapropriate moments and so on).
>
> This is not the case any longer.

Maybe. But there were many bugs in modem_run, usbfs, usb drivers in
kernel, ACPI, IRQ routing and so on. They caused modem_run to hang or do
something stupid without even telling user what is broken. In my
experience when one bug was fixed other appeared in the next kernel. So
even if now everything works ok you have no guarantee that next release
won't break something... :)


>> Now I am using pure kernel driver and firmware loader and it works 100%
>> ok. There were no problems with it for long time. And I don't even want to
>> look at this userspace driver again.
>
> Conversely people (including myself) found the kernel implementation to be
> buggy, and when userspace breaks, you can simply restart it.. when the kernel
> breaks, you have to reboot.

1. But you still use the kernel even with userspace driver. So it still
can break forcing you to reboot.
2. I have no problems with kernel driver. All problems that I saw in the
past were problems in other subsystems (usbfs, usb drivers, ACPI, IRQ, ...).
3. Restarting modem_run was never enought for me. I always at least had to
unplug the modem from USB port. Or more often reboot. So I see no gain
here.

>> Since Linux newer was (or is going to be) userspace-driver OS, maybe we
>> should leave it that way...
>
> No.
>
> What can be done in userspace, within valid performance constraints, usually
> should be. This has always been the Linux kernel way.

But not device drivers. I do not think that Linux has good infrastructure
for drivers in userspace (yes I know that there are some). Also are you
sure that performance and latencies are ok with userspace driver even if
system is under load?


> The speedtouch modem is a USB device, and there are many existing userspace
> "driver" implementations for USB devices. Including speedtouch.
>
> I'm not necessarily saying this code shouldn't be in the kernel, I'd just be
> interested to know why it has to be.

Well, as you are saing it hasn't... :) But since it is there and works
for me I am interested in not changing this state without really good
reason.


Grzegorz Kulewski

2005-09-04 18:44:12

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

> -----Messaggio originale-----
> Da: Francois Romieu [mailto:[email protected]]
> Inviato: domenica 4 settembre 2005 17.33
> A: Giampaolo Tomassoni
> Cc: [email protected];
> [email protected]
> Oggetto: Re: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1
>
> ...omissis...
>
> I'd be happily surprized to see more documented ADSL PCI/USB device in the
> near future. :o(

OT Question. What about an open adsl device? The linux community had been bumped out by the adsl market for at least a couple of years, and nobody knows (or tells) why...

That could be a definitive answer. Is there anybody interested in this?


>
> ...omissis...
>
> > Finally, the fact that ATMSAR is device-unspecific makes it easier to
> > maintain, I guess.
>
> Ok. Your suggestion may have more impact if there is a patch to convert
> the sole existing in-kernel driver to use this module.

Mmmh. I can try to do this, but I would prefer to hear Sands about this.


>
> ...omissis
>
> An uniform codingstyle is useful when people need to review code.
> Something is wrong when a reviewer must uncipher a piece of code.
> You will find areas in the kernel whose trends differ but a codingstyle
> from Mars is usually a hint. So it is not _only_ a matter of taste.

Ok, ok. I'll (try to) behave...


>
> ...omissis...
>
> You may have more feedback/review then. I only gave a cursory look at the
> code.

Right, that's what I'm looking for.


>
> ...omissis...
>
> Rather the "typedef struct atmsar_dev atmsar_dev_t;" (yes, I know the "It
> saves typing" argument). Maybe something could be done at the same time
> regarding the need for the forward declarations.

Well, fine. I'll "struct _whatever *". But atmsar_dev_t no, that nonono: it mimics the atm_dev_t typedef... It's all around the idea a developer needs to use atmsar_dev_t instead of atm_dev_t...


>
> ...omissis...
>
> s/what/why/
>
> And no, documenting a call to skb_reserve is silly.

...


>
> ...omissis...
>
> The value returned by sprintf and friends contains the needed offset, i.e.
> buf += sprintf(buf, ...);.

I used an strcpy() to put the constant string in the buffer. However, I'm changing it this way:

if(skip-- == 0) {
count = strlen(strcpy(page, "dnrate:\t"));
if(dev->rx_speed != ATMSAR_SPEED_UNSPEC)
count += sprintf(
&page[count],
"%ld kbps\n",
dev->rx_speed
);
else
count += strlen(strcpy(&page[count], "unknown\n"));
return(count);
}


> [...]
> > > - "return" is not a function.
> >
> > Not even for() or while(). But doesn't they look cute this way?
>
> No.
>
> for (), while (), return rc;

...


> [...]
> > > - consider 'goto' to handle the errors instead of deep nesting
> >
> > I prefer not using goto when not required to. Nesting is far
> more readable
> > to my opinion.
>
> OTOH, it makes ugly code to have it fit in a 80 columns console.
>
> [...]
> > Anyway, which are the functions you are objecting?
>
> atmSend. Probably others.
>
> If you can make the code look like existing in-kernel code (not fs/cifs
> please) say network or ata driver code and you do not need goto, it's fine
> too.

Mmmmh. I'll check it out.


> > > - +const atmsar_aalops_t opsAALR = {
> > > + ATM_AAL0,
> > > + "raw",
> > > -> use .foo = baz instead.
> >
> > atmasr_aalops_t is not an exported structure (you'll find just an opaque
> > definition in include/linux/atmsar.h), so it is not meant to be
> statically
> > declared by device drivers. But I guess that the problem is readability,
> > right?
>
> struct foo zoy {
> .bar = barbar,
> .baz = bazbaz,
> .quuz = ...
> };

...


> [...]
> > May I ask if this is just your own contribution or if you are
> in charge of
> > something in the linux and/or linux-atm projects?
>
> /me scratches head
>
> http://ww.google.com/search?hl=en&q=romieu+linux+cabal

That was to give the right wedge to your hints. If you were just around this list, your hints had a different value than if you were a committer. Am I wrong?

Thanks,

-----------------------------------
Giampaolo Tomassoni - IT Consultant
Piazza VIII Aprile 1948, 4
I-53044 Chiusi (SI) - Italy
Ph: +39-0578-21100

2005-09-04 18:36:36

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: [ATMSAR] Request for review - update #1

> -----Messaggio originale-----
> Da: Alistair John Strachan [mailto:[email protected]]
> Inviato: domenica 4 settembre 2005 18.21
>
> ...omissis...
>
> Just out of curiosity, is there ANY reason why this has to be done in the
> kernel? The PPPoATM module for pppd implements (via linux-atm) a
> completely
> userspace ATM decoder.. if anything, now redundant ATM stack code
> should be
> REMOVED from Linux!

This may be true for AAL5 support, which is the way by which data is actually transferred between ADSL DSLAMs and CPE equipment.

This may not be generally true, however: most providers are already delivering internet+voice solutions over ADSL channels (here in Italy, in example, Telecom offers Alice Mia, which is an ADSL line with internet access and VoIP for added voice capabilities). Albeit the voice part of these solutions are actually based on VoIP technology, it is not the best way to do this. In the future, I believe we will easily see internet + voice sols based over AAL5 + AAL2/3, or even multi voice channels over AAL2/3 over ADSL (replacing ISDN PRIs and multi-BRIs -based lines for PABX connection).

When (and if) that will happen, we will probabily need a kernel-based solution since cell timing and QoS is a much stricter requirement with non-AAL5 encodings, such that it is easier to attain from inside the kernel than from userland.

So, I'm not that shure all the ATM code is to be stripped out of the kernel. Maybe it can be done with the PPPoATM network interface. But probably it can't be done with the ATM core and the ATM SAR code. Wherever the latter will be in ATMUSB, in ATMSAR or in a device driver.

The PPPoATM module is a network interface. It stays on the other side of the ATM world: [netif] <-> [PPPoATM] <-> [atmif] <-> [ATM] <-> [ATMSAR] <-> [device driver]. I'm not a PPPoATM expert, but it may probably be possible to have all the PPPoATM code in userland. But the [ATM] <-> [ATMSAR] <-> [device driver] chain is probably too close to hardware to gain any benefit by "userlanding" it.


>
> ...omissis...
>
>
> --
> Cheers,
> Alistair.

Cheers,

giampaolo

2005-09-04 18:50:54

by Alistair John Strachan

[permalink] [raw]
Subject: Re: R: [ATMSAR] Request for review - update #1

On Sunday 04 September 2005 19:36, Giampaolo Tomassoni wrote:
[snip]
>
> This may be true for AAL5 support, which is the way by which data is
> actually transferred between ADSL DSLAMs and CPE equipment.
>
> This may not be generally true, however: most providers are already
> delivering internet+voice solutions over ADSL channels (here in Italy, in
> example, Telecom offers Alice Mia, which is an ADSL line with internet
> access and VoIP for added voice capabilities). Albeit the voice part of
> these solutions are actually based on VoIP technology, it is not the best
> way to do this. In the future, I believe we will easily see internet +
> voice sols based over AAL5 + AAL2/3, or even multi voice channels over
> AAL2/3 over ADSL (replacing ISDN PRIs and multi-BRIs -based lines for PABX
> connection).
>
> When (and if) that will happen, we will probabily need a kernel-based
> solution since cell timing and QoS is a much stricter requirement with
> non-AAL5 encodings, such that it is easier to attain from inside the kernel
> than from userland.
>
> So, I'm not that shure all the ATM code is to be stripped out of the
> kernel. Maybe it can be done with the PPPoATM network interface. But
> probably it can't be done with the ATM core and the ATM SAR code. Wherever
> the latter will be in ATMUSB, in ATMSAR or in a device driver.

The above is a decent technical justification for why the code is generally
required; it's set my mind at rest, I thought maybe this was only for the
speedtouch modem crew.

I was aware of ATM's ability to interleave data and "digital voice" services;
ATM is widely deployed across Europe in preparation for "pure digital"
consumer telephony..

My primary concern with the ATM code is that it's not (yet) an often used part
of the kernel; there are a number of viable applications out there, but most
can happily use linux-atm and/or pppd. I can see VoIP clients doing the same.

However, for embedded or non-pure-client purposes, there's a reason for an
in-kernel implementation.

I don't know enough about the applications of the "ATM stack" versus using a
userspace library, so I think as long as the patch is cleaned up and can be
reused, it's okay to put in the kernel.

>
> The PPPoATM module is a network interface. It stays on the other side of
> the ATM world: [netif] <-> [PPPoATM] <-> [atmif] <-> [ATM] <-> [ATMSAR] <->
> [device driver]. I'm not a PPPoATM expert, but it may probably be possible
> to have all the PPPoATM code in userland. But the [ATM] <-> [ATMSAR] <->
> [device driver] chain is probably too close to hardware to gain any benefit
> by "userlanding" it.
>

I agree, if there are users beyond the speedtouch modem, it may make sense to
have this code in the kernel. Thanks for fielding my questions.

--
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

2005-09-04 19:11:26

by matthieu castet

[permalink] [raw]
Subject: Re: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

Giampaolo Tomassoni wrote:
>>-----Messaggio originale-----
>>Da: Francois Romieu [mailto:[email protected]]
>>Inviato: domenica 4 settembre 2005 17.33
>>A: Giampaolo Tomassoni
>>Cc: [email protected];
>>[email protected]
>>Oggetto: Re: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1
>>
>>...omissis...
>>
>>I'd be happily surprized to see more documented ADSL PCI/USB device in the
>>near future. :o(
>
>
> OT Question. What about an open adsl device? The linux community had been bumped out by the adsl market for at least a couple of years, and nobody knows (or tells) why...
>
> That could be a definitive answer. Is there anybody interested in this?
>
>
The problem is that lot's of new devices implement part of their dsp
function in the kernel space instead of in the device.
And as company don't want to publish their dsp algorith and open source
it, we can't have open source driver for it.

That the case for bewan device (that even include a libm in their
source) and for pulsar pci device...

2005-09-04 19:34:00

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

> -----Messaggio originale-----
> Da: matthieu castet [mailto:[email protected]]
> Inviato: domenica 4 settembre 2005 21.11
>
> ...omissis...
>
> The problem is that lot's of new devices implement part of their dsp
> function in the kernel space instead of in the device.
> And as company don't want to publish their dsp algorith and open source
> it, we can't have open source driver for it.
>
> That the case for bewan device (that even include a libm in their
> source) and for pulsar pci device...

Nonono: I meant exactly to do an open card with an open dsp software. Next time hardware producers will think twice before refraining from disclosing card details...

After all, most producers didn't ever need to disclose their firmware as long as it is a binary file to be uploaded to the card. But still it took a lot of time to have a working ADSL driver under Linux, just because producers didn't want to disclose port assignments and the like. I.e.: they preferred not to disclose anything instead that just refraining to disclose the firmware, which would had to be enough for their purposes.

This is a behaviour that the linux community shall discourage. Designing an open hardware and software solution for ADSL connection would be a great way to avoid something like this in the future... You don't disclose? I offer an alternative which bypasses you.

The matter is not so easy, however: the ADSL standard is complex and dsp software has to take into account a lot of ADSL "flavors" (DSLAM producers often offer enhancements to the standard way), but it shouldn't be too difficult to the linux community to put together the needed gray matter...

Cheers,

giampaolo

2005-09-04 19:42:28

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

> -----Messaggio originale-----
> Da: matthieu castet [mailto:[email protected]]
> Inviato: domenica 4 settembre 2005 21.11
>
> ...omissis...
>
> The problem is that lot's of new devices implement part of their dsp
> function in the kernel space instead of in the device.
> And as company don't want to publish their dsp algorith and open source
> it, we can't have open source driver for it.
>
> That the case for bewan device (that even include a libm in their
> source) and for pulsar pci device...

Nonono: I meant exactly to do an open card with an open dsp software. Next time hardware producers will think twice before refraining from disclosing card details...

After all, most producers didn't ever need to disclose their firmware as long as it is a binary file to be uploaded to the card. But still it took a lot of time to have a working ADSL driver under Linux, just because producers didn't want to disclose port assignments and the like. I.e.: they preferred not to disclose anything instead that just refraining to disclose the firmware, which would had to be enough for their purposes.

This is a behaviour that the linux community shall discourage. Designing an open hardware and software solution for ADSL connection would be a great way to avoid something like this in the future... You don't disclose? I offer an alternative which bypasses you.

The matter is not so easy, however: the ADSL standard is complex and dsp software has to take into account a lot of ADSL "flavors" (DSLAM producers often offer enhancements to the standard way), but it shouldn't be too difficult to the linux community to put together the needed gray matter...

Anyway, all these speculations are definitely OT. Sorry about that.

Cheers,

giampaolo

2005-09-05 06:06:34

by Zoran Stojsavljevic

[permalink] [raw]
Subject: Re: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

Giampaolo Tomassoni wrote:

>Also, I believe that adsl will carry much more services then just AAL5 for internet connection in the future. Even if the ATMSAR actually lacks of AAL1 and AAL2/3 capabilities, adding them in a single, specialized module is much easier than swimming in a usb+atm middle layer.
>
>
Giampaolo,

Should read: ...even if the ATMSAR actually lacks of AAL1, AAL2 and AAL3/4 [where AAL3/4 is obsolete] capabilities...

Just wanted to make it more precise according to the ATM standards, this was the only intention of my "patch".

Peace to all! :o)



2005-09-05 06:22:09

by Giampaolo Tomassoni

[permalink] [raw]
Subject: R: R: [Linux-ATM-General] [ATMSAR] Request for review - update #1

> Giampaolo,
>
> Should read: ...even if the ATMSAR actually lacks of AAL1, AAL2
> and AAL3/4 [where AAL3/4 is obsolete] capabilities...
>
> Just wanted to make it more precise according to the ATM
> standards, this was the only intention of my "patch".

You're right. Sorry about that.

Thanks,

Giampaolo

2005-09-05 09:36:48

by David Woodhouse

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Sun, 2005-09-04 at 17:20 +0100, Alistair John Strachan wrote:
> Just out of curiosity, is there ANY reason why this has to be done in the
> kernel? The PPPoATM module for pppd implements (via linux-atm) a completely
> userspace ATM decoder.. if anything, now redundant ATM stack code should be
> REMOVED from Linux!

No. The pppoatm module for pppd uses the kernel ATM stack and kernel
PPPoATM functionality. I suspect you're thinking of the pseudo-tty hack
used by the userspace code.

> Most distributions (to my knowledge) supporting the speedtouch modem do so
> using the method prescribed on speedtouch.sf.net; an entirely userspace
> procedure. pppd does all the ATM magic.

Fedora doesn't; it uses the kernel driver.

--
dwmw2


2005-09-05 13:48:10

by Alistair John Strachan

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Monday 05 September 2005 10:36, David Woodhouse wrote:
> On Sun, 2005-09-04 at 17:20 +0100, Alistair John Strachan wrote:
> > Just out of curiosity, is there ANY reason why this has to be done in the
> > kernel? The PPPoATM module for pppd implements (via linux-atm) a
> > completely userspace ATM decoder.. if anything, now redundant ATM stack
> > code should be REMOVED from Linux!
>
> No. The pppoatm module for pppd uses the kernel ATM stack and kernel
> PPPoATM functionality. I suspect you're thinking of the pseudo-tty hack
> used by the userspace code.

I'm not sure which module you're referring to, but the patch recommended by
the speedtouch people links to linux-atm, and does not require kernel ATM or
kernel pppoatm functionality, or use any kernel modules. I do notice it does
a system ("/sbin/modprobe pppoatm"); but this is definitely not required; I'm
speaking to you from a speedtouch DSL connection, no module loaded or
compiled in, no ATM support in the kernel.

http://devzero.co.uk/~alistair/linuxmisc/ppp-2.4.3-atm.diff

>
> > Most distributions (to my knowledge) supporting the speedtouch modem do
> > so using the method prescribed on speedtouch.sf.net; an entirely
> > userspace procedure. pppd does all the ATM magic.
>
> Fedora doesn't; it uses the kernel driver.

I stand corrected.

--
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

2005-09-05 13:54:17

by David Woodhouse

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Mon, 2005-09-05 at 14:52 +0100, Alistair John Strachan wrote:
> I'm not sure which module you're referring to, but the patch recommended by
> the speedtouch people links to linux-atm, and does not require kernel ATM or
> kernel pppoatm functionality, or use any kernel modules. I do notice it does
> a system ("/sbin/modprobe pppoatm"); but this is definitely not required; I'm
> speaking to you from a speedtouch DSL connection, no module loaded or
> compiled in, no ATM support in the kernel.

Then you're not using the pppoatm plugin; you needn't bother applying
that patch. You're probably just using the pseudo-tty hack.

--
dwmw2

2005-09-05 14:14:11

by Alistair John Strachan

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Monday 05 September 2005 14:56, David Woodhouse wrote:
> On Mon, 2005-09-05 at 14:52 +0100, Alistair John Strachan wrote:
> > I'm not sure which module you're referring to, but the patch recommended
> > by the speedtouch people links to linux-atm, and does not require kernel
> > ATM or kernel pppoatm functionality, or use any kernel modules. I do
> > notice it does a system ("/sbin/modprobe pppoatm"); but this is
> > definitely not required; I'm speaking to you from a speedtouch DSL
> > connection, no module loaded or compiled in, no ATM support in the
> > kernel.
>
> Then you're not using the pppoatm plugin; you needn't bother applying
> that patch. You're probably just using the pseudo-tty hack.

Ahh yes, I was confusing the pppd module with pppoa3, a userspace ppp-over-atm
handler. Thanks for the correction.

Still, I don't feel this detracts from the point that client ATM DSL device
"drivers" can exist happily in userspace.

--
Cheers,
Alistair.

'No sense being pessimistic, it probably wouldn't work anyway.'
Third year Computer Science undergraduate.
1F2 55 South Clerk Street, Edinburgh, UK.

2005-09-05 14:30:10

by David Woodhouse

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

On Mon, 2005-09-05 at 15:18 +0100, Alistair John Strachan wrote:
> Still, I don't feel this detracts from the point that client ATM DSL
> device "drivers" can exist happily in userspace.

Indeed they can. There's been lots of academic research into
microkernels which supports your assertion.

--
dwmw2

2005-09-05 14:46:45

by Duncan Sands

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

Hi Alistair,

> Just out of curiosity, is there ANY reason why this has to be done in the
> kernel? The PPPoATM module for pppd implements (via linux-atm) a completely
> userspace ATM decoder.. if anything, now redundant ATM stack code should be
> REMOVED from Linux!

the main advantage of the kernel module is that it is integrated into the
kernel's ATM layer. That means that you can use all those great features
the ATM layer provides to do more with your modem. This doesn't matter for
most people: they have a PPPoA or PPPoE connection and aren't looking to do
clever tricks; the user mode driver is fine for them. But it is not enough
for everyone. For example, my ISP uses "routed IP", which is not supported
by the user mode driver, but works fine with the kernel driver thanks to the
ATM layer.

All the best,

Duncan.

PS: linux-atm and the pppoatm module are used with the kernel driver;
presumably you meant pppoa2 or pppoa3.

2005-09-05 15:04:51

by Duncan Sands

[permalink] [raw]
Subject: Re: [ATMSAR] Request for review - update #1

Hi Alistair,

> > The instalation is currently (with firmware loader instead of modem_run)
> > very simple: USE="atm" emerge ppp, download firmware and place it in
> > /lib/firmware, compile the kernel with speedtch support.
>
> Compared to "place the firmware in /lib/firmware" on many other distros, this
> sounds like a lot of work! The kernel speedtch provides no advantages to its
> userspace alternative.

historically the main problem with using the kernel driver was getting hold of
an ATM aware pppd. The step "USE="atm" emerge ppp" looks like gentoos way of
installing such a pppd. Nowadays several major distributions ship with an
ATM aware pppd, so if you are using one there is nothing to be done. Likewise,
most distributions ship a kernel with speedtch support. So if you're using
such a distribution all you have to do to use the kernel driver is
"place the firmware in /lib/firmware".

On the other hand, it is misleading to say that with the user mode driver
all you have to do is place the firmware in /lib/firmware. That's only
true if your distribution (eg: Mandrake) explicitly supports the user mode
driver and has pre-installed and configured it for you. If you don't have
such a distribution then setting up the user mode driver, while not difficult,
does require some work.

> > I tried to use userspace driver some time ago but it wasn't working for me
> > so I gave up. I was using modem_run with kernel driver for long time to
> > load the firmware but there were many problems with it too (nearly every
> > kernel or modem_run upgrade was breaking something, modem_run was hanging
> > in D state in most unapropriate moments and so on).
>
> This is not the case any longer.

I'm the one who fixed most of those problems by the way.

> > Now I am using pure kernel driver and firmware loader and it works 100%
> > ok. There were no problems with it for long time. And I don't even want to
> > look at this userspace driver again.
>
> Conversely people (including myself) found the kernel implementation to be
> buggy, and when userspace breaks, you can simply restart it.. when the kernel
> breaks, you have to reboot.

Tell me what problems you've been seeing and I will try to fix them.

All the best,

Duncan.