Bug #302

Fix nagging make_dev() warnings

Added by frank over 8 years ago. Updated about 8 years ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:-
Target version:-

Description

Currently I'm receiving the following warning whenever moused is invoked:

Starting moused:
WARNING: driver sc should register devices with make_dev() (dev_t = "#sc/255")

Actually nothing appears to be wrong with the sysmouse, so this is quite
annoying and I've tried to get rid of these messages. Please consider the
two patches below for fixing this issue.

Note that spec_vnops.c.patch just reimplements a feature of the FreeBSD
4.x kernel (which in my opinion doesn't hurt). This prevents repetitions
of the above warning, but it occurs once at the initial start of moused.
The second patch corrects this (though it's not clear to me why it occurs
at all).

Frank Josellis

--- 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;
}
}

--- spec_vnops.c.patch ends here ---

--- syscons.c.patch begins here ---
--- 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,
--- syscons.c.patch ends here ---

History

#1 Updated by corecode over 8 years ago

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

#2 Updated by frank over 8 years ago

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 ---

#3 Updated by frank over 8 years ago

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(si, struct cdev *, sizeof(*si), 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) & D_DISK) == 0) {
cp = devtoname(dev);
if (*cp == '#') {
--- patch_2 ends here ---

#4 Updated by frank about 8 years ago

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

#5 Updated by dillon about 8 years ago

: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
<>

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(si, struct cdev *, sizeof(*si), 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.

#6 Updated by dillon about 8 years ago

: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
<>

#7 Updated by frank about 8 years ago

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

Also available in: Atom PDF