Re: [PATCH] Explicitly disable BT radio using rfkill interface on

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 2006 bytes --]

Hi Alan:


Alan Stern wrote:
> On Wed, 20 May 2009, Mario Limonciello wrote:
>
>   
>
> 	echo 0 >/sys/bus/usb/devices/.../power/persist
>
> This will also prevent reset-resume after hibernation.
>
>   
Yes, as it turns out this works properly for a single resume if I apply
that to the bits of all the child devices.  I take it that bit is just
for debugging purposes, and any solution centered around it would not be
a good solution.
>
> You can't.  Instead you have to arrange things so that when the
> intentional change was made, it left behind a timestamp indicator.  If
> that timestamp if present and not more than a few seconds in the past,
> you know the change was intentional.
>
>
>   
So I took up your advice here and tried to assemble a new udev rule and
function that would get called out upon when coming out of S3. It takes
the Vendor and Product IDs of the parent device of the device we just
lost and passes them to the new function.  The new function then tries
to walk that USB bus again to find the child device that it is supposed
to operate upon.

There is "remove" event in the udev log for that BT radio child device. 
I tried to extract as many attributes as I could from that information
and put it in the rule.  The details from this are in attribute_walk.out
(attached).

Unfortunately, this rule isn't getting ran after S3 even though I see
the remove event.  I can run udevadm test PATH (where PATH is the one of
the BT device we lost) and see that it should be spawning the rule.

However, running udevadm test -a remove PATH, I just see that the rule
gets matched, but not executed.  I can't help but think this is all very
close to functional and there is either something silly i'm doing wrong
or a deficiency of udev that's preventing the attributes from being
matched on the removal event.  Would you mind taking a look and/or
commenting?

-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: resuscitate.patch --]
[-- Type: text/x-patch; name="resuscitate.patch", Size: 4327 bytes --]

=== modified file 'extras/hid2hci/70-hid2hci.rules'
--- extras/hid2hci/70-hid2hci.rules	2009-06-26 06:17:23 +0000
+++ extras/hid2hci/70-hid2hci.rules	2009-06-30 21:49:54 +0000
@@ -1,7 +1,7 @@
 # do not edit this file, it will be overwritten on update
 
-ACTION!="add", GOTO="hid2hci_end"
 SUBSYSTEM!="usb", GOTO="hid2hci_end"
+ACTION!="add|change", GOTO="addchange_end"
 
 # Variety of Dell Bluetooth devices - it looks like a bit of an odd rule,
 # because it is matching on a mouse device that is self powered, but that
@@ -22,4 +22,17 @@
 ATTR{idVendor}=="0458", ATTR{idProduct}=="1000", RUN+="hid2hci --method csr -v $attr{idVendor} -p $attr{idProduct} --mode hci"
 ATTR{idVendor}=="05ac", ATTR{idProduct}=="1000", RUN+="hid2hci --method csr -v $attr{idVendor} -p $attr{idProduct} --mode hci"
 
+LABEL="addchange_end"
+
+# When a Dell device recovers from S3, the mouse child needs to be repoked
+# Well, unfortunately the only event we see is the BT device disappearing, so
+# we have to run with that and chase down the mouse device on USB bus as a child.
+ATTR{bDeviceClass}=="e0", \
+ATTR{bDeviceSubClass}=="01", \
+ATTR{bDeviceProtocol}=="01", \
+ATTR{idVendor}=="413c", \
+ATTR{bmAttributes}=="e0", \
+IMPORT{parent}="ID_*", \
+RUN+="hid2hci --method dell -v $env{ID_VENDOR_ID} -p $env{ID_MODEL_ID} --mode hci -s 02"
+
 LABEL="hid2hci_end"

=== modified file 'extras/hid2hci/hid2hci.c'
--- extras/hid2hci/hid2hci.c	2009-06-16 17:30:22 +0000
+++ extras/hid2hci/hid2hci.c	2009-06-30 21:49:27 +0000
@@ -271,6 +271,28 @@
 	return 0;
 }
 
+static struct usb_device* find_resuscitated_device(uint16_t base_vendor, uint16_t base_product, uint8_t bInterfaceProtocol)
+{
+	struct usb_bus *bus;
+	struct usb_device *dev;
+	int i,j,k,l;
+
+	usb_find_busses();
+	usb_find_devices();
+
+	for (bus = usb_get_busses(); bus; bus = bus->next)
+		for (dev = bus->devices; dev; dev = dev->next)
+			if (dev->descriptor.idVendor == base_vendor &&
+			    dev->descriptor.idProduct == base_product)
+				for (i = 0; i < dev->num_children; i++)
+					for (j = 0; j < dev->children[i]->descriptor.bNumConfigurations; j++)
+						for (k = 0; k < dev->children[i]->config[j].bNumInterfaces; k++)
+							for (l = 0; l < dev->children[i]->config[j].interface[k].num_altsetting; l++)
+								if (dev->children[i]->config[j].interface[k].altsetting[l].bInterfaceProtocol == bInterfaceProtocol)
+									return dev->children[i];
+	return NULL;
+}
+
 static void usage(char* error)
 {
 	if (error)
@@ -289,6 +311,7 @@
 		"\t-v, --vendor=        Vendor ID to act upon\n"
 		"\t-p, --product=       Product ID to act upon\n"
 		"\t-m, --method=        Method to use to switch [csr, logitech, dell]\n"
+		"\t-s, --resuscitate=   Find the child device with this bInterfaceProtocol to run on \n"
 		"\n");
 	if (error)
 		exit(1);
@@ -301,6 +324,7 @@
 	{ "vendor",	required_argument, 0, 'v' },
 	{ "product",	required_argument, 0, 'p' },
 	{ "method",	required_argument, 0, 'm' },
+	{ "resuscitate",required_argument, 0, 's' },
 	{ 0, 0, 0, 0 }
 };
 
@@ -309,8 +333,9 @@
 	struct device_info dev = { NULL, HCI, 0, 0 };
 	int opt, quiet = 0;
 	int (*method)(struct device_info *dev) = NULL;
+	uint8_t resuscitate = 0;
 
-	while ((opt = getopt_long(argc, argv, "+r:v:p:m:qh", main_options, NULL)) != -1) {
+	while ((opt = getopt_long(argc, argv, "+s:r:v:p:m:qh", main_options, NULL)) != -1) {
 		switch (opt) {
 		case 'r':
 			if (optarg && !strcmp(optarg, "hid"))
@@ -341,6 +366,9 @@
 			break;
 		case 'h':
 			usage(NULL);
+        case 's':
+            sscanf(optarg, "%2hx", (short unsigned int*) &resuscitate);
+            break;
 		default:
 			exit(0);
 		}
@@ -355,7 +383,16 @@
 
 	usb_init();
 
-	if (!find_device(&dev)) {
+	if (resuscitate) {
+		dev.dev = find_resuscitated_device(dev.vendor, dev.product, resuscitate);
+		if (!quiet && !dev.dev) {
+			fprintf(stderr, "Device %04x:%04x was unable to resucitate any child devices.\n",dev.vendor,dev.product);
+			exit(1);
+		}
+		dev.vendor = dev.dev->descriptor.idVendor;
+		dev.product = dev.dev->descriptor.idProduct;
+	}
+	else if (!find_device(&dev)) {
 		if (!quiet)
 			fprintf(stderr, "Device %04x:%04x not found on USB bus.\n",
 				dev.vendor, dev.product);


[-- Attachment #1.3: attribute_walk.out --]
[-- Type: text/plain, Size: 3589 bytes --]


Udevadm info starts with the device specified by the devpath and then
walks up the chain of parent devices. It prints for every device
found, all possible attributes in the udev rules key format.
A rule to match, can be composed by the attributes of the device
and the attributes from one single parent device.

  looking at device '/devices/pci0000:00/0000:00:04.0/usb3/3-4/3-4.3':
    KERNEL=="3-4.3"
    SUBSYSTEM=="usb"
    DRIVER=="usb"
    ATTR{configuration}==""
    ATTR{bNumInterfaces}==" 4"
    ATTR{bConfigurationValue}=="1"
    ATTR{bmAttributes}=="e0"
    ATTR{bMaxPower}=="100mA"
    ATTR{urbnum}=="186"
    ATTR{idVendor}=="413c"
    ATTR{idProduct}=="8156"
    ATTR{bcdDevice}=="0172"
    ATTR{bDeviceClass}=="e0"
    ATTR{bDeviceSubClass}=="01"
    ATTR{bDeviceProtocol}=="01"
    ATTR{bNumConfigurations}=="1"
    ATTR{bMaxPacketSize0}=="64"
    ATTR{speed}=="12"
    ATTR{busnum}=="3"
    ATTR{devnum}=="7"
    ATTR{version}==" 2.00"
    ATTR{maxchild}=="0"
    ATTR{quirks}=="0x0"
    ATTR{authorized}=="1"
    ATTR{manufacturer}=="Dell Computer Corp"
    ATTR{product}=="Dell Wireless 370 Bluetooth Mini-card"

  looking at parent device '/devices/pci0000:00/0000:00:04.0/usb3/3-4':
    KERNELS=="3-4"
    SUBSYSTEMS=="usb"
    DRIVERS=="usb"
    ATTRS{configuration}==""
    ATTRS{bNumInterfaces}==" 1"
    ATTRS{bConfigurationValue}=="1"
    ATTRS{bmAttributes}=="e0"
    ATTRS{bMaxPower}==" 94mA"
    ATTRS{urbnum}=="195"
    ATTRS{idVendor}=="0a5c"
    ATTRS{idProduct}=="4500"
    ATTRS{bcdDevice}=="0100"
    ATTRS{bDeviceClass}=="09"
    ATTRS{bDeviceSubClass}=="00"
    ATTRS{bDeviceProtocol}=="00"
    ATTRS{bNumConfigurations}=="1"
    ATTRS{bMaxPacketSize0}=="8"
    ATTRS{speed}=="12"
    ATTRS{busnum}=="3"
    ATTRS{devnum}=="2"
    ATTRS{version}==" 2.00"
    ATTRS{maxchild}=="3"
    ATTRS{quirks}=="0x0"
    ATTRS{authorized}=="1"
    ATTRS{manufacturer}=="Broadcom"
    ATTRS{product}=="BCM2046B1"

  looking at parent device '/devices/pci0000:00/0000:00:04.0/usb3':
    KERNELS=="usb3"
    SUBSYSTEMS=="usb"
    DRIVERS=="usb"
    ATTRS{configuration}==""
    ATTRS{bNumInterfaces}==" 1"
    ATTRS{bConfigurationValue}=="1"
    ATTRS{bmAttributes}=="e0"
    ATTRS{bMaxPower}=="  0mA"
    ATTRS{urbnum}=="75"
    ATTRS{idVendor}=="1d6b"
    ATTRS{idProduct}=="0001"
    ATTRS{bcdDevice}=="0206"
    ATTRS{bDeviceClass}=="09"
    ATTRS{bDeviceSubClass}=="00"
    ATTRS{bDeviceProtocol}=="00"
    ATTRS{bNumConfigurations}=="1"
    ATTRS{bMaxPacketSize0}=="64"
    ATTRS{speed}=="12"
    ATTRS{busnum}=="3"
    ATTRS{devnum}=="1"
    ATTRS{version}==" 1.10"
    ATTRS{maxchild}=="4"
    ATTRS{quirks}=="0x0"
    ATTRS{authorized}=="1"
    ATTRS{manufacturer}=="Linux 2.6.30-10-generic ohci_hcd"
    ATTRS{product}=="OHCI Host Controller"
    ATTRS{serial}=="0000:00:04.0"
    ATTRS{authorized_default}=="1"

  looking at parent device '/devices/pci0000:00/0000:00:04.0':
    KERNELS=="0000:00:04.0"
    SUBSYSTEMS=="pci"
    DRIVERS=="ohci_hcd"
    ATTRS{vendor}=="0x10de"
    ATTRS{device}=="0x0aa5"
    ATTRS{subsystem_vendor}=="0x1028"
    ATTRS{subsystem_device}=="0x0271"
    ATTRS{class}=="0x0c0310"
    ATTRS{irq}=="21"
    ATTRS{local_cpus}=="ff"
    ATTRS{local_cpulist}=="0-7"
    ATTRS{modalias}=="pci:v000010DEd00000AA5sv00001028sd00000271bc0Csc03i10"
    ATTRS{broken_parity_status}=="0"
    ATTRS{msi_bus}==""

  looking at parent device '/devices/pci0000:00':
    KERNELS=="pci0000:00"
    SUBSYSTEMS==""
    DRIVERS==""


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Wed, Jul 1, 2009 at 00:02, Mario
Limonciello<mario_limonciello@dell.com> wrote:

> However, running udevadm test -a remove PATH, I just see that the rule
> gets matched, but not executed.  I can't help but think this is all very
> close to functional and there is either something silly i'm doing wrong
> or a deficiency of udev that's preventing the attributes from being
> matched on the removal event.

It's obviously impossible to read any attributes from a device which
is removed. :)

Kay

RE: [PATCH] Explicitly disable BT radio using rfkill interface on suspend

[Mario_Limonciello]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="windows-1254", Size: 411 bytes --]

Hi Kay:

> It's obviously impossible to read any attributes from a device which
> is removed. :)

I was hoping UDEV would be caching this information in a database.  Is there any plan for that eventually?

Would you have any other recommendations on a better way to go about this?
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þ\x1a-¦[ þ)í…æèw*\x1fjg¬±¨\x1e¶‰šŽŠÝ¢jÿ¾\a«þG«éÿ¢¸\f¢·¦j:+v‰¨ŠwèjØm¶Ÿÿþø\x1e¯ù\x1e®w¥þŠàþf£¢·hšâúÿ†Ù¥

RE: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Mario,

> > It's obviously impossible to read any attributes from a device which
> > is removed. :)
> 
> I was hoping UDEV would be caching this information in a database.  Is there any plan for that eventually?
> 
> Would you have any other recommendations on a better way to go about this?

what are you trying to solve here. And why do you need to disable a
Bluetooth radio on suspend? Are you trying to hack around a bug?

Regards

Marcel

RE: [PATCH] Explicitly disable BT radio using rfkill interface on suspend

[Mario_Limonciello]

Hi Marcel:

> what are you trying to solve here. And why do you need to disable a
> Bluetooth radio on suspend? Are you trying to hack around a bug?

The problem is that a reset-resume is being ran on the BT radio device
as well as the two virtual HID devices.  So the OS thinks that the BT
radio device is still there after resuming from S3, but it isn't as the
hardware is then in a different state after S3.

The initial solution I proposed was in the dell_laptop kernel module to
run turn off/on explicitly using the rfkill interface that dell_laptop
creates, but that not a good solution so Alan proposed adding something
to userspace to run when the BT radio disappears but that the rest of
the virtual HID devices are still there.

RE: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Mario,

> > what are you trying to solve here. And why do you need to disable a
> > Bluetooth radio on suspend? Are you trying to hack around a bug?
> 
> The problem is that a reset-resume is being ran on the BT radio device
> as well as the two virtual HID devices.  So the OS thinks that the BT
> radio device is still there after resuming from S3, but it isn't as the
> hardware is then in a different state after S3.
> 
> The initial solution I proposed was in the dell_laptop kernel module to
> run turn off/on explicitly using the rfkill interface that dell_laptop
> creates, but that not a good solution so Alan proposed adding something
> to userspace to run when the BT radio disappears but that the rest of
> the virtual HID devices are still there.

you know that we re-wrote RFKILL completely. You can kill a Bluetooth
device from within the kernel. And this looks like a hardkill of the
Bluetooth interface.

The only think that you can't do is send input events around to trigger
RFKILL. That is broken and will be removed.

Regards

Marcel

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 492 bytes --]

Hi Marcel:

Marcel Holtmann wrote:
>
> you know that we re-wrote RFKILL completely. You can kill a Bluetooth
> device from within the kernel. And this looks like a hardkill of the
> Bluetooth interface.
>
>   
That's pretty similar to what I was trying to accomplish with the
original patch that started all of this discussion.  I'll attach that
original patch so you can see what I'm talking about.
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Explicitly-disable-BT-radio-using-rfkill-interface-o.patch --]
[-- Type: text/x-patch; name="0001-Explicitly-disable-BT-radio-using-rfkill-interface-o.patch", Size: 1842 bytes --]

From ab8b8ad20dede82f9d13293e94e76b4dd360fcf4 Mon Sep 17 00:00:00 2001
From: Mario Limonciello <Mario_Limonciello@Dell.com>
Date: Mon, 18 May 2009 21:07:59 +0100
Subject: [PATCH] Explicitly disable BT radio using rfkill interface on suspend.
 Dell BT devices need to be removed from the bus and reinserted
 so that userspace udev rules can transition the devices into the
 proper mode after S3 or S4.

---
 drivers/platform/x86/dell-laptop.c |   19 +++++++++++++++++++
 1 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/drivers/platform/x86/dell-laptop.c b/drivers/platform/x86/dell-laptop.c
index af9f430..90a3d7c 100644
--- a/drivers/platform/x86/dell-laptop.c
+++ b/drivers/platform/x86/dell-laptop.c
@@ -186,6 +186,24 @@ static int dell_rfkill_set(int radio, enum rfkill_state state)
 	return 0;
 }
 
+static int dell_rfkill_suspend(struct device *dev, pm_message_t state)
+{
+	struct rfkill *rfkill = to_rfkill(dev);
+
+	/* store state for the resume handler */
+	rfkill->state_for_resume = rfkill->state;
+
+	/* kill radio explicitly if it's on. it needs proper
+	 * reinitialization post suspend */
+	rfkill->toggle_radio(NULL,RFKILL_STATE_SOFT_BLOCKED);
+
+	/* mark class device as suspended */
+	if (dev->power.power_state.event != state.event)
+		dev->power.power_state = state;
+
+	return 0;
+}
+
 static int dell_wifi_set(void *data, enum rfkill_state state)
 {
 	return dell_rfkill_set(1, state);
@@ -266,6 +284,7 @@ static int dell_setup_rfkill(void)
 		bluetooth_rfkill->name = "dell-bluetooth";
 		bluetooth_rfkill->toggle_radio = dell_bluetooth_set;
 		bluetooth_rfkill->get_state = dell_bluetooth_get;
+		bluetooth_rfkill->dev.class->suspend = dell_rfkill_suspend;
 		ret = rfkill_register(bluetooth_rfkill);
 		if (ret)
 			goto err_bluetooth;
-- 
1.5.4.3


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Matthew Garrett]

On Wed, Jul 01, 2009 at 12:12:55PM -0500, Mario Limonciello wrote:
> Hi Marcel:
> 
> Marcel Holtmann wrote:
> >
> > you know that we re-wrote RFKILL completely. You can kill a Bluetooth
> > device from within the kernel. And this looks like a hardkill of the
> > Bluetooth interface.
> >
> >   
> That's pretty similar to what I was trying to accomplish with the
> original patch that started all of this discussion.  I'll attach that
> original patch so you can see what I'm talking about.

I disliked this for a couple of reasons. The first is that it forces an 
rfkill even on hardware that doens't have this behaviour. The second is 
that it's working around a quirk on hardware that this driver really 
isn't reponsible for.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Matthew,

> > > you know that we re-wrote RFKILL completely. You can kill a Bluetooth
> > > device from within the kernel. And this looks like a hardkill of the
> > > Bluetooth interface.
> > >
> > >   
> > That's pretty similar to what I was trying to accomplish with the
> > original patch that started all of this discussion.  I'll attach that
> > original patch so you can see what I'm talking about.
> 
> I disliked this for a couple of reasons. The first is that it forces an 
> rfkill even on hardware that doens't have this behaviour. The second is 
> that it's working around a quirk on hardware that this driver really 
> isn't reponsible for.

I don't agree with how the patch is done, but in theory it is what
RFKILL is all about (after the re-write). However it has to be hardkill
and not a softkill. The softkill is a userspace decision while the
hardkill comes from actually hardware or firmware in this case.

Seems the Dell hardware/firmware is kinda stupid and inconsistent here
and I have no problem solution. However adding a special HID driver with
this quirk might be better anyway.

Regards

Marcel

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Matthew Garrett]

On Wed, Jul 01, 2009 at 03:13:33PM -0700, Marcel Holtmann wrote:
> Hi Matthew,
> > I disliked this for a couple of reasons. The first is that it forces an 
> > rfkill even on hardware that doens't have this behaviour. The second is 
> > that it's working around a quirk on hardware that this driver really 
> > isn't reponsible for.
> 
> I don't agree with how the patch is done, but in theory it is what
> RFKILL is all about (after the re-write). However it has to be hardkill
> and not a softkill. The softkill is a userspace decision while the
> hardkill comes from actually hardware or firmware in this case.
> 
> Seems the Dell hardware/firmware is kinda stupid and inconsistent here
> and I have no problem solution. However adding a special HID driver with
> this quirk might be better anyway.

I'm really still not quite clear on what the issue here is. At boot, 
there are hid devices that need to be quirked into hci mode. Over 
suspend these devices return to their original state. So something needs 
to be done to quirk them back on resume. Why is this a kernel issue at 
all?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 906 bytes --]

Hi Matthew:

Matthew Garrett wrote:
> On Wed, Jul 01, 2009 at 03:13:33PM -0700, Marcel Holtmann wrote:
>   
>
> I'm really still not quite clear on what the issue here is. At boot, 
> there are hid devices that need to be quirked into hci mode. Over 
> suspend these devices return to their original state. So something needs 
> to be done to quirk them back on resume. Why is this a kernel issue at 
> all?
>
>   
At boot, the top level device exposes two virtual HID devices.  A
userspace utility prods one of the HID devices and then the top level
device exposes an HCI device.  Upon S3/resume, a reset-resume happens,
and the HCI device is gone.  The system thinks that they are in the same
state after S3 (with the exception of the missing HCI device) so a UDEV
ADD or CHANGE event doesn't happen.

-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Mario,

> > I'm really still not quite clear on what the issue here is. At boot, 
> > there are hid devices that need to be quirked into hci mode. Over 
> > suspend these devices return to their original state. So something needs 
> > to be done to quirk them back on resume. Why is this a kernel issue at 
> > all?
> >
> >   
> At boot, the top level device exposes two virtual HID devices.  A
> userspace utility prods one of the HID devices and then the top level
> device exposes an HCI device.  Upon S3/resume, a reset-resume happens,
> and the HCI device is gone.  The system thinks that they are in the same
> state after S3 (with the exception of the missing HCI device) so a UDEV
> ADD or CHANGE event doesn't happen.

if this is about the uevent issue again, then RFKILL is not the solution
for it. This should be fixed completely in userspace.

Regards

Marcel

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Matthew Garrett]

On Wed, Jul 01, 2009 at 05:23:44PM -0500, Mario Limonciello wrote:

> At boot, the top level device exposes two virtual HID devices.  A
> userspace utility prods one of the HID devices and then the top level
> device exposes an HCI device.  Upon S3/resume, a reset-resume happens,
> and the HCI device is gone.  The system thinks that they are in the same
> state after S3 (with the exception of the missing HCI device) so a UDEV
> ADD or CHANGE event doesn't happen.

Well, since the state has changed, that sounds like a shortcoming in the 
USB stack.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Alan Stern]

On Wed, 1 Jul 2009, Matthew Garrett wrote:

> On Wed, Jul 01, 2009 at 03:13:33PM -0700, Marcel Holtmann wrote:
> > Hi Matthew,
> > > I disliked this for a couple of reasons. The first is that it forces an 
> > > rfkill even on hardware that doens't have this behaviour. The second is 
> > > that it's working around a quirk on hardware that this driver really 
> > > isn't reponsible for.
> > 
> > I don't agree with how the patch is done, but in theory it is what
> > RFKILL is all about (after the re-write). However it has to be hardkill
> > and not a softkill. The softkill is a userspace decision while the
> > hardkill comes from actually hardware or firmware in this case.
> > 
> > Seems the Dell hardware/firmware is kinda stupid and inconsistent here
> > and I have no problem solution. However adding a special HID driver with
> > this quirk might be better anyway.
> 
> I'm really still not quite clear on what the issue here is. At boot, 
> there are hid devices that need to be quirked into hci mode. Over 
> suspend these devices return to their original state. So something needs 
> to be done to quirk them back on resume. Why is this a kernel issue at 
> all?

I believe the original problem was this: The HCI activation is done in
userspace, triggered by HID device detection.  After suspend the HCI
device is gone but the HID devices remain, so there's nothing to tell
userspace to reactivate the HCI.

> > At boot, the top level device exposes two virtual HID devices.  A
> > userspace utility prods one of the HID devices and then the top level
> > device exposes an HCI device.  Upon S3/resume, a reset-resume happens,
> > and the HCI device is gone.  The system thinks that they are in the same
> > state after S3 (with the exception of the missing HCI device) so a UDEV
> > ADD or CHANGE event doesn't happen.
>
> Well, since the state has changed, that sounds like a shortcoming in the
> USB stack.

It's hard to tell exactly what's going on from Mario's description.  
IIUC, after resume the system knows that the HCI device is gone.  It's
the HID devices that are in the same state as before.

A dmesg log including boot and a suspend-resume cycle, from a kernel
with CONFIG_USB_DEBUG enabled, would help clarify the situation.

Alan Stern

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Alan Stern]

On Thu, 9 Jul 2009, Mario Limonciello wrote:

> Hi Alan:
> 
> Alan Stern wrote:
> > It's hard to tell exactly what's going on from Mario's description.  
> > IIUC, after resume the system knows that the HCI device is gone.  It's
> > the HID devices that are in the same state as before.
> >
> > A dmesg log including boot and a suspend-resume cycle, from a kernel
> > with CONFIG_USB_DEBUG enabled, would help clarify the situation.
> >
> > Alan Stern
> >
> >   
> Sorry for the delay, had some hardware failures.  Here's a dmesg log
> with CONFIG_USB_DEBUG turned on and a suspend-resume cycle.

Right.  The log clearly shows that HID devices 4-1.1 and 4-1.2 survive 
the suspend-resume cycle intact, whereas the BT device 4-1.3 is gone.

Since the BT device was created by means of a userspace script in the 
first place, it seems logical that a userspace script should respond to 
the uevent announcing its disappearance and try to bring it back to 
life.

Last I heard, you had created a udev rule which was supposed to match
the removal event for the BT device, but which udev wasn't executing.  
Is that still the case?  If it is, you should post the rule together
with the udev log for a resume.  Maybe somebody will be able to figure 
out why the rule isn't being executed.

Also, I noticed that your C program for reviving the BT device seemed
more complex than necessary.  You should know beforehand that if the BT
device's path is 4-1.3 then the corresponding mouse device's path will
be 4-1.2 (just decrement the last byte of the pathname).  You can then
match this device up with libusb by reading the busnum and devnum
attributes from the sysfs directory.

Alan Stern

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 1744 bytes --]

Hi Alan:

Alan Stern wrote:
> Right.  The log clearly shows that HID devices 4-1.1 and 4-1.2 survive 
> the suspend-resume cycle intact, whereas the BT device 4-1.3 is gone.
>
> Since the BT device was created by means of a userspace script in the 
> first place, it seems logical that a userspace script should respond to 
> the uevent announcing its disappearance and try to bring it back to 
> life.
>
> Last I heard, you had created a udev rule which was supposed to match
> the removal event for the BT device, but which udev wasn't executing.  
> Is that still the case?  If it is, you should post the rule together
> with the udev log for a resume.  Maybe somebody will be able to figure 
> out why the rule isn't being executed.
>   
Yeah, Kay already identified why the rule wasn't being executed.  It was
reliant on properties of the device that aren't cached in the udev database.
> Also, I noticed that your C program for reviving the BT device seemed
> more complex than necessary.  You should know beforehand that if the BT
> device's path is 4-1.3 then the corresponding mouse device's path will
> be 4-1.2 (just decrement the last byte of the pathname).  You can then
> match this device up with libusb by reading the busnum and devnum
> attributes from the sysfs directory.
>   
I wasn't sure I wanted to jump to this conclusion as I can't predict if
this will change for future hardware that supports these features.
> Alan Stern
>
>   
I'm at a loss at what else can really be done from userspace.  If I
can't match the device on removal and send it to the userspace app, not
sure how else it can be revived.
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Alan Stern]

On Tue, 14 Jul 2009, Mario Limonciello wrote:

> Yeah, Kay already identified why the rule wasn't being executed.  It was
> reliant on properties of the device that aren't cached in the udev database.
> > Also, I noticed that your C program for reviving the BT device seemed
> > more complex than necessary.  You should know beforehand that if the BT
> > device's path is 4-1.3 then the corresponding mouse device's path will
> > be 4-1.2 (just decrement the last byte of the pathname).  You can then
> > match this device up with libusb by reading the busnum and devnum
> > attributes from the sysfs directory.
> >   
> I wasn't sure I wanted to jump to this conclusion as I can't predict if
> this will change for future hardware that supports these features.

For now you should worry more about getting it to work.  Later on you 
can worry about future hardware changes.

> > Alan Stern
> >
> >   
> I'm at a loss at what else can really be done from userspace.  If I
> can't match the device on removal and send it to the userspace app, not
> sure how else it can be revived.

You can relax the matching criterion for the removal rule.  At the time 
the device was created, you can store all the information you will need 
to create it again -- including any matching info which isn't present 
at removal time.

Can you post examples of the creation and removal udev events?  Then 
I'll be able to see exactly what information is present.

Alan Stern

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Tue, Jul 14, 2009 at 18:40, Mario
Limonciello<mario_limonciello@dell.com> wrote:

>> Last I heard, you had created a udev rule which was supposed to match
>> the removal event for the BT device, but which udev wasn't executing.
>> Is that still the case?  If it is, you should post the rule together
>> with the udev log for a resume.  Maybe somebody will be able to figure
>> out why the rule isn't being executed.
>>
> Yeah, Kay already identified why the rule wasn't being executed.  It was
> reliant on properties of the device that aren't cached in the udev database.

Right, sysfs is gone when the remove event is handled. And stuffing
all sysfs properties in the udev database would just be totally
insane. :)

>> Also, I noticed that your C program for reviving the BT device seemed
>> more complex than necessary.  You should know beforehand that if the BT
>> device's path is 4-1.3 then the corresponding mouse device's path will
>> be 4-1.2 (just decrement the last byte of the pathname).  You can then
>> match this device up with libusb by reading the busnum and devnum
>> attributes from the sysfs directory.
>>
> I wasn't sure I wanted to jump to this conclusion as I can't predict if
> this will change for future hardware that supports these features.

What the action you want to take at removal? You can set
ENV{REMOVE_CMD}= and it will be passed to RUN+= for that exact device
when it goes away.

You can also just store any other custom property in the udev
database, and it will be there on remove.

Kay

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 809 bytes --]

Hi Kay:

Kay Sievers wrote:
>
> What the action you want to take at removal? You can set
> ENV{REMOVE_CMD}= and it will be passed to RUN+= for that exact device
> when it goes away.
>
> You can also just store any other custom property in the udev
> database, and it will be there on remove.
>   
Thanks for the recommendation on ENV{REMOVE_CMD}.  With some minor
modifications to my old patch, I've got it working using ENV{REMOVE_CMD}
now reliably post suspend.

Although rewalking the child devices to find the right one to prod might
seem tedious, it should be more future-proof in case later hardware ends
up changing.

Would you mind reviewing the attached patch for submission to udev then?

Thanks,
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: resuscitate.patch --]
[-- Type: text/x-patch; name="resuscitate.patch", Size: 3996 bytes --]

=== modified file 'extras/hid2hci/70-hid2hci.rules'
--- extras/hid2hci/70-hid2hci.rules	2009-06-26 06:17:23 +0000
+++ extras/hid2hci/70-hid2hci.rules	2009-07-14 17:25:17 +0000
@@ -11,6 +11,17 @@
 ATTR{bInterfaceClass}=="03", ATTR{bInterfaceSubClass}=="01", ATTR{bInterfaceProtocol}=="02", ATTRS{bDeviceClass}=="00", ATTRS{idVendor}=="413c", ATTRS{bmAttributes}=="e0", \
     RUN+="hid2hci --method dell -v $attr{idVendor} -p $attr{idProduct} --mode hci"
 
+# When a Dell device recovers from S3, the mouse child needs to be repoked
+# Well, unfortunately the only event we see is the BT device disappearing, so
+# we have to run with that and chase down the mouse device on USB bus as a child.
+ATTR{bDeviceClass}=="e0", \
+ATTR{bDeviceSubClass}=="01", \
+ATTR{bDeviceProtocol}=="01", \
+ATTR{idVendor}=="413c", \
+ATTR{bmAttributes}=="e0", \
+IMPORT{parent}="ID_*", \
+ENV{REMOVE_CMD}="hid2hci --method dell -v $env{ID_VENDOR_ID} -p $env{ID_MODEL_ID} --mode hci -s 02"
+
 ENV{DEVTYPE}!="usb_device", GOTO="hid2hci_end"
 
 # Logitech devices

=== modified file 'extras/hid2hci/hid2hci.c'
--- extras/hid2hci/hid2hci.c	2009-06-16 17:30:22 +0000
+++ extras/hid2hci/hid2hci.c	2009-07-14 17:20:01 +0000
@@ -271,6 +271,28 @@
 	return 0;
 }
 
+static struct usb_device* find_resuscitated_device(uint16_t base_vendor, uint16_t base_product, uint8_t bInterfaceProtocol)
+{
+	struct usb_bus *bus;
+	struct usb_device *dev;
+	int i,j,k,l;
+
+	usb_find_busses();
+	usb_find_devices();
+
+	for (bus = usb_get_busses(); bus; bus = bus->next)
+		for (dev = bus->devices; dev; dev = dev->next)
+			if (dev->descriptor.idVendor == base_vendor &&
+			    dev->descriptor.idProduct == base_product)
+				for (i = 0; i < dev->num_children; i++)
+					for (j = 0; j < dev->children[i]->descriptor.bNumConfigurations; j++)
+						for (k = 0; k < dev->children[i]->config[j].bNumInterfaces; k++)
+							for (l = 0; l < dev->children[i]->config[j].interface[k].num_altsetting; l++)
+								if (dev->children[i]->config[j].interface[k].altsetting[l].bInterfaceProtocol == bInterfaceProtocol)
+									return dev->children[i];
+	return NULL;
+}
+
 static void usage(char* error)
 {
 	if (error)
@@ -289,6 +311,7 @@
 		"\t-v, --vendor=        Vendor ID to act upon\n"
 		"\t-p, --product=       Product ID to act upon\n"
 		"\t-m, --method=        Method to use to switch [csr, logitech, dell]\n"
+		"\t-s, --resuscitate=   Find the child device with this bInterfaceProtocol to run on \n"
 		"\n");
 	if (error)
 		exit(1);
@@ -301,6 +324,7 @@
 	{ "vendor",	required_argument, 0, 'v' },
 	{ "product",	required_argument, 0, 'p' },
 	{ "method",	required_argument, 0, 'm' },
+	{ "resuscitate",required_argument, 0, 's' },
 	{ 0, 0, 0, 0 }
 };
 
@@ -309,8 +333,9 @@
 	struct device_info dev = { NULL, HCI, 0, 0 };
 	int opt, quiet = 0;
 	int (*method)(struct device_info *dev) = NULL;
+	uint8_t resuscitate = 0;
 
-	while ((opt = getopt_long(argc, argv, "+r:v:p:m:qh", main_options, NULL)) != -1) {
+	while ((opt = getopt_long(argc, argv, "+s:r:v:p:m:qh", main_options, NULL)) != -1) {
 		switch (opt) {
 		case 'r':
 			if (optarg && !strcmp(optarg, "hid"))
@@ -339,6 +364,9 @@
 		case 'q':
 			quiet = 1;
 			break;
+		case 's':
+			sscanf(optarg, "%2hx", (short unsigned int*) &resuscitate);
+			break;
 		case 'h':
 			usage(NULL);
 		default:
@@ -355,7 +383,16 @@
 
 	usb_init();
 
-	if (!find_device(&dev)) {
+	if (resuscitate) {
+		dev.dev = find_resuscitated_device(dev.vendor, dev.product, resuscitate);
+		if (!quiet && !dev.dev) {
+			fprintf(stderr, "Device %04x:%04x was unable to resucitate any child devices.\n",dev.vendor,dev.product);
+			exit(1);
+		}
+		dev.vendor = dev.dev->descriptor.idVendor;
+		dev.product = dev.dev->descriptor.idProduct;
+	}
+	else if (!find_device(&dev)) {
 		if (!quiet)
 			fprintf(stderr, "Device %04x:%04x not found on USB bus.\n",
 				dev.vendor, dev.product);


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Mario,

> > What the action you want to take at removal? You can set
> > ENV{REMOVE_CMD}= and it will be passed to RUN+= for that exact device
> > when it goes away.
> >
> > You can also just store any other custom property in the udev
> > database, and it will be there on remove.
> >   
> Thanks for the recommendation on ENV{REMOVE_CMD}.  With some minor
> modifications to my old patch, I've got it working using ENV{REMOVE_CMD}
> now reliably post suspend.
> 
> Although rewalking the child devices to find the right one to prod might
> seem tedious, it should be more future-proof in case later hardware ends
> up changing.
> 
> Would you mind reviewing the attached patch for submission to udev then?

+       for (bus = usb_get_busses(); bus; bus = bus->next)
+               for (dev = bus->devices; dev; dev = dev->next)
+                       if (dev->descriptor.idVendor = base_vendor &&
+                           dev->descriptor.idProduct = base_product)
+                               for (i = 0; i < dev->num_children; i++)
+                                       for (j = 0; j < dev->children[i]->descriptor.bNumConfigurations; j++)
+                                               for (k = 0; k < dev->children[i]->config[j].bNumInterfaces; k++)
+                                                       for (l = 0; l < dev->children[i]->config[j].interface[k].num_altsetting; l++)
+                                                               if (dev->children[i]->config[j].interface[k].altsetting[l].bInterfaceProtocol = bInterfaceProtocol)
+                                                                       return dev->children[i];
+       return NULL;
+}

welcome to the land of fully unreadable code. You need to fix this and
split it up if needed. Nobody has ever a chance to understand what this
code does if it goes wrong.

Regards

Marcel

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Tue, Jul 14, 2009 at 20:46, Marcel Holtmann<marcel@holtmann.org> wrote:
>> > What the action you want to take at removal? You can set
>> > ENV{REMOVE_CMD}= and it will be passed to RUN+= for that exact device
>> > when it goes away.
>> >
>> > You can also just store any other custom property in the udev
>> > database, and it will be there on remove.
>> >
>> Thanks for the recommendation on ENV{REMOVE_CMD}.  With some minor
>> modifications to my old patch, I've got it working using ENV{REMOVE_CMD}
>> now reliably post suspend.
>>
>> Although rewalking the child devices to find the right one to prod might
>> seem tedious, it should be more future-proof in case later hardware ends
>> up changing.
>>
>> Would you mind reviewing the attached patch for submission to udev then?
>
> +       for (bus = usb_get_busses(); bus; bus = bus->next)
> +               for (dev = bus->devices; dev; dev = dev->next)
> +                       if (dev->descriptor.idVendor = base_vendor &&
> +                           dev->descriptor.idProduct = base_product)
> +                               for (i = 0; i < dev->num_children; i++)
> +                                       for (j = 0; j < dev->children[i]->descriptor.bNumConfigurations; j++)
> +                                               for (k = 0; k < dev->children[i]->config[j].bNumInterfaces; k++)
> +                                                       for (l = 0; l < dev->children[i]->config[j].interface[k].num_altsetting; l++)
> +                                                               if (dev->children[i]->config[j].interface[k].altsetting[l].bInterfaceProtocol = bInterfaceProtocol)
> +                                                                       return dev->children[i];
> +       return NULL;
> +}
>
> welcome to the land of fully unreadable code. You need to fix this and
> split it up if needed. Nobody has ever a chance to understand what this
> code does if it goes wrong.

That looks crazy. What are you trying to solve here?

We also don't want these weird multiline rules with on match per line,
and the use of properties  merged by earlier rules. I complained too
many times about this now.

That whole thing get pretty annoying, it does not seem to be ready to
be shipped with udev. Please properly fix _and_ test the stuff now.
Your earlier version already broke random keyboards and hubs, because
you obviously did not test it properly. If you don't provide a sane
solution now, I'm going to remove it from udev, and you need to create
your own package.

Thanks,
Kay

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1.1: Type: text/plain, Size: 2358 bytes --]

Hi Kay:

Kay Sievers wrote:
> On Tue, Jul 14, 2009 at 20:46, Marcel Holtmann<marcel@holtmann.org> wrote:
>   
>
> That looks crazy. What are you trying to solve here?
>   
My apologies.  I'll split that up into some more readable for loops.  I
was just trying to walk through the bus to find the appropriate
usb_device to match and return.
> We also don't want these weird multiline rules with on match per line,
> and the use of properties  merged by earlier rules. I complained too
> many times about this now.
>   
I'll switch over to one line.  I had thought it was more readable as
multiline.
Since the rule is referring to the parent's information, I wasn't sure
that this was doable without importing the properties of the parent's
earlier rules.
> That whole thing get pretty annoying, it does not seem to be ready to
> be shipped with udev. 
> Please properly fix _and_ test the stuff now.
> Your earlier version already broke random keyboards and hubs, because
> you obviously did not test it properly. If you don't provide a sane
> solution now, I'm going to remove it from udev, and you need to create
> your own package.
>   
The utility itself is stable.  The previous error with the matching
against hubs within docking stations was something I didn't anticipate
matching up the chain, indeed my mistake.  It was corrected by being
more specific on things to match on, which is what I am attempting again
with this second rule. 

Attached is an updated patch.  Again, i'm having to rely on earlier
rules for parent device information.  If there is a better way to do
this, can you please advise rather than be rash about removing this
utility from udev?  There shouldn't be any worry for mismatching devices
this time.  The new rule looks specifically for Dell Wireless Class
Controllers, with a Bluetooth protocol not matching further up the chain.

I broke up the for loops by relying on find_device instead.  My code is
quite similar to the example that is on the libusb documentation for
matching a property:  http://libusb.sourceforge.net/doc/examples-code.html
I'm not really sure of any way to make it more readable other than
adding superfluous spacing and comments as it's a very basic example.

Thanks,

-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: resuscitate.patch --]
[-- Type: text/x-patch; name="resuscitate.patch", Size: 3596 bytes --]

=== modified file 'extras/hid2hci/70-hid2hci.rules'
--- extras/hid2hci/70-hid2hci.rules	2009-06-26 06:17:23 +0000
+++ extras/hid2hci/70-hid2hci.rules	2009-07-14 20:56:36 +0000
@@ -11,6 +11,11 @@
 ATTR{bInterfaceClass}=="03", ATTR{bInterfaceSubClass}=="01", ATTR{bInterfaceProtocol}=="02", ATTRS{bDeviceClass}=="00", ATTRS{idVendor}=="413c", ATTRS{bmAttributes}=="e0", \
     RUN+="hid2hci --method dell -v $attr{idVendor} -p $attr{idProduct} --mode hci"
 
+# When a Dell device recovers from S3, the mouse child needs to be repoked
+# Unfortunately the only event seen is the BT device disappearing, so the mouse
+# device needs to be chased down on the USB bus.
+ATTR{bDeviceClass}=="e0", ATTR{bDeviceSubClass}=="01", ATTR{bDeviceProtocol}=="01", ATTR{idVendor}=="413c", ATTR{bmAttributes}=="e0", IMPORT{parent}="ID_*", ENV{REMOVE_CMD}="hid2hci --method dell -v $env{ID_VENDOR_ID} -p $env{ID_MODEL_ID} --mode hci -s 02"
+
 ENV{DEVTYPE}!="usb_device", GOTO="hid2hci_end"
 
 # Logitech devices

=== modified file 'extras/hid2hci/hid2hci.c'
--- extras/hid2hci/hid2hci.c	2009-06-16 17:30:22 +0000
+++ extras/hid2hci/hid2hci.c	2009-07-14 20:56:39 +0000
@@ -271,6 +271,23 @@
 	return 0;
 }
 
+static int find_resuscitated_device(struct device_info* devinfo, uint8_t bInterfaceProtocol)
+{
+	int i,j,k,l;
+
+	/* Using the base device, attempt to find the child with the
+	 * matching bInterfaceProtocol */
+	for (i = 0; i < devinfo->dev->num_children; i++)
+		for (j = 0; j < devinfo->dev->children[i]->descriptor.bNumConfigurations; j++)
+			for (k = 0; k < devinfo->dev->children[i]->config[j].bNumInterfaces; k++)
+				for (l = 0; l < devinfo->dev->children[i]->config[j].interface[k].num_altsetting; l++)
+					if (devinfo->dev->children[i]->config[j].interface[k].altsetting[l].bInterfaceProtocol == bInterfaceProtocol) {
+						devinfo->dev = devinfo->dev->children[i];
+						return 1;
+					}
+	return 0;
+}
+
 static void usage(char* error)
 {
 	if (error)
@@ -289,6 +306,7 @@
 		"\t-v, --vendor=        Vendor ID to act upon\n"
 		"\t-p, --product=       Product ID to act upon\n"
 		"\t-m, --method=        Method to use to switch [csr, logitech, dell]\n"
+		"\t-s, --resuscitate=   Find the child device with this bInterfaceProtocol to run on \n"
 		"\n");
 	if (error)
 		exit(1);
@@ -301,6 +319,7 @@
 	{ "vendor",	required_argument, 0, 'v' },
 	{ "product",	required_argument, 0, 'p' },
 	{ "method",	required_argument, 0, 'm' },
+	{ "resuscitate",required_argument, 0, 's' },
 	{ 0, 0, 0, 0 }
 };
 
@@ -309,8 +328,9 @@
 	struct device_info dev = { NULL, HCI, 0, 0 };
 	int opt, quiet = 0;
 	int (*method)(struct device_info *dev) = NULL;
+	uint8_t resuscitate = 0;
 
-	while ((opt = getopt_long(argc, argv, "+r:v:p:m:qh", main_options, NULL)) != -1) {
+	while ((opt = getopt_long(argc, argv, "+s:r:v:p:m:qh", main_options, NULL)) != -1) {
 		switch (opt) {
 		case 'r':
 			if (optarg && !strcmp(optarg, "hid"))
@@ -339,6 +359,9 @@
 		case 'q':
 			quiet = 1;
 			break;
+		case 's':
+			sscanf(optarg, "%2hx", (short unsigned int*) &resuscitate);
+			break;
 		case 'h':
 			usage(NULL);
 		default:
@@ -362,6 +385,13 @@
 		exit(1);
 	}
 
+	if (resuscitate && !find_resuscitated_device(&dev, resuscitate)) {
+		if (!quiet)
+			fprintf(stderr, "Device %04x:%04x was unable to resucitate any child devices.\n",
+				dev.vendor,dev.product);
+		exit(1);
+	}
+
 	if (!quiet)
 		printf("Attempting to switch device %04x:%04x to %s mode ",
 			dev.vendor, dev.product, dev.mode ? "HID" : "HCI");


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Marcel Holtmann]

Hi Mario,

> I'll switch over to one line.  I had thought it was more readable as
> multiline.
> Since the rule is referring to the parent's information, I wasn't sure
> that this was doable without importing the properties of the parent's
> earlier rules. 

what is wrong with using ATTRS here?

> > That whole thing get pretty annoying, it does not seem to be ready to
> > be shipped with udev. 
> > Please properly fix _and_ test the stuff now.
> > Your earlier version already broke random keyboards and hubs, because
> > you obviously did not test it properly. If you don't provide a sane
> > solution now, I'm going to remove it from udev, and you need to create
> > your own package.
> >   
> The utility itself is stable.  The previous error with the matching
> against hubs within docking stations was something I didn't anticipate
> matching up the chain, indeed my mistake.  It was corrected by being
> more specific on things to match on, which is what I am attempting
> again with this second rule.  
> 
> Attached is an updated patch.  Again, i'm having to rely on earlier
> rules for parent device information.  If there is a better way to do
> this, can you please advise rather than be rash about removing this
> utility from udev?  There shouldn't be any worry for mismatching
> devices this time.  The new rule looks specifically for Dell Wireless
> Class Controllers, with a Bluetooth protocol not matching further up
> the chain.
> 
> I broke up the for loops by relying on find_device instead.  My code
> is quite similar to the example that is on the libusb documentation
> for matching a property:
> http://libusb.sourceforge.net/doc/examples-code.html
> I'm not really sure of any way to make it more readable other than
> adding superfluous spacing and comments as it's a very basic example.

Split more up into proper matching functions and with extra variables to
pointers. This is still unreadable.

Regards

Marcel

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Tue, Jul 14, 2009 at 23:00, Mario
Limonciello<mario_limonciello@dell.com> wrote:

Ok, I took a look first over the stuff we currently have.

What kind of directory is that? I've never seen such a thing:
  sprintf(devname, "%s/usb/hid/hiddev%d", devpath, i);

> I'll split that up into some more readable for loops.  I was
> just trying to walk through the bus to find the appropriate usb_device to
> match and return.

Why do you need to *find* the device at all? Same question for the
normal call case too, not only the resume case.

We hook into the event for _this_ device, and we also have $DEVNAME in
the environment already that points to the raw USB device node file,
which we can directly open() without any *find*. Why do we need to
iterate over all other USB devices to find ourself? What do I miss?

> Again, i'm having to rely on earlier rules
> for parent device information.  If there is a better way to do this, can you
> please advise?

What is the event you hook in, the bluetooth USB interface? The USB
device you need to re-configure is still the parent of that device,
right?

Thanks,
Kay

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Tue, Jul 14, 2009 at 23:24, Kay Sievers<kay.sievers@vrfy.org> wrote:
> On Tue, Jul 14, 2009 at 23:00, Mario
> Limonciello<mario_limonciello@dell.com> wrote:
>
> Ok, I took a look first over the stuff we currently have.
>
> What kind of directory is that? I've never seen such a thing:
>  sprintf(devname, "%s/usb/hid/hiddev%d", devpath, i);

That all needs to be fixed. We are not hooking into USB device events
and write to hard-coded /dev/hidraw* devices. If these devices need to
be handled with hidraw, the tools needs to hook into hidraw events.

>> I'll split that up into some more readable for loops.  I was
>> just trying to walk through the bus to find the appropriate usb_device to
>> match and return.
>
> Why do you need to *find* the device at all? Same question for the
> normal call case too, not only the resume case.

Seems libusb is too stupid to handle a specific device, and
unfortunately even the new libusb seems to be not better regarding
this. It really needs an interface to select a specific device by
whatever _unique_ property, not by vid/pid, instead of this braindead
brute-force searching across all possible devices to find itself.

So, seems there is not hid2hci fault, and it can not do much here.

Kay

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Alan Stern]

On Wed, 15 Jul 2009, Kay Sievers wrote:

> > What kind of directory is that? I've never seen such a thing:
> >  sprintf(devname, "%s/usb/hid/hiddev%d", devpath, i);
> 
> That all needs to be fixed. We are not hooking into USB device events
> and write to hard-coded /dev/hidraw* devices. If these devices need to
> be handled with hidraw, the tools needs to hook into hidraw events.

That's absolutely right.  At the time the USB device event takes place,
the HID driver might not even be loaded yet!

> > Why do you need to *find* the device at all? Same question for the
> > normal call case too, not only the resume case.

This is the difficult aspect of the application.  The program needs to
poke at an HID device when it receives an event concerning an HCI
device.  Mario doesn't want to assume there will be any fixed relation
between the two devices (although it should be safe to assume they will
always have the same parent hub).

Thus some sort of search appears to be necessary.  It's not clear
whether the search result can be saved (say in udev's database) so that
later "resuscitate" invocations don't need to repeat the search.

> Seems libusb is too stupid to handle a specific device, and
> unfortunately even the new libusb seems to be not better regarding
> this. It really needs an interface to select a specific device by
> whatever _unique_ property, not by vid/pid, instead of this braindead
> brute-force searching across all possible devices to find itself.

The unique identifier appropriate for libusb is a (bus number, device
number) pair.  These values will not change across a suspend/resume.  
I don't know whether libusb has an API to open the particular device
given by those numbers, but it ought to.

Alan Stern

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Wed, Jul 15, 2009 at 04:07, Alan Stern<stern@rowland.harvard.edu> wrote:
> On Wed, 15 Jul 2009, Kay Sievers wrote:
>
>> > What kind of directory is that? I've never seen such a thing:
>> >  sprintf(devname, "%s/usb/hid/hiddev%d", devpath, i);
>>
>> That all needs to be fixed. We are not hooking into USB device events
>> and write to hard-coded /dev/hidraw* devices. If these devices need to
>> be handled with hidraw, the tools needs to hook into hidraw events.
>
> That's absolutely right.  At the time the USB device event takes place,
> the HID driver might not even be loaded yet!

Right, and even when it's loaded, there is no guarantee, that this
device is already created by udev.

>> > Why do you need to *find* the device at all? Same question for the
>> > normal call case too, not only the resume case.
>
> This is the difficult aspect of the application.  The program needs to
> poke at an HID device when it receives an event concerning an HCI
> device.  Mario doesn't want to assume there will be any fixed relation
> between the two devices (although it should be safe to assume they will
> always have the same parent hub).
>
> Thus some sort of search appears to be necessary.  It's not clear
> whether the search result can be saved (say in udev's database) so that
> later "resuscitate" invocations don't need to repeat the search.

Yeah, what a hardware. :) These are all *devices* not *interfaces*
right? And we poke a sibling device which is a HID device to manage
the other sibling which shows up as the bluetooth device then?

>> Seems libusb is too stupid to handle a specific device, and
>> unfortunately even the new libusb seems to be not better regarding
>> this. It really needs an interface to select a specific device by
>> whatever _unique_ property, not by vid/pid, instead of this braindead
>> brute-force searching across all possible devices to find itself.
>
> The unique identifier appropriate for libusb is a (bus number, device
> number) pair.

Yeah, that would be good to use. That's what the device nodes use too.

> These values will not change across a suspend/resume.
> I don't know whether libusb has an API to open the particular device
> given by those numbers, but it ought to.

I didn't see anything like that. If that can't be done, I'll just add
the few needed things to switch the device to code inside of udev and
drop that libusb thing entirely.

Thanks,
Kay

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 676 bytes --]

Hi Marcel:

Thanks for the feedback.

Marcel Holtmann wrote:
> Hi Mario,
>
> what is wrong with using ATTRS here?
>   
I don't believe ATTRS will be able to locate the vid/pid of the parent
device.  Currently I'm using IMPORT to pull that stuff into the env of
this device.  If this is possible without IMPORT and $env, I'd be like
to learn the syntax to do so.
> Split more up into proper matching functions and with extra variables to
> pointers. This is still unreadable.
>   
OK, I'll follow up with a patch cleaning up per this and other things
later in this thread.


-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Mario Limonciello]

[-- Attachment #1: Type: text/plain, Size: 3448 bytes --]

Hi Kay & Alan

Kay Sievers wrote:
> On Wed, Jul 15, 2009 at 04:07, Alan Stern<stern@rowland.harvard.edu> wrote:
>   
>> On Wed, 15 Jul 2009, Kay Sievers wrote:
>>
>>     
>>>> What kind of directory is that? I've never seen such a thing:
>>>>  sprintf(devname, "%s/usb/hid/hiddev%d", devpath, i);
>>>>         
>>> That all needs to be fixed. We are not hooking into USB device events
>>> and write to hard-coded /dev/hidraw* devices. If these devices need to
>>> be handled with hidraw, the tools needs to hook into hidraw events.
>>>       
>> That's absolutely right.  At the time the USB device event takes place,
>> the HID driver might not even be loaded yet!
>>     
>
> Right, and even when it's loaded, there is no guarantee, that this
> device is already created by udev.
>
>   
So this code for switch_logitech was originally authored by Marcel.  I
can try to help clean this part up, but I'd like to treat that as an
independent problem to follow up to after I get the logic for getting
this Dell HW working after S3.
>>>> Why do you need to *find* the device at all? Same question for the
>>>> normal call case too, not only the resume case.
>>>>         
>> This is the difficult aspect of the application.  The program needs to
>> poke at an HID device when it receives an event concerning an HCI
>> device.  Mario doesn't want to assume there will be any fixed relation
>> between the two devices (although it should be safe to assume they will
>> always have the same parent hub).
>>
>> Thus some sort of search appears to be necessary.  It's not clear
>> whether the search result can be saved (say in udev's database) so that
>> later "resuscitate" invocations don't need to repeat the search.
>>     
>
> Yeah, what a hardware. :) These are all *devices* not *interfaces*
> right? And we poke a sibling device which is a HID device to manage
> the other sibling which shows up as the bluetooth device then?
>   
Yes, these are all individual devices.  It would be a safe assumption
that they have the same parent hub, so perhaps the search could be
simplified.
>   
>>> Seems libusb is too stupid to handle a specific device, and
>>> unfortunately even the new libusb seems to be not better regarding
>>> this. It really needs an interface to select a specific device by
>>> whatever _unique_ property, not by vid/pid, instead of this braindead
>>> brute-force searching across all possible devices to find itself.
>>>       
>> The unique identifier appropriate for libusb is a (bus number, device
>> number) pair.
>>     
>
> Yeah, that would be good to use. That's what the device nodes use too.
>   

>   
>> These values will not change across a suspend/resume.
>> I don't know whether libusb has an API to open the particular device
>> given by those numbers, but it ought to.
>>     
>
> I didn't see anything like that. If that can't be done, I'll just add
> the few needed things to switch the device to code inside of udev and
> drop that libusb thing entirely.
>
>   
I'll look into switching to this pair instead.  I didn't see any obvious
method to select devices by these properties, but I've just skimmed
libusb source looking for them.  If I don't find them, i'll resubmit the
patch with Marcel's recommendations of using more pointers for readability.
-- 
Mario Limonciello
*Dell | Linux Engineering*
mario_limonciello@dell.com


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

Re: [PATCH] Explicitly disable BT radio using rfkill interface on

[Kay Sievers]

On Thu, Jul 16, 2009 at 00:24, Mario
Limonciello<mario_limonciello@dell.com> wrote:

>> Right, and even when it's loaded, there is no guarantee, that this
>> device is already created by udev.
>>
> So this code for switch_logitech was originally authored by Marcel.  I
> can try to help clean this part up, but I'd like to treat that as an
> independent problem to follow up to after I get the logic for getting
> this Dell HW working after S3.

Sure, it still needs to be fixed or ripped out before the next udev
release. We can not keep stuff like this in the udev tree. People use
this as a starting point for their new tools, and therefore we can not
ship such broken things.

>>> These values will not change across a suspend/resume.
>>> I don't know whether libusb has an API to open the particular device
>>> given by those numbers, but it ought to.
>>
>> I didn't see anything like that. If that can't be done, I'll just add
>> the few needed things to switch the device to code inside of udev and
>> drop that libusb thing entirely.
>>
> I'll look into switching to this pair instead.  I didn't see any obvious
> method to select devices by these properties, but I've just skimmed
> libusb source looking for them.  If I don't find them, i'll resubmit the
> patch with Marcel's recommendations of using more pointers for readability.

Fine, just put all that in a function that accepts bus and dev num as
a parameter to find the device. If libusb can not be fixed, I'll put
the needed bits in the udev tree and drop libusb completely. As said,
we are not going to search over all devices to find what we already
have.

Thanks,
Kay