Bell confusion with master vs. slave devices in Xorg 1.7.6

[Alan Coopersmith]

Several users have complained to me about their X server bells on
recent builds of OpenSolaris - definitely with Xorg 1.7.x releases,
I don't know if it affected Xorg 1.6.x before that as well.

The complaints I've gotten are two-fold: the bell volume is
uncontrollable, and when the beep sounds, it plays twice
(stuck at the loud volume).

It looks like we're seeing this because unlike the evdev driver,
the kbd driver has a BellProc set to sound the keyboard device
bell (if present - the Solaris kernel redirects to an audio
device if not).

When the keyboard gets associated with the Virtual Core Keyboard,
the DeepCopyDeviceClasses copies the BellProc pointer to the
Virtual Core Keyboard.   When an XBell request comes in, the
ProcBell function in dix/devices.c loops through all the devices,
and calls the BellProc in the master and any associated slaves,
resulting in the KbdBell() function from kbd_drv getting called
twice, once off the Virtual Core Keyboard and once off the slave
representing the physical keyboard.

If you try to silence this with 'xset b 0' though, it does not
loop through all the devices, and only sets the volume on the
master.   You can see the slave still have the default volume of
50 by running 'xinput get-feedbacks keyboard'.   When ProcBell
loops through the devices though, it uses the volume from each
device.

Fixing this portion seems a simple matter of changing ProcBell to use
the master keyboard volume on all slaves of that device as well:

--- dix/devices.c~	Fri Apr 16 19:29:30 2010
+++ dix/devices.c	Fri Apr 16 20:54:31 2010
@@ -2037,7 +2037,7 @@
 	    if (rc != Success)
 		return rc;
             XkbHandleBell(FALSE, FALSE, dev, newpercent,
-                          &dev->kbdfeed->ctrl, 0, None, NULL, client);
+                          &keybd->kbdfeed->ctrl, 0, None, NULL, client);
         }
     }

This would let you keep per-device bell settings, and have those used when
you call XDeviceBell on the specific device instead of the master.  It could
also allow you to have xset called in your .xinitrc and have its volume setting
stick even if the physical keyboard hasn't been associated with the core device
yet, but that is currently thwarted by the DeepCopy overriding the core devices
keyboard feedback settings when the device is connected to the master.

Alternatively, DoChangeKeyboardControl could set the bell volumes on all the
slaves, but that also doesn't solve the problem of initializing the settings
from .xinitrc.

As for the double bell, the two options there seem to be either removing the
copying of BellProc in the DeepCopy code, or changing ProcBell to call it on
either the master or the slave, but not both.  Since the Solaris implementation
of xf86OSRingBell also makes noise, via /dev/audio, I'm tempted to do both.
By not copying BellProc, the virtual keyboard is left with it's BellProc set to
CoreKeyboardBell, which unwraps down to xf86OSRingBell, so that would be used
when no other bell device is present, but if any slaves have a bell, we could
skip the master.

--- Xi/exevents.c~	Fri Apr 16 21:48:19 2010
+++ Xi/exevents.c	Fri Apr 16 21:48:37 2010
@@ -413,7 +413,6 @@
                     return;
                 }
             }
-            (*k)->BellProc = it->BellProc;
             (*k)->CtrlProc = it->CtrlProc;
             (*k)->ctrl     = it->ctrl;
             if ((*k)->xkb_sli)

--- dix/devices.c~	2010-04-16 21:49:26.132804948 -0700
+++ dix/devices.c	2010-04-16 22:19:16.742406867 -0700
@@ -2015,6 +2015,7 @@ ProcBell(ClientPtr client)
     int base = keybd->kbdfeed->ctrl.bell;
     int newpercent;
     int rc;
+    int rung = 0;
     REQUEST(xBellReq);
     REQUEST_SIZE_MATCH(xBellReq);

@@ -2029,17 +2030,28 @@ ProcBell(ClientPtr client)
     else
 	newpercent = base - newpercent + stuff->percent;

+    /* first try to ring the bells on the slaves */
     for (dev = inputInfo.devices; dev; dev = dev->next) {
-        if ((dev == keybd || (!IsMaster(dev) && dev->u.master == keybd)) &&
+        if ((!IsMaster(dev) && dev->u.master == keybd) &&
             dev->kbdfeed && dev->kbdfeed->BellProc) {

 	    rc = XaceHook(XACE_DEVICE_ACCESS, client, dev, DixBellAccess);
-	    if (rc != Success)
-		return rc;
-            XkbHandleBell(FALSE, FALSE, dev, newpercent,
-                          &keybd->kbdfeed->ctrl, 0, None, NULL, client);
+	    if (rc == Success) {
+		XkbHandleBell(FALSE, FALSE, dev, newpercent,
+			      &keybd->kbdfeed->ctrl, 0, None, NULL, client);
+		rung++;
+	    }
         }
     }
+    /* if no slaves found with bells, ring the master's bell */
+    if (!rung && keybd->kbdfeed && keybd->kbdfeed->BellProc) {
+
+	rc = XaceHook(XACE_DEVICE_ACCESS, client, keybd, DixBellAccess);
+	if (rc != Success)
+	    return rc;
+	XkbHandleBell(FALSE, FALSE, keybd, newpercent,
+		      &keybd->kbdfeed->ctrl, 0, None, NULL, client);
+    }

     return Success;
 }

Are these the right approaches to take?  Is there a better way to solve these
problems before the bells drive the users batty?

-- 
	-Alan Coopersmith-        alan.coopersmith at oracle.com
	 Oracle Solaris Platform Engineering: X Window System