Refine doc & code about lkmpg HOT 7 CLOSED

5. Unmached code and comment
   

   lkmpg/examples/print_string.c

   Lines 44 to 56 in 637e707

   	/* ttys were originally hardware devices, which (usually) strictly
   	* followed the ASCII standard. In ASCII, to move to a new line you
   	* need two characters, a carriage return and a line feed. On Unix,
   	* the ASCII line feed is used for both purposes - so we can not
   	* just use \n, because it would not have a carriage return and the
   	* next line will start at the column right after the line feed.
   	*
   	* This is why text files are different between Unix and MS Windows.
   	* In CP/M and derivatives, like MS-DOS and MS Windows, the ASCII
   	* standard was strictly adhered to, and therefore a newline requires
   	* both a LF and a CR.
   	*/
   	(ttyops->write)(my_tty, "\015\012", 2);

   
   "\015\012" is \v\b rather than CRLF \r\n.

from lkmpg.

Hi @minghu6 ,
Sorry for the late to reply.

1. Move the confusing note about struct proc_ops replacing with struct file_operations on linux >=5.6:
   

   lkmpg/lkmpg.tex

   Lines 957 to 990 in 637e707

   	Since Linux v5.6, the \cpp|proc_ops| structure was introduced to replace the use of the \cpp|file_operations| structure when registering proc handlers.
   	\subsection{The file structure}
   	\label{sec:file_struct}
   	Each device is represented in the kernel by a file structure, which is defined in \src{include/linux/fs.h}.
   	Be aware that a file is a kernel level structure and never appears in a user space program.
   	It is not the same thing as a \cpp|FILE|, which is defined by glibc and would never appear in a kernel space function.
   	Also, its name is a bit misleading; it represents an abstract open `file', not a file on a disk, which is represented by a structure named \cpp|inode|.
   	An instance of struct file is commonly named \cpp|filp|.
   	You'll also see it referred to as a struct file object.
   	Resist the temptation.
   	Go ahead and look at the definition of file.
   	Most of the entries you see, like struct dentry are not used by device drivers, and you can ignore them.
   	This is because drivers do not fill file directly; they only use structures contained in file which are created elsewhere.
   	\subsection{Registering A Device}
   	\label{sec:register_device}
   	As discussed earlier, char devices are accessed through device files, usually located in \verb|/dev|.
   	This is by convention. When writing a driver, it is OK to put the device file in your current directory.
   	Just make sure you place it in \verb|/dev| for a production driver.
   	The major number tells you which driver handles which device file.
   	The minor number is used only by the driver itself to differentiate which device it is operating on, just in case the driver handles more than one device.
   	Adding a driver to your system means registering it with the kernel.
   	This is synonymous with assigning it a major number during the module's initialization.
   	You do this by using the \cpp|register_chrdev| function, defined by \src{include/linux/fs.h}.
   	\begin{code}
   	int register_chrdev(unsigned int major, const char *name, struct file_operations *fops);
   	\end{code}

   The struct proc_ops would replace the struct file-operations only when create proc releated defined on <linux/proc_fs.h>
   In other word, regist device doesn't be affected, still using struct file_operations. It's confusing write this note above the regist device code (FOE ME, I HAVE SPENT PLENTY OF TIME ON SEARCHING HOW TO REGIST DEVICE USING STRUCT PROC_OPS)
   I highly recommend to move this note down to next code example which replacing takes place truely!

Yes, it won't affect the registering of the device, but the paragraph here actually talks about the file operation. And, it seems like the file operation will be related to the proc_ops. So, I think the description of proc_ops replacing the file_ops is still needed in the section here.
But still, it might confuse others. So, I will add more sentences to hint that this description talks about the file_ops and proc_ops rather than registering the device.

2. lkmpg/examples/chardev.c

   Lines 25 to 39 in 637e707

   	#define BUF_LEN 80 /* Max length of the message from the device */
   	/* Global variables are declared as static, so are global within the file. */
   	static int major; /* major number assigned to our device driver */
   	enum {
   	CDEV_NOT_USED = 0,
   	CDEV_EXCLUSIVE_OPEN = 1,
   	};
   	/* Is device open? Used to prevent multiple access to device */
   	static atomic_t already_open = ATOMIC_INIT(CDEV_NOT_USED);
   	static char msg[BUF_LEN]; /* The msg the device will give when asked */

   The msg char array should be declared with length of BUF_LEN + 1 since BUF_LEN is max length of message.

The macro, BUF_LEN, is the length of the buffer, not the message length.
So, I think there doesn't need to change.

3. lkmpg/examples/chardev.c

   Line 152 in 637e707

   return -EINVAL;

   On device_write, It's prefered to use EOPNOTSUPP instead of EINVAL since the former raises unsupported error, the latter raises invalid argument.

Yes, it should uses the EOPNOTSUPP.
Thanks.

4. Improve read/write fucntion code among fsproc{1-3}.c:
   

   lkmpg/examples/procfs3.c

   Lines 23 to 40 in 637e707

   	static ssize_t procfs_read(struct file *filp, char __user *buffer,
   	size_t length, loff_t *offset)
   	{
   	static int finished = 0;
   	if (finished) {
   	pr_debug("procfs_read: END\n");
   	finished = 0;
   	return 0;
   	}
   	finished = 1;
   	if (copy_to_user(buffer, procfs_buffer, procfs_buffer_size))
   	return -EFAULT;
   	pr_debug("procfs_read: read %lu bytes\n", procfs_buffer_size);
   	return procfs_buffer_size;
   	}

   lkmpg/examples/procfs3.c

   Lines 41 to 53 in 637e707

   	static ssize_t procfs_write(struct file *file, const char __user *buffer,
   	size_t len, loff_t *off)
   	{
   	if (len > PROCFS_MAX_SIZE)
   	procfs_buffer_size = PROCFS_MAX_SIZE;
   	else
   	procfs_buffer_size = len;
   	if (copy_from_user(procfs_buffer, buffer, procfs_buffer_size))
   	return -EFAULT;
   	pr_debug("procfs_write: write %lu bytes\n", procfs_buffer_size);
   	return procfs_buffer_size;
   	}

These code are a little wired and far from decent, the parameter loff_t * is ignored incorrectly and so the behavior is quite different from normal read/write.I've reimplemented read/write code, here is full code:

It need more modern c standard config than C90, for example ccflags-y += -std=gnu17.

The read/write functions are a little weird. I will modify them.
For using modern C standards, we need to consider more.
Since kernel version v5.18, especially after this commit "Kbuild: move to -std=gnu11", kernel supports C11 features. We have the option to move to it.
But considering LKMPG is supporting the v5.x version, we must stick to the C89 still a while.
However, for C17, it might be the wrong standard to use.

from lkmpg.

5. Unmached code and comment
   

   lkmpg/examples/print_string.c

   Lines 44 to 56 in 637e707

   	/* ttys were originally hardware devices, which (usually) strictly
   	* followed the ASCII standard. In ASCII, to move to a new line you
   	* need two characters, a carriage return and a line feed. On Unix,
   	* the ASCII line feed is used for both purposes - so we can not
   	* just use \n, because it would not have a carriage return and the
   	* next line will start at the column right after the line feed.
   	*
   	* This is why text files are different between Unix and MS Windows.
   	* In CP/M and derivatives, like MS-DOS and MS Windows, the ASCII
   	* standard was strictly adhered to, and therefore a newline requires
   	* both a LF and a CR.
   	*/
   	(ttyops->write)(my_tty, "\015\012", 2);

   "\015\012" is \v\b rather than CRLF \r\n.

The "\015\012" seems correct.
According to the ASCII table, CR is 015 and LF is 012.
And, \v\b is \013\010.
I might be wrong, please provide more information about this.

Thanks

from lkmpg.

3. lkmpg/examples/chardev.c

   Line 152 in 637e707

   return -EINVAL;

   On device_write, It's prefered to use EOPNOTSUPP instead of EINVAL since the former raises unsupported error, the latter raises invalid argument.

Yes, it should uses the EOPNOTSUPP. Thanks.

Sorry, I recheck the write man page to make sure that the change here is suitable to the definition.
In the man page, it doesn't have any error value that is EOPNOTSUPP, ENOTSUP, or even ENOTSUPP (kernel defined).
And from the description of EINVAL in the manual:

EINVAL fd is attached to an object which is unsuitable for
       writing; or the file was opened with the O_DIRECT flag,
       and either the address specified in buf, the value
       specified in count, or the file offset is not suitably
       aligned.

fd is attached to an object which is unsuitable for writing

The EINVAL is correct. We don't have to change to EOPNOTSUPP.

from lkmpg.

2. lkmpg/examples/chardev.c

   Lines 25 to 39 in 637e707

   	#define BUF_LEN 80 /* Max length of the message from the device */
   	/* Global variables are declared as static, so are global within the file. */
   	static int major; /* major number assigned to our device driver */
   	enum {
   	CDEV_NOT_USED = 0,
   	CDEV_EXCLUSIVE_OPEN = 1,
   	};
   	/* Is device open? Used to prevent multiple access to device */
   	static atomic_t already_open = ATOMIC_INIT(CDEV_NOT_USED);
   	static char msg[BUF_LEN]; /* The msg the device will give when asked */

   The msg char array should be declared with length of BUF_LEN + 1 since BUF_LEN is max length of message.

The macro, BUF_LEN, is the length of the buffer, not the message length. So, I think there doesn't need to change.

Oops, I saw the comments.
Sorry again.

from lkmpg.

Ready to close?

from lkmpg.

Sure, I think.
If there are any other opinions we can just reopen it.

from lkmpg.