diff mbox

bluetooth: Add hci_h4p driver

Message ID 20141214192124.GA22392@amd (mailing list archive)
State New, archived
Headers show

Commit Message

Pavel Machek Dec. 14, 2014, 7:21 p.m. UTC
Hi!


> > Hacks surrounding bluetooth address were removed; this results in
> > working driver with address that is probably not unique.
> 
> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in userspace. You can use the btmgmt public-addr command for testing.
> 

Ok, it took me a while to figure out that --enable-experimental is
needed.

Then help was playing tricks with me:

For more information on the usage of each command use:
    btmgmt <command> --help
root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help
Set Public Address for hci0 failed with status 0x0b (Rejected)
root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr
Usage: btmgmt public-addr <address>


root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06
Set Public Address for hci0 failed with status 0x0b (Rejected)

Hmm. Since setting public address only works when interface is down,
and we lose all the settings during up, this does not work at all,
right?

Anyway, you might want to apply this to lessen the confusion.

Only in bluez-5.26/tools: btmgmt.c~

Comments

Marcel Holtmann Dec. 14, 2014, 8:28 p.m. UTC | #1
Hi Pavel,

>>> Hacks surrounding bluetooth address were removed; this results in
>>> working driver with address that is probably not unique.
>> 
>> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in userspace. You can use the btmgmt public-addr command for testing.
>> 
> 
> Ok, it took me a while to figure out that --enable-experimental is
> needed.
> 
> Then help was playing tricks with me:
> 
> For more information on the usage of each command use:
>    btmgmt <command> --help
> root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help
> Set Public Address for hci0 failed with status 0x0b (Rejected)
> root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr
> Usage: btmgmt public-addr <address>

that is indeed confusing. Something went wrong with that tool. Just keep in mind that tool is just for internal testing by the developers. Someone would still need to write a proper integration with listening for the appropriate events and dig out the actual address from the memory location.

> root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06
> Set Public Address for hci0 failed with status 0x0b (Rejected)
> 
> Hmm. Since setting public address only works when interface is down,
> and we lose all the settings during up, this does not work at all,
> right?

It does keep it. The address is stored in hdev->public_addr which is actually different from hdev->bdaddr which is the current address in use.

However I am not sure that hdev->set_bdaddr is executed again. Same as hdev->setup is not executed twice. The controller loosing all states is something we have not yet encountered. At least not while the hci_dev stays around.

I wonder if the drastic power off might be better hooked into a platform RFKILL switch. And with that you would just unregister hci_dev and re-register it when the RFKILL switch is unblocked.

The other option is to introduce a quirk that allows running hdev->setup and hdev->set_bdaddr all the time when you bring up your device.

Another alternate idea for getting something upstream for testing is to ignore the whole firmware loading in the kernel and bring up the controller with HCI_QUIRK_EXTERNAL_CONFIG. Doing that would allow you to do bette testing and evolve the driver in small tests.

So what this quirk does is that the hci_dev comes up unconfigured. So mgmt interface will only show it as unconfigured controller. However you can open it with HCI User Channel and run all HCI commands manually. That is what src/shared/hci.[ch] is good at. Once you are done, you just flip the switch via btmgmt ext-config and then the kernel goes and treats it as fully configured.

Regards

Marcel
Pavel Machek Dec. 14, 2014, 10:41 p.m. UTC | #2
Hi!

> >>> Hacks surrounding bluetooth address were removed; this results in
> >>> working driver with address that is probably not unique.
> >> 
> >> Just set HCI_QUIRK_INVALID_BDADDR and let someone deal with that in userspace. You can use the btmgmt public-addr command for testing.
> >> 
> > 
> > Ok, it took me a while to figure out that --enable-experimental is
> > needed.
> > 
> > Then help was playing tricks with me:
> > 
> > For more information on the usage of each command use:
> >    btmgmt <command> --help
> > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr --help
> > Set Public Address for hci0 failed with status 0x0b (Rejected)
> > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr
> > Usage: btmgmt public-addr <address>
> 
> that is indeed confusing. Something went wrong with that tool. Just keep in mind that tool is just for internal testing by the developers. Someone would still need to write a proper integration with listening for the appropriate events and dig out the actual address from the memory location.
>

You have patch that helps that issues in the email :-).

> > root@n900:/my/bluez-5.26/tools# ./btmgmt public-addr 01:02:03:04:05:06
> > Set Public Address for hci0 failed with status 0x0b (Rejected)
> > 
> > Hmm. Since setting public address only works when interface is down,
> > and we lose all the settings during up, this does not work at all,
> > right?
> 
> It does keep it. The address is stored in hdev->public_addr which is actually different from hdev->bdaddr which is the current address in use.
> 
> However I am not sure that hdev->set_bdaddr is executed again. Same
> as hdev->setup is not executed twice. The controller loosing all
> states is something we have not yet encountered. At least not
> while the hci_dev stays around.

It looks like that's the exact issue. It seems to me like set_bdaddr
is actually called while the device is "down".

> I wonder if the drastic power off might be better hooked into a
> platform RFKILL switch. And with that you would just unregister
> hci_dev and re-register it when the RFKILL switch is unblocked.

Ok, so kernel currently wants bluetooth out of reset at 

> The other option is to introduce a quirk that allows running hdev->setup and hdev->set_bdaddr all the time when you bring up your device.
> 
> Another alternate idea for getting something upstream for testing is to ignore the whole firmware loading in the kernel and bring up the controller with HCI_QUIRK_EXTERNAL_CONFIG. Doing that would allow you to do bette testing and evolve the driver in small tests.
>

Actually, firmware loading is _not_ the biggest problem. That is
pretty self-contained now.

I guess I should just configure it at probe time, put it into reset
during rmmod, keep it out of reset otherwise, and see how it works.

Thanks,

									Pavel
diff mbox

Patch

diff -ur bluez-5.26.ofic/tools/btmgmt.c bluez-5.26/tools/btmgmt.c
--- bluez-5.26.ofic/tools/btmgmt.c	2014-12-14 12:32:19.742595000 +0100
+++ bluez-5.26/tools/btmgmt.c	2014-12-14 20:06:40.432497000 +0100
@@ -2603,7 +2603,7 @@ 
 
 static void static_addr_usage(void)
 {
-	printf("Usage: btmgmt static-addr <address>\n");
+	printf("Usage: btmgmt static-addr ??:??:??:??:??:??\n");
 }
 
 static void cmd_static_addr(struct mgmt *mgmt, uint16_t index,
@@ -2660,7 +2660,8 @@ 
 	struct mgmt_cp_set_public_address cp;
 
 	if (argc < 2) {
-		printf("Usage: btmgmt public-addr <address>\n");
+		printf("Usage: btmgmt public-addr ??:??:??:??:??:??\n"
+		       "Note: interface must be down for this to work\n");
 		exit(EXIT_FAILURE);
 	}
 
@@ -2934,7 +2935,7 @@ 
 
 static void add_device_usage(void)
 {
-	printf("Usage: btmgmt add-device [-a action] [-t type] <address>\n");
+	printf("Usage: btmgmt add-device [-a action] [-t type] ??:??:??:??:??:??\n");
 }
 
 static struct option add_device_options[] = {
@@ -3007,7 +3008,7 @@ 
 
 static void del_device_usage(void)
 {
-	printf("Usage: btmgmt del-device [-t type] <address>\n");
+	printf("Usage: btmgmt del-device [-t type] ??:??:??:??:??:??\n");
 }
 
 static struct option del_device_options[] = {
@@ -3153,7 +3154,7 @@ 
 
 	printf("\n"
 		"For more information on the usage of each command use:\n"
-		"\tbtmgmt <command> --help\n" );
+		"\tbtmgmt <command>\n" );
 }
 
 static struct option main_options[] = {