DragonFlyBSD

Frank W. Josellis wrote:

--- spec_vnops.c.patch begins here ---
--- sys/vfs/specfs/spec_vnops.c.orig 2006-08-12 02:26:21.000000000 0200
++ sys/vfs/specfs/spec_vnops.c 2006-08-30 14:02:41.000000000 0200
@ -268,9 +268,10 @
}
if ((dev_dflags(dev) & D_DISK) == 0) {
cp = devtoname(dev);
- if (*cp '#') {
if (*cp '#' && (dev_dflags(dev) & D_NAGGED) == 0) {
printf("WARNING: driver %s should register devices with make_dev() (dev_t = \"%s\")\n",
dev_dname(dev), cp);
+ dev->si_ops->head.flags |= D_NAGGED;
}
}

i'm not sure if dev->si_ops->head.flags is the right place to add D_NAGGED, but something like this seems to be the right thing.

--- sys/dev/misc/syscons/syscons.c.orig 2006-07-28 04:17:36.000000000 0200
++ sys/dev/misc/syscons/syscons.c 2006-08-30 14:02:41.000000000 +0200
@ -197,7 +197,7 @
static d_mmap_t scmmap;

static struct dev_ops sc_ops = {
- { "sc", CDEV_MAJOR, D_TTY | D_KQFILTER },
+ { "sc", CDEV_MAJOR, D_TTY | D_KQFILTER | D_NAGGED },
.d_open = scopen,
.d_close = scclose,
.d_read = scread,

this is the wrong fix, for sure. the nagging has a reason, actually, so the cause should be fixed, not just the nagging.

cheers
simon

On Thu, 7 Sep 2006, Simon 'corecode' Schubert wrote:

[...]

i'm not sure if dev->si_ops->head.flags is the right place to add D_NAGGED,
but something like this seems to be the right thing.

Let's recall that in kernel mode dev_t is a pointer to a struct specinfo
(defined in <sys/conf.h>), where the former struct cdevsw member from
FreeBSD has been replaced by a struct dev_ops for Dragonfly. With that in
mind it's not difficult to see where these flags should go. However, we
still need an explanation why the warning is issued at all, and meanwhile
I think that this becomes clear from inspecting the function spec_open()
in spec_vnops.c:

Here we deal with a dev_t dev obtained via udev2dev(), in which respect
Dragonfly behaves significantly different from FreeBSD. Indeed, from
sys/kern/kern_conf.c we see that udev2dev() calls hashdev(), and that for
the returned dev_t the member dev->si_name is not set as it could be
expected for FreeBSD 4.x. In particular, for the dev in question we expect
devtoname(dev) to return "consolectl" with FreeBSD 4.x, whereas we get
"#sc/255" with Dragonfly. But that's not a bug, that's a feature resulting
from using udev2dev(). You may wish to turn on some debugging output for
hashdev() to see what devtoname() actually returns for the various
devices.

So the problem appears to be solved by just dropping that obsolete
warning, it's completely useless now (patch below).

Regards,
Frank Josellis

--- patch begins here ---
--- sys/vfs/specfs/spec_vnops.c.orig 2006-08-12 02:25:58.000000000 0200
++ sys/vfs/specfs/spec_vnops.c 2006-09-08 08:21:33.000000000 +0200
@ -266,13 +266,6 @
dev->si_bsize_phys = DEV_BSIZE;
vinitvmio(vp, IDX_TO_OFF(INT_MAX));
}
- if ((dev_dflags(dev) & D_DISK) == 0) {
- cp = devtoname(dev);
- if (*cp == '#') {
- printf("WARNING: driver %s should register devices with make_dev() (dev_t = \"%s\")\n",
- dev_dname(dev), cp);
- }
- }

/*
 * If we were handed a file pointer we may be able to install a
--- patch ends here ---

Well, I see there is a hidden bug, that's not just a feature. This is not
a fix, but at least it gives a hint where the problem is located. Please
have a look at the patches below. With patch #1 for hashdev() "consolectl"
is correctly retrieved from the device hash table and, consequently, the
warning doesn't show up. Patch #2 indicates why the original hashdev()
failed. It provides some debugging output for spec_open(), and in case
somebody is interested, I have posted a corresponding dmesg output at

http://www.senax.net/downloads/dmesg.debug

For "consolectl" it shows that dev->si_ops->head.name was changed from
"sc" to "intercept", which should not happen. Apparently something is
messed up in sys/kern/tty_cons.c, but I haven't yet checked the details.

Regards,
Frank Josellis

--- patch_1 begins here ---
--- src/sys/kern/kern_conf.c.orig 2006-09-10 03:26:39.000000000 0200
++ src/sys/kern/kern_conf.c 2006-09-14 21:16:53.000000000 0200
@ -124,8 +124,8 @
udev = makeudev(x, y);
hash = udev % DEVT_HASH;
LIST_FOREACH(si, &dev_hash[hash], si_hash) {
- if (si->si_ops ops && si->si_udev udev)
- return (si);
if (si->si_ops->head.maj ops->head.maj && si->si_udev udev)
+ return (si);
}
if (stashed >= DEVT_STASH) {
MALLOC, M_DEVT,
--- patch_1 ends here ---

--- patch_2 begins here ---
--- src/sys/vfs/specfs/spec_vnops.c.orig 2006-09-10 03:26:41.000000000 0200
++ src/sys/vfs/specfs/spec_vnops.c 2006-09-14 21:44:43.000000000 0200
@ -266,6 +266,10 @
dev->si_bsize_phys = DEV_BSIZE;
vinitvmio(vp, IDX_TO_OFF(INT_MAX));
}
if ( dev->si_ops->head.maj != 2) {
+ printf("spec_open: \"%s\" (s, %p)\n",
+ devtoname(dev), dev_dname(dev), dev);
+ }
if ((dev_dflags(dev) x%x D_DISK) == 0) {
cp = devtoname(dev);
if (*cp == '#') {
--- patch_2 ends here ---

An inspection of sys/kern/tty_cons.c shows that the reported replacement
"sc" -> "intercept" is indeed intentional. It just means that the dev_ops
has been substituted as desired. Then, however, requiring

[*] (si->si_ops ops && si->si_udev udev)

in hashdev() looks inappropriate, as this prevents the intercepted device
from being successfully looked up from the hash table. Strictly speaking,
dev_ops_get(12,255) returns the ops of the original device, and this is
used by udev2dev() for a subsequent hashdev() call. As a consequence one
has si_ops != ops and [*] is not matched. Once the interception is
established, the next attempt to open consolectl thus results in the
creation of a new device whose si_ops is the original one. This effects
a complete bypassing of the intercepted device.

I can't figure out why the condition "si->si_ops == ops" in [*] had been
added. For testing purposes I dropped this to see what goes wrong, but
everything still looks fine. Anyway, in view if the new intercept code it
appears that here is a point which should be reconsidered.

Regards,
Frank Josellis

:An inspection of sys/kern/tty_cons.c shows that the reported replacement
:"sc" > "intercept" is indeed intentional. It just means that the dev_ops
:has been substituted as desired. Then, however, requiring
:
:[*] (si>si_ops ops && si->si_udev udev)
:
:in hashdev() looks inappropriate, as this prevents the intercepted device
:from being successfully looked up from the hash table. Strictly speaking,
:dev_ops_get(12,255) returns the ops of the original device, and this is
:used by udev2dev() for a subsequent hashdev() call. As a consequence one
:has si_ops != ops and [*] is not matched. Once the interception is
:established, the next attempt to open consolectl thus results in the
:creation of a new device whose si_ops is the original one. This effects
:a complete bypassing of the intercepted device.
:
:I can't figure out why the condition "si->si_ops ops" in [*] had been
:added. For testing purposes I dropped this to see what goes wrong, but
:everything still looks fine. Anyway, in view if the new intercept code it
:appears that here is a point which should be reconsidered.
:
:Regards,
:Frank Josellis

Ok, it took a few readings to decipher your posting, but I understand
    what you are getting at.

Basically the interception is only modifying the device and is not
    modifying the link structure, so when udev2dev() calls dev_ops_get(),
    dev_ops_get() returns the original ops structure instead of the
    intercepted ops structure and this results in hashdev() being
    called with the wrong ops.

The second part of this is more uncertain.  Do we even want udev2dev()
    to create new devices with the override ops instead of the original
    ops if one does not already exist?  I don't think we do, because
    the console override code would have no visibility into the newly
    created devices using its override ops vector.   In fact, I don't think
    this case can occur anyway since the console device must already exist
    in order to be overriden.

So this leaves us with just the first bug... the interception is causing
    udev2dev() to call hashdev() with the original ops vector and thus 
    create a new device with the original ops instead of finding the device
    with the overridden ops.

I hate doing hacks, but I think in this case I'd rather solve the
    problem quickly and get it off my radar screen.  Here's a patch, I
    am committing it right now.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

Index: kern_conf.c
=================================================================
RCS file: /cvs/src/sys/kern/kern_conf.c,v
retrieving revision 1.15
diff u -r1.15 kern_conf.c
-- kern_conf.c 10 Sep 2006 01:26:39 -0000 1.15
+++ kern_conf.c 26 Sep 2006 17:57:16 -0000
@ -114,7 +114,7 @
*/
static
cdev_t
-hashdev(struct dev_ops *ops, int x, int y)
+hashdev(struct dev_ops *ops, int x, int y, int allow_override) {
struct cdev *si;
udev_t udev;
@ -124,8 +124,10 @
udev = makeudev(x, y);
hash = udev % DEVT_HASH;
LIST_FOREACH(si, &dev_hash[hash], si_hash) {
- if (si->si_ops ops && si->si_udev udev)
+ if (si->si_udev udev &&
+ (allow_override || si->si_ops ops)) {
return (si);
+ }
}
if (stashed >= DEVT_STASH) {
MALLOC, M_DEVT,
@ -185,7 +187,7 @
ops = dev_ops_get(umajor(x), uminor(x));
if (ops == NULL)
return(NOCDEV);
- dev = hashdev(ops, umajor(x), uminor(x));
+ dev = hashdev(ops, umajor(x), uminor(x), TRUE);
return(dev);
}

@ -245,7 +247,7 @ * compile the cdevsw and install the device
*/
compile_dev_ops(ops);
- dev = hashdev(ops, ops->head.maj, minor);
+ dev = hashdev(ops, ops->head.maj, minor, FALSE);

/*
     * Set additional fields (XXX DEVFS interface goes here)
@ -267,7 +269,7 @
{
     cdev_t dev;

- dev = hashdev(ops, ops->head.maj, minor);
+ dev = hashdev(ops, ops->head.maj, minor, FALSE);
return(dev);
}

@ -280,7 +282,7 @ {
cdev_t dev;

- dev = hashdev(odev->si_ops, umajor(odev->si_udev), minor);
+ dev = hashdev(odev->si_ops, umajor(odev->si_udev), minor, FALSE);

/*
 * Copy cred requirements and name info XXX DEVFS.

:Index: kern_conf.c
:===================================================================
:RCS file: /cvs/src/sys/kern/kern_conf.c,v
:retrieving revision 1.15
:diff u -r1.15 kern_conf.c
:-- kern_conf.c 10 Sep 2006 01:26:39 -0000 1.15
:+++ kern_conf.c 26 Sep 2006 17:57:16 -0000
:@ -114,7 +114,7 @
: */
: static
: cdev_t
:-hashdev(struct dev_ops *ops, int x, int y)
:+hashdev(struct dev_ops *ops, int x, int y, int allow_override)
: {
:...

While explaining some make_dev related stuff to Victor I realized that
    I made a mistake in my commit.... I had forgotten why I had the ops check
    in there in the first place.

The reason is that the kernel can create shadow devices which
    are supposed to be invisible to userland.  This is why the ops pointer
    is compared in hashdev().

So we cannot just allow any ops vector mismatch to be overridden.
    I have made a followup commit which adds a flag that hashdev() can
    check for and I added a comment to the code describing the reason.

-Matt
                    Matthew Dillon 
                    <dillon@backplane.com>

All right, the problem and any related questions are gone. Thanks.

Just let me state two remarks which you may or may not find useful when
cleaning up the code some day:

(1) The comment in src/sys/sys/conf.h, line 87, can make believe that
SI_ADHOC serves to distinguish between make_dev() created devices and
others (in a way complementary to FreeBSD's SI_NAMED). The role of
SI_ADHOC turns factually out to be totally different from that, and one
might think of a better description here.

(2) In src/sys/kern/kern_conf.c, line 49, cdevsw_ALLOCSTART is defined,
but this is nowhere else used. Looks like old stuff that somebody should
have removed from FreeBSD-4 long ago.

Regards,
Frank Josellis