Re: Request for review of adjtimex(2) man page

[Michael Kerrisk (man-pages)]

Hello John,

Following up, long after the fact

First of all, a belated thanks for your comments.

On 01/09/2015 11:51 PM, John Stultz wrote:
> On Wed, Jan 7, 2015 at 4:53 AM, Michael Kerrisk (man-pages)
>  wrote:
>> Hello all,
>>
>> Recently, I made made a number of changes to the adjtimex(2)
>> man page, to try and add a bit more detail, since the page
>> was formerly in a very sorry state.
> 
> Yes. My apologies for some of that. Its been on my todo to try to fix
> this up, but I just haven't been able to get to it.
> 
> 
>> I would be happy if some NTP/time-knowledgeable folk (John, Richard,
>> I'm kind of hoping you) would review the page to see if I've injected
>> any errors. Furthermore, notwithstanding my attempt to improve the page,
>> there remain many gaps in details in the page. I've added a large number
>> of FIXMEs in the draft below, and would be happy if anyone can supply
>> some content to fill any of the gaps.
>>
>> Cheers,
>>
>> Michael
>>
>> .\" Copyright (c) 1995 Michael Chastain (m...@shell.portal.com), 15 April 
>> 1995.
>> .\" and Copyright (C) 2014 Michael Kerrisk 
>> .\"
>> .\" %%%LICENSE_START(GPLv2+_DOC_FULL)
>> .\" This is free documentation; you can redistribute it and/or
>> .\" modify it under the terms of the GNU General Public License as
>> .\" published by the Free Software Foundation; either version 2 of
>> .\" the License, or (at your option) any later version.
>> .\"
>> .\" The GNU General Public License's references to "object code"
>> .\" and "executables" are to be interpreted as the output of any
>> .\" document formatting or typesetting system, including
>> .\" intermediate and printed output.
>> .\"
>> .\" This manual is distributed in the hope that it will be useful,
>> .\" but WITHOUT ANY WARRANTY; without even the implied warranty of
>> .\" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> .\" GNU General Public License for more details.
>> .\"
>> .\" You should have received a copy of the GNU General Public
>> .\" License along with this manual; if not, see
>> .\" .
>> .\" %%%LICENSE_END
>> .\"
>> .\" Modified 1997-01-31 by Eric S. Raymond 
>> .\" Modified 1997-07-30 by Paul Slootman 
>> .\" Modified 2004-05-27 by Michael Kerrisk 
>> .\"
>> .TH ADJTIMEX 2 2014-12-31 "Linux" "Linux Programmer's Manual"
>> .SH NAME
>> adjtimex \- tune kernel clock
>> .SH SYNOPSIS
>> .nf
>> .BR "#define _BSD_SOURCE" "  /* See feature_test_macros(7) */"
>> .B #include 
>>
>> .BI "int adjtimex(struct timex *" "buf" );
>> .fi
>> .SH DESCRIPTION
>> Linux uses David L. Mills' clock adjustment algorithm (see RFC\ 5905).
>> The system call
>> .BR adjtimex ()
>> reads and optionally sets adjustment parameters for this algorithm.
>> It takes a pointer to a
>> .I timex
>> structure, updates kernel parameters from field values,
>> and returns the same structure with current kernel values.
>> This structure is declared as follows:
>> .PP
>> .in +4n
>> .nf
>> struct timex {
>> int  modes;  /* Mode selector */
>> long offset; /* Time offset; nanoseconds, if STA_NANO
>> status flag is set, otherwise microseconds */
>> long freq;   /* Frequency offset, in units of 2^-16 ppm
>> (parts per million, see NOTES below) */
>> long maxerror;   /* Maximum error (microseconds) */
>> long esterror;   /* Estimated error (microseconds) */
>> int  status; /* Clock command/status */
>> long constant;   /* PLL (phase-locked loop) time constant */
>> long precision;  /* Clock precision (microseconds, read-only) */
>> long tolerance;  /* Clock frequency tolerance (ppm, read-only) */
>> struct timeval time;
>>  /* Current time (read-only, except for
>> ADJ_SETOFFSET); upon return, time.tv_usec
>> contains nanoseconds, if STA_NANO status
>> flag is set, otherwise microseconds */
>> long tick;   /* Microseconds between clock ticks */
>> long ppsfreq;/* PPS (pulse per second) frequency (in units
>> of 2^-16 ppm\-\-see NOTES, read-only) */
>> long jitter; /* PPS jitter (read-only); nanoseconds, if
>> STA_NANO status flag is set, otherwise
>> microseconds */
>> int  shift;  /* PPS interval duration (seconds, read-only) */
>> long stabil; /* PPS stability (2^-16 ppm\-\-see NOTES,
>> read-only) */
>> long jitcnt; /* PPS jitter limit exceeded (read-only) */
>> long calcnt; /* PPS calibration intervals (read-only) */
>> long errcnt; /* PPS calibration errors (read-only) */
>> long stbcnt; /* PPS stability limit exceeded (read-only) */
>> int tai; /* TAI offset, as set by previous ADJ_TAI
>> operation (seconds, read-only,
>> since Linux 2.6.26) */
>> /

Re: Request for review of adjtimex(2) man page

[John Stultz]

On Wed, Jan 7, 2015 at 4:53 AM, Michael Kerrisk (man-pages)
 wrote:
> Hello all,
>
> Recently, I made made a number of changes to the adjtimex(2)
> man page, to try and add a bit more detail, since the page
> was formerly in a very sorry state.

Yes. My apologies for some of that. Its been on my todo to try to fix
this up, but I just haven't been able to get to it.


> I would be happy if some NTP/time-knowledgeable folk (John, Richard,
> I'm kind of hoping you) would review the page to see if I've injected
> any errors. Furthermore, notwithstanding my attempt to improve the page,
> there remain many gaps in details in the page. I've added a large number
> of FIXMEs in the draft below, and would be happy if anyone can supply
> some content to fill any of the gaps.
>
> Cheers,
>
> Michael
>
> .\" Copyright (c) 1995 Michael Chastain (m...@shell.portal.com), 15 April 
> 1995.
> .\" and Copyright (C) 2014 Michael Kerrisk 
> .\"
> .\" %%%LICENSE_START(GPLv2+_DOC_FULL)
> .\" This is free documentation; you can redistribute it and/or
> .\" modify it under the terms of the GNU General Public License as
> .\" published by the Free Software Foundation; either version 2 of
> .\" the License, or (at your option) any later version.
> .\"
> .\" The GNU General Public License's references to "object code"
> .\" and "executables" are to be interpreted as the output of any
> .\" document formatting or typesetting system, including
> .\" intermediate and printed output.
> .\"
> .\" This manual is distributed in the hope that it will be useful,
> .\" but WITHOUT ANY WARRANTY; without even the implied warranty of
> .\" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> .\" GNU General Public License for more details.
> .\"
> .\" You should have received a copy of the GNU General Public
> .\" License along with this manual; if not, see
> .\" .
> .\" %%%LICENSE_END
> .\"
> .\" Modified 1997-01-31 by Eric S. Raymond 
> .\" Modified 1997-07-30 by Paul Slootman 
> .\" Modified 2004-05-27 by Michael Kerrisk 
> .\"
> .TH ADJTIMEX 2 2014-12-31 "Linux" "Linux Programmer's Manual"
> .SH NAME
> adjtimex \- tune kernel clock
> .SH SYNOPSIS
> .nf
> .BR "#define _BSD_SOURCE" "  /* See feature_test_macros(7) */"
> .B #include 
>
> .BI "int adjtimex(struct timex *" "buf" );
> .fi
> .SH DESCRIPTION
> Linux uses David L. Mills' clock adjustment algorithm (see RFC\ 5905).
> The system call
> .BR adjtimex ()
> reads and optionally sets adjustment parameters for this algorithm.
> It takes a pointer to a
> .I timex
> structure, updates kernel parameters from field values,
> and returns the same structure with current kernel values.
> This structure is declared as follows:
> .PP
> .in +4n
> .nf
> struct timex {
> int  modes;  /* Mode selector */
> long offset; /* Time offset; nanoseconds, if STA_NANO
> status flag is set, otherwise microseconds */
> long freq;   /* Frequency offset, in units of 2^-16 ppm
> (parts per million, see NOTES below) */
> long maxerror;   /* Maximum error (microseconds) */
> long esterror;   /* Estimated error (microseconds) */
> int  status; /* Clock command/status */
> long constant;   /* PLL (phase-locked loop) time constant */
> long precision;  /* Clock precision (microseconds, read-only) */
> long tolerance;  /* Clock frequency tolerance (ppm, read-only) */
> struct timeval time;
>  /* Current time (read-only, except for
> ADJ_SETOFFSET); upon return, time.tv_usec
> contains nanoseconds, if STA_NANO status
> flag is set, otherwise microseconds */
> long tick;   /* Microseconds between clock ticks */
> long ppsfreq;/* PPS (pulse per second) frequency (in units
> of 2^-16 ppm\-\-see NOTES, read-only) */
> long jitter; /* PPS jitter (read-only); nanoseconds, if
> STA_NANO status flag is set, otherwise
> microseconds */
> int  shift;  /* PPS interval duration (seconds, read-only) */
> long stabil; /* PPS stability (2^-16 ppm\-\-see NOTES,
> read-only) */
> long jitcnt; /* PPS jitter limit exceeded (read-only) */
> long calcnt; /* PPS calibration intervals (read-only) */
> long errcnt; /* PPS calibration errors (read-only) */
> long stbcnt; /* PPS stability limit exceeded (read-only) */
> int tai; /* TAI offset, as set by previous ADJ_TAI
> operation (seconds, read-only,
> since Linux 2.6.26) */
> /* Further padding bytes to allow for future expansion */
> };
> .fi
> .in
> .PP
> The
> .I modes
> field determines which parameters, if any, to set.
> It is a bit mask containing a
> .RI bitwise- or
> combination of zero or more of the following bits:
> .TP
> .

Re: Request for review of adjtimex(2) man page

[Michael Kerrisk (man-pages)]

Hi Laurent,

On 7 January 2015 at 14:37, Laurent Georget  wrote:
> Hello,
>
>
>>I've added a large number
>> of FIXMEs in the draft below, and would be happy if anyone can supply
>> some content to fill any of the gaps.
>
> Isn't the man page going to be very long and hard to read if we explain all
> the NTP internals in adjtimex.2?

That's not my intention. But in various places it would be useful for
the reader to know where they can go for further information.

> I think we could create a second man page
> (for instance ntp.7) in section 7 to provide extensive explanations and refer
> to it from adjtimex.2. Does that seem okay?

Maybe. But I'm not sure that it's needed. See above.

>> .BR STA_PLL
>> Enable phase-locked loop (PLL) updates (read-write) via
>> .\" FIXME Any pointer to further information about what this means?
>> .\"   (It was not immediately obvious from a scan of the RFC, whether
>> .\"   this is described in the RFC.)
>> .BR ADJ_OFFSET .
>
>> .BR STA_FLL
>> Select frequency-locked loop (FLL) mode (read-write).
>> .\" FIXME Any pointer to further information about what this means?
>> .\"   (It was not immediately obvious from a scan of the RFC, whether
>> .\"   this is described in the RFC.)
>
> In this particular case, for example, PLL and FLL are the two modes by which
> the clock offset can be controlled. Roughly speaking, PLL is phase locking
> (the clock time is adjusted to the source time) and FLL is frequency locking
> (the clock frequency is adjusted to the source frequency). Is that level of
> details be sufficient for adjtimex.2?
>
> Valuable resources that go beyond the content of the RFC are publications and
> presentations from Pr. David Mills (http://www.eecis.udel.edu/~mills/) like
> http://www.eecis.udel.edu/~mills/database/brief/clock/clock.pdf.

Thanks for the pointers.

Cheers,

Michael


-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Linux/UNIX System Programming Training: http://man7.org/training/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Request for review of adjtimex(2) man page

[Laurent Georget]

Hello,


>I've added a large number
> of FIXMEs in the draft below, and would be happy if anyone can supply
> some content to fill any of the gaps.

Isn't the man page going to be very long and hard to read if we explain all 
the NTP internals in adjtimex.2? I think we could create a second man page
(for instance ntp.7) in section 7 to provide extensive explanations and refer
to it from adjtimex.2. Does that seem okay?

> .BR STA_PLL
> Enable phase-locked loop (PLL) updates (read-write) via
> .\" FIXME Any pointer to further information about what this means?
> .\"   (It was not immediately obvious from a scan of the RFC, whether
> .\"   this is described in the RFC.)
> .BR ADJ_OFFSET .

> .BR STA_FLL
> Select frequency-locked loop (FLL) mode (read-write).
> .\" FIXME Any pointer to further information about what this means?
> .\"   (It was not immediately obvious from a scan of the RFC, whether
> .\"   this is described in the RFC.)

In this particular case, for example, PLL and FLL are the two modes by which
the clock offset can be controlled. Roughly speaking, PLL is phase locking
(the clock time is adjusted to the source time) and FLL is frequency locking
(the clock frequency is adjusted to the source frequency). Is that level of
details be sufficient for adjtimex.2?

Valuable resources that go beyond the content of the RFC are publications and
presentations from Pr. David Mills (http://www.eecis.udel.edu/~mills/) like
http://www.eecis.udel.edu/~mills/database/brief/clock/clock.pdf.

Cheers,

Laurent



signature.asc
Description: OpenPGP digital signature

Request for review of adjtimex(2) man page

[Michael Kerrisk (man-pages)]

Hello all,

Recently, I made made a number of changes to the adjtimex(2)
man page, to try and add a bit more detail, since the page
was formerly in a very sorry state.

I would be happy if some NTP/time-knowledgeable folk (John, Richard,
I'm kind of hoping you) would review the page to see if I've injected 
any errors. Furthermore, notwithstanding my attempt to improve the page, 
there remain many gaps in details in the page. I've added a large number
of FIXMEs in the draft below, and would be happy if anyone can supply
some content to fill any of the gaps.

Cheers,

Michael

.\" Copyright (c) 1995 Michael Chastain (m...@shell.portal.com), 15 April 1995.
.\" and Copyright (C) 2014 Michael Kerrisk 
.\"
.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
.\" This is free documentation; you can redistribute it and/or
.\" modify it under the terms of the GNU General Public License as
.\" published by the Free Software Foundation; either version 2 of
.\" the License, or (at your option) any later version.
.\"
.\" The GNU General Public License's references to "object code"
.\" and "executables" are to be interpreted as the output of any
.\" document formatting or typesetting system, including
.\" intermediate and printed output.
.\"
.\" This manual is distributed in the hope that it will be useful,
.\" but WITHOUT ANY WARRANTY; without even the implied warranty of
.\" MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
.\" GNU General Public License for more details.
.\"
.\" You should have received a copy of the GNU General Public
.\" License along with this manual; if not, see
.\" .
.\" %%%LICENSE_END
.\"
.\" Modified 1997-01-31 by Eric S. Raymond 
.\" Modified 1997-07-30 by Paul Slootman 
.\" Modified 2004-05-27 by Michael Kerrisk 
.\"
.TH ADJTIMEX 2 2014-12-31 "Linux" "Linux Programmer's Manual"
.SH NAME
adjtimex \- tune kernel clock
.SH SYNOPSIS
.nf
.BR "#define _BSD_SOURCE" "  /* See feature_test_macros(7) */"
.B #include 

.BI "int adjtimex(struct timex *" "buf" );
.fi
.SH DESCRIPTION
Linux uses David L. Mills' clock adjustment algorithm (see RFC\ 5905).
The system call
.BR adjtimex ()
reads and optionally sets adjustment parameters for this algorithm.
It takes a pointer to a
.I timex
structure, updates kernel parameters from field values,
and returns the same structure with current kernel values.
This structure is declared as follows:
.PP
.in +4n
.nf
struct timex {
int  modes;  /* Mode selector */
long offset; /* Time offset; nanoseconds, if STA_NANO
status flag is set, otherwise microseconds */
long freq;   /* Frequency offset, in units of 2^-16 ppm
(parts per million, see NOTES below) */
long maxerror;   /* Maximum error (microseconds) */
long esterror;   /* Estimated error (microseconds) */
int  status; /* Clock command/status */
long constant;   /* PLL (phase-locked loop) time constant */
long precision;  /* Clock precision (microseconds, read-only) */
long tolerance;  /* Clock frequency tolerance (ppm, read-only) */
struct timeval time;
 /* Current time (read-only, except for
ADJ_SETOFFSET); upon return, time.tv_usec
contains nanoseconds, if STA_NANO status
flag is set, otherwise microseconds */
long tick;   /* Microseconds between clock ticks */
long ppsfreq;/* PPS (pulse per second) frequency (in units
of 2^-16 ppm\-\-see NOTES, read-only) */
long jitter; /* PPS jitter (read-only); nanoseconds, if
STA_NANO status flag is set, otherwise
microseconds */
int  shift;  /* PPS interval duration (seconds, read-only) */
long stabil; /* PPS stability (2^-16 ppm\-\-see NOTES,
read-only) */
long jitcnt; /* PPS jitter limit exceeded (read-only) */
long calcnt; /* PPS calibration intervals (read-only) */
long errcnt; /* PPS calibration errors (read-only) */
long stbcnt; /* PPS stability limit exceeded (read-only) */
int tai; /* TAI offset, as set by previous ADJ_TAI
operation (seconds, read-only,
since Linux 2.6.26) */
/* Further padding bytes to allow for future expansion */
};
.fi
.in
.PP
The
.I modes
field determines which parameters, if any, to set.
It is a bit mask containing a
.RI bitwise- or
combination of zero or more of the following bits:
.TP
.BR ADJ_OFFSET
Set time offset from
.IR buf.offset .
.TP
.BR ADJ_FREQUENCY
Set frequency offset from
.IR buf.freq .
.TP
.BR ADJ_MAXERROR
Set maximum time error from
.IR buf.maxerror .
.TP
.BR ADJ_ESTERROR
Set estimated time error from
.IR buf.esterror .
.TP
.BR ADJ_STATUS
Set clock status from
.IR buf.status .
.TP
.BR ADJ_TIMECONST
Set PLL time constant from
.IR buf.constant .
If the
.B STA_NANO
status flag (see

Re: [Request for review] Revised delete_module(2) manual page

[Michael Kerrisk (man-pages)]

Lucas,

On Wed, Oct 24, 2012 at 7:27 AM, Lucas De Marchi
 wrote:
> Hi Michael,
>
>
> On Sun, Oct 21, 2012 at 5:36 AM, Michael Kerrisk (man-pages)
>  wrote:
>> Ping!
>>
>> Rusty (et al.) I'm pretty sure the new page text is okay, but I would
>> like someone knowledgeable to confirm.
>
> One more thing:
>
>> .SH "SEE ALSO"
>> .BR create_module (2),
>> .BR init_module (2),
>> .BR query_module (2),
>> .BR lsmod (8),
>> .BR rmmod (8)
>
> Shouldn't we remove the reference to query_module(2) and
> create_module(2)? They don't exist anymore (and I miss a bigger
> warning on their man pages).

I think the SEE ALSO links should be kept, but you are right that the
warnings that query_module(2) and create_module(2) no longer exist
should be more prominent on those pages. By chance, I'd already made
that change.

> Last, I think there should be a ref here to modprobe (because of -r
> flag), not lsmod. The rest looks good.

I'll add modprobe(8); I think lsmod(8) is worth keeping.

Cheers,

Michael

-- 
Michael Kerrisk
Linux man-pages maintainer; http://www.kernel.org/doc/man-pages/
Author of "The Linux Programming Interface"; http://man7.org/tlpi/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Request for review] Revised delete_module(2) manual page

[Rusty Russell]

"Michael Kerrisk (man-pages)"  writes:

> Ping!
>
> Rusty (et al.) I'm pretty sure the new page text is okay, but I would
> like someone knowledgeable to confirm.

Yes, sorry, I did read it, and had nothing to add.

Ack,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Request for review] Revised delete_module(2) manual page

[Lucas De Marchi]

Hi Michael,


On Sun, Oct 21, 2012 at 5:36 AM, Michael Kerrisk (man-pages)
 wrote:
> Ping!
>
> Rusty (et al.) I'm pretty sure the new page text is okay, but I would
> like someone knowledgeable to confirm.

One more thing:

> .SH "SEE ALSO"
> .BR create_module (2),
> .BR init_module (2),
> .BR query_module (2),
> .BR lsmod (8),
> .BR rmmod (8)

Shouldn't we remove the reference to query_module(2) and
create_module(2)? They don't exist anymore (and I miss a bigger
warning on their man pages).

Last, I think there should be a ref here to modprobe (because of -r
flag), not lsmod. The rest looks good.


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Request for review] Revised delete_module(2) manual page

[Michael Kerrisk (man-pages)]

Ping!

Rusty (et al.) I'm pretty sure the new page text is okay, but I would
like someone knowledgeable to confirm.

Thanks,

Michael

-- Forwarded message --
From: Michael Kerrisk (man-pages) 
Date: Fri, Oct 12, 2012 at 10:47 AM
Subject: Re: [Request for review] Revised delete_module(2) manual page


Hi Rusty,

Thanks for taking a look at this page. In the light of your comments,
I've substantially reworked the page, and further review would not go
amiss, in case I made a misstep along the way.

On Thu, Oct 11, 2012 at 5:02 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>
>> Hello Kees, Rusty,
>>
>> The current delete_module(2) page is severely out of date (basically,
>> its content corresponds to 2.4 days, and was even pretty thin in
>> covering that). So, I took a shot at revising the page to Linux 2.6+
>> reality. Would it be possible that you could review it?
>
> OK.  Main suggestion is that I discussed with Lucas removing the
> !O_NONBLOCK case.  It's not supported by modprobe -r, and almost
> unheard-of for rmmod (it's --wait).
>
> In practice, people want the unload-or-fail semantics, or the
> force-unload semantics.

Okay -- I've substantially reworked the page to reflect these idea.

>> Otherwise, by default,
>> .BR delete_module ()
>> marks a module so that no new references are permitted.
>> If the module's reference count
>> (i.e., the number of processes currently using the module) is nonzero,
>> it then places the caller in an uninterruptible sleep
>> state until all reference count is zero,
>> at which point the call unblocks.
>> When the reference count reaches zero, the module is unloaded.
>
> So this should be inverted:
>
> Otherwise (assuming O_NONBLOCK, see flags below), if the
> module's reference count (i.e., the number of processes
> currently using the module) is nonzero, the call fails.

Got it. See my reworked text.

[...]

> NOTES:
>
> If O_NONBLOCK is not set, then the kernel may enter uninterruptible
> sleep until the module reference count reaches zero.  This is not
> generally desirable, so this flag may be compulsory in future kernel
> configurations.

I've added some text under NOTES.

Okay, below (and attached) is the new version of the page. Let me know
of any concerns.

Cheers,

Michael


.\" Copyright (C) 2012 Michael Kerrisk 
.\"
.\" Permission is granted to make and distribute verbatim copies of this
.\" manual provided the copyright notice and this permission notice are
.\" preserved on all copies.
.\"
.\" Permission is granted to copy and distribute modified versions of this
.\" manual under the conditions for verbatim copying, provided that the
.\" entire resulting derived work is distributed under the terms of a
.\" permission notice identical to this one.
.\"
.\" Since the Linux kernel and libraries are constantly changing, this
.\" manual page may be incorrect or out-of-date.  The author(s) assume no
.\" responsibility for errors or omissions, or for damages resulting from
.\" the use of the information contained herein.  The author(s) may not
.\" have taken the same level of care in the production of this manual,
.\" which is licensed free of charge, as they might when working
.\" professionally.
.\"
.\" Formatted or processed versions of this manual, if unaccompanied by
.\" the source, must acknowledge the copyright and authors of this work.
.\"
.TH DELETE_MODULE 2 2012-10-12 "Linux" "Linux Programmer's Manual"
.SH NAME
delete_module \- unload a kernel module
.SH SYNOPSIS
.nf
.BI "int delete_module(const char *" name ", int " flags );
.fi

.IR Note :
There is no glibc wrapper for this system call; see NOTES.
.SH DESCRIPTION
The
.BR delete_module ()
system call attempts to remove the unused loadable module entry
identified by
.IR name .
If the module has an
.I exit
function, then that function is executed before unloading the module.
The
.IR flags
argument is used to modify the behavior of the system call,
as described below.
This system call requires privilege.

Module removal is attempted according to the following rules:
.IP 1. 4
If there are other loaded modules that depend on
(i.e., refer to symbols defined in) this module,
then the call fails.
.IP 2.
Otherwise, if the reference count for the module
(i.e., the  number  of processes currently using the module)
is zero, then the module is immediately unloaded.
.IP 3.
If a module has a nonzero reference count,
then the behavior depends on the bits set in
.IR flags .
In normal usage (see NOTES), the
.BR O_NONBLOCK
flag is always specified, and the
.BR O_TRUNC
flag may additionally be specified.

Re: [Request for review] Revised delete_module(2) manual page

[Michael Kerrisk (man-pages)]

Hi Rusty,

Thanks for taking a look at this page. In the light of your comments,
I've substantially reworked the page, and further review would not go
amiss, in case I made a misstep along the way.

On Thu, Oct 11, 2012 at 5:02 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>
>> Hello Kees, Rusty,
>>
>> The current delete_module(2) page is severely out of date (basically,
>> its content corresponds to 2.4 days, and was even pretty thin in
>> covering that). So, I took a shot at revising the page to Linux 2.6+
>> reality. Would it be possible that you could review it?
>
> OK.  Main suggestion is that I discussed with Lucas removing the
> !O_NONBLOCK case.  It's not supported by modprobe -r, and almost
> unheard-of for rmmod (it's --wait).
>
> In practice, people want the unload-or-fail semantics, or the
> force-unload semantics.

Okay -- I've substantially reworked the page to reflect these idea.

>> Otherwise, by default,
>> .BR delete_module ()
>> marks a module so that no new references are permitted.
>> If the module's reference count
>> (i.e., the number of processes currently using the module) is nonzero,
>> it then places the caller in an uninterruptible sleep
>> state until all reference count is zero,
>> at which point the call unblocks.
>> When the reference count reaches zero, the module is unloaded.
>
> So this should be inverted:
>
> Otherwise (assuming O_NONBLOCK, see flags below), if the
> module's reference count (i.e., the number of processes
> currently using the module) is nonzero, the call fails.

Got it. See my reworked text.

[...]

> NOTES:
>
> If O_NONBLOCK is not set, then the kernel may enter uninterruptible
> sleep until the module reference count reaches zero.  This is not
> generally desirable, so this flag may be compulsory in future kernel
> configurations.

I've added some text under NOTES.

Okay, below (and attached) is the new version of the page. Let me know
of any concerns.

Cheers,

Michael


.\" Copyright (C) 2012 Michael Kerrisk 
.\"
.\" Permission is granted to make and distribute verbatim copies of this
.\" manual provided the copyright notice and this permission notice are
.\" preserved on all copies.
.\"
.\" Permission is granted to copy and distribute modified versions of this
.\" manual under the conditions for verbatim copying, provided that the
.\" entire resulting derived work is distributed under the terms of a
.\" permission notice identical to this one.
.\"
.\" Since the Linux kernel and libraries are constantly changing, this
.\" manual page may be incorrect or out-of-date.  The author(s) assume no
.\" responsibility for errors or omissions, or for damages resulting from
.\" the use of the information contained herein.  The author(s) may not
.\" have taken the same level of care in the production of this manual,
.\" which is licensed free of charge, as they might when working
.\" professionally.
.\"
.\" Formatted or processed versions of this manual, if unaccompanied by
.\" the source, must acknowledge the copyright and authors of this work.
.\"
.TH DELETE_MODULE 2 2012-10-12 "Linux" "Linux Programmer's Manual"
.SH NAME
delete_module \- unload a kernel module
.SH SYNOPSIS
.nf
.BI "int delete_module(const char *" name ", int " flags );
.fi

.IR Note :
There is no glibc wrapper for this system call; see NOTES.
.SH DESCRIPTION
The
.BR delete_module ()
system call attempts to remove the unused loadable module entry
identified by
.IR name .
If the module has an
.I exit
function, then that function is executed before unloading the module.
The
.IR flags
argument is used to modify the behavior of the system call,
as described below.
This system call requires privilege.

Module removal is attempted according to the following rules:
.IP 1. 4
If there are other loaded modules that depend on
(i.e., refer to symbols defined in) this module,
then the call fails.
.IP 2.
Otherwise, if the reference count for the module
(i.e., the  number  of processes currently using the module)
is zero, then the module is immediately unloaded.
.IP 3.
If a module has a nonzero reference count,
then the behavior depends on the bits set in
.IR flags .
In normal usage (see NOTES), the
.BR O_NONBLOCK
flag is always specified, and the
.BR O_TRUNC
flag may additionally be specified.
.\" O_TRUNC == KMOD_REMOVE_FORCE in kmod library
.\" O_NONBLOCK == KMOD_REMOVE_NOWAIT in kmod library
The various combinations for
.I flags
have the following effect:
.RS 4
.TP
.B flags == O_NONBLOCK
The call returns immediately, with an error.
.TP
.B flags == (O_NONBLOCK | O_TRUNC)
The module is unloaded immediately,
regardless of whether it has a nonzero reference count.
.TP
.B flags == 0
If
.I flags
does not specify
.BR O_NONBLOCK ,
the following steps occur:
.RS
.IP * 3
The module is marked so that no new references are permitted.
.IP *
If the module's reference count is nonzero,
the caller is placed in an uninterruptible sleep state
.RB ( TASK_UNINTERRUPTIBLE 

Re: [Request for review] Revised delete_module(2) manual page

[Rusty Russell]

Lucas De Marchi  writes:
> What do you think? Mark as deprecated now and remove when kernel
> removes it? Or remove now?

Complain now, and I'll queue the removal in two merge windows.

Thats gives us a chance just in case someone actually uses this; if so I
want to talk to them about what it is they want!

Thanks,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Request for review] Revised delete_module(2) manual page

[Lucas De Marchi]

Hi,

On Thu, Oct 11, 2012 at 12:02 AM, Rusty Russell  wrote:
> "Michael Kerrisk (man-pages)"  writes:
>
>> Hello Kees, Rusty,
>>
>> The current delete_module(2) page is severely out of date (basically,
>> its content corresponds to 2.4 days, and was even pretty thin in
>> covering that). So, I took a shot at revising the page to Linux 2.6+
>> reality. Would it be possible that you could review it?
>
> OK.  Main suggestion is that I discussed with Lucas removing the
> !O_NONBLOCK case.  It's not supported by modprobe -r, and almost
> unheard-of for rmmod (it's --wait).
>
> In practice, people want the unload-or-fail semantics, or the
> force-unload semantics.


I'm all for removing this option. My idea was to complain loudly if
user tries to use it:
http://git.kernel.org/?p=utils/kernel/kmod/kmod.git;a=commit;h=8447b865aaac9139485dccdcc576725ddec2e7fa

But maybe it's good to just remove it altogether


>
>> Otherwise, by default,
>> .BR delete_module ()
>> marks a module so that no new references are permitted.
>> If the module's reference count
>> (i.e., the number of processes currently using the module) is nonzero,
>> it then places the caller in an uninterruptible sleep
>> state until all reference count is zero,
>> at which point the call unblocks.
>> When the reference count reaches zero, the module is unloaded.
>
> So this should be inverted:
>
> Otherwise (assuming O_NONBLOCK, see flags below), if the
> module's reference count (i.e., the number of processes
> currently using the module) is nonzero, the call fails.
>
>> The
>> .IR flags
>> argument can be used to modify the behavior of the system call.
>
> It is usually set to O_NONBLOCK, which may be required in future kernel
> versions (see NOTES).
>
>> The following values can be ORed in this argument:
>> .TP
>> .B O_TRUNC
>> .\"   KMOD_REMOVE_FORCE in kmod library
>> Force unloading of the module, even if the following conditions are true:
>> .RS
>> .IP * 3
>> The module has no
>> .I exit
>> function.
>> By default, attempting to unload a module that has no
>> .I exit
>> function fails.
>> .IP *
>> The reference count for (i.e., the number of processes currently using)
>> this module is nonzero.
> ...
>> .IP
>> Using this flag taints the kernel (TAINT_FORCED_RMMOD).
>> .IP
>> .IR "Using this flag is dangerous!"
>> If the kernel was not built with
>> .BR CONFIG_MODULE_FORCE_UNLOAD ,
>> this flag is silently ignored.
>
> NOTES:
>
> If O_NONBLOCK is not set, then the kernel may enter uninterruptible
> sleep until the module reference count reaches zero.  This is not
> generally desirable, so this flag may be compulsory in future kernel
> configurations.

What do you think? Mark as deprecated now and remove when kernel
removes it? Or remove now?


Lucas De Marchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [Request for review] Revised delete_module(2) manual page

[Rusty Russell]

"Michael Kerrisk (man-pages)"  writes:

> Hello Kees, Rusty,
>
> The current delete_module(2) page is severely out of date (basically,
> its content corresponds to 2.4 days, and was even pretty thin in
> covering that). So, I took a shot at revising the page to Linux 2.6+
> reality. Would it be possible that you could review it?

OK.  Main suggestion is that I discussed with Lucas removing the
!O_NONBLOCK case.  It's not supported by modprobe -r, and almost
unheard-of for rmmod (it's --wait).

In practice, people want the unload-or-fail semantics, or the
force-unload semantics.

> Otherwise, by default,
> .BR delete_module ()
> marks a module so that no new references are permitted.
> If the module's reference count
> (i.e., the number of processes currently using the module) is nonzero,
> it then places the caller in an uninterruptible sleep
> state until all reference count is zero,
> at which point the call unblocks.
> When the reference count reaches zero, the module is unloaded.

So this should be inverted:

Otherwise (assuming O_NONBLOCK, see flags below), if the
module's reference count (i.e., the number of processes
currently using the module) is nonzero, the call fails.

> The
> .IR flags
> argument can be used to modify the behavior of the system call.

It is usually set to O_NONBLOCK, which may be required in future kernel
versions (see NOTES).

> The following values can be ORed in this argument:
> .TP
> .B O_TRUNC
> .\"   KMOD_REMOVE_FORCE in kmod library
> Force unloading of the module, even if the following conditions are true:
> .RS
> .IP * 3
> The module has no
> .I exit
> function.
> By default, attempting to unload a module that has no
> .I exit
> function fails.
> .IP *
> The reference count for (i.e., the number of processes currently using)
> this module is nonzero.
...
> .IP
> Using this flag taints the kernel (TAINT_FORCED_RMMOD).
> .IP
> .IR "Using this flag is dangerous!"
> If the kernel was not built with
> .BR CONFIG_MODULE_FORCE_UNLOAD ,
> this flag is silently ignored.

NOTES:

If O_NONBLOCK is not set, then the kernel may enter uninterruptible
sleep until the module reference count reaches zero.  This is not
generally desirable, so this flag may be compulsory in future kernel
configurations.

Cheers,
Rusty.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Duncan Sands]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Duncan Sands]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[David Woodhouse]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Alistair John Strachan]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[David Woodhouse]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Alistair John Strachan]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> 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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Zoran Stojsavljevic]

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)



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> -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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> -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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[matthieu castet]

Giampaolo Tomassoni wrote:

-Messaggio originale-
Da: Francois Romieu [mailto:[EMAIL PROTECTED]
Inviato: domenica 4 settembre 2005 17.33
A: Giampaolo Tomassoni
Cc: linux-kernel@vger.kernel.org;
[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...

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Alistair John Strachan]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> -Messaggio originale-
> Da: Francois Romieu [mailto:[EMAIL PROTECTED]
> Inviato: domenica 4 settembre 2005 17.33
> A: Giampaolo Tomassoni
> Cc: linux-kernel@vger.kernel.org;
> [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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> -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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Grzegorz Kulewski]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Jiri Slaby]

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 www.fi.muni.cz/~xslaby
~\-/~  [EMAIL PROTECTED]  ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Alistair John Strachan]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Grzegorz Kulewski]

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Alistair John Strachan]

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.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Francois Romieu]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Giampaolo Tomassoni]

> -Messaggio originale-
> Da: [EMAIL PROTECTED]
> [mailto:[EMAIL PROTECTED] conto di
> Francois Romieu
> Inviato: domenica 4 settembre 2005 14.01
> A: Giampaolo Tomassoni
> Cc: linux-kernel@vger.kernel.org;
> [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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Francois Romieu]

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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Request for review

[Patrizio Bassi]

Giampaolo Tomassoni ha scritto:

Dears,

I wrote a first release of a SAR helper module for Linux 2.6.x.

It is conceptually similar to the Duncan Sands' usb_atm module, but it is not 
constrained to usb devices and is a bit different from it in its implementation 
details.

It seems to me that scores some points in this:

- supplies a coherent interface which allows the device driver to bypass
almost any interaction with the atm layer;

- supports ATM header&hec check, correction and generation, which is
most useful for dumb atm devices (ie: most ADSL modems);

- supports automatic and fast routing of received cells to destinating
vcc;

- actually supports AALraw, AAL0 and AAL5;

- aal decoding/encoding routines are designed as atmsar plug-ins, so
that further types may easily be supported;

- implements speed- and memory-concerned techniques for per-vcc
decoding;

- allows using dma-streaming techniques in sending cells to device;

- supports tx buffer adjusting against device needs;

- avoids vcc open/close paradigm handling in device driver;

- yields a uniform view to SAR status in /proc.


I'm contacting you firstly because I would like to hear your point of view 
about the whole idea (am I reinventing the wheel?), then because I would like 
to have my code reviewed, and lastly because I have some question about the 
matter.

I prepared the SAR module as a patch against the Linux 2.6.12.3 kernel tree. It 
is attached to this message as a unified diff.

Also, in order to allow you to evaluate it, I prepared a driver adopting the 
SAR module's API for the Globespan Pulsar ADSL PCI card. This driver is made 
after the one from Guy Ellis (see http://adsl4linux.no-ip.org/pulsar.html for 
further reference). My version is a two-part driver: a GPL one ( 
http://www.tomassoni.biz/download/pulsar/pulsaradsl-1.0.1-source.tar.bz2 ) and 
a proprietary library ( 
http://www.tomassoni.biz/download/pulsar/libpulsar_fw.a.bz2 ) which shall be 
decompressed and put in the pulsaradsl-1.0.1 directory in order to build the 
driver. The libpulsar_fw.a is the one for GCC 3. If you need a version of this 
library for another GCC flavor, please see the above reported Ellis' pulsar 
site.

Note also that I'm allowing you to download a copy of the libpulsar_fw.a 
library only for the purpose of evaluating the SAR module: it is not meant to 
be used otherway.

My intention, if the maintainer of the ATM stack under Linux (Chas Williams) 
and the author of the usb_atm module (Duncan Sands) agree, is to merge the 
atmsar module into the linux tree, thereby replacing the atm+sar code in the 
Verrept-Sands' atm_usb module, which will then contain only the usb handling 
code and eventually will relay on the atmsar module for its atm+sar ops. I'm 
also looking for help by Sands to do this. Also, it could be interesting to 
have the Ellis' driver use this module, as the Pulsar PCI ADSL card is actually 
the only ADSL PCI modem of which I'm aware.

Questions are spread in comments in the atmsar sources. I can of course ask them by 
e-mail, but let the Q&A phase follow your feedbacks about the need for this 
brand-new wheel...

Regards,

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



i see a sort of non-sense in your (argh!!) binary (but just for 
evaluation) library.



-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: Request for review

[Jiri Slaby]

Giampaolo Tomassoni napsal(a):


Dears,

I wrote a first release of a SAR helper module for Linux 2.6.x.

It is conceptually similar to the Duncan Sands' usb_atm module, but it is not 
constrained to usb devices and is a bit different from it in its implementation 
details.

It seems to me that scores some points in this:

- supplies a coherent interface which allows the device driver to bypass
almost any interaction with the atm layer;

- supports ATM header&hec check, correction and generation, which is
most useful for dumb atm devices (ie: most ADSL modems);

- supports automatic and fast routing of received cells to destinating
vcc;

- actually supports AALraw, AAL0 and AAL5;

- aal decoding/encoding routines are designed as atmsar plug-ins, so
that further types may easily be supported;

- implements speed- and memory-concerned techniques for per-vcc
decoding;

- allows using dma-streaming techniques in sending cells to device;

- supports tx buffer adjusting against device needs;

- avoids vcc open/close paradigm handling in device driver;

- yields a uniform view to SAR status in /proc.


I'm contacting you firstly because I would like to hear your point of view 
about the whole idea (am I reinventing the wheel?), then because I would like 
to have my code reviewed, and lastly because I have some question about the 
matter.

I prepared the SAR module as a patch against the Linux 2.6.12.3 kernel tree. It 
is attached to this message as a unified diff.

Also, in order to allow you to evaluate it, I prepared a driver adopting the 
SAR module's API for the Globespan Pulsar ADSL PCI card. This driver is made 
after the one from Guy Ellis (see http://adsl4linux.no-ip.org/pulsar.html for 
further reference). My version is a two-part driver: a GPL one ( 
http://www.tomassoni.biz/download/pulsar/pulsaradsl-1.0.1-source.tar.bz2 ) and 
a proprietary library ( 
http://www.tomassoni.biz/download/pulsar/libpulsar_fw.a.bz2 ) which shall be 
decompressed and put in the pulsaradsl-1.0.1 directory in order to build the 
driver. The libpulsar_fw.a is the one for GCC 3. If you need a version of this 
library for another GCC flavor, please see the above reported Ellis' pulsar 
site.

Note also that I'm allowing you to download a copy of the libpulsar_fw.a 
library only for the purpose of evaluating the SAR module: it is not meant to 
be used otherway.

My intention, if the maintainer of the ATM stack under Linux (Chas Williams) 
and the author of the usb_atm module (Duncan Sands) agree, is to merge the 
atmsar module into the linux tree, thereby replacing the atm+sar code in the 
Verrept-Sands' atm_usb module, which will then contain only the usb handling 
code and eventually will relay on the atmsar module for its atm+sar ops. I'm 
also looking for help by Sands to do this. Also, it could be interesting to 
have the Ellis' driver use this module, as the Pulsar PCI ADSL card is actually 
the only ADSL PCI modem of which I'm aware.
 

+void   atmsar_rx_decode_52bytes(atmsar_dev_t* dev, const void* 
cell)
why the spaces between void and function name? The readability of the 
code is bad :(.

+{ sarDecode(dev, ntohl(*(unsigned*)cell), &((char*)cell)[4]); }
and
+static inline void dumpCell(char* dst, const ATM_CELL* cell) {
What's this? See CodingStyle, chapter 3.

CodingStyle, chapter 2:
-The limit on the length of lines is 80 columns and this is a hard limit.-
Please keep to it.

Fix this:
/l/latest/xxx/drivers/atm/sar.c:254:5: warning: "DBG" is not defined
/l/latest/xxx/drivers/atm/sar.c:307:5: warning: "DBG" is not defined

Remove trailing spaces from the end of lines, please.

   return(sar);
no parens around return value.

   if(len < ATM_CELL_PAYLOAD)
space between if and (

   atmsar_vcc_t *sar = (atmsar_vcc_t*)kmalloc(sizeof(sar), GFP_KERNEL);
this should be sizeof(*sar)
   if(sar != NULL)
   memset(sar, 0, sizeof(*sar));
use kzalloc instead of this piece (you may need to upgrade your kernel 
to some latest version)


   struct sk_buff* skbIn
* after space, not before

   int i;
   atm_dev_deregister(dev->atmdev);
   for(i = 0; i < ATMSAR_N_HASHVCCS; ++i)
space between for and (
   while(dev->vccs_hash[i] != NULL)
   atmClose(dev->vccs_hash[i]->vcc);
unsigned int is better if you don't need to count in negative, this is 
only hint.


thanks,

--
Jiri Slaby www.fi.muni.cz/~xslaby
~\-/~  [EMAIL PROTECTED]  ~\-/~
241B347EC88228DE51EE A49C4A73A25004CB2A10

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/