[PATCH 0/4] toshiba_bluetooth: Clean up driver plus a bugfix

[PATCH 0/4] toshiba_bluetooth: Clean up driver plus a bugfix

[Azael Avalos]

These patches introduce three new functions, cleaning up the driver
code, checking for errors and returning accordingly, and also fixes
bug 93911.

Azael Avalos (4):
  toshiba_bluetooth: Add three new functions to the driver
  toshiba_bluetooth: Clean up *_add function and disable BT device at
    removal
  toshiba_bluetooth: Clean toshiba_bluetooth_enable function
  toshiba_bluetooth: Remove BT enable code from *_notify function

 drivers/platform/x86/toshiba_bluetooth.c | 133 ++++++++++++++++++++++---------
 1 file changed, 97 insertions(+), 36 deletions(-)

-- 
2.2.2

[PATCH 1/4] toshiba_bluetooth: Add three new functions to the driver

[Azael Avalos]

This patch introduces three new functions, which are going to be used
by the next patches.

The functions introduced are toshiba_bluetooth_present,
toshiba_bluetooth_status and toshiba_bluetooth_disable, which queries
the presence of the device, queries the status and disables the
device respectively.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_bluetooth.c | 51 ++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index 2cb1ea6..b479a70 100644
--- a/drivers/platform/x86/toshiba_bluetooth.c
+++ b/drivers/platform/x86/toshiba_bluetooth.c
@@ -57,6 +57,38 @@ static struct acpi_driver toshiba_bt_rfkill_driver = {
 	.drv.pm =	&toshiba_bt_pm,
 };
 
+static int toshiba_bluetooth_present(acpi_handle handle)
+{
+	acpi_status result;
+	u64 bt_present;
+
+	result = acpi_evaluate_integer(handle, "_STA", NULL, &bt_present);
+	if (ACPI_FAILURE(result)) {
+		pr_err("ACPI call to query Bluetooth presence failed");
+		return -ENXIO;
+	} else if (!bt_present) {
+		pr_info("Bluetooth device not present\n");
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+static int toshiba_bluetooth_status(acpi_handle handle)
+{
+	acpi_status result;
+	u64 status;
+
+	result = acpi_evaluate_integer(handle, "BTST", NULL, &status);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not get Bluetooth device status\n");
+		return -ENXIO;
+	}
+
+	pr_info("Bluetooth status %llu\n", status);
+
+	return status;
+}
 
 static int toshiba_bluetooth_enable(acpi_handle handle)
 {
@@ -85,6 +117,25 @@ static int toshiba_bluetooth_enable(acpi_handle handle)
 	return -ENODEV;
 }
 
+static int toshiba_bluetooth_disable(acpi_handle handle)
+{
+	acpi_status result;
+
+	result = acpi_evaluate_object(handle, "BTPF", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not power OFF Bluetooth device\n");
+		return -ENXIO;
+	}
+
+	result = acpi_evaluate_object(handle, "DUSB", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not detach USB Bluetooth device\n");
+		return -ENXIO;
+	}
+
+	return 0;
+}
+
 static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
 {
 	toshiba_bluetooth_enable(device->handle);
-- 
2.2.2

[PATCH 2/4] toshiba_bluetooth: Clean up *_add function and disable BT device at removal

[Azael Avalos]

This patch cleans the toshiba_bluetooth_add function by using the
recently introduced function toshiba_bluetooth_present, simplifying
its code and returning appropriate error values.

Also, disables the BT device at the removal of the driver, by using
the function toshiba_bluetooth_disable.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_bluetooth.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 deletions(-)

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index b479a70..b8404c7 100644
--- a/drivers/platform/x86/toshiba_bluetooth.c
+++ b/drivers/platform/x86/toshiba_bluetooth.c
@@ -62,6 +62,11 @@ static int toshiba_bluetooth_present(acpi_handle handle)
 	acpi_status result;
 	u64 bt_present;
 
+	/*
+	 * Some Toshiba laptops may have a fake TOS6205 device in
+	 * their ACPI BIOS, so query the _STA method to see if there
+	 * is really anything there.
+	 */
 	result = acpi_evaluate_integer(handle, "_STA", NULL, &bt_present);
 	if (ACPI_FAILURE(result)) {
 		pr_err("ACPI call to query Bluetooth presence failed");
@@ -150,23 +155,18 @@ static int toshiba_bt_resume(struct device *dev)
 
 static int toshiba_bt_rfkill_add(struct acpi_device *device)
 {
-	acpi_status status;
-	u64 bt_present;
-	int result = -ENODEV;
+	int result;
 
-	/*
-	 * Some Toshiba laptops may have a fake TOS6205 device in
-	 * their ACPI BIOS, so query the _STA method to see if there
-	 * is really anything there, before trying to enable it.
-	 */
-	status = acpi_evaluate_integer(device->handle, "_STA", NULL,
-				       &bt_present);
+	result = toshiba_bluetooth_present(device->handle);
+	if (result)
+		return result;
 
-	if (!ACPI_FAILURE(status) && bt_present) {
-		pr_info("Detected Toshiba ACPI Bluetooth device - "
-			"installing RFKill handler\n");
-		result = toshiba_bluetooth_enable(device->handle);
-	}
+	pr_info("Toshiba ACPI Bluetooth device driver\n");
+
+	/* Enable the BT device */
+	result = toshiba_bluetooth_enable(device->handle);
+	if (result)
+		return result;
 
 	return result;
 }
@@ -174,7 +174,7 @@ static int toshiba_bt_rfkill_add(struct acpi_device *device)
 static int toshiba_bt_rfkill_remove(struct acpi_device *device)
 {
 	/* clean up */
-	return 0;
+	return toshiba_bluetooth_disable(device->handle);
 }
 
 module_acpi_driver(toshiba_bt_rfkill_driver);
-- 
2.2.2

[PATCH 3/4] toshiba_bluetooth: Clean toshiba_bluetooth_enable function

[Azael Avalos]

This patch cleans the toshiba_bluetooth_enable function, removing
some unneeded code and returning appropriate error values.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_bluetooth.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index b8404c7..0343d20 100644
--- a/drivers/platform/x86/toshiba_bluetooth.c
+++ b/drivers/platform/x86/toshiba_bluetooth.c
@@ -97,29 +97,21 @@ static int toshiba_bluetooth_status(acpi_handle handle)
 
 static int toshiba_bluetooth_enable(acpi_handle handle)
 {
-	acpi_status res1, res2;
-	u64 result;
-
-	/*
-	 * Query ACPI to verify RFKill switch is set to 'on'.
-	 * If not, we return silently, no need to report it as
-	 * an error.
-	 */
-	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
-	if (ACPI_FAILURE(res1))
-		return res1;
-	if (!(result & 0x01))
-		return 0;
+	acpi_status result;
 
-	pr_info("Re-enabling Toshiba Bluetooth\n");
-	res1 = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
-	res2 = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
-	if (!ACPI_FAILURE(res1) || !ACPI_FAILURE(res2))
-		return 0;
+	result = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not attach USB Bluetooth device\n");
+		return -ENXIO;
+	}
 
-	pr_warn("Failed to re-enable Toshiba Bluetooth\n");
+	result = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not power ON Bluetooth device\n");
+		return -ENXIO;
+	}
 
-	return -ENODEV;
+	return 0;
 }
 
 static int toshiba_bluetooth_disable(acpi_handle handle)
-- 
2.2.2

[PATCH 3/4] toshiba_bluetooth: Clean toshiba_bluetooth_enable function

[Azael Avalos]

This patch cleans the toshiba_bluetooth_enable function, removing
some unneeded code and returning appropriate error values.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_bluetooth.c | 32 ++++++++++++--------------------
 1 file changed, 12 insertions(+), 20 deletions(-)

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index b8404c7..0343d20 100644
--- a/drivers/platform/x86/toshiba_bluetooth.c
+++ b/drivers/platform/x86/toshiba_bluetooth.c
@@ -97,29 +97,21 @@ static int toshiba_bluetooth_status(acpi_handle handle)
 
 static int toshiba_bluetooth_enable(acpi_handle handle)
 {
-	acpi_status res1, res2;
-	u64 result;
-
-	/*
-	 * Query ACPI to verify RFKill switch is set to 'on'.
-	 * If not, we return silently, no need to report it as
-	 * an error.
-	 */
-	res1 = acpi_evaluate_integer(handle, "BTST", NULL, &result);
-	if (ACPI_FAILURE(res1))
-		return res1;
-	if (!(result & 0x01))
-		return 0;
+	acpi_status result;
 
-	pr_info("Re-enabling Toshiba Bluetooth\n");
-	res1 = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
-	res2 = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
-	if (!ACPI_FAILURE(res1) || !ACPI_FAILURE(res2))
-		return 0;
+	result = acpi_evaluate_object(handle, "AUSB", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not attach USB Bluetooth device\n");
+		return -ENXIO;
+	}
 
-	pr_warn("Failed to re-enable Toshiba Bluetooth\n");
+	result = acpi_evaluate_object(handle, "BTPO", NULL, NULL);
+	if (ACPI_FAILURE(result)) {
+		pr_err("Could not power ON Bluetooth device\n");
+		return -ENXIO;
+	}
 
-	return -ENODEV;
+	return 0;
 }
 
 static int toshiba_bluetooth_disable(acpi_handle handle)
-- 
2.2.2

[PATCH 4/4] toshiba_bluetooth: Remove BT enable code from *_notify function

[Azael Avalos]

Bug 93911 reported a broken handling of the BT device, causing the
driver to get stuck in a loop enabling/disabling the device whenever
the device is deactivated by the kill switch as follows:

1. The user activated the kill switch, causing the system to generate
   a 0x90 (status change) event and disabling the BT device.
2. The driver catches the event and re-enables the BT device.
3. The system detects the device being activated, but since the kill
   switch is activated, disables the BT device (again) and generates
   a 0x90 event (again).
4. Repeat from 2.

This patch removes the toshiba_bluetooth_enable call from the
toshiba_bluetooth_notify function, as we do not always need to
re-activate the device everytime we receive an event, we need to act
depending on the status of the BT device.

For now, we simply print the event received and the status of the BT
device, so we can later add code to handle special circumstances
depending on the status of the device.

Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
---
 drivers/platform/x86/toshiba_bluetooth.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index 0343d20..07edded 100644
--- a/drivers/platform/x86/toshiba_bluetooth.c
+++ b/drivers/platform/x86/toshiba_bluetooth.c
@@ -2,6 +2,7 @@
  * Toshiba Bluetooth Enable Driver
  *
  * Copyright (C) 2009 Jes Sorensen <Jes.Sorensen@gmail.com>
+ * Copyright (C) 2015 Azael Avalos <coproscefalo@gmail.com>
  *
  * Thanks to Matthew Garrett for background info on ACPI innards which
  * normal people aren't meant to understand :-)
@@ -25,6 +26,10 @@
 #include <linux/types.h>
 #include <linux/acpi.h>
 
+#define BT_KILLSWITCH_MASK	0x01
+#define BT_PLUGGED_MASK		0x40
+#define BT_POWER_MASK		0x80
+
 MODULE_AUTHOR("Jes Sorensen <Jes.Sorensen@gmail.com>");
 MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
 MODULE_LICENSE("GPL");
@@ -135,7 +140,20 @@ static int toshiba_bluetooth_disable(acpi_handle handle)
 
 static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
 {
-	toshiba_bluetooth_enable(device->handle);
+	int status;
+
+	pr_info("Received event %x\n", event);
+
+	/* Query the status of the Bluetooth device */
+	status = toshiba_bluetooth_status(device->handle);
+	if (status < 0)
+		return;
+
+	pr_info("Switch status %x\n", status & BT_KILLSWITCH_MASK);
+	pr_info("Power status %x\n", status & BT_POWER_MASK);
+	pr_info("Plug status %x\n", status & BT_PLUGGED_MASK);
+
+	/* TODO: Add event handling here depending on Bluetooth status */
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.2.2

Re: [PATCH 4/4] toshiba_bluetooth: Remove BT enable code from *_notify function

[Darren Hart]

On Wed, Mar 18, 2015 at 01:12:59PM -0600, Azael Avalos wrote:
> Bug 93911 reported a broken handling of the BT device, causing the
> driver to get stuck in a loop enabling/disabling the device whenever
> the device is deactivated by the kill switch as follows:
> 
> 1. The user activated the kill switch, causing the system to generate
>    a 0x90 (status change) event and disabling the BT device.
> 2. The driver catches the event and re-enables the BT device.

It does? The toshiba_bluetooth_enable() call is supposed to exit silently if
BTST is not on. Is this check not working on some systems?

> 3. The system detects the device being activated, but since the kill
>    switch is activated, disables the BT device (again) and generates
>    a 0x90 event (again).
> 4. Repeat from 2.
> 
> This patch removes the toshiba_bluetooth_enable call from the
> toshiba_bluetooth_notify function, as we do not always need to
> re-activate the device everytime we receive an event, we need to act
> depending on the status of the BT device.
> 
> For now, we simply print the event received and the status of the BT
> device, so we can later add code to handle special circumstances
> depending on the status of the device.
> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
>  drivers/platform/x86/toshiba_bluetooth.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
> index 0343d20..07edded 100644
> --- a/drivers/platform/x86/toshiba_bluetooth.c
> +++ b/drivers/platform/x86/toshiba_bluetooth.c
> @@ -2,6 +2,7 @@
>   * Toshiba Bluetooth Enable Driver
>   *
>   * Copyright (C) 2009 Jes Sorensen <Jes.Sorensen@gmail.com>
> + * Copyright (C) 2015 Azael Avalos <coproscefalo@gmail.com>
>   *
>   * Thanks to Matthew Garrett for background info on ACPI innards which
>   * normal people aren't meant to understand :-)
> @@ -25,6 +26,10 @@
>  #include <linux/types.h>
>  #include <linux/acpi.h>
>  
> +#define BT_KILLSWITCH_MASK	0x01
> +#define BT_PLUGGED_MASK		0x40
> +#define BT_POWER_MASK		0x80
> +
>  MODULE_AUTHOR("Jes Sorensen <Jes.Sorensen@gmail.com>");
>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
>  MODULE_LICENSE("GPL");
> @@ -135,7 +140,20 @@ static int toshiba_bluetooth_disable(acpi_handle handle)
>  
>  static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
>  {
> -	toshiba_bluetooth_enable(device->handle);
> +	int status;
> +
> +	pr_info("Received event %x\n", event);
> +
> +	/* Query the status of the Bluetooth device */
> +	status = toshiba_bluetooth_status(device->handle);
> +	if (status < 0)
> +		return;
> +
> +	pr_info("Switch status %x\n", status & BT_KILLSWITCH_MASK);
> +	pr_info("Power status %x\n", status & BT_POWER_MASK);
> +	pr_info("Plug status %x\n", status & BT_PLUGGED_MASK);

This seems a bit noisy for the info level as this information seems more aimed
at a developer working to complete the TODO below... debug seems more
appropriate.

> +
> +	/* TODO: Add event handling here depending on Bluetooth status */

Before if the RFKILL was disabled, this would have called
toshiba_bluetooth_enable() which would have checked BTST, found it on, and
enabled the device. In addition to killing the call to
toshiba_bluetooth_enable() when RFKILL is on, this also removes the call when
it's switched off.

I think I'm missing a path. Where does enable occur when the user turns the
radio back on now?


>  }
>  
>  #ifdef CONFIG_PM_SLEEP
> -- 
> 2.2.2
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center

Re: [PATCH 4/4] toshiba_bluetooth: Remove BT enable code from *_notify function

[Azael Avalos]

Hi Darren,

2015-03-25 11:50 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Mar 18, 2015 at 01:12:59PM -0600, Azael Avalos wrote:
>> Bug 93911 reported a broken handling of the BT device, causing the
>> driver to get stuck in a loop enabling/disabling the device whenever
>> the device is deactivated by the kill switch as follows:
>>
>> 1. The user activated the kill switch, causing the system to generate
>>    a 0x90 (status change) event and disabling the BT device.
>> 2. The driver catches the event and re-enables the BT device.
>
> It does? The toshiba_bluetooth_enable() call is supposed to exit silently if
> BTST is not on. Is this check not working on some systems?

Apparently not, I looked at the file and saw the comment block
and the check for the KS status and all seemed fine to me, but somehow,
and as you say, it is not working on some systems.

>
>> 3. The system detects the device being activated, but since the kill
>>    switch is activated, disables the BT device (again) and generates
>>    a 0x90 event (again).
>> 4. Repeat from 2.
>>
>> This patch removes the toshiba_bluetooth_enable call from the
>> toshiba_bluetooth_notify function, as we do not always need to
>> re-activate the device everytime we receive an event, we need to act
>> depending on the status of the BT device.
>>
>> For now, we simply print the event received and the status of the BT
>> device, so we can later add code to handle special circumstances
>> depending on the status of the device.
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>>  drivers/platform/x86/toshiba_bluetooth.c | 20 +++++++++++++++++++-
>>  1 file changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
>> index 0343d20..07edded 100644
>> --- a/drivers/platform/x86/toshiba_bluetooth.c
>> +++ b/drivers/platform/x86/toshiba_bluetooth.c
>> @@ -2,6 +2,7 @@
>>   * Toshiba Bluetooth Enable Driver
>>   *
>>   * Copyright (C) 2009 Jes Sorensen <Jes.Sorensen@gmail.com>
>> + * Copyright (C) 2015 Azael Avalos <coproscefalo@gmail.com>
>>   *
>>   * Thanks to Matthew Garrett for background info on ACPI innards which
>>   * normal people aren't meant to understand :-)
>> @@ -25,6 +26,10 @@
>>  #include <linux/types.h>
>>  #include <linux/acpi.h>
>>
>> +#define BT_KILLSWITCH_MASK   0x01
>> +#define BT_PLUGGED_MASK              0x40
>> +#define BT_POWER_MASK                0x80
>> +
>>  MODULE_AUTHOR("Jes Sorensen <Jes.Sorensen@gmail.com>");
>>  MODULE_DESCRIPTION("Toshiba Laptop ACPI Bluetooth Enable Driver");
>>  MODULE_LICENSE("GPL");
>> @@ -135,7 +140,20 @@ static int toshiba_bluetooth_disable(acpi_handle handle)
>>
>>  static void toshiba_bt_rfkill_notify(struct acpi_device *device, u32 event)
>>  {
>> -     toshiba_bluetooth_enable(device->handle);
>> +     int status;
>> +
>> +     pr_info("Received event %x\n", event);
>> +
>> +     /* Query the status of the Bluetooth device */
>> +     status = toshiba_bluetooth_status(device->handle);
>> +     if (status < 0)
>> +             return;
>> +
>> +     pr_info("Switch status %x\n", status & BT_KILLSWITCH_MASK);
>> +     pr_info("Power status %x\n", status & BT_POWER_MASK);
>> +     pr_info("Plug status %x\n", status & BT_PLUGGED_MASK);
>
> This seems a bit noisy for the info level as this information seems more aimed
> at a developer working to complete the TODO below... debug seems more
> appropriate.

Ok, will do, or simply remove them depending on comments below.

>
>> +
>> +     /* TODO: Add event handling here depending on Bluetooth status */
>
> Before if the RFKILL was disabled, this would have called
> toshiba_bluetooth_enable() which would have checked BTST, found it on, and
> enabled the device. In addition to killing the call to
> toshiba_bluetooth_enable() when RFKILL is on, this also removes the call when
> it's switched off.

Well, instead of the TODO, I can add checks for these cases, and that way we
have the same functionality as before, without triggering the enable/disable
loop on recent devices.

>
> I think I'm missing a path. Where does enable occur when the user turns the
> radio back on now?

On the systems tested (even one of mine), it is done internally and a 0x90
event gets generated, and its happening for both cases (enable/disable),
the user flips the KS and the BT device gets detached and powered off,
deactivate the KS and the device gets powered and re-attached.

>
>
>>  }
>>
>>  #ifdef CONFIG_PM_SLEEP
>> --
>> 2.2.2
>>
>>
>
> --
> Darren Hart
> Intel Open Source Technology Center


Cheers
Azael



-- 
-- El mundo apesta y vosotros apestais tambien --

Re: [PATCH 0/4] toshiba_bluetooth: Clean up driver plus a bugfix

[Darren Hart]

On Wed, Mar 18, 2015 at 01:12:54PM -0600, Azael Avalos wrote:
> These patches introduce three new functions, cleaning up the driver
> code, checking for errors and returning accordingly, and also fixes
> bug 93911.
> 
> Azael Avalos (4):
>   toshiba_bluetooth: Add three new functions to the driver
>   toshiba_bluetooth: Clean up *_add function and disable BT device at
>     removal
>   toshiba_bluetooth: Clean toshiba_bluetooth_enable function

These look good.

>   toshiba_bluetooth: Remove BT enable code from *_notify function

Questions on this one separately.

> 
>  drivers/platform/x86/toshiba_bluetooth.c | 133 ++++++++++++++++++++++---------
>  1 file changed, 97 insertions(+), 36 deletions(-)
> 
> -- 
> 2.2.2
> 
> 

-- 
Darren Hart
Intel Open Source Technology Center