Errata

from http://www.makelinux.net/ldd3/?u=chp-7-sect-6


The text says
"The return value from these functions is 0 if the work was successfully added to the queue; a nonzero result means that this work_struct structure was already waiting in the queue, and was not added a second time."

but the return values are actually opposite according to the source code.

from /examples/scull/scull.init

I had problems loading scull.ko using the script scull.init.

That is because the function load_device tries to load the .o file and not the .ko file.

Original version, which does not work:

# Load and create files
function load_device () {

if [ -f $MODDIR/$DEVICE.o ]; then
devpath=$MODDIR/$DEVICE.o
else if [ -f ./$DEVICE.o ]; then
devpath=./$DEVICE.o

...

}

This version works:

# Load and create files
function load_device () {

if [ -f $MODDIR/$DEVICE.ko ]; then
devpath=$MODDIR/$DEVICE.ko
else if [ -f ./$DEVICE.ko ]; then
devpath=./$DEVICE.ko

...

}

"... building modules for 2.6.x requires that you have a configured and built kernel
tree on your system."

Technically, no. All you need (at least for 2.6.18 that I'm playing with) is a tree
that has been configured and had "make modules_prepare" run against it.

"... building modules for 2.6.x requires that you have a configured and built kernel
tree on your system."

No, only a tree that's been "make modules_prepare"d. the same inaccurate claim is made
2/3 of the way down p. 17.

I am not a kernel developer. Not sure about the following. Still,
1) For completness.
2) In view of the `Initiallization and Shutdown' section on pages 31 and 32.
3) In order to help newbies.
4) It seems to me that recent 2.6 kernels emit warnings about that.

I would modify:
static int hello_init(void)
to:
static int __init hello_init(void)

and:
static void hello_exit(void)
to:
static void __exit hello_exit(void)

In addition, you should add to the explanation text that, some more aspects of the example
code are elaborated in the `Initiallization and Shutdown' section on pages 31 and 32.

As on p.15, building modules doesn't need a configured and built kernel tree, just a configured and
"make modules_prepare"d tree at least for the latest versions of the kernel. (Perhaps this wasn't
true at the time of printing.)

there is no vermagic.o anymore -- instead, there's a "vermagic.h" header file these days.

"version.h" is automatically included by "module.h" ... not these days, it isn't.

the set of dependencies in Fig 2-2 doesn't appear to match the actual dependencies
(at least in the 2.6.18 kernel.)

the dependencies as listed by "lsmod" for the 2.6.18 kernel is, in part:

...
lp 12872 0
parport_pc 26788 0
parport 36424 2 lp,parport_pc
...

that would seem to differ from what the dependency arrows show in that figure, but
it's possible it was different back in 2.6.10.

I'm not sure that picture is technically accurate anymore, but I could be wrong.

If you already include module.h, then there is no need to additionally include
moduleparam.h since (at least at the moment) module.h includes that heasder file
directly already.

Personally, I think that's a bad idea, and I've even written a wiki page on it:

http://fsdev.net/wiki/index.php?title=Moduleparam.h

"item2" -> "item1", no?

another incorrect reference to vermagic.o, should refer to vermagic.h.

1. module_param_array should be included in its correct form as was already pointed out in another submission:

module_param_array(name,type,nump,perm);

2. For the module_param listed on page 41, "intarry" is not a valid data type:

"The type can be one of bool, charp, int, invbool, long, short, ushort, uint, ulong, or intarray."

This should be:

"The type can be one of bool, charp, int, invbool, long, short, ushort, uint, or ulong."

"request_chrdev_region" -> "register_chrdev_region"

The code is wrong off the positive paths.
1) the call to alloc_chrdev_region (beginning on p.48) could fail, w/o ever filling in dev,
so the code scull_major(dev) within the else block operates on value never initialized
2) the if (result<0) as well doesn't consider the possibility of the failure of alloc_chrdev_region(), only handling the failure of register_chrdev_region() call (on p.48)

int err, devno = MKDEV(scull_major, scull_minor + index);
should be:
int err;
dev_t devno = MKDEV(scull_major, scull_minor + index);

"Shut down the device on last close" is misleading, because it supposes release function is called
on every file close. Should read: "Shut down the device"

"writing a single byte in scull consumes 8,000 or 12,000 bytes of memory..."

That total counts the size of the first quantum pointer array and the first quantum,
but shouldn't you also add in the size of the first scull_qset structure you have to
allocate? Sure, it's tiny, but it should still be counted, no?

To be technically correct, sizeof(char *) should say sizeof(void *) since dptr->data
is of type void ** (i.e. pointer to void *).

CONFIG_INIT_DEBUG does not seem to exist anymore.

The code example on this page returns "Invalid argument"

# ./setconsole 1
./setconsole: ioctl(stdin, TIOCLINUX): Invalid argument

Code correction: 11 is the TIOCL_SETKMSGREDIRECT cmd number

include/linux/tiocl.h:#define TIOCL_SETKMSGREDIRECT 11 /* restrict kernel
messages to a vt */

Also, drivers/char/vt.c explains TIOCLINUX, not tty_io.c
I'm still trying to figure out why it always returns EINVAL.

My kernel version is:
# uname -r
2.6.17-1.2174_FC5

Original code:
int limit = count - 80; /* Don't print more than this */

Did you mean something like this:
int limit = (count > 80) ? 80 : count;

The example shows the read_seqretry() as:

} while read_seqretry(&the_lock, seq);

There must be parenthesis around the condition of 'while' in C.
So the line must be changed to:

} while (read_seqretry(&the_lock, seq));

FIOQSIZE ioctl returns the size of symbolic link (S_IFLNK) files
as well as regular and directory files.

I think it would be better if the text said: "... when applied to
any other file type such as device files, however, it ..."

The example of using the DEFINE_WAIT macro on page 156 has the parameter "my_wait." The text on page 157 begins "in which name ..."

"name" does not appear elsewhere on pages 156-7. This is apparently a reference to the string "name" in the definition of the DEFINE_WAIT macro, i.e.

#define DEFINE_WAIT(name) DEFINE_WAIT_FUNC(name, autoremove_wake_function)

Perhaps page 156 should say:

DEFINE_WAIT( <name> );
where <name> is the name of the wait queue entry variable, e.g.
DEFINE_WAIT (my_wait);

This paragraph explains why there is no problem if the wakeup happens immediately
before the call to schedule(). I think it should also explain why the following
scenario is not a race condition:
a) Wakeup occurs immediately before the call to prepare_to_wait()
b) Call to prepare_to_wait() sets process state to TASK_INTERRUPTIBLE
c) While the spacefree() function is running, the process is pre-empted by an unrelated
process.
d) The process is now still marked as TASK_INTERRUPTIBLE and will therefore not be
re-scheduled and will never execute the call to finish_wait() - so it will sleep
unless/until an interruption comes along...

"Note that there is no way to perform exclusive waits with wait_event and its
variants".

However, in linux-2.6.10/include/linux/wait.h the following is defined:

#define wait_event_interruptible_exclusive(wq, condition)

This disagrees with the above statement.

"...as long as that difference doesnt exceed the overflow
time, you can get the work done without claiming exclusive..."

The difference would never exceed the overflow time. Assume the register is one byte
and wrap around 256. The difference of any two values of unsigned char will not exceed
256, which is the value of the "overflow time". Therefore, It Should be "as long as
that difference doesn't exceed half of the overflow time".

The timer API description for mod_timer and del_timer_sync show an incorrect return type of int.

The Quick Reference list of same functions on page 270 at end of chapter shows the correct return
value of void for these functions.

The referenced sentence suggests using readb, yet later on page 251 it
says readb is discouraged and one should use ioread8 instead. Suggest
changing readb on p266 to ioread8.

Text refers to "readb" function - however on page 251, use of readb is described
as being 'discouraged', so text should refer to "ioread8" function (p251) instead.

Thank you very much at this BOOK!

On some systems module not correct work. This module normaly load and write value to port (parallel port), byt read every time return 0xff. This problem related with PnP (Plug and Play). Diagnostic this situation: (Thanks by Adam Belay <ambx1[AT]neo.rr[DOT]com> url http://www.mjmwired.net/kernel/Documentation/pnp.txt)

1) cd /sys/devices/pnp0/00:09
2) cat resources
(in my system)
state = disabled
io 0x378-0x37f
irq 5

I not yet fully completed cured short module, but i write own mini module, see below. It work fine.
If you have question mail me.

/*
* $Id: hello.c,v 1.5 2004/10/26 03:32:21 corbet Exp $
*/
#include <linux/init.h>
#include <linux/module.h>
#include <linux/unistd.h>
#include <linux/config.h>
#include <linux/sched.h>
#include <linux/delay.h>
#include <linux/errno.h>
#include <linux/interrupt.h>
#include <linux/ioport.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/pci.h>
#include <linux/pnp.h>
#include <linux/sysctl.h>

#include <asm/io.h>
#include <asm/dma.h>
#include <asm/uaccess.h>
#include <linux/parport.h>
#include <linux/parport_pc.h>
#include <linux/via.h>
#include <asm/parport.h>

#define base 0x378 /* printer port base address */
#define value 243 /* numeric value to send to printer port */
#define value_off 0



static const struct pnp_device_id parport_pc_pnp_tbl[] = {
/* Standard LPT Printer Port */
{.id = "PNP0400", .driver_data = 0},
/* ECP Printer Port */
{.id = "PNP0401", .driver_data = 0},
{ }
};

static struct pnp_driver short_trunc_a_pnp_driver = {
.name = "short_trunc_a",
.id_table = parport_pc_pnp_tbl,
/* .probe = parport_pc_pnp_probe,
.remove = parport_pc_pnp_remove,
*/
};

static int hello_init(void)
{
int r=0;
/* This call Enebled Parallel Port */
pnp_register_driver (&short_trunc_a_pnp_driver);

outb(value, base);
printk(KERN_ALERT "Hello, world\n");
return 0;
}

static void hello_exit(void)
{
printk(KERN_ALERT "Goodbye, cruel world\n");
}

module_init(hello_init);
module_exit(hello_exit);

Thank you very much at this BOOK!

On some systems module not correct work. This module normaly load and write value to port (parallel port), byt read every time return 0xff. This problem related with PnP (Plug and Play). Diagnostic this situation: (Thanks by Adam Belay <ambx1[AT]neo.rr[DOT]com> url http://www.mjmwired.net/kernel/Documentation/pnp.txt)

1) cd /sys/devices/pnp0/00:09
2) cat resources
(in my system)
state = disabled
io 0x378-0x37f
irq 5

I not yet fully completed cured short module, but i write own mini module, see below. It work fine.
If you have question mail me.

/*
* $Id: hello.c,v 1.5 2004/10/26 03:32:21 corbet Exp $
*/
#include <linux/init.h>
#include <linux/module.h>
#include <linux/unistd.h>
#include <linux/config.h>
#include <linux/sched.h>
#include <linux/delay.h>
#include <linux/errno.h>
#include <linux/interrupt.h>
#include <linux/ioport.h>
#include <linux/kernel.h>
#include <linux/slab.h>
#include <linux/pci.h>
#include <linux/pnp.h>
#include <linux/sysctl.h>

#include <asm/io.h>
#include <asm/dma.h>
#include <asm/uaccess.h>
#include <linux/parport.h>
#include <linux/parport_pc.h>
#include <linux/via.h>
#include <asm/parport.h>

#define base 0x378 /* printer port base address */
#define value 243 /* numeric value to send to printer port */
#define value_off 0



static const struct pnp_device_id parport_pc_pnp_tbl[] = {
/* Standard LPT Printer Port */
{.id = "PNP0400", .driver_data = 0},
/* ECP Printer Port */
{.id = "PNP0401", .driver_data = 0},
{ }
};

static struct pnp_driver short_trunc_a_pnp_driver = {
.name = "short_trunc_a",
.id_table = parport_pc_pnp_tbl,
/* .probe = parport_pc_pnp_probe,
.remove = parport_pc_pnp_remove,
*/
};

static int hello_init(void)
{
int r=0;
/* This call Enebled Parallel Port */
pnp_register_driver (&short_trunc_a_pnp_driver);

outb(value, base);
printk(KERN_ALERT "Hello, world\n");
return 0;
}

static void hello_exit(void)
{
printk(KERN_ALERT "Goodbye, cruel world\n");
}

module_init(hello_init);
module_exit(hello_exit);

Actually is an error in the code of short.c

this is the patch:

diff --git a/short/short.c b/short/short.c
index 8bd39ed..1ca14f0 100644
--- a/short/short.c
+++ b/short/short.c
@@ -58,6 +58,7 @@ module_param(use_mem, int, 0);
because it's what we want to use in the code */
static unsigned long base = 0x378;
unsigned long short_base = 0;
+unsigned long short_base_pre = 0; // pre remapping
module_param(base, long, 0);

/* The interrupt line is undefined by default. "short_irq" is as above */
@@ -572,8 +573,9 @@ int short_init(void)
return -ENODEV;
}

+ short_base_pre = short_base;
/* also, ioremap it */
- short_base = (unsigned long) ioremap(short_base, SHORT_NR_PORTS);
+ short_base = (unsigned long) ioremap(short_base_pre, SHORT_NR_PORTS);
/* Hmm... we should check the return value */
}
/* Here we register our device - should not fail thereafter */
@@ -681,7 +683,7 @@ void short_cleanup(void)
unregister_chrdev(major, "short");
if (use_mem) {
iounmap((void __iomem *)short_base);
- release_mem_region(short_base, SHORT_NR_PORTS);
+ release_mem_region(short_base_pre, SHORT_NR_PORTS);
} else {
release_region(short_base,SHORT_NR_PORTS);
}

----- EOP -----

In release_mem_region you should not use the remapped address, but the original one, otherwise the resource will not be freed and will fail with:

"Trying to free nonexistent resource <address-range>"

This at least on a similar device on 2.6.17.4 on powerpc arch.

Many thanks for this gorgeous book, I really would like to buy 4th edition as soon as it will be released, maybe updated to >= 2.6.31 and with more documentation on standard interfaces (e.g. kfifo and similar), complex real world examples (video drivers, embedded peripherals...). It was a joy reading and experimenting this book.

ioread32_rep reads count 32-bit values starting at buf.

->

ioread32_rep reads count 32-bit values starting at addr.

These functions read or write count values from the given buf to the given addr.

->

These functions read or write count values from the given addr to the given buf.

The code

*index = (new >= (short_buffer + PAGE_SIZE)) ? short_buffer : new;

should read

*index = (new >= (short_buffer + PAGE_SIZE)) ? (short_buffer + (new - short_buffer) % PAGE_SIZE):
new;

The current code works since delta is always 16 but if someone was to try to use this code in a
situation where delta took on other values then index might not be correct when the wrap around
occurred.

"IRQ5 is used for the serial ATA and IEEE1394 controllers;"
should read:
"IRQ5 is used for the serial ATA and USB2 controllers;"

if (!dev->bulk_out_endpointAddr &&
!(endpoint->bEndpointAddress & USB_DIR_IN) &&
...
--->
if (!dev->bulk_out_endpointAddr &&
!(endpoint->bEndpointAddress & USB_DIR_OUT) &&
...
########################################

It seems this was corrected from an earlier version, but it is still not quite correct.

The example provides sample code outlining how to memory map a particular region in memory.

unsigned long pfn = page_to_pfn(simple_region_start + off);

This is a problem. page_to_pfn() accepts a page struct as an argument. "simple_region_start" and "off" are *not* page structs, but rather a physical address and an address offset. This is seen by the code:

unsigned long off = vma->vm_pgoff << PAGE_SHIFT;

and in the paragraph before: "...beginning at physical address simple_region_start (which should be page-aligned)..."

To correct this, one should do:

unsigned long pfn = (simple_region_start + off) >> PAGE_SHIFT;

This generates the correct page frame number.

Beginning (at least) with 2.6.18 the second parameter to aio_read and aio_write
is an iovec pointer instead of a pointer to a char buffer with the third parameter,
count, indicating the number of iovecs. This issue, of course, affects all of "An
Asynchronous I/O example" on pages 439 and 440.

As an aside, this affects building the sample scullX drivers on fedora core 5 and 6
machines. Also note that main.c in those example drivers includes <linux/config.h>.
With Fedora core 6 you don't need full source to develop and test modules, just the
kernel-devel set. The Fedora core 6 system no longer includes kernel source and
recommends building external modules with just kernel-devel.

<linux/config.h> is not present, at least on my Fedora core 6 system with kernel-devel
loaded. It does not seem to be needed (if you ignore compile warnings that the file
doesn't exist) to build scullc etc.

My builds of scullc, etc., on an older Fedora Core system with source present succeed
as shipped since this is a 2.14 system and aio_read|write uses a char pointer for the
second parameter.

As of 2.6.15 tty flip buffers have now changed. See Alan Cox comment below.

Alan Cox patch 2.6.15-rc1

This replaces the tty flip buffers with kmalloc objects in rings. In the
normal situation for an IRQ driven serial port at typical speeds the
behaviour is pretty much the same, two buffers end up allocated and the
kernel cycles between them as before.

When there are delays or at high speed we now behave far better as the
buffer pool can grow a bit rather than lose characters. This also means
that we can operate at higher speeds reliably.

The following might be a suitable substitute for the example code

for (i = 0; i < data_size; ++i) {
if (tty_request_buffer_room(tty, 1) == 0)
break;
tty_insert_flip_char(tty, data[i], TTY_NORMAL);
}
tty_schedule_flip(tty);

[sample code] in file scull/pipe.c, scull_p_open method;
Every invocation of the scull_p_open method will reset the read and write pointers
(rp, wp) to buffer beginning. Doing so will cause a deadlock by putting the reader
and writer in an infinite sleep in some situations.

*To reproduce the bug:
- cat "file_BIGGER_than_buffer_size" > /dev/scullpipe0
- "cat" will sleep cause the buffer is full
- cat /dev/scullpipe0
- Buffer pointers will be reset by the statement dev->rp=dev->wp=dev->buffer. cat
will sleep thinking that the buffer is empty (dev->rp == dev->wp)
- reader and writer will infinitly sleep

* Solution:
Buffer pointers shouldn't be reset if any process opened the device before us (and
possibly changed rp or wp). Check could be done using the reference counters nreaders
and nwriters.

--- pipe.c 2006-12-22 04:21:23.000000000 +0200
+++ pipe.c 2006-12-22 06:26:24.000000000 +0200
@@ -75,9 +75,11 @@
return -ENOMEM;
}
}
- dev->buffersize = scull_p_buffer;
- dev->end = dev->buffer + dev->buffersize;
- dev->rp = dev->wp = dev->buffer; /* rd and wr from the beginning */
+ if (dev->nreaders == 0 && dev->nwriters == 0) {
+ dev->buffersize = scull_p_buffer;
+ dev->end = dev->buffer + dev->buffersize;
+ dev->rp = dev->wp = dev->buffer; /* rd and wr from the beginning */
+ }