Subject: Re: Call for review: The New Block/Character Device Switch Configuration Framework
To: None <maekawa@math.sci.kobe-u.ac.jp, tech-kern@NetBSD.org>
From: None <eeh@netbsd.org>
List: tech-kern
Date: 06/28/2001 17:45:49
: Here is a proposal for a framework to support the auto-generation of
: bdevsw/cdevsw table by config(8) and the dynamic assignment of major number.



:        The New Block/Character Device Switch Configuration Framework

Looks promising.  

But this is a partial solution at best.  It does nothing to
resolve the issues involved in dynamically generating device
nodes in /dev, and until that issue is resolved, these changes
will not really allow dynamic reconfiguration.

To properly review the future impact of these changes, it would 
be beneficial to know what the ultimate goal is and how you 
propose to reach it.

Also, if you are planning to go with a fully dynamic configuration
system, the device major/minor numbers, and the device switch 
table (list, whatever) should eventually go away.  The device
number's only raison d'etre is to provide a mapping from the
mknod generated device node in the filesystem to the config
generated/autoconfig attached driver instance in the kernel.

: The framework of dynamic assignment of major number is very simple.
: 	- Prepare two ranges (i.e. four variables) in the kernel.
: 	  One is the range from low of block major number to high.
: 	  Another is the range from low of character major number to high.

Can't config determine the active ranges from the device numbers
it reads by parsing the files files?  Or, more specifically, why
do we need to specify the range?

What happens with device drivers in LKMs?  Do the major numbers
need to be specified in one of the files files?  What happens if
the major number for an LKM is not specified in a files file?

It would be helpful if you separated out functionality into a
set of formal specifications for say, initialization and internal
kernel routines, autoconfig and device attachment, device drivers, 
and devsw internals.  This will help abstraction and prevent, e.g.
a device driver from calling routines it shouldn't and cause 
problems when you change things later.

: 	struct cdevsw *devswctty;

: 	void
: 	fdesc_init()
: 	{
: 		...

: 		devswctty = devsw_lookup("ctty", DEVCHR);
: 		if (devswctty == NULL)
: 			panic("not found cdevsw of ctty");
: 		devctty = makedev(devswctty->d_major, 0);

<note about this being evil, operating on device nodes.>

: 3.2.2.	Userland - config(8)

: The 'fd' driver have both bdevsw/cdevsw interfaces.
: If 'fd' is defined in your kernel configuration file,
: config(8) generates below:

If you plan to change the way devsw is handled, you should 
simplify everything by merging the cdevsw and bdevsw 
structures into a single structure, and have pointers to 
the same structure in both bdevsw and cdevsw.  It will 
clean things up significantly.

I would also suggest adding a pointer to the devsw 
structure to struct device.  


: 4.1.2.	New entries of bdevsw/cdevsw

: d_major		- An alias to the major number to avoid linear searching tables
: 		  (should be -1 in the source. In initialization phase or
: 		   dynamically attach phase, filled out by proper value.)

I would merge the bdev/cdev structures, requiring two major numbers.
It would also make sense to pull these out of the devsw structure and
have them separately looked up in a table.

: d_name		- An alias of bdevsw/cdevsw to search by name
: 		  (should be same as a devbase and be null-teminated.)

I would suggest not attaching the name to the devsw.  Rather, attaching
a pointer to the devsw to the cfdriver structure and using using the
name there to look up the devsw structure.  It should simplify things
in future when you really do have dynamic device configuration and
everything tends to be hung off the device structure.

: 4.1.3.	Variables

: bdevsw_dynlow	- Low of block major number
: bdevsw_dynhigh	- High of block major number
: cdevsw_dynlow	- Low of character major number
: cdevsw_dynhigh	- High of character major number

Are these public variables?  Why are they public variables?  If
they are not public variables then they really shouldn't be
in the interface description.

: 4.2.	Userland - config(8)

: 4.2.1.	Grammer

: device-switch <name> chr <num> [blk <num>] [<options>]

`device-switch' is ugly and, really, an implementation detail of the
current device system.  I would suggest changing it to `major' or
`device-major' instead.  (And while I'm at it, I'll recommend expanding
the rest to `char' and `block' for legibility.)

: name		- The prefix of bdevsw/cdevsw entry (required)
: chr		- A character major number (required)
: blk		- A block major number (optional)
: options		- Conditions to determine whether should be attached or not
: 		  (optional)

: 4.2.2.	Structures and Variables

: struct devswi {
: 	struct devswi	*di_next;	/* linked list */
: 	const char	*di_srcfile;	/* the name of the "devsw" file */
: 	u_short		di_srcline;	/* the line number */
: 	const char	*di_name;	/* [bc]devsw name */
: 	int		di_cmajor;	/* character major */
: 	int		di_bmajor;	/* block major */
: 	struct nvlist	*di_opts;	/* options */
: };

: struct devswi *alldevswi; /* list of all device switch */
: struct devswi **nextdevswi; /* to construct a linked list */

: struct hashtab *cdevswitab; /* character devswi lookup */
: struct hashtab *bdevswitab; /* block devswi lookup */

: int maxcdevswi; /* max number of character major */
: int maxbdevswi; /* max number of block major */

Are these variables internal or external?  Are the just used
to implement dynamic devsw tables, or do you expect drivers to
traverse them?  If the former, I don't care about them much.  If
the latter, you need to describe how they should be used by 
device drivers, and what happens when you add dynamic /dev
nodes.

: 4.3.	Functions

: 4.3.1.	Kernel

: 	void devsw_init(void);
: 	void *devsw_get(int maj, enum devswtype type);
: 	void *devsw_lookup(const char *name, enum devswtype type);
: 	dev_t devsw_chr2blk_dev(dev_t chrdev);
: 	dev_t devsw_blk2chr_dev(dev_t blkdev);

Hm.  If you have a single, merged devsw structure you probably don't
need devsw_chr2blk_dev() and devsw_blk2chr_dev().

: 	int devsw_attach(void *devsw, int maj, enum devswtype type);
: 	void devsw_detach(void *devsw, enum devswtype type);

: 4.3.2.	Userland - config(8)

: 	int adddevsw(const char *name, int cmaj, int bmaj, struct nvlist *opts);
: 	int emitdevsw(FILE *fp);
: 	int fixdevsw(void);


: 5.1.2.	Userland - config(8)

: int adddevsw(const char *name, int cmaj, int bmaj, struct nvlist *opts);

: Make a node which is associated with the name 'name' and character major
: number 'cmaj' and block major number 'bmaj' and the options 'opts'.
: The options are used to determine whether this device switch should be
: attached or not in fixdevsw().

This is all just creating some entries in some C file generated by
config(8), right?  It doesn't actually change things in /dev, does it?

Eduardo