Re: [PATCH] Documentation/kbuild: major edit of modules.txt

[matt mooney <mfm@muteddisk.com>]

On 17:33 Fri 17 Sep     , Michal Marek wrote:
> On 16.9.2010 11:10, matt mooney wrote:
> > A couple of notes: 
> >     I chose to use the environment variable $PWD in the examples for both the commandline
> >     and Makefile; however, I was wondering if I should have set PWD in the Makefile and
> >     used $(PWD) instead.
> 
> I'd rather keep the example Makefile minimalistic.

Ok, I was planning on still using the environment variable if that is okay with
you.

> 
> > Also, the $@ variable was removed from the 'make' command within
> >     the Makefile. I did not understand the purpose of this for building kernel modules and
> >     it would break my test build.
> 
> $@ is the name of the target, so in
> 		all::
> 			$(MAKE) -C $(KERNELDIR) M=`pwd` $@
> this translates to '$(MAKE) -C $(KERNELDIR) M=`pwd` all' which is the
> default. So with your change it does the same, but I'm surprised that
> the $@ didn't work for you.

I think we should leave this out anyway to simplify the command syntax.

> [...]
> > -	$KDIR refers to the path to the kernel source top-level directory
> > +	$KDIR is the path to the kernel source directory.
> 
> I'm not a native English speaker, but I read the new sentence as "the
> $KDIR variable holds the path to the kernel source directory, you can
> use the following commands verbatim", instead of "$KDIR is used in place
> of the path to the kernel source directory, replace it with the actual
> path".

You are right, I will reword this.

> >  
> > -	make -C $KDIR M=`pwd`
> > -		Will build the module(s) located in current directory.
> > -		All output files will be located in the same directory
> > -		as the module source.
> > -		No attempts are made to update the kernel source, and it is
> > -		a precondition that a successful make has been executed
> > -		for the kernel.
> > +	make -C $KDIR
> > +		-C is used to specify where to find the kernel source.
> > +		'make' will actually change to the specified directory
> > +		when executing and will change back when finished.
> >  
> > -	make -C $KDIR M=`pwd` modules
> > -		The modules target is implied when no target is given.
> > -		Same functionality as if no target was specified.
> > -		See description above.
> > +	make -C $KDIR M=$PWD
> > +		M= is used to tell kbuild that an external module is being
> > +		built. The option given to M= is the directory where the
> > +		external module (kbuild file) is located.
> >  
> > -	make -C $KDIR M=`pwd` modules_install
> > -		Install the external module(s).
> > -		Installation default is in /lib/modules/<kernel-version>/extra,
> > -		but may be prefixed with INSTALL_MOD_PATH - see separate
> > -		chapter.
> > +	make -C $KDIR SUBDIRS=$PWD
> > +		Same as M=. The SUBDIRS= syntax is kept for backwards
> > +		compatibility, but its usage is deprecated.
> 
> While you are rewriting the file, you can also drop the reference to
> SUBDIRS. It has been deprecated for over six years, so it doesn't need
> to be documented anymore, IMO. BTW, I like how you swapped the two
> sections, it is more logical that way.
> 
> [...]
> > -	Examples (module foo.ko, consist of bar.o, baz.o):
> > -		make -C $KDIR M=`pwd` bar.lst
> [...]
> > +		make -C $KDIR M=$PWD bar.o
> 
> make -C $KDIR M=`pwd` bar.lst was a valid command, see make help:
> 
>   dir/file.lst    - Build specified mixed source/assembly target only
>                     (requires a recent binutils and recent build
> (System.map))
> 
> Perhaps there should be a short sentence explaining what the four
> commands do.

Ah, an explanation may help.

> The rest looks OK to me (although I can't really comment on the language
> part), thanks a lot for looking into this.

I have actually reedited some of my changes.

After further review of the document, I believe some modifications might help.
First, I am thinking that the "Options" and "Targets" sections could introduce
the full command syntax and then list the individual items with an explanation.

For example:

make -C $KDIR M=$PWD

     -C
        info on the option
     M        ...

Next, section 3 "Example commands" seems redundant because the commands are
first explained in section 2.1. Also, section 4 seems to not be fully divided
into sections; looking at the table of contents, it does not list
sub-sections. Each example within that section, IMHO, should be an individual
section and not fall under the heading "Shared Makefile for module and kernel."
The new headings could be something like:

4.1 Shared Makefile
4.2 Kbuild file and Makefile

Backwards compatibility can be included in 4.2 if it is still needed? An
explanation that obj-m is the module being built would be nice along with a
sub-section showing how multiple modules can be built at the same time. And,
the intro in section 5 could use some serious rewording,

Besides that, the rest of the document looks good and only needs some minor
corrections, such as EXTRA_CFLAGS -> ccflags-y.

Let me know what you think.

Thanks,
matt