Re: [PATCH] struct char_device

Linus Torvalds (torvalds@transmeta.com)
Tue, 22 May 2001 15:37:54 -0700 (PDT)


On Tue, 22 May 2001 Andries.Brouwer@cwi.nl wrote:
>
> The operations are different, but all bdev/cdev code is identical.
>
> So the choice is between two uglies:
> (i) have some not entirely trivial amount of code twice in the kernel
> (ii) have a union at the point where the struct operations
> is assigned.
>
> I preferred the union.

I would much prefer a union of pointers over a pointer to a union.

So I'd much rather have the inode have a

union {
struct block_device *block;
struct char_device *char;
} dev;

and then have people do

cdev = inode->dev.char;

to get the right information, than to have

union block_char_union {
struct block_device block;
struct char_device char;
};

.. with struct inode containing ..
union block_char_union *dev;

Why? Because if you have a "struct inode", you also have enough
information to decide _which_ of the two types of pointers you have, so
you can do the proper dis-ambiguation of the union and properly select
either 'inode->dev.char' or 'inode->dev.block' depending on other
information in the inode.

In contrast, if you have a pointer to a union, you don't have information
of which sub-type it is, and you'd have to carry that along some other way
(for example, by having common fields at the beginning). Which I think is
broken.

So my suggestion for eventual interfaces:

- have functions like

struct block_dev *bdget(struct inode *);
struct char_dev *cdget(struct inode *);

which populate the "inode->dev" union pointer, which in turn is _only_
a cache of the lookup. Right now we do this purely based on "dev_t",
and I think that is bogus. We should never pass a "dev_t" around
without an inode, I think.

And we should not depend on the "inode->dev.xxxx" pointer being valid all
the time, as there is absolutely zero point in initializing the pointer
every time the inode is read just because somebody does a "ls -l /dev".
Thus the "cache" part above.

- NO reason to try to make "struct block_dev" and "struct char_dev" look
similar. They will have some commonality for lookup purposes (that
issue is similar, as Andries points out), and maybe that commonality
can be separated out into a sub-structure or something. But apart from
that, they have absolutely nothing to do with each other, and I'd
rather not have them have even a _superficial_ connection.

Block devices will have the "request queue" pointer, and the size and
partitioning information. Character devices currently would not have
much more than the operations pointer and name, but who knows..

But the most important thing is to be able to do this in steps. One of the
reasons Andries has had patches for a long time is that it was never very
gradual. Al's patch is gradual, and I like that.

Linus

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