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

[PATCH v2 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.

Changes since v1:
- Removed pr_info calls from *_notify function
- Added checks to the status of the BT device removing the TODO
  comment and retaining the previous functionality of the driver

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 | 139 +++++++++++++++++++++++--------
 1 file changed, 103 insertions(+), 36 deletions(-)

-- 
2.2.2

[PATCH v2 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 v2 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 v2 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 v2 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 acts according to the BT device status, activating the
device only when it is disabled and returning silently if the KS is
activated, this way we retain the previous functionality but without
affecting the newer devices that trigger the enable/disable loop.

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

diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
index 0343d20..94bec3c 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,26 @@ 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);
+	bool killswitch;
+	bool powered;
+	bool plugged;
+	int status;
+
+	/* Query the status of the Bluetooth device */
+	status = toshiba_bluetooth_status(device->handle);
+	if (status < 0)
+		return;
+
+	killswitch = (status & BT_KILLSWITCH_MASK) ? true : false;
+	powered = (status & BT_POWER_MASK) ? true : false;
+	plugged = (status & BT_PLUGGED_MASK) ? true : false;
+
+	/* Verify RFKill switch is set to on, if not, we return silently. */
+	if (!killswitch)
+		return;
+	/* Check if the device is not powered or attached and the KS is off */
+	if (killswitch && (!powered || !plugged))
+		toshiba_bluetooth_enable(device->handle);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.2.2

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

[Darren Hart]

On Wed, Mar 25, 2015 at 02:19:17PM -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.
> 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 acts according to the BT device status, activating the
> device only when it is disabled and returning silently if the KS is
> activated, this way we retain the previous functionality but without
> affecting the newer devices that trigger the enable/disable loop.
> 
> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
> ---
>  drivers/platform/x86/toshiba_bluetooth.c | 26 +++++++++++++++++++++++++-
>  1 file changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
> index 0343d20..94bec3c 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,26 @@ 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);
> +	bool killswitch;
> +	bool powered;
> +	bool plugged;
> +	int status;
> +
> +	/* Query the status of the Bluetooth device */
> +	status = toshiba_bluetooth_status(device->handle);
> +	if (status < 0)
> +		return;
> +
> +	killswitch = (status & BT_KILLSWITCH_MASK) ? true : false;
> +	powered = (status & BT_POWER_MASK) ? true : false;
> +	plugged = (status & BT_PLUGGED_MASK) ? true : false;
> +
> +	/* Verify RFKill switch is set to on, if not, we return silently. */
> +	if (!killswitch)
> +		return;
> +	/* Check if the device is not powered or attached and the KS is off */
> +	if (killswitch && (!powered || !plugged))
> +		toshiba_bluetooth_enable(device->handle);

The second check for killswitch isn't necessary, or it could be a single
conditional.

The difference here from the test in *_enable() is that you ALSO check for
!powered || !plugged, while *_enable() only tests for killswitch.

This set of tests is thus partially redundant with *_enable().

Let's identify how *_enable() is used. There are three cases as I understand it.
I'll describe them as I understand them, correct me if I get something wrong.

1) toshiba_bt_rfkill_add()
2) toshiba_bt_rfkill_notify()
3) toshiba_bt_resume()

This covers the initial scan of the DSDT (and potentially subsequent ones for
hotplug) via "add", it covers rfkill change events, and resume from suspend.

We apparently don't know when the firmware may handle the AUSB and BTPO by
itself and when we need to call it explicitly without testing for the rfkill,
powered, and plugged status explicitly. It seems possible that some systems may
handle this for us at power on or at resume, just as they do for changes to
rfkill.

That suggests to me the checks for rfkill, powered, and plugged states should be
done in the _enable() function itself, rather than at all the call sites.

So... would a better fix be to push these two additional tests into
toshiba_bluetooth_enable() before it calls AUSB and BTPO and then retain the
call the _enable() here?

Thanks,

-- 
Darren Hart
Intel Open Source Technology Center

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

[Azael Avalos]

Hi Darren,

2015-03-25 15:24 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Mar 25, 2015 at 02:19:17PM -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.
>> 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 acts according to the BT device status, activating the
>> device only when it is disabled and returning silently if the KS is
>> activated, this way we retain the previous functionality but without
>> affecting the newer devices that trigger the enable/disable loop.
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>>  drivers/platform/x86/toshiba_bluetooth.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
>> index 0343d20..94bec3c 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,26 @@ 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);
>> +     bool killswitch;
>> +     bool powered;
>> +     bool plugged;
>> +     int status;
>> +
>> +     /* Query the status of the Bluetooth device */
>> +     status = toshiba_bluetooth_status(device->handle);
>> +     if (status < 0)
>> +             return;
>> +
>> +     killswitch = (status & BT_KILLSWITCH_MASK) ? true : false;
>> +     powered = (status & BT_POWER_MASK) ? true : false;
>> +     plugged = (status & BT_PLUGGED_MASK) ? true : false;
>> +
>> +     /* Verify RFKill switch is set to on, if not, we return silently. */
>> +     if (!killswitch)
>> +             return;
>> +     /* Check if the device is not powered or attached and the KS is off */
>> +     if (killswitch && (!powered || !plugged))
>> +             toshiba_bluetooth_enable(device->handle);
>
> The second check for killswitch isn't necessary, or it could be a single
> conditional.

Ok, will remove it, was just added as an extra check.

>
> The difference here from the test in *_enable() is that you ALSO check for
> !powered || !plugged, while *_enable() only tests for killswitch.
>
> This set of tests is thus partially redundant with *_enable().

This second check is added to cover the new and old devices, if we call the
*_enable() function without it, we might end up in the loop as described by
the bug report.

This way we are ensuring to only activate the device whenever its off/detached.

>
> Let's identify how *_enable() is used. There are three cases as I understand it.
> I'll describe them as I understand them, correct me if I get something wrong.
>
> 1) toshiba_bt_rfkill_add()
> 2) toshiba_bt_rfkill_notify()
> 3) toshiba_bt_resume()
>
> This covers the initial scan of the DSDT (and potentially subsequent ones for
> hotplug) via "add", it covers rfkill change events, and resume from suspend.
>
> We apparently don't know when the firmware may handle the AUSB and BTPO by
> itself and when we need to call it explicitly without testing for the rfkill,
> powered, and plugged status explicitly. It seems possible that some systems may
> handle this for us at power on or at resume, just as they do for changes to
> rfkill.

Right, one of my systems (the one I made tests with) handle resume internally,
of course the driver was calling *_enable() but it was doing nothing
since it was
already activated internally.

However, we do need to call *_add() which in turn calls *_enable() otherwise
the device will never be attached/powered at power on, once attached/powered
the system (firmware?) takes care of its status internally.

>
> That suggests to me the checks for rfkill, powered, and plugged states should be
> done in the _enable() function itself, rather than at all the call sites.
>
> So... would a better fix be to push these two additional tests into
> toshiba_bluetooth_enable() before it calls AUSB and BTPO and then retain the
> call the _enable() here?

Sure, I can apply these changes there.

What I wanted was to isolate what each call does as a preparation for
upcoming patches where I will be purging toshiba_acpi from the BT
RFKill code and add it to toshiba_bluetooth (where it belongs).

>
> Thanks,
>
> --
> Darren Hart
> Intel Open Source Technology Center


Cheers
Azael


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

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

[Azael Avalos]

Hi Darren,

2015-03-25 15:24 GMT-06:00 Darren Hart <dvhart@infradead.org>:
> On Wed, Mar 25, 2015 at 02:19:17PM -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.
>> 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 acts according to the BT device status, activating the
>> device only when it is disabled and returning silently if the KS is
>> activated, this way we retain the previous functionality but without
>> affecting the newer devices that trigger the enable/disable loop.
>>
>> Signed-off-by: Azael Avalos <coproscefalo@gmail.com>
>> ---
>>  drivers/platform/x86/toshiba_bluetooth.c | 26 +++++++++++++++++++++++++-
>>  1 file changed, 25 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/platform/x86/toshiba_bluetooth.c b/drivers/platform/x86/toshiba_bluetooth.c
>> index 0343d20..94bec3c 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,26 @@ 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);
>> +     bool killswitch;
>> +     bool powered;
>> +     bool plugged;
>> +     int status;
>> +
>> +     /* Query the status of the Bluetooth device */
>> +     status = toshiba_bluetooth_status(device->handle);
>> +     if (status < 0)
>> +             return;
>> +
>> +     killswitch = (status & BT_KILLSWITCH_MASK) ? true : false;
>> +     powered = (status & BT_POWER_MASK) ? true : false;
>> +     plugged = (status & BT_PLUGGED_MASK) ? true : false;
>> +
>> +     /* Verify RFKill switch is set to on, if not, we return silently. */
>> +     if (!killswitch)
>> +             return;
>> +     /* Check if the device is not powered or attached and the KS is off */
>> +     if (killswitch && (!powered || !plugged))
>> +             toshiba_bluetooth_enable(device->handle);
>
> The second check for killswitch isn't necessary, or it could be a single
> conditional.

Ok, will remove it, was just added as an extra check.

>
> The difference here from the test in *_enable() is that you ALSO check for
> !powered || !plugged, while *_enable() only tests for killswitch.
>
> This set of tests is thus partially redundant with *_enable().

This second check is added to cover the new and old devices, if we call the
*_enable() function without it, we might end up in the loop as described by
the bug report.

This way we are ensuring to only activate the device whenever its off/detached.

>
> Let's identify how *_enable() is used. There are three cases as I understand it.
> I'll describe them as I understand them, correct me if I get something wrong.
>
> 1) toshiba_bt_rfkill_add()
> 2) toshiba_bt_rfkill_notify()
> 3) toshiba_bt_resume()
>
> This covers the initial scan of the DSDT (and potentially subsequent ones for
> hotplug) via "add", it covers rfkill change events, and resume from suspend.
>
> We apparently don't know when the firmware may handle the AUSB and BTPO by
> itself and when we need to call it explicitly without testing for the rfkill,
> powered, and plugged status explicitly. It seems possible that some systems may
> handle this for us at power on or at resume, just as they do for changes to
> rfkill.

Right, one of my systems (the one I made tests with) handle resume internally,
of course the driver was calling *_enable() but it was doing nothing
since it was
already activated internally.

However, we do need to call *_add() which in turn calls *_enable() otherwise
the device will never be attached/powered at power on, once attached/powered
the system (firmware?) takes care of its status internally.

>
> That suggests to me the checks for rfkill, powered, and plugged states should be
> done in the _enable() function itself, rather than at all the call sites.
>
> So... would a better fix be to push these two additional tests into
> toshiba_bluetooth_enable() before it calls AUSB and BTPO and then retain the
> call the _enable() here?

Sure, I can apply these changes there.

What I wanted was to isolate what each call does as a preparation for
upcoming patches where I will be purging toshiba_acpi from the BT
RFKill code and add it to toshiba_bluetooth (where it belongs).

>
> Thanks,
>
> --
> Darren Hart
> Intel Open Source Technology Center


Cheers
Azael


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

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

[Darren Hart]

On Wed, Mar 25, 2015 at 04:18:19PM -0600, Azael Avalos wrote:
> > That suggests to me the checks for rfkill, powered, and plugged states should be
> > done in the _enable() function itself, rather than at all the call sites.
> >
> > So... would a better fix be to push these two additional tests into
> > toshiba_bluetooth_enable() before it calls AUSB and BTPO and then retain the
> > call the _enable() here?
> 
> Sure, I can apply these changes there.
> 
> What I wanted was to isolate what each call does as a preparation for
> upcoming patches where I will be purging toshiba_acpi from the BT
> RFKill code and add it to toshiba_bluetooth (where it belongs).

OK, I'll wait for v3 and pull it in. Thanks Azael.

-- 
Darren Hart
Intel Open Source Technology Center

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

[Darren Hart]

On Wed, Mar 25, 2015 at 04:18:19PM -0600, Azael Avalos wrote:
> > That suggests to me the checks for rfkill, powered, and plugged states should be
> > done in the _enable() function itself, rather than at all the call sites.
> >
> > So... would a better fix be to push these two additional tests into
> > toshiba_bluetooth_enable() before it calls AUSB and BTPO and then retain the
> > call the _enable() here?
> 
> Sure, I can apply these changes there.
> 
> What I wanted was to isolate what each call does as a preparation for
> upcoming patches where I will be purging toshiba_acpi from the BT
> RFKill code and add it to toshiba_bluetooth (where it belongs).

OK, I'll wait for v3 and pull it in. Thanks Azael.

-- 
Darren Hart
Intel Open Source Technology Center