* [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier
@ 2021-09-28 21:36 Luiz Augusto von Dentz
2021-09-28 21:36 ` [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry Luiz Augusto von Dentz
` (3 more replies)
0 siblings, 4 replies; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-28 21:36 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This moves code from hci_suspend_notifier to hci_{suspend,resume}_dev
since some driver may handle pm directly using
HCI_QUIRK_NO_SUSPEND_NOTIFIER they would instead call
hci_{suspend,resume}_dev directly and we want that to have the same
behavior regardless of where pm is being handled.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
v2: Clarify why some code is being move into hci_{suspend,resume}_dev and
move debugfs entries to hci_vhci driver so it emulates more closely the
behavior of the driver implementing pm handling and calling the likes of
hci_suspend_dev.
net/bluetooth/hci_core.c | 116 ++++++++++++++++++++++-----------------
1 file changed, 67 insertions(+), 49 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index aeec5a3031a6..ea063ce4d7af 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3626,55 +3626,12 @@ static int hci_suspend_notifier(struct notifier_block *nb, unsigned long action,
struct hci_dev *hdev =
container_of(nb, struct hci_dev, suspend_notifier);
int ret = 0;
- u8 state = BT_RUNNING;
-
- /* If powering down, wait for completion. */
- if (mgmt_powering_down(hdev)) {
- set_bit(SUSPEND_POWERING_DOWN, hdev->suspend_tasks);
- ret = hci_suspend_wait_event(hdev);
- if (ret)
- goto done;
- }
- /* Suspend notifier should only act on events when powered. */
- if (!hdev_is_powered(hdev) ||
- hci_dev_test_flag(hdev, HCI_UNREGISTER))
- goto done;
-
- if (action == PM_SUSPEND_PREPARE) {
- /* Suspend consists of two actions:
- * - First, disconnect everything and make the controller not
- * connectable (disabling scanning)
- * - Second, program event filter/accept list and enable scan
- */
- ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
- if (!ret)
- state = BT_SUSPEND_DISCONNECT;
-
- /* Only configure accept list if disconnect succeeded and wake
- * isn't being prevented.
- */
- if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev))) {
- ret = hci_change_suspend_state(hdev,
- BT_SUSPEND_CONFIGURE_WAKE);
- if (!ret)
- state = BT_SUSPEND_CONFIGURE_WAKE;
- }
+ if (action == PM_SUSPEND_PREPARE)
+ ret = hci_suspend_dev(hdev);
+ else if (action == PM_POST_SUSPEND)
+ ret = hci_resume_dev(hdev);
- hci_clear_wake_reason(hdev);
- mgmt_suspending(hdev, state);
-
- } else if (action == PM_POST_SUSPEND) {
- ret = hci_change_suspend_state(hdev, BT_RUNNING);
-
- mgmt_resuming(hdev, hdev->wake_reason, &hdev->wake_addr,
- hdev->wake_addr_type);
- }
-
-done:
- /* We always allow suspend even if suspend preparation failed and
- * attempt to recover in resume.
- */
if (ret)
bt_dev_err(hdev, "Suspend notifier action (%lu) failed: %d",
action, ret);
@@ -4017,16 +3974,77 @@ EXPORT_SYMBOL(hci_release_dev);
/* Suspend HCI device */
int hci_suspend_dev(struct hci_dev *hdev)
{
+ int ret;
+ u8 state = BT_RUNNING;
+
+ bt_dev_dbg(hdev, "");
+
+ /* Suspend should only act on when powered. */
+ if (!hdev_is_powered(hdev) ||
+ hci_dev_test_flag(hdev, HCI_UNREGISTER))
+ return 0;
+
+ /* If powering down, wait for completion. */
+ if (mgmt_powering_down(hdev)) {
+ set_bit(SUSPEND_POWERING_DOWN, hdev->suspend_tasks);
+ ret = hci_suspend_wait_event(hdev);
+ if (ret)
+ goto done;
+ }
+
+ /* Suspend consists of two actions:
+ * - First, disconnect everything and make the controller not
+ * connectable (disabling scanning)
+ * - Second, program event filter/accept list and enable scan
+ */
+ ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
+ if (!ret)
+ state = BT_SUSPEND_DISCONNECT;
+
+ /* Only configure accept list if disconnect succeeded and wake
+ * isn't being prevented.
+ */
+ if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev))) {
+ ret = hci_change_suspend_state(hdev, BT_SUSPEND_CONFIGURE_WAKE);
+ if (!ret)
+ state = BT_SUSPEND_CONFIGURE_WAKE;
+ }
+
+ hci_clear_wake_reason(hdev);
+ mgmt_suspending(hdev, state);
+
+done:
+ /* We always allow suspend even if suspend preparation failed and
+ * attempt to recover in resume.
+ */
hci_sock_dev_event(hdev, HCI_DEV_SUSPEND);
- return 0;
+ return ret;
}
EXPORT_SYMBOL(hci_suspend_dev);
/* Resume HCI device */
int hci_resume_dev(struct hci_dev *hdev)
{
+ int ret;
+
+ bt_dev_dbg(hdev, "");
+
+ /* Resume should only act on when powered. */
+ if (!hdev_is_powered(hdev) ||
+ hci_dev_test_flag(hdev, HCI_UNREGISTER))
+ return 0;
+
+ /* If powering down don't attempt to resume */
+ if (mgmt_powering_down(hdev))
+ return 0;
+
+ ret = hci_change_suspend_state(hdev, BT_RUNNING);
+
+ mgmt_resuming(hdev, hdev->wake_reason, &hdev->wake_addr,
+ hdev->wake_addr_type);
+
hci_sock_dev_event(hdev, HCI_DEV_RESUME);
- return 0;
+ return ret;
}
EXPORT_SYMBOL(hci_resume_dev);
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry
2021-09-28 21:36 [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier Luiz Augusto von Dentz
@ 2021-09-28 21:36 ` Luiz Augusto von Dentz
2021-09-29 13:51 ` Marcel Holtmann
2021-09-28 21:36 ` [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry Luiz Augusto von Dentz
` (2 subsequent siblings)
3 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-28 21:36 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds force_suspend which can be used to force the controller into
suspend/resume state.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++
1 file changed, 49 insertions(+)
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index cc3679f3491d..52f9106faeae 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -21,6 +21,7 @@
#include <linux/skbuff.h>
#include <linux/miscdevice.h>
+#include <linux/debugfs.h>
#include <net/bluetooth/bluetooth.h>
#include <net/bluetooth/hci_core.h>
@@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type,
return 0;
}
+static ssize_t force_suspend_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct hci_dev *hdev = file->private_data;
+ char buf[3];
+
+ buf[0] = hdev->suspended ? 'Y' : 'N';
+ buf[1] = '\n';
+ buf[2] = '\0';
+ return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t force_suspend_write(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct hci_dev *hdev = file->private_data;
+ bool enable;
+ int err;
+
+ err = kstrtobool_from_user(user_buf, count, &enable);
+ if (err)
+ return err;
+
+ if (hdev->suspended == enable)
+ return -EALREADY;
+
+ if (enable)
+ err = hci_suspend_dev(hdev);
+ else
+ err = hci_resume_dev(hdev);
+
+ if (err)
+ return err;
+
+ return count;
+}
+
+static const struct file_operations force_suspend_fops = {
+ .open = simple_open,
+ .read = force_suspend_read,
+ .write = force_suspend_write,
+ .llseek = default_llseek,
+};
+
static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
{
struct hci_dev *hdev;
@@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
return -EBUSY;
}
+ debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
+ &force_suspend_fops);
+
hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
skb_put_u8(skb, 0xff);
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry
2021-09-28 21:36 [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier Luiz Augusto von Dentz
2021-09-28 21:36 ` [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry Luiz Augusto von Dentz
@ 2021-09-28 21:36 ` Luiz Augusto von Dentz
2021-09-29 13:53 ` Marcel Holtmann
2021-09-28 23:08 ` [v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier bluez.test.bot
2021-09-29 13:55 ` [PATCH v2 1/3] " Marcel Holtmann
3 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-28 21:36 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
This adds force_prevent_wake which can be used to force a certain state
while interacting with force_suspend.
Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
---
drivers/bluetooth/hci_vhci.c | 48 ++++++++++++++++++++++++++++++++++++
1 file changed, 48 insertions(+)
diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
index 52f9106faeae..60a408a49828 100644
--- a/drivers/bluetooth/hci_vhci.c
+++ b/drivers/bluetooth/hci_vhci.c
@@ -137,6 +137,51 @@ static const struct file_operations force_suspend_fops = {
.llseek = default_llseek,
};
+static bool prevent_wake;
+
+static ssize_t force_prevent_wake_read(struct file *file, char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ char buf[3];
+
+ buf[0] = prevent_wake ? 'Y' : 'N';
+ buf[1] = '\n';
+ buf[2] = '\0';
+ return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static bool hci_debugfs_prevent_wake(struct hci_dev *hdev)
+{
+ return prevent_wake;
+}
+
+static ssize_t force_prevent_wake_write(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct hci_dev *hdev = file->private_data;
+ bool enable;
+ int err;
+
+ err = kstrtobool_from_user(user_buf, count, &enable);
+ if (err)
+ return err;
+
+ if (prevent_wake == enable)
+ return -EALREADY;
+
+ hdev->prevent_wake = hci_debugfs_prevent_wake;
+
+ return count;
+}
+
+static const struct file_operations force_prevent_wake_fops = {
+ .open = simple_open,
+ .read = force_prevent_wake_read,
+ .write = force_prevent_wake_write,
+ .llseek = default_llseek,
+};
+
static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
{
struct hci_dev *hdev;
@@ -198,6 +243,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
&force_suspend_fops);
+ debugfs_create_file("force_prevent_wake", 0644, hdev->debugfs, hdev,
+ &force_prevent_wake_fops);
+
hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
skb_put_u8(skb, 0xff);
--
2.31.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* RE: [v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier
2021-09-28 21:36 [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier Luiz Augusto von Dentz
2021-09-28 21:36 ` [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry Luiz Augusto von Dentz
2021-09-28 21:36 ` [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry Luiz Augusto von Dentz
@ 2021-09-28 23:08 ` bluez.test.bot
2021-09-29 13:55 ` [PATCH v2 1/3] " Marcel Holtmann
3 siblings, 0 replies; 10+ messages in thread
From: bluez.test.bot @ 2021-09-28 23:08 UTC (permalink / raw)
To: linux-bluetooth, luiz.dentz
[-- Attachment #1: Type: text/plain, Size: 2072 bytes --]
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=554555
---Test result---
Test Summary:
CheckPatch FAIL 0.75 seconds
GitLint PASS 0.92 seconds
BuildKernel PASS 660.61 seconds
TestRunner: Setup PASS 488.50 seconds
TestRunner: l2cap-tester PASS 10.79 seconds
TestRunner: bnep-tester PASS 5.50 seconds
TestRunner: mgmt-tester FAIL 92.82 seconds
TestRunner: rfcomm-tester PASS 6.83 seconds
TestRunner: sco-tester PASS 7.27 seconds
TestRunner: smp-tester PASS 7.01 seconds
TestRunner: userchan-tester PASS 5.93 seconds
Details
##############################
Test: CheckPatch - FAIL - 0.75 seconds
Run checkpatch.pl script with rule in .checkpatch.conf
[v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier\CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#237: FILE: net/bluetooth/hci_core.c:4044:
+ mgmt_resuming(hdev, hdev->wake_reason, &hdev->wake_addr,
+ hdev->wake_addr_type);
total: 0 errors, 0 warnings, 1 checks, 138 lines checked
NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.
/github/workspace/src/12523893.patch has style problems, please review.
NOTE: Ignored message types: UNKNOWN_COMMIT_ID
NOTE: If any of the errors are false positives, please report
them to the maintainer, see CHECKPATCH in MAINTAINERS.
##############################
Test: TestRunner: mgmt-tester - FAIL - 92.82 seconds
Run test-runner with mgmt-tester
Total: 452, Passed: 451 (99.8%), Failed: 1, Not Run: 0
Failed Test Cases
Read Exp Feature - Success Failed 0.128 seconds
---
Regards,
Linux Bluetooth
[-- Attachment #2: l2cap-tester.log --]
[-- Type: application/octet-stream, Size: 51539 bytes --]
[-- Attachment #3: bnep-tester.log --]
[-- Type: application/octet-stream, Size: 3906 bytes --]
[-- Attachment #4: mgmt-tester.log --]
[-- Type: application/octet-stream, Size: 626722 bytes --]
[-- Attachment #5: rfcomm-tester.log --]
[-- Type: application/octet-stream, Size: 14762 bytes --]
[-- Attachment #6: sco-tester.log --]
[-- Type: application/octet-stream, Size: 13924 bytes --]
[-- Attachment #7: smp-tester.log --]
[-- Type: application/octet-stream, Size: 11830 bytes --]
[-- Attachment #8: userchan-tester.log --]
[-- Type: application/octet-stream, Size: 7740 bytes --]
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry
2021-09-28 21:36 ` [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry Luiz Augusto von Dentz
@ 2021-09-29 13:51 ` Marcel Holtmann
2021-09-29 18:06 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-29 13:51 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This adds force_suspend which can be used to force the controller into
> suspend/resume state.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> index cc3679f3491d..52f9106faeae 100644
> --- a/drivers/bluetooth/hci_vhci.c
> +++ b/drivers/bluetooth/hci_vhci.c
> @@ -21,6 +21,7 @@
>
> #include <linux/skbuff.h>
> #include <linux/miscdevice.h>
> +#include <linux/debugfs.h>
>
> #include <net/bluetooth/bluetooth.h>
> #include <net/bluetooth/hci_core.h>
> @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type,
> return 0;
> }
>
> +static ssize_t force_suspend_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct hci_dev *hdev = file->private_data;
> + char buf[3];
> +
> + buf[0] = hdev->suspended ? 'Y' : 'N';
> + buf[1] = '\n';
> + buf[2] = '\0';
> + return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> +}
> +
> +static ssize_t force_suspend_write(struct file *file,
> + const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct hci_dev *hdev = file->private_data;
> + bool enable;
> + int err;
> +
> + err = kstrtobool_from_user(user_buf, count, &enable);
> + if (err)
> + return err;
> +
> + if (hdev->suspended == enable)
> + return -EALREADY;
I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver.
> +
> + if (enable)
> + err = hci_suspend_dev(hdev);
> + else
> + err = hci_resume_dev(hdev);
> +
> + if (err)
> + return err;
> +
> + return count;
> +}
> +
> +static const struct file_operations force_suspend_fops = {
> + .open = simple_open,
> + .read = force_suspend_read,
> + .write = force_suspend_write,
> + .llseek = default_llseek,
> +};
> +
> static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> {
> struct hci_dev *hdev;
> @@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> return -EBUSY;
> }
>
> + debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
> + &force_suspend_fops);
> +
> hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry
2021-09-28 21:36 ` [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry Luiz Augusto von Dentz
@ 2021-09-29 13:53 ` Marcel Holtmann
2021-09-29 18:07 ` Luiz Augusto von Dentz
0 siblings, 1 reply; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-29 13:53 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This adds force_prevent_wake which can be used to force a certain state
> while interacting with force_suspend.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> drivers/bluetooth/hci_vhci.c | 48 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
>
> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> index 52f9106faeae..60a408a49828 100644
> --- a/drivers/bluetooth/hci_vhci.c
> +++ b/drivers/bluetooth/hci_vhci.c
> @@ -137,6 +137,51 @@ static const struct file_operations force_suspend_fops = {
> .llseek = default_llseek,
> };
>
> +static bool prevent_wake;
this needs to be in vhci_data since it should be per vhci.
> +
> +static ssize_t force_prevent_wake_read(struct file *file, char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + char buf[3];
> +
> + buf[0] = prevent_wake ? 'Y' : 'N';
> + buf[1] = '\n';
> + buf[2] = '\0';
> + return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> +}
> +
> +static bool hci_debugfs_prevent_wake(struct hci_dev *hdev)
> +{
> + return prevent_wake;
> +}
The hci_debugfs prefix here is rather misleading. This is vhci_prevent_wake actually. And just move it to a more closer position with all the other hdev-> callbacks.
> +
> +static ssize_t force_prevent_wake_write(struct file *file,
> + const char __user *user_buf,
> + size_t count, loff_t *ppos)
> +{
> + struct hci_dev *hdev = file->private_data;
> + bool enable;
> + int err;
> +
> + err = kstrtobool_from_user(user_buf, count, &enable);
> + if (err)
> + return err;
> +
> + if (prevent_wake == enable)
> + return -EALREADY;
> +
> + hdev->prevent_wake = hci_debugfs_prevent_wake;
You need to set these with the other hdev-> callback and not in debugfs callback.
> +
> + return count;
> +}
> +
> +static const struct file_operations force_prevent_wake_fops = {
> + .open = simple_open,
> + .read = force_prevent_wake_read,
> + .write = force_prevent_wake_write,
> + .llseek = default_llseek,
> +};
> +
> static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> {
> struct hci_dev *hdev;
> @@ -198,6 +243,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
> &force_suspend_fops);
>
> + debugfs_create_file("force_prevent_wake", 0644, hdev->debugfs, hdev,
> + &force_prevent_wake_fops);
> +
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier
2021-09-28 21:36 [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier Luiz Augusto von Dentz
` (2 preceding siblings ...)
2021-09-28 23:08 ` [v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier bluez.test.bot
@ 2021-09-29 13:55 ` Marcel Holtmann
3 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-29 13:55 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
> This moves code from hci_suspend_notifier to hci_{suspend,resume}_dev
> since some driver may handle pm directly using
> HCI_QUIRK_NO_SUSPEND_NOTIFIER they would instead call
> hci_{suspend,resume}_dev directly and we want that to have the same
> behavior regardless of where pm is being handled.
>
> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> ---
> v2: Clarify why some code is being move into hci_{suspend,resume}_dev and
> move debugfs entries to hci_vhci driver so it emulates more closely the
> behavior of the driver implementing pm handling and calling the likes of
> hci_suspend_dev.
>
> net/bluetooth/hci_core.c | 116 ++++++++++++++++++++++-----------------
> 1 file changed, 67 insertions(+), 49 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry
2021-09-29 13:51 ` Marcel Holtmann
@ 2021-09-29 18:06 ` Luiz Augusto von Dentz
2021-09-29 18:13 ` Marcel Holtmann
0 siblings, 1 reply; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 18:06 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Sep 29, 2021 at 6:51 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds force_suspend which can be used to force the controller into
> > suspend/resume state.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> > index cc3679f3491d..52f9106faeae 100644
> > --- a/drivers/bluetooth/hci_vhci.c
> > +++ b/drivers/bluetooth/hci_vhci.c
> > @@ -21,6 +21,7 @@
> >
> > #include <linux/skbuff.h>
> > #include <linux/miscdevice.h>
> > +#include <linux/debugfs.h>
> >
> > #include <net/bluetooth/bluetooth.h>
> > #include <net/bluetooth/hci_core.h>
> > @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type,
> > return 0;
> > }
> >
> > +static ssize_t force_suspend_read(struct file *file, char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct hci_dev *hdev = file->private_data;
> > + char buf[3];
> > +
> > + buf[0] = hdev->suspended ? 'Y' : 'N';
> > + buf[1] = '\n';
> > + buf[2] = '\0';
> > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> > +}
> > +
> > +static ssize_t force_suspend_write(struct file *file,
> > + const char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct hci_dev *hdev = file->private_data;
> > + bool enable;
> > + int err;
> > +
> > + err = kstrtobool_from_user(user_buf, count, &enable);
> > + if (err)
> > + return err;
> > +
> > + if (hdev->suspended == enable)
> > + return -EALREADY;
>
> I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver.
Ok, but I guess hdev->suspended should be kept as is, right?
>
> > +
> > + if (enable)
> > + err = hci_suspend_dev(hdev);
> > + else
> > + err = hci_resume_dev(hdev);
> > +
> > + if (err)
> > + return err;
> > +
> > + return count;
> > +}
> > +
> > +static const struct file_operations force_suspend_fops = {
> > + .open = simple_open,
> > + .read = force_suspend_read,
> > + .write = force_suspend_write,
> > + .llseek = default_llseek,
> > +};
> > +
> > static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> > {
> > struct hci_dev *hdev;
> > @@ -149,6 +195,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> > return -EBUSY;
> > }
> >
> > + debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
> > + &force_suspend_fops);
> > +
> > hci_skb_pkt_type(skb) = HCI_VENDOR_PKT;
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry
2021-09-29 13:53 ` Marcel Holtmann
@ 2021-09-29 18:07 ` Luiz Augusto von Dentz
0 siblings, 0 replies; 10+ messages in thread
From: Luiz Augusto von Dentz @ 2021-09-29 18:07 UTC (permalink / raw)
To: Marcel Holtmann; +Cc: linux-bluetooth
Hi Marcel,
On Wed, Sep 29, 2021 at 6:53 AM Marcel Holtmann <marcel@holtmann.org> wrote:
>
> Hi Luiz,
>
> > This adds force_prevent_wake which can be used to force a certain state
> > while interacting with force_suspend.
> >
> > Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
> > ---
> > drivers/bluetooth/hci_vhci.c | 48 ++++++++++++++++++++++++++++++++++++
> > 1 file changed, 48 insertions(+)
> >
> > diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
> > index 52f9106faeae..60a408a49828 100644
> > --- a/drivers/bluetooth/hci_vhci.c
> > +++ b/drivers/bluetooth/hci_vhci.c
> > @@ -137,6 +137,51 @@ static const struct file_operations force_suspend_fops = {
> > .llseek = default_llseek,
> > };
> >
> > +static bool prevent_wake;
>
> this needs to be in vhci_data since it should be per vhci.
Ack.
> > +
> > +static ssize_t force_prevent_wake_read(struct file *file, char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + char buf[3];
> > +
> > + buf[0] = prevent_wake ? 'Y' : 'N';
> > + buf[1] = '\n';
> > + buf[2] = '\0';
> > + return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
> > +}
> > +
> > +static bool hci_debugfs_prevent_wake(struct hci_dev *hdev)
> > +{
> > + return prevent_wake;
> > +}
>
> The hci_debugfs prefix here is rather misleading. This is vhci_prevent_wake actually. And just move it to a more closer position with all the other hdev-> callbacks.
Ack.
> > +
> > +static ssize_t force_prevent_wake_write(struct file *file,
> > + const char __user *user_buf,
> > + size_t count, loff_t *ppos)
> > +{
> > + struct hci_dev *hdev = file->private_data;
> > + bool enable;
> > + int err;
> > +
> > + err = kstrtobool_from_user(user_buf, count, &enable);
> > + if (err)
> > + return err;
> > +
> > + if (prevent_wake == enable)
> > + return -EALREADY;
> > +
> > + hdev->prevent_wake = hci_debugfs_prevent_wake;
>
> You need to set these with the other hdev-> callback and not in debugfs callback.
Will do and also change its naming as well.
> > +
> > + return count;
> > +}
> > +
> > +static const struct file_operations force_prevent_wake_fops = {
> > + .open = simple_open,
> > + .read = force_prevent_wake_read,
> > + .write = force_prevent_wake_write,
> > + .llseek = default_llseek,
> > +};
> > +
> > static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> > {
> > struct hci_dev *hdev;
> > @@ -198,6 +243,9 @@ static int __vhci_create_device(struct vhci_data *data, __u8 opcode)
> > debugfs_create_file("force_suspend", 0644, hdev->debugfs, hdev,
> > &force_suspend_fops);
> >
> > + debugfs_create_file("force_prevent_wake", 0644, hdev->debugfs, hdev,
> > + &force_prevent_wake_fops);
> > +
>
> Regards
>
> Marcel
>
--
Luiz Augusto von Dentz
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry
2021-09-29 18:06 ` Luiz Augusto von Dentz
@ 2021-09-29 18:13 ` Marcel Holtmann
0 siblings, 0 replies; 10+ messages in thread
From: Marcel Holtmann @ 2021-09-29 18:13 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
Hi Luiz,
>>> This adds force_suspend which can be used to force the controller into
>>> suspend/resume state.
>>>
>>> Signed-off-by: Luiz Augusto von Dentz <luiz.von.dentz@intel.com>
>>> ---
>>> drivers/bluetooth/hci_vhci.c | 49 ++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/bluetooth/hci_vhci.c b/drivers/bluetooth/hci_vhci.c
>>> index cc3679f3491d..52f9106faeae 100644
>>> --- a/drivers/bluetooth/hci_vhci.c
>>> +++ b/drivers/bluetooth/hci_vhci.c
>>> @@ -21,6 +21,7 @@
>>>
>>> #include <linux/skbuff.h>
>>> #include <linux/miscdevice.h>
>>> +#include <linux/debugfs.h>
>>>
>>> #include <net/bluetooth/bluetooth.h>
>>> #include <net/bluetooth/hci_core.h>
>>> @@ -91,6 +92,51 @@ static int vhci_get_codec_config_data(struct hci_dev *hdev, __u8 type,
>>> return 0;
>>> }
>>>
>>> +static ssize_t force_suspend_read(struct file *file, char __user *user_buf,
>>> + size_t count, loff_t *ppos)
>>> +{
>>> + struct hci_dev *hdev = file->private_data;
>>> + char buf[3];
>>> +
>>> + buf[0] = hdev->suspended ? 'Y' : 'N';
>>> + buf[1] = '\n';
>>> + buf[2] = '\0';
>>> + return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
>>> +}
>>> +
>>> +static ssize_t force_suspend_write(struct file *file,
>>> + const char __user *user_buf,
>>> + size_t count, loff_t *ppos)
>>> +{
>>> + struct hci_dev *hdev = file->private_data;
>>> + bool enable;
>>> + int err;
>>> +
>>> + err = kstrtobool_from_user(user_buf, count, &enable);
>>> + if (err)
>>> + return err;
>>> +
>>> + if (hdev->suspended == enable)
>>> + return -EALREADY;
>>
>> I think that we have to have bool suspended in vhci_data struct here. It needs to be local to the driver.
>
> Ok, but I guess hdev->suspended should be kept as is, right?
yes, that is for the core to handle. The transport driver just needs to not mess or depend on core internals.
Regards
Marcel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-09-29 18:13 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-28 21:36 [PATCH v2 1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier Luiz Augusto von Dentz
2021-09-28 21:36 ` [PATCH v2 2/3] Bluetooth: hci_vhci: Add force_suspend entry Luiz Augusto von Dentz
2021-09-29 13:51 ` Marcel Holtmann
2021-09-29 18:06 ` Luiz Augusto von Dentz
2021-09-29 18:13 ` Marcel Holtmann
2021-09-28 21:36 ` [PATCH v2 3/3] Bluetooth: hci_vhci: Add force_prevent_wake entry Luiz Augusto von Dentz
2021-09-29 13:53 ` Marcel Holtmann
2021-09-29 18:07 ` Luiz Augusto von Dentz
2021-09-28 23:08 ` [v2,1/3] Bluetooth: Make use of hci_{suspend,resume}_dev on suspend notifier bluez.test.bot
2021-09-29 13:55 ` [PATCH v2 1/3] " Marcel Holtmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.