[PATCH] module: fix oops on loading module with malformed ELF section header
th the patch, loading a malformed kernel module no longer
results in the oops.
Signed-off-by: Abhishek Sharma
---
kernel/module/main.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 98fedfdb8db5..39f66bb12683 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1787,15 +1787,6 @@ static int elf_validity_cache_copy(struct load_info
*info, int flags)
i, shdr->sh_type);
return err;
}
- if (strcmp(info->secstrings + shdr->sh_name,
- ".gnu.linkonce.this_module") == 0) {
- num_mod_secs++;
- mod_idx = i;
- } else if (strcmp(info->secstrings + shdr->sh_name,
- ".modinfo") == 0) {
- num_info_secs++;
- info_idx = i;
- }
if (shdr->sh_flags & SHF_ALLOC) {
if (shdr->sh_name >= strhdr->sh_size) {
@@ -1803,6 +1794,15 @@ static int elf_validity_cache_copy(struct load_info
*info, int flags)
i, shdr->sh_type);
return -ENOEXEC;
}
+ if (strcmp(info->secstrings + shdr->sh_name,
+ ".gnu.linkonce.this_module") == 0) {
+ num_mod_secs++;
+ mod_idx = i;
+ } else if (strcmp(info->secstrings +
shdr->sh_name,
+ ".modinfo") == 0) {
+ num_info_secs++;
+ info_idx = i;
+ }
}
break;
}
--
2.40.1
[PATCH] net: wireless: search and hold bss in cfg80211_connect_done
If BSS instance is not provided in __cfg80211_connect_result then
a get bss is performed. This can return NULL if the BSS for the
given SSID is expired due to delayed scheduling of connect result event
in rdev->event_work. This can cause WARN_ON(!cr->bss) in
__cfg80211_connect_result to be triggered and cause cascading
failures. To mitigate this, initiate a get bss call in
cfg80211_connect_done itself and hold it to ensure that the BSS
instance does not get expired.
Signed-off-by: Abhishek Kumar
---
net/wireless/sme.c | 21 +
1 file changed, 13 insertions(+), 8 deletions(-)
diff --git a/net/wireless/sme.c b/net/wireless/sme.c
index 07756ca5e3b5..52f65991accd 100644
--- a/net/wireless/sme.c
+++ b/net/wireless/sme.c
@@ -724,15 +724,8 @@ void __cfg80211_connect_result(struct net_device *dev,
}
#endif
- if (!cr->bss && (cr->status == WLAN_STATUS_SUCCESS)) {
+ if (cr->status == WLAN_STATUS_SUCCESS)
WARN_ON_ONCE(!wiphy_to_rdev(wdev->wiphy)->ops->connect);
- cr->bss = cfg80211_get_bss(wdev->wiphy, NULL, cr->bssid,
- wdev->ssid, wdev->ssid_len,
- wdev->conn_bss_type,
- IEEE80211_PRIVACY_ANY);
- if (cr->bss)
- cfg80211_hold_bss(bss_from_pub(cr->bss));
- }
if (wdev->current_bss) {
cfg80211_unhold_bss(wdev->current_bss);
@@ -882,6 +875,18 @@ void cfg80211_connect_done(struct net_device *dev,
ev->cr.fils.update_erp_next_seq_num =
params->fils.update_erp_next_seq_num;
if (params->fils.update_erp_next_seq_num)
ev->cr.fils.erp_next_seq_num = params->fils.erp_next_seq_num;
+
+ /* Acquire and hold the bss if bss is not provided in argument.
+* This ensures that the BSS does not get expired if the schedule
+* of the rdev->event_work gets delayed.
+*/
+ if (!params->bss && params->bssid)
+ params->bss = cfg80211_get_bss(wdev->wiphy, NULL,
+ params->bssid,
+ wdev->ssid, wdev->ssid_len,
+ wdev->conn_bss_type,
+ IEEE80211_PRIVACY_ANY);
+
if (params->bss)
cfg80211_hold_bss(bss_from_pub(params->bss));
ev->cr.bss = params->bss;
--
2.31.0.rc2.261.g7f71774620-goog
[PATCH] drivers: android: binder.c: Fix indentation of multi-line comment
Fixed alignment of multi-line comment.
Added a * for each line of the comment.
Signed-off-by: Abhishek C
---
drivers/android/binder.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/android/binder.c b/drivers/android/binder.c
index c119736ca56a..700719c58147 100644
--- a/drivers/android/binder.c
+++ b/drivers/android/binder.c
@@ -4617,8 +4617,9 @@ static long binder_ioctl(struct file *filp, unsigned int
cmd, unsigned long arg)
unsigned int size = _IOC_SIZE(cmd);
void __user *ubuf = (void __user *)arg;
- /*pr_info("binder_ioctl: %d:%d %x %lx\n",
- proc->pid, current->pid, cmd, arg);*/
+ /* pr_info("binder_ioctl: %d:%d %x %lx\n",
+* proc->pid, current->pid, cmd, arg);
+*/
binder_selftest_alloc(>alloc);
@@ -5750,8 +5751,8 @@ static int __init binder_init(void)
if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
strcmp(binder_devices_param, "") != 0) {
/*
- * Copy the module_parameter string, because we don't want to
- * tokenize it in-place.
+* Copy the module_parameter string, because we don't want to
+* tokenize it in-place.
*/
device_names = kstrdup(binder_devices_param, GFP_KERNEL);
if (!device_names) {
--
2.25.1
[PATCH v3 1/1] Bluetooth: Remove unneeded commands for suspend
During suspend, there are a few scan enable and set event filter
commands that don't need to be sent unless there are actual BR/EDR
devices capable of waking the system. Check the HCI_PSCAN bit before
writing scan enable and use a new dev flag, HCI_EVENT_FILTER_CONFIGURED
to control whether to clear the event filter.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Archie Pusaka
Reviewed-by: Alain Michaud
---
Changes in v3:
* Minor change to if statement
Changes in v2:
* Removed hci_dev_lock from hci_cc_set_event_filter since flags are
set/cleared atomically
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_event.c | 27 +++
net/bluetooth/hci_request.c | 44 +++--
3 files changed, 55 insertions(+), 17 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ba2f439bc04d34..ea4ae551c42687 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -320,6 +320,7 @@ enum {
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
HCI_WIDEBAND_SPEECH_ENABLED,
+ HCI_EVENT_FILTER_CONFIGURED,
HCI_DUT_MODE,
HCI_VENDOR_DIAG,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 67668be3461e93..f4a734f8a9ac88 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -395,6 +395,29 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev,
struct sk_buff *skb)
hci_dev_unlock(hdev);
}
+static void hci_cc_set_event_filter(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ __u8 status = *((__u8 *)skb->data);
+ struct hci_cp_set_event_filter *cp;
+ void *sent;
+
+ BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+ if (status)
+ return;
+
+ sent = hci_sent_cmd_data(hdev, HCI_OP_SET_EVENT_FLT);
+ if (!sent)
+ return;
+
+ cp = (struct hci_cp_set_event_filter *)sent;
+
+ if (cp->flt_type == HCI_FLT_CLEAR_ALL)
+ hci_dev_clear_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+ else
+ hci_dev_set_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+}
+
static void hci_cc_read_class_of_dev(struct hci_dev *hdev, struct sk_buff *skb)
{
struct hci_rp_read_class_of_dev *rp = (void *) skb->data;
@@ -3328,6 +3351,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev,
struct sk_buff *skb,
hci_cc_write_scan_enable(hdev, skb);
break;
+ case HCI_OP_SET_EVENT_FLT:
+ hci_cc_set_event_filter(hdev, skb);
+ break;
+
case HCI_OP_READ_CLASS_OF_DEV:
hci_cc_read_class_of_dev(hdev, skb);
break;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index e55976db4403e7..75a42178c82d9b 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1131,14 +1131,14 @@ static void hci_req_clear_event_filter(struct
hci_request *req)
{
struct hci_cp_set_event_filter f;
- memset(, 0, sizeof(f));
- f.flt_type = HCI_FLT_CLEAR_ALL;
- hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED))
+ return;
- /* Update page scan state (since we may have modified it when setting
-* the event filter).
-*/
- __hci_req_update_scan(req);
+ if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) {
+ memset(, 0, sizeof(f));
+ f.flt_type = HCI_FLT_CLEAR_ALL;
+ hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ }
}
static void hci_req_set_event_filter(struct hci_request *req)
@@ -1147,6 +1147,10 @@ static void hci_req_set_event_filter(struct hci_request
*req)
struct hci_cp_set_event_filter f;
struct hci_dev *hdev = req->hdev;
u8 scan = SCAN_DISABLED;
+ bool scanning = test_bit(HCI_PSCAN, >flags);
+
+ if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
+ return;
/* Always clear event filter when starting */
hci_req_clear_event_filter(req);
@@ -1167,12 +1171,13 @@ static void hci_req_set_event_filter(struct hci_request
*req)
scan = SCAN_PAGE;
}
- if (scan)
+ if (scan && !scanning) {
set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
- else
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ } else if (!scan && scanning) {
set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
-
- hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ }
}
static void cancel_adv_timeout(struct hci_dev *hdev)
@@ -1315,9 +1320,14 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum
suspended_state next)
hdev->advertising_paused = tr
[PATCH v3 0/1] Bluetooth: Suspend improvements
Hi Marcel (and linux bluetooth), Here are a few suspend improvements based on user reports we saw on ChromeOS and feedback from Hans de Goede on the mailing list. I have tested this using our ChromeOS suspend/resume automated tests (full SRHealth test coverage and some suspend resume stress tests). Thanks Abhishek Changes in v3: * Minor change to if statement Changes in v2: * Removed hci_dev_lock from hci_cc_set_event_filter since flags are set/cleared atomically Abhishek Pandit-Subedi (1): Bluetooth: Remove unneeded commands for suspend include/net/bluetooth/hci.h | 1 + net/bluetooth/hci_event.c | 27 +++ net/bluetooth/hci_request.c | 44 +++-- 3 files changed, 55 insertions(+), 17 deletions(-) -- 2.31.0.rc0.254.gbdcc3b1a9d-goog
[PATCH v2 0/1] Bluetooth: Suspend improvements
Hi Marcel (and linux bluetooth), Here are a few suspend improvements based on user reports we saw on ChromeOS and feedback from Hans de Goede on the mailing list. I have tested this using our ChromeOS suspend/resume automated tests (full SRHealth test coverage and some suspend resume stress tests). Thanks Abhishek Changes in v2: * Removed hci_dev_lock from hci_cc_set_event_filter since flags are set/cleared atomically Abhishek Pandit-Subedi (1): Bluetooth: Remove unneeded commands for suspend include/net/bluetooth/hci.h | 1 + net/bluetooth/hci_event.c | 24 net/bluetooth/hci_request.c | 44 +++-- 3 files changed, 52 insertions(+), 17 deletions(-) -- 2.30.1.766.gb4fecdf3b7-goog
[PATCH v2 1/1] Bluetooth: Remove unneeded commands for suspend
During suspend, there are a few scan enable and set event filter
commands that don't need to be sent unless there are actual BR/EDR
devices capable of waking the system. Check the HCI_PSCAN bit before
writing scan enable and use a new dev flag, HCI_EVENT_FILTER_CONFIGURED
to control whether to clear the event filter.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Archie Pusaka
Reviewed-by: Alain Michaud
---
Changes in v2:
* Removed hci_dev_lock from hci_cc_set_event_filter since flags are
set/cleared atomically
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_event.c | 24
net/bluetooth/hci_request.c | 44 +++--
3 files changed, 52 insertions(+), 17 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ba2f439bc04d34..ea4ae551c42687 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -320,6 +320,7 @@ enum {
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
HCI_WIDEBAND_SPEECH_ENABLED,
+ HCI_EVENT_FILTER_CONFIGURED,
HCI_DUT_MODE,
HCI_VENDOR_DIAG,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 67668be3461e93..6eadc999ea1474 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -395,6 +395,26 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev,
struct sk_buff *skb)
hci_dev_unlock(hdev);
}
+static void hci_cc_set_event_filter(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ __u8 status = *((__u8 *)skb->data);
+ struct hci_cp_set_event_filter *cp;
+ void *sent;
+
+ BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+ sent = hci_sent_cmd_data(hdev, HCI_OP_SET_EVENT_FLT);
+ if (!sent || status)
+ return;
+
+ cp = (struct hci_cp_set_event_filter *)sent;
+
+ if (cp->flt_type == HCI_FLT_CLEAR_ALL)
+ hci_dev_clear_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+ else
+ hci_dev_set_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+}
+
static void hci_cc_read_class_of_dev(struct hci_dev *hdev, struct sk_buff *skb)
{
struct hci_rp_read_class_of_dev *rp = (void *) skb->data;
@@ -3328,6 +3348,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev,
struct sk_buff *skb,
hci_cc_write_scan_enable(hdev, skb);
break;
+ case HCI_OP_SET_EVENT_FLT:
+ hci_cc_set_event_filter(hdev, skb);
+ break;
+
case HCI_OP_READ_CLASS_OF_DEV:
hci_cc_read_class_of_dev(hdev, skb);
break;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index e55976db4403e7..75a42178c82d9b 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1131,14 +1131,14 @@ static void hci_req_clear_event_filter(struct
hci_request *req)
{
struct hci_cp_set_event_filter f;
- memset(, 0, sizeof(f));
- f.flt_type = HCI_FLT_CLEAR_ALL;
- hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED))
+ return;
- /* Update page scan state (since we may have modified it when setting
-* the event filter).
-*/
- __hci_req_update_scan(req);
+ if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) {
+ memset(, 0, sizeof(f));
+ f.flt_type = HCI_FLT_CLEAR_ALL;
+ hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ }
}
static void hci_req_set_event_filter(struct hci_request *req)
@@ -1147,6 +1147,10 @@ static void hci_req_set_event_filter(struct hci_request
*req)
struct hci_cp_set_event_filter f;
struct hci_dev *hdev = req->hdev;
u8 scan = SCAN_DISABLED;
+ bool scanning = test_bit(HCI_PSCAN, >flags);
+
+ if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
+ return;
/* Always clear event filter when starting */
hci_req_clear_event_filter(req);
@@ -1167,12 +1171,13 @@ static void hci_req_set_event_filter(struct hci_request
*req)
scan = SCAN_PAGE;
}
- if (scan)
+ if (scan && !scanning) {
set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
- else
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ } else if (!scan && scanning) {
set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
-
- hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ }
}
static void cancel_adv_timeout(struct hci_dev *hdev)
@@ -1315,9 +1320,14 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum
suspended_state next)
hdev->advertising_paused = true;
hdev->advertising_old_state = old_state;
- /*
Re: [PATCH v3] ath10k: skip the wait for completion to recovery in shutdown path
This patch seems to address the comments on v2. Overall this patch LGTM.
Reviewed-by: Abhishek Kumar
On Tue, Feb 23, 2021 at 6:29 AM Youghandhar Chintala
wrote:
>
> Currently in the shutdown callback we wait for recovery to complete
> before freeing up the resources. This results in additional two seconds
> delay during the shutdown and thereby increase the shutdown time.
>
> As an attempt to take less time during shutdown, remove the wait for
> recovery completion in the shutdown callback and added an API to freeing
> the reosurces in which they were common for shutdown and removing
> the module.
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
>
> Signed-off-by: Youghandhar Chintala
> Change-Id: I65bc27b5adae1fedc7f7b367ef13aafbd01f8c0c
> ---
> Changes from v2:
> -Corrected commit text and added common API for freeing the
> resources for shutdown and unloading the module
> ---
> drivers/net/wireless/ath/ath10k/snoc.c | 29 ++
> 1 file changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/snoc.c
> b/drivers/net/wireless/ath/ath10k/snoc.c
> index 84666f72bdfa..70b3f2bd1c81 100644
> --- a/drivers/net/wireless/ath/ath10k/snoc.c
> +++ b/drivers/net/wireless/ath/ath10k/snoc.c
> @@ -1781,17 +1781,11 @@ static int ath10k_snoc_probe(struct platform_device
> *pdev)
> return ret;
> }
>
> -static int ath10k_snoc_remove(struct platform_device *pdev)
> +static int ath10k_snoc_free_resources(struct ath10k *ar)
> {
> - struct ath10k *ar = platform_get_drvdata(pdev);
> struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
>
> - ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> -
> - reinit_completion(>driver_recovery);
> -
> - if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, _snoc->flags))
> - wait_for_completion_timeout(>driver_recovery, 3 * HZ);
> + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc free resources\n");
>
> set_bit(ATH10K_SNOC_FLAG_UNREGISTERING, _snoc->flags);
>
> @@ -1805,12 +1799,29 @@ static int ath10k_snoc_remove(struct platform_device
> *pdev)
> return 0;
> }
>
> +static int ath10k_snoc_remove(struct platform_device *pdev)
> +{
> + struct ath10k *ar = platform_get_drvdata(pdev);
> + struct ath10k_snoc *ar_snoc = ath10k_snoc_priv(ar);
> +
> + ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc remove\n");
> +
> + reinit_completion(>driver_recovery);
> +
> + if (test_bit(ATH10K_SNOC_FLAG_RECOVERY, _snoc->flags))
> + wait_for_completion_timeout(>driver_recovery, 3 * HZ);
> +
> + ath10k_snoc_free_resources(ar);
> +
> + return 0;
> +}
> +
> static void ath10k_snoc_shutdown(struct platform_device *pdev)
> {
> struct ath10k *ar = platform_get_drvdata(pdev);
>
> ath10k_dbg(ar, ATH10K_DBG_SNOC, "snoc shutdown\n");
> - ath10k_snoc_remove(pdev);
> + ath10k_snoc_free_resources(ar);
> }
>
> static struct platform_driver ath10k_snoc_driver = {
> --
> 2.29.0
>
[PATCH 2/2] Bluetooth: Remove unneeded commands for suspend
During suspend, there are a few scan enable and set event filter
commands that don't need to be sent unless there are actual BR/EDR
devices capable of waking the system. Check the HCI_PSCAN bit before
writing scan enable and use a new dev flag, HCI_EVENT_FILTER_CONFIGURED
to control whether to clear the event filter.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Archie Pusaka
Reviewed-by: Alain Michaud
---
include/net/bluetooth/hci.h | 1 +
net/bluetooth/hci_event.c | 31 ++
net/bluetooth/hci_request.c | 44 +++--
3 files changed, 59 insertions(+), 17 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index ba2f439bc04d34..ea4ae551c42687 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -320,6 +320,7 @@ enum {
HCI_BREDR_ENABLED,
HCI_LE_SCAN_INTERRUPTED,
HCI_WIDEBAND_SPEECH_ENABLED,
+ HCI_EVENT_FILTER_CONFIGURED,
HCI_DUT_MODE,
HCI_VENDOR_DIAG,
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 67668be3461e93..17847e672b98cf 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -395,6 +395,33 @@ static void hci_cc_write_scan_enable(struct hci_dev *hdev,
struct sk_buff *skb)
hci_dev_unlock(hdev);
}
+static void hci_cc_set_event_filter(struct hci_dev *hdev, struct sk_buff *skb)
+{
+ __u8 status = *((__u8 *)skb->data);
+ struct hci_cp_set_event_filter *cp;
+ void *sent;
+
+ BT_DBG("%s status 0x%2.2x", hdev->name, status);
+
+ sent = hci_sent_cmd_data(hdev, HCI_OP_SET_EVENT_FLT);
+ if (!sent)
+ return;
+
+ cp = (struct hci_cp_set_event_filter *)sent;
+
+ hci_dev_lock(hdev);
+
+ if (status)
+ goto done;
+
+ if (cp->flt_type == HCI_FLT_CLEAR_ALL)
+ hci_dev_clear_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+ else
+ hci_dev_set_flag(hdev, HCI_EVENT_FILTER_CONFIGURED);
+done:
+ hci_dev_unlock(hdev);
+}
+
static void hci_cc_read_class_of_dev(struct hci_dev *hdev, struct sk_buff *skb)
{
struct hci_rp_read_class_of_dev *rp = (void *) skb->data;
@@ -3328,6 +3355,10 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev,
struct sk_buff *skb,
hci_cc_write_scan_enable(hdev, skb);
break;
+ case HCI_OP_SET_EVENT_FLT:
+ hci_cc_set_event_filter(hdev, skb);
+ break;
+
case HCI_OP_READ_CLASS_OF_DEV:
hci_cc_read_class_of_dev(hdev, skb);
break;
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index e55976db4403e7..75a42178c82d9b 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1131,14 +1131,14 @@ static void hci_req_clear_event_filter(struct
hci_request *req)
{
struct hci_cp_set_event_filter f;
- memset(, 0, sizeof(f));
- f.flt_type = HCI_FLT_CLEAR_ALL;
- hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ if (!hci_dev_test_flag(req->hdev, HCI_BREDR_ENABLED))
+ return;
- /* Update page scan state (since we may have modified it when setting
-* the event filter).
-*/
- __hci_req_update_scan(req);
+ if (hci_dev_test_flag(req->hdev, HCI_EVENT_FILTER_CONFIGURED)) {
+ memset(, 0, sizeof(f));
+ f.flt_type = HCI_FLT_CLEAR_ALL;
+ hci_req_add(req, HCI_OP_SET_EVENT_FLT, 1, );
+ }
}
static void hci_req_set_event_filter(struct hci_request *req)
@@ -1147,6 +1147,10 @@ static void hci_req_set_event_filter(struct hci_request
*req)
struct hci_cp_set_event_filter f;
struct hci_dev *hdev = req->hdev;
u8 scan = SCAN_DISABLED;
+ bool scanning = test_bit(HCI_PSCAN, >flags);
+
+ if (!hci_dev_test_flag(hdev, HCI_BREDR_ENABLED))
+ return;
/* Always clear event filter when starting */
hci_req_clear_event_filter(req);
@@ -1167,12 +1171,13 @@ static void hci_req_set_event_filter(struct hci_request
*req)
scan = SCAN_PAGE;
}
- if (scan)
+ if (scan && !scanning) {
set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
- else
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ } else if (!scan && scanning) {
set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
-
- hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ }
}
static void cancel_adv_timeout(struct hci_dev *hdev)
@@ -1315,9 +1320,14 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum
suspended_state next)
hdev->advertising_paused = true;
hdev->advertising_old_state = old_state;
- /*
[PATCH 0/2] Bluetooth: Suspend improvements
Hi Marcel (and linux bluetooth), Here are a few suspend improvements based on user reports we saw on ChromeOS and feedback from Hans de Goede on the mailing list. I have tested this using our ChromeOS suspend/resume automated tests (full SRHealth test coverage and some suspend resume stress tests). Thanks Abhishek Abhishek Pandit-Subedi (2): Bluetooth: Notify suspend on le conn failed Bluetooth: Remove unneeded commands for suspend include/net/bluetooth/hci.h | 1 + net/bluetooth/hci_conn.c| 10 + net/bluetooth/hci_event.c | 31 ++ net/bluetooth/hci_request.c | 44 +++-- 4 files changed, 69 insertions(+), 17 deletions(-) -- 2.30.1.766.gb4fecdf3b7-goog
[PATCH 1/2] Bluetooth: Notify suspend on le conn failed
When suspending, Bluetooth disconnects all connected peers devices. If
an LE connection is started but isn't completed, we will see an LE
Create Connection Cancel instead of an HCI disconnect. This just adds
a check to see if an LE cancel was the last disconnected device and wake
the suspend thread when that is the case.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Archie Pusaka
---
Here is an HCI trace when the issue occurred.
< HCI Command: LE Create Connection (0x08|0x000d) plen 25
#18 [hci0] 2021-02-03 21:42:35.130208
Scan interval: 60.000 msec (0x0060)
Scan window: 60.000 msec (0x0060)
Filter policy: White list is not used (0x00)
Peer address type: Random (0x01)
Peer address: D9:DC:6B:61:EB:3A (Static)
Own address type: Public (0x00)
Min connection interval: 15.00 msec (0x000c)
Max connection interval: 30.00 msec (0x0018)
Connection latency: 20 (0x0014)
Supervision timeout: 3000 msec (0x012c)
Min connection length: 0.000 msec (0x)
Max connection length: 0.000 msec (0x)
> HCI Event: Command Status (0x0f) plen 4
> #187778 [hci0] 2021-02-03 21:42:35.131184
LE Create Connection (0x08|0x000d) ncmd 1
Status: Success (0x00)
< HCI Command: LE Create Connection Cancel (0x08|0x000e) plen 0
#187805 [hci0] 2021-02-03 21:42:37.183336
> HCI Event: Command Complete (0x0e) plen 4
> #187806 [hci0] 2021-02-03 21:42:37.192394
LE Create Connection Cancel (0x08|0x000e) ncmd 1
Status: Success (0x00)
> HCI Event: LE Meta Event (0x3e) plen 19
> #187807 [hci0] 2021-02-03 21:42:37.193400
LE Connection Complete (0x01)
Status: Unknown Connection Identifier (0x02)
Handle: 0
Role: Master (0x00)
Peer address type: Random (0x01)
Peer address: D9:DC:6B:61:EB:3A (Static)
Connection interval: 0.00 msec (0x)
Connection latency: 0 (0x)
Supervision timeout: 0 msec (0x)
Master clock accuracy: 0x00
...
@ MGMT Event: Controller Suspended (0x002d) plen 1
{0x0002} [hci0] 2021-02-03 21:42:39.178780
Suspend state: Controller running (failed to suspend) (0)
@ MGMT Event: Controller Suspended (0x002d) plen 1
{0x0001} [hci0] 2021-02-03 21:42:39.178780
Suspend state: Controller running (failed to suspend) (0)
...
< HCI Command: Set Event Filter (0x03|0x0005) plen 1
#187808 [hci0] 2021-02-04 09:23:07.313591
Type: Clear All Filters (0x00)
net/bluetooth/hci_conn.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/net/bluetooth/hci_conn.c b/net/bluetooth/hci_conn.c
index 6ffa89e3ba0a85..468d31f3303d7a 100644
--- a/net/bluetooth/hci_conn.c
+++ b/net/bluetooth/hci_conn.c
@@ -772,6 +772,16 @@ void hci_le_conn_failed(struct hci_conn *conn, u8 status)
hci_conn_del(conn);
+ /* The suspend notifier is waiting for all devices to disconnect and an
+* LE connect cancel will result in an hci_le_conn_failed. Once the last
+* connection is deleted, we should also wake the suspend queue to
+* complete suspend operations.
+*/
+ if (list_empty(>conn_hash.list) &&
+ test_and_clear_bit(SUSPEND_DISCONNECTING, hdev->suspend_tasks)) {
+ wake_up(>suspend_wait_q);
+ }
+
/* Since we may have temporarily stopped the background scanning in
* favor of connection establishment, we should restart it.
*/
--
2.30.1.766.gb4fecdf3b7-goog
Re: [PATCH 1/3] cfg80211: Add wiphy flag to trigger STA disconnect after hardware restart
LGTM! If no one has any objections, I would be happy to see this
considered for upstream.
Reviewed-by: Abhishek Kumar
Thanks
Abhishek
On Tue, Dec 15, 2020 at 9:30 AM Youghandhar Chintala
wrote:
>
> Many wifi drivers (e.g. ath10k using qualcomm wifi chipsets)
> support silent target hardware restart/recovery. Out of these
> drivers which support target hw restart, certain chipsets
> have the wifi mac sequence number addition for transmitted
> frames done by the firmware. For such chipsets, a silent
> target hardware restart breaks the continuity of the wifi
> mac sequence number, since the wifi mac sequence number
> restarts from 0 after the restart, which in-turn leads
> to the peer access point dropping all the frames from device
> until it receives the frame with the mac sequence which was
> expected by the AP.
>
> Add a wiphy flag for the driver to indicate that it needs a
> trigger for STA disconnect after hardware restart.
>
> Tested on ath10k using WCN3990, QCA6174.
>
> Signed-off-by: Youghandhar Chintala
> ---
> include/net/cfg80211.h | 4
> 1 file changed, 4 insertions(+)
>
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index ab249ca..7fba6f6 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -4311,6 +4311,9 @@ struct cfg80211_ops {
> * @WIPHY_FLAG_HAS_STATIC_WEP: The device supports static WEP key
> installation
> * before connection.
> * @WIPHY_FLAG_SUPPORTS_EXT_KEK_KCK: The device supports bigger kek and kck
> keys
> + * @WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART: The device needs a trigger to
> + * disconnect STA after target hardware restart. This flag should be
> + * exposed by drivers which support target recovery.
> */
> enum wiphy_flags {
> WIPHY_FLAG_SUPPORTS_EXT_KEK_KCK = BIT(0),
> @@ -4337,6 +4340,7 @@ enum wiphy_flags {
> WIPHY_FLAG_SUPPORTS_5_10_MHZ= BIT(22),
> WIPHY_FLAG_HAS_CHANNEL_SWITCH = BIT(23),
> WIPHY_FLAG_HAS_STATIC_WEP = BIT(24),
> + WIPHY_FLAG_STA_DISCONNECT_ON_HW_RESTART = BIT(25),
> };
>
> /**
> --
> 2.7.4
>
Re: [PATCH 3/3] ath10k: Set wiphy flag to trigger sta disconnect on hardware restart
> I'm not sure what you mean. But if you are saying that we should move > ath10k_hw_params_list entirely to firmware then that is a huge task as > we would need to make changes in every firmware branch, and there are so > many different branches that I have lost count. And due to backwards > compatibility we still need to have ath10k_hw_params_list in ath10k for > few years. > Apologies for late reply on this thread. Yes you got me right( to move ath10k_hw_params_list entirely to firmware ). I wanted to trigger an idea and know what other people's views are, or atleast what are the challenges. As you said the task is much bigger with so many firmware branches. As long as you feel it is scalable for coming years, we should be good. As far as the patch is concerned, it LGTM. Reviewed-by: Abhishek Kumar Thanks Abhishek
Re: [PATCH 2/3] mac80211: Add support to trigger sta disconnect on hardware restart
Since using DELBA frame to APs to re-establish BA session has a dependency on APs and also some APs may not honour the DELBA frame. I am fine with having the disconnect/reconnect solution. The change looks good to me. Reviewed-by: Abhishek Kumar Thanks Abhishek On Thu, Jan 28, 2021 at 12:08 AM wrote: > > On 2020-12-15 23:10, Felix Fietkau wrote: > > On 2020-12-15 18:23, Youghandhar Chintala wrote: > >> Currently in case of target hardware restart, we just reconfig and > >> re-enable the security keys and enable the network queues to start > >> data traffic back from where it was interrupted. > >> > >> Many ath10k wifi chipsets have sequence numbers for the data > >> packets assigned by firmware and the mac sequence number will > >> restart from zero after target hardware restart leading to mismatch > >> in the sequence number expected by the remote peer vs the sequence > >> number of the frame sent by the target firmware. > >> > >> This mismatch in sequence number will cause out-of-order packets > >> on the remote peer and all the frames sent by the device are dropped > >> until we reach the sequence number which was sent before we restarted > >> the target hardware > >> > >> In order to fix this, we trigger a sta disconnect, for the targets > >> which expose this corresponding wiphy flag, in case of target hw > >> restart. After this there will be a fresh connection and thereby > >> avoiding the dropping of frames by remote peer. > >> > >> The right fix would be to pull the entire data path into the host > >> which is not feasible or would need lots of complex changes and > >> will still be inefficient. > > How about simply tracking which tids have aggregation enabled and send > > DELBA frames for those after the restart? > > It would mean less disruption for affected stations and less ugly hacks > > in the stack for unreliable hardware. > > > > - Felix > > Hi Felix, > > We did try to send an ADDBA frame to the AP once the SSR happened. The > AP ack’ed the frame and the new BA session with renewed sequence number > was established. But still, the AP did not respond to the ping requests > with the new sequence number. It did not respond until one of the two > happened. > 1. The sequence number was more than the sequence number that DUT had > used before SSR happened > 2. DUT disconnected and then reconnected. > The other option is to send a DELBA frame to the AP and make the AP also > force to establish the BA session from its side. This we feel can have > some interoperability issues as some of the AP’s may not honour the > DELBA frame and will continue to use the earlier BA session that it had > established. Given that re-negotiating the BA session is prone to IOT > issues, we feel that it would be good to go with the > Disconnect/Reconnect solution which is foolproof and will work in all > scenarios. > > Regards, > Youghandhar
Re: [PATCH v3] Bluetooth: btrtl: Enable WBS for the specific Realtek devices
Thanks Max. Patch v3 looks good to me.
Reviewed-by: Abhishek Pandit-Subedi
On Thu, Jan 21, 2021 at 5:59 PM wrote:
>
> From: Max Chou
>
> By this change, it will enable WBS supported on the specific Realtek BT
> devices, such as RTL8822C and RTL8852A.
> In the future, it's able to maintain what the Realtek devices support WBS
> here.
>
> Tested-by: Hilda Wu
> Reviewed-by: Abhishek Pandit-Subedi
> Signed-off-by: Max Chou
>
> ---
> change in v3
> -remove the null check due to unnecessary
> ---
> drivers/bluetooth/btrtl.c | 29 +++--
> 1 file changed, 23 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 24f03a1f8d57..a21d6abc93c4 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,6 +38,19 @@
> .hci_ver = (hciv), \
> .hci_bus = (bus)
>
> +enum btrtl_chip_id {
> + CHIP_ID_8723A, /* index 0 for RTL8723A*/
> + CHIP_ID_8723B, /* index 1 for RTL8723B*/
> + CHIP_ID_8821A, /* index 2 for RTL8821A*/
> + CHIP_ID_8761A, /* index 3 for RTL8761A*/
> + CHIP_ID_8822B = 8, /* index 8 for RTL8822B */
> + CHIP_ID_8723D, /* index 9 for RTL8723D */
> + CHIP_ID_8821C, /* index 10 for RTL8821C */
> + CHIP_ID_8822C = 13, /* index 13 for RTL8822C */
> + CHIP_ID_8761B, /* index 14 for RTL8761B */
> + CHIP_ID_8852A = 18, /* index 18 for RTL8852A */
> +};
> +
> struct id_table {
> __u16 match_flags;
> __u16 lmp_subver;
> @@ -58,6 +71,7 @@ struct btrtl_device_info {
> u8 *cfg_data;
> int cfg_len;
> bool drop_fw;
> + int project_id;
> };
>
> static const struct id_table ic_id_table[] = {
> @@ -307,8 +321,10 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev,
>
> /* Find project_id in table */
> for (i = 0; i < ARRAY_SIZE(project_id_to_lmp_subver); i++) {
> - if (project_id == project_id_to_lmp_subver[i].id)
> + if (project_id == project_id_to_lmp_subver[i].id) {
> + btrtl_dev->project_id = project_id;
> break;
> + }
> }
>
> if (i >= ARRAY_SIZE(project_id_to_lmp_subver)) {
> @@ -719,18 +735,19 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> */
> set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
>
> - if (!btrtl_dev->ic_info)
> - goto done;
> -
> /* Enable central-peripheral role (able to create new connections with
> * an existing connection in slave role).
> */
> - switch (btrtl_dev->ic_info->lmp_subver) {
> - case RTL_ROM_LMP_8822B:
> + /* Enable WBS supported for the specific Realtek devices. */
> + switch (btrtl_dev->project_id) {
> + case CHIP_ID_8822C:
> + case CHIP_ID_8852A:
> set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
> + set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, >quirks);
> break;
> default:
> rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
> + rtl_dev_dbg(hdev, "WBS supported not enabled.");
> break;
> }
>
> --
> 2.17.1
>
Re: [PATCH v2] Bluetooth: btrtl: Enable WBS for the specific Realtek devices
Hi Max,
On Mon, Jan 18, 2021 at 10:07 PM wrote:
>
> From: Max Chou
>
> By this change, it will enable WBS supported on the specific Realtek BT
> devices, such as RTL8822C and RTL8852A.
> In the future, it's able to maintain what the Realtek devices support WBS
> here.
>
> Tested-by: Hilda Wu
> Reviewed-by: Abhishek Pandit-Subedi
> Signed-off-by: Max Chou
>
> ---
> v2:
> - edit the null check
> ---
> drivers/bluetooth/btrtl.c | 28
> 1 file changed, 24 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 24f03a1f8d57..363e23b15faf 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,6 +38,19 @@
> .hci_ver = (hciv), \
> .hci_bus = (bus)
>
> +enum btrtl_chip_id {
> + CHIP_ID_8723A, /* index 0 for RTL8723A*/
> + CHIP_ID_8723B, /* index 1 for RTL8723B*/
> + CHIP_ID_8821A, /* index 2 for RTL8821A*/
> + CHIP_ID_8761A, /* index 3 for RTL8761A*/
> + CHIP_ID_8822B = 8, /* index 8 for RTL8822B */
> + CHIP_ID_8723D, /* index 9 for RTL8723D */
> + CHIP_ID_8821C, /* index 10 for RTL8821C */
> + CHIP_ID_8822C = 13, /* index 13 for RTL8822C */
> + CHIP_ID_8761B, /* index 14 for RTL8761B */
> + CHIP_ID_8852A = 18, /* index 18 for RTL8852A */
> +};
> +
> struct id_table {
> __u16 match_flags;
> __u16 lmp_subver;
> @@ -58,6 +71,7 @@ struct btrtl_device_info {
> u8 *cfg_data;
> int cfg_len;
> bool drop_fw;
> + int project_id;
> };
>
> static const struct id_table ic_id_table[] = {
> @@ -307,8 +321,10 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev,
>
> /* Find project_id in table */
> for (i = 0; i < ARRAY_SIZE(project_id_to_lmp_subver); i++) {
> - if (project_id == project_id_to_lmp_subver[i].id)
> + if (project_id == project_id_to_lmp_subver[i].id) {
> + btrtl_dev->project_id = project_id;
> break;
> + }
> }
>
> if (i >= ARRAY_SIZE(project_id_to_lmp_subver)) {
> @@ -719,18 +735,22 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> */
> set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
>
> - if (!btrtl_dev->ic_info)
> + if (!btrtl_dev->project_id)
> goto done;
Since btrtl_dev->project_id is not a pointer, this check should not be
necessary. The switch below has a default case that prints when
project_id = 0.
>
> /* Enable central-peripheral role (able to create new connections with
> * an existing connection in slave role).
> */
> - switch (btrtl_dev->ic_info->lmp_subver) {
> - case RTL_ROM_LMP_8822B:
> + /* Enable WBS supported for the specific Realtek devices. */
> + switch (btrtl_dev->project_id) {
> + case CHIP_ID_8822C:
> + case CHIP_ID_8852A:
> set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
> + set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, >quirks);
> break;
> default:
> rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
> + rtl_dev_dbg(hdev, "WBS supported not enabled.");
> break;
> }
>
> --
> 2.17.1
>
[PATCH 1/3] Bluetooth: btusb: Refactor gpio reset
Refactor gpio reset to use a common gpio reset function.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
drivers/bluetooth/btusb.c | 59 +--
1 file changed, 19 insertions(+), 40 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index b14102fba6018..03341e6cbf3ed 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -547,6 +547,7 @@ struct btusb_data {
struct usb_endpoint_descriptor *diag_rx_ep;
struct gpio_desc *reset_gpio;
+ unsigned int reset_duration_ms;
__u8 cmdreq_type;
__u8 cmdreq;
@@ -566,15 +567,21 @@ struct btusb_data {
unsigned cmd_timeout_cnt;
};
-static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
+static void btusb_toggle_gpio(struct gpio_desc *desc, unsigned int duration)
+{
+ gpiod_set_value_cansleep(desc, 1);
+ msleep(duration);
+ gpiod_set_value_cansleep(desc, 0);
+}
+
+static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
{
struct btusb_data *data = hci_get_drvdata(hdev);
- struct gpio_desc *reset_gpio = data->reset_gpio;
if (++data->cmd_timeout_cnt < 5)
return;
- if (!reset_gpio) {
+ if (!data->reset_gpio) {
bt_dev_err(hdev, "No way to reset. Ignoring and continuing");
return;
}
@@ -592,39 +599,7 @@ static void btusb_intel_cmd_timeout(struct hci_dev *hdev)
}
bt_dev_err(hdev, "Initiating HW reset via gpio");
- gpiod_set_value_cansleep(reset_gpio, 1);
- msleep(100);
- gpiod_set_value_cansleep(reset_gpio, 0);
-}
-
-static void btusb_rtl_cmd_timeout(struct hci_dev *hdev)
-{
- struct btusb_data *data = hci_get_drvdata(hdev);
- struct gpio_desc *reset_gpio = data->reset_gpio;
-
- if (++data->cmd_timeout_cnt < 5)
- return;
-
- if (!reset_gpio) {
- bt_dev_err(hdev, "No gpio to reset Realtek device, ignoring");
- return;
- }
-
- /* Toggle the hard reset line. The Realtek device is going to
-* yank itself off the USB and then replug. The cleanup is handled
-* correctly on the way out (standard USB disconnect), and the new
-* device is detected cleanly and bound to the driver again like
-* it should be.
-*/
- if (test_and_set_bit(BTUSB_HW_RESET_ACTIVE, >flags)) {
- bt_dev_err(hdev, "last reset failed? Not resetting again");
- return;
- }
-
- bt_dev_err(hdev, "Reset Realtek device via gpio");
- gpiod_set_value_cansleep(reset_gpio, 1);
- msleep(200);
- gpiod_set_value_cansleep(reset_gpio, 0);
+ btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
}
static void btusb_qca_cmd_timeout(struct hci_dev *hdev)
@@ -4462,7 +4437,8 @@ static int btusb_probe(struct usb_interface *intf,
hdev->shutdown = btusb_shutdown_intel;
hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
- hdev->cmd_timeout = btusb_intel_cmd_timeout;
+ hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+ data->reset_duration_ms = 100;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, >quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, >quirks);
@@ -4476,7 +4452,8 @@ static int btusb_probe(struct usb_interface *intf,
hdev->hw_error = btintel_hw_error;
hdev->set_diag = btintel_set_diag;
hdev->set_bdaddr = btintel_set_bdaddr;
- hdev->cmd_timeout = btusb_intel_cmd_timeout;
+ hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+ data->reset_duration_ms = 100;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, >quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, >quirks);
@@ -4490,7 +4467,8 @@ static int btusb_probe(struct usb_interface *intf,
hdev->hw_error = btintel_hw_error;
hdev->set_diag = btintel_set_diag;
hdev->set_bdaddr = btintel_set_bdaddr;
- hdev->cmd_timeout = btusb_intel_cmd_timeout;
+ hdev->cmd_timeout = btusb_gpio_cmd_timeout;
+ data->reset_duration_ms = 100;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, >quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, >quirks);
@@ -4557,7 +4535,8 @@ static int btusb_probe(struct usb_interface *intf,
(id->driver_info & BTUSB_REALTEK)) {
hde
[PATCH 0/3] Bluetooth: btusb: Expose hw reset to debugfs
Hi Marcel, This series updates the reset gpio based recovery from command timeouts with the following changes: * Refactor duplicate code to use a single btusb_gpio_cmd_timeout * Reduce the number of command timeouts needed for reset to 3 * Expose the gpio based hardware reset to debugfs for testing The last one is important to us so that we can confirm new chips support hardware based reset (which is part of our requirements for BT chips). We will probably also add the 'toggle_hw_reset' debugfs entry to other drivers that support hardware based reset (i.e. hci_qca, hci_h5, etc) in subsequent changes. Thanks Abhishek Abhishek Pandit-Subedi (3): Bluetooth: btusb: Refactor gpio reset Bluetooth: btusb: Trigger gpio reset quicker Bluetooth: btusb: Expose reset gpio to debugfs drivers/bluetooth/btusb.c | 107 +++--- 1 file changed, 66 insertions(+), 41 deletions(-) -- 2.30.0.284.gd98b1dd5eaa7-goog
[PATCH 3/3] Bluetooth: btusb: Expose reset gpio to debugfs
If btusb has a reset gpio, expose it to userspace so we can toggle the
reset gpio directly. This is useful for testing and error recovery.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
drivers/bluetooth/btusb.c | 46 +++
1 file changed, 46 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 880e9cd4ee713..702be1871ed88 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -6,6 +6,7 @@
* Copyright (C) 2005-2008 Marcel Holtmann
*/
+#include
#include
#include
#include
@@ -574,6 +575,46 @@ static void btusb_toggle_gpio(struct gpio_desc *desc,
unsigned int duration)
gpiod_set_value_cansleep(desc, 0);
}
+#ifdef CONFIG_DEBUG_FS
+static ssize_t btusb_debugfs_has_reset_gpio(struct file *file,
+ char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct hci_dev *hdev = file->private_data;
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ char buf[3];
+
+ buf[0] = data->reset_gpio ? 'Y' : 'N';
+ buf[1] = '\n';
+ buf[2] = '\0';
+
+ return simple_read_from_buffer(user_buf, count, ppos, buf, 2);
+}
+
+static ssize_t btusb_debugfs_reset_gpio(struct file *file,
+ const char __user *user_buf,
+ size_t count, loff_t *ppos)
+{
+ struct hci_dev *hdev = file->private_data;
+ struct btusb_data *data = hci_get_drvdata(hdev);
+
+ if (!data->reset_gpio)
+ return -EOPNOTSUPP;
+
+ bt_dev_warn(hdev, "Debugfs triggering HW reset via gpio");
+ btusb_toggle_gpio(data->reset_gpio, data->reset_duration_ms);
+
+ return count;
+}
+
+static const struct file_operations reset_gpio_fops = {
+ .open = simple_open,
+ .read = btusb_debugfs_has_reset_gpio,
+ .write = btusb_debugfs_reset_gpio,
+ .llseek = default_llseek,
+};
+#endif
+
static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
{
struct btusb_data *data = hci_get_drvdata(hdev);
@@ -4625,6 +4666,11 @@ static int btusb_probe(struct usb_interface *intf,
if (err < 0)
goto out_free_dev;
+#ifdef CONFIG_DEBUG_FS
+ debugfs_create_file("toggle_hw_reset", 0644, hdev->debugfs, hdev,
+ _gpio_fops);
+#endif
+
usb_set_intfdata(intf, data);
return 0;
--
2.30.0.284.gd98b1dd5eaa7-goog
[PATCH 2/3] Bluetooth: btusb: Trigger gpio reset quicker
Currently, btusb will only trigger gpio reset during cmd_timeout after
5 commands fail. This number is arbitrarily large and can result in
resets taking longer to occur than necessary.
Reduce this number to 3, which was chosen as a recommended value by
Intel (their firmware allow two commands in flight so they recommend
resetting on the third failed command).
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Daniel Winkler
---
drivers/bluetooth/btusb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 03341e6cbf3ed..880e9cd4ee713 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -578,7 +578,7 @@ static void btusb_gpio_cmd_timeout(struct hci_dev *hdev)
{
struct btusb_data *data = hci_get_drvdata(hdev);
- if (++data->cmd_timeout_cnt < 5)
+ if (++data->cmd_timeout_cnt < 3)
return;
if (!data->reset_gpio) {
--
2.30.0.284.gd98b1dd5eaa7-goog
Re: [PATCH] Bluetooth: btrtl: Enable WBS for the specific Realtek devices
Hi Max,
On Mon, Jan 18, 2021 at 3:28 AM wrote:
>
> From: Max Chou
>
> By this change, it will enable WBS supported on the specific Realtek BT
> devices, such as RTL8822C and RTL8852A.
> In the future, it's able to maintain what the Realtek devices support WBS
> here.
>
> Tested-by: Hilda Wu
> Signed-off-by: Max Chou
> ---
> drivers/bluetooth/btrtl.c | 26 +++---
> 1 file changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
> index 24f03a1f8d57..835819c47ae6 100644
> --- a/drivers/bluetooth/btrtl.c
> +++ b/drivers/bluetooth/btrtl.c
> @@ -38,6 +38,19 @@
> .hci_ver = (hciv), \
> .hci_bus = (bus)
>
> +enum btrtl_chip_id {
> + CHIP_ID_8723A, /* index 0 for RTL8723A*/
> + CHIP_ID_8723B, /* index 1 for RTL8723B*/
> + CHIP_ID_8821A, /* index 2 for RTL8821A*/
> + CHIP_ID_8761A, /* index 3 for RTL8761A*/
> + CHIP_ID_8822B = 8, /* index 8 for RTL8822B */
> + CHIP_ID_8723D, /* index 9 for RTL8723D */
> + CHIP_ID_8821C, /* index 10 for RTL8821C */
> + CHIP_ID_8822C = 13, /* index 13 for RTL8822C */
> + CHIP_ID_8761B, /* index 14 for RTL8761B */
> + CHIP_ID_8852A = 18, /* index 18 for RTL8852A */
> +};
> +
> struct id_table {
> __u16 match_flags;
> __u16 lmp_subver;
> @@ -58,6 +71,7 @@ struct btrtl_device_info {
> u8 *cfg_data;
> int cfg_len;
> bool drop_fw;
> + int project_id;
> };
>
> static const struct id_table ic_id_table[] = {
> @@ -307,8 +321,10 @@ static int rtlbt_parse_firmware(struct hci_dev *hdev,
>
> /* Find project_id in table */
> for (i = 0; i < ARRAY_SIZE(project_id_to_lmp_subver); i++) {
> - if (project_id == project_id_to_lmp_subver[i].id)
> + if (project_id == project_id_to_lmp_subver[i].id) {
> + btrtl_dev->project_id = project_id;
> break;
> + }
> }
>
> if (i >= ARRAY_SIZE(project_id_to_lmp_subver)) {
> @@ -725,12 +741,16 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
> /* Enable central-peripheral role (able to create new connections with
> * an existing connection in slave role).
> */
> - switch (btrtl_dev->ic_info->lmp_subver) {
> - case RTL_ROM_LMP_8822B:
> + /* Enable WBS supported for the specific Realtek devices. */
> + switch (btrtl_dev->project_id) {
> + case CHIP_ID_8822C:
> + case CHIP_ID_8852A:
> set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
> + set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, >quirks);
> break;
> default:
> rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
> + rtl_dev_dbg(hdev, "WBS supported not enabled.");
> break;
> }
There is a null-check on btrtl_dev->ic_info right above this switch
statement
(https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=b649813eadbc062d8682f7a20aa025275707dd1f).
Is this still necessary with this change? (Sorry, I missed this during
our previous review).
>
> --
> 2.17.1
>
Abhishek
[PATCH v2] Bluetooth: btrtl: Add null check in setup
btrtl_dev->ic_info is only available from the controller on cold boot (the lmp subversion matches the device model and this is used to look up the ic_info). On warm boots (firmware already loaded), btrtl_dev->ic_info is null. Fixes: 05672a2c14a4 (Bluetooth: btrtl: Enable central-peripheral role) Signed-off-by: Abhishek Pandit-Subedi --- Changes in v2: - Added nullcheck with goto done drivers/bluetooth/btrtl.c | 4 1 file changed, 4 insertions(+) diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c index 1abf6a4d6727..24f03a1f8d57 100644 --- a/drivers/bluetooth/btrtl.c +++ b/drivers/bluetooth/btrtl.c @@ -719,6 +719,9 @@ int btrtl_setup_realtek(struct hci_dev *hdev) */ set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks); + if (!btrtl_dev->ic_info) + goto done; + /* Enable central-peripheral role (able to create new connections with * an existing connection in slave role). */ @@ -731,6 +734,7 @@ int btrtl_setup_realtek(struct hci_dev *hdev) break; } +done: btrtl_free(btrtl_dev); return ret; } -- 2.29.2.729.g45daf8777d-goog
[PATCH] Bluetooth: btrtl: Add null check in setup
btrtl_dev->ic_info is only available from the controller on cold boot
(the lmp subversion matches the device model and this is used to look up
the ic_info). On warm boots (firmware already loaded),
btrtl_dev->ic_info is null.
Fixes: 05672a2c14a4 (Bluetooth: btrtl: Enable central-peripheral role)
Signed-off-by: Abhishek Pandit-Subedi
---
drivers/bluetooth/btrtl.c | 23 +--
1 file changed, 13 insertions(+), 10 deletions(-)
diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 1abf6a4d672734f..978f3c773856b05 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -719,16 +719,19 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
*/
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
- /* Enable central-peripheral role (able to create new connections with
-* an existing connection in slave role).
-*/
- switch (btrtl_dev->ic_info->lmp_subver) {
- case RTL_ROM_LMP_8822B:
- set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
- break;
- default:
- rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
- break;
+ if (btrtl_dev->ic_info) {
+ /* Enable central-peripheral role (able to create new
+* connections with an existing connection in slave role).
+*/
+ switch (btrtl_dev->ic_info->lmp_subver) {
+ case RTL_ROM_LMP_8822B:
+ set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
+ break;
+ default:
+ rtl_dev_dbg(hdev,
+ "Central-peripheral role not enabled.");
+ break;
+ }
}
btrtl_free(btrtl_dev);
--
2.29.2.729.g45daf8777d-goog
Re: [PATCH 3/3] ath10k: Set wiphy flag to trigger sta disconnect on hardware restart
On Tue, Dec 15, 2020 at 9:24 AM Youghandhar Chintala
wrote:
>
> From: Rakesh Pillai
>
> Currently after the hardware restart triggered from the driver,
> the station interface connection remains intact, since a disconnect
> trigger is not sent to userspace. This can lead to a problem in
> hardwares where the wifi mac sequence is added by the firmware.
>
> After the firmware restart, during subsytem recovery, the firmware
> restarts its wifi mac sequence number. Hence AP to which our device
> is connected will receive frames with a wifi mac sequence number jump
> to the past, thereby resulting in the AP dropping all these frames,
> until the frame arrives with a wifi mac sequence number which AP was
> expecting.
>
> To avoid such frame drops, its better to trigger a station disconnect
> upon the hardware restart. Indicate this support via a WIPHY flag
> to mac80211, if the hardware params flag mentions the support to
> add wifi mac sequence numbers for TX frames in the firmware.
>
> All the other hardwares, except WCN3990, are not affected by this
> change, since the hardware params flag is not set for any hardware
> except for WCN3990
>
> Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.1-01040-QCAHLSWMTPLZ-1
> Tested-on: QCA6174 hw3.2 PCI WLAN.RM.4.4.1-00110-QCARMSWP-1
> Tested-on: QCA6174 hw3.2 SDIO WLAN.RMH.4.4.1-00048
>
> Signed-off-by: Rakesh Pillai
> Signed-off-by: Youghandhar Chintala
> ---
> drivers/net/wireless/ath/ath10k/core.c | 15 +++
> drivers/net/wireless/ath/ath10k/hw.h | 3 +++
> drivers/net/wireless/ath/ath10k/mac.c | 3 +++
> 3 files changed, 21 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.c
> b/drivers/net/wireless/ath/ath10k/core.c
> index 796107b..4155f94 100644
> --- a/drivers/net/wireless/ath/ath10k/core.c
> +++ b/drivers/net/wireless/ath/ath10k/core.c
> @@ -90,6 +90,7 @@ static const struct ath10k_hw_params
> ath10k_hw_params_list[] = {
> .hw_filter_reset_required = true,
> .fw_diag_ce_download = false,
> .tx_stats_over_pktlog = true,
> + .tx_mac_seq_by_fw = false,
Probably orthogonal to this patch, there is a static array maintained
for different hardware configs and the structure members like
"tx_mac_seq_by_fw" are initialized. This does not seem to be scalable
and probably these parameters can be auto populated based on FW
capabilities and so we don't have to maintain the static array.
Thoughts?
-Abhishek
[PATCH] Bluetooth: btrtl: Enable central-peripheral role
Enable the central-peripheral role on RTL8822CE. This enables creating
connections while there is an existing connection in the slave role.
This change can be confirmed in userspace via `bluetoothctl show` which
will now show "Roles: central-peripheral".
Reviewed-by: Daniel Winkler
Signed-off-by: Abhishek Pandit-Subedi
---
drivers/bluetooth/btrtl.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/drivers/bluetooth/btrtl.c b/drivers/bluetooth/btrtl.c
index 94df4e94999d5c8..1abf6a4d672734f 100644
--- a/drivers/bluetooth/btrtl.c
+++ b/drivers/bluetooth/btrtl.c
@@ -714,13 +714,24 @@ int btrtl_setup_realtek(struct hci_dev *hdev)
ret = btrtl_download_firmware(hdev, btrtl_dev);
- btrtl_free(btrtl_dev);
-
/* Enable controller to do both LE scan and BR/EDR inquiry
* simultaneously.
*/
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, >quirks);
+ /* Enable central-peripheral role (able to create new connections with
+* an existing connection in slave role).
+*/
+ switch (btrtl_dev->ic_info->lmp_subver) {
+ case RTL_ROM_LMP_8822B:
+ set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
+ break;
+ default:
+ rtl_dev_dbg(hdev, "Central-peripheral role not enabled.");
+ break;
+ }
+
+ btrtl_free(btrtl_dev);
return ret;
}
EXPORT_SYMBOL_GPL(btrtl_setup_realtek);
--
2.29.2.729.g45daf8777d-goog
Re: [PATCH v1] Bluetooth: Set missing suspend task bits
Hi Dmitry,
https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=295fa2a5647b13681594bb1bcc76c74619035218
should fix this issue.
Your issue seems the same as the one I encountered -- the
SUSPEND_DISABLE bit (0x4) wasn't being cleared by the request
completion handler.
Thanks,
Abhishek
On Mon, Dec 21, 2020 at 6:35 AM Dmitry Osipenko wrote:
>
> 04.12.2020 06:14, Howard Chung пишет:
> > From: Abhishek Pandit-Subedi
> >
> > When suspending, mark SUSPEND_SCAN_ENABLE and SUSPEND_SCAN_DISABLE tasks
> > correctly when either classic or le scanning is modified.
> >
> > Signed-off-by: Abhishek Pandit-Subedi
> > Signed-off-by: Howard Chung
> > Reviewed-by: Alain Michaud
> > ---
> >
> > net/bluetooth/hci_request.c | 8
> > 1 file changed, 8 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
> > index 80dc451d6e124..71bffd7454720 100644
> > --- a/net/bluetooth/hci_request.c
> > +++ b/net/bluetooth/hci_request.c
> > @@ -707,6 +707,9 @@ void hci_req_add_le_scan_disable(struct hci_request
> > *req, bool rpa_le_conn)
> > return;
> > }
> >
> > + if (hdev->suspended)
> > + set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> > +
> > if (use_ext_scan(hdev)) {
> > struct hci_cp_le_set_ext_scan_enable cp;
> >
> > @@ -1159,6 +1162,11 @@ static void hci_req_set_event_filter(struct
> > hci_request *req)
> > scan = SCAN_PAGE;
> > }
> >
> > + if (scan)
> > + set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
> > + else
> > + set_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
> > +
> > hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
> > }
> >
> >
>
> Hi,
>
> This commit caused a regression on entering into suspend for Broadcom
> Bluetooth 4330 on Nexus 7:
>
> Bluetooth: hci0: Timed out waiting for suspend events
> Bluetooth: hci0: Suspend timeout bit: 4
> Bluetooth: hci0: Suspend notifier action (3) failed: -110
>
> I don't see this problem using BCM4329 chip on another device.
>
> Please fix, thanks in advance.
[PATCH] Bluetooth: Pause service discovery for suspend
Just like MGMT_OP_START_DISCOVERY, we should reject
MGMT_OP_START_SERVICE_DISCOVERY with MGMT_STATUS_BUSY when we are paused
for suspend.
Signed-off-by: Abhishek Pandit-Subedi
---
On ChromeOS, we started getting reports of scanning failing after
resuming from suspend. The root cause was that Start Service Discovery
was being called while discovery was supposed to be paused for suspend
and it was screwing up some internal state. Adding this check
immediately fixed it.
The fix was tested by doing the following:
* Set Discovery Filter ({'transport': 'auto'})
* Start Discovery
* Suspend
* Resume
* Check the Discovering property
Without the fix, this test failed when checking the Discovering
property above.
net/bluetooth/mgmt.c | 8
1 file changed, 8 insertions(+)
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index fa0f7a4a1d2fc8a..608dda5403b7327 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -4798,6 +4798,14 @@ static int start_service_discovery(struct sock *sk,
struct hci_dev *hdev,
goto failed;
}
+ if (hdev->discovery_paused) {
+ err = mgmt_cmd_complete(sk, hdev->id,
+ MGMT_OP_START_SERVICE_DISCOVERY,
+ MGMT_STATUS_BUSY, >type,
+ sizeof(cp->type));
+ goto failed;
+ }
+
uuid_count = __le16_to_cpu(cp->uuid_count);
if (uuid_count > max_uuid_count) {
bt_dev_err(hdev, "service_discovery: too big uuid_count value
%u",
--
2.29.2.729.g45daf8777d-goog
[PATCH 0/1] Bluetooth: Further improvements for suspend tasks
Hi Marcel, This patch further improves suspend handling by getting rid of the separate function used for always configuring LE scan. Instead, we only configure LE scan if it is necessary and properly set the task bits depending on what actions were taken. The previously sent-up CL was incomplete due to a merge problem between ChromeOS and upstream. We merged https://patchwork.kernel.org/project/bluetooth/patch/20200917164632.BlueZ.v2.4.I3774a8f0d748c7c6ec3402c4adcead32810c9164@changeid/ but the upstream version didn't have the same changes: https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=36afe87ac10fd71f98c40ccf9923b83e0d3fab68 This fix was tested after reverting all our local patches, applying the upstream patches and this patch on top. Thanks Abhishek Abhishek Pandit-Subedi (1): Bluetooth: Remove hci_req_le_suspend_config net/bluetooth/hci_request.c | 25 - 1 file changed, 8 insertions(+), 17 deletions(-) -- 2.29.2.576.ga3fc446d84-goog
[PATCH 1/1] Bluetooth: Remove hci_req_le_suspend_config
Add a missing SUSPEND_SCAN_ENABLE in passive scan, remove the separate
function for configuring le scan during suspend and update the request
complete function to clear both enable and disable tasks.
Fixes: dce0a4be8054 ("Bluetooth: Set missing suspend task bits")
Reviewed-by: Alain Michaud
Signed-off-by: Abhishek Pandit-Subedi
---
net/bluetooth/hci_request.c | 25 -
1 file changed, 8 insertions(+), 17 deletions(-)
diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c
index 71bffd745472043..5aa7bd5030a218c 100644
--- a/net/bluetooth/hci_request.c
+++ b/net/bluetooth/hci_request.c
@@ -1087,6 +1087,8 @@ void hci_req_add_le_passive_scan(struct hci_request *req)
if (hdev->suspended) {
window = hdev->le_scan_window_suspend;
interval = hdev->le_scan_int_suspend;
+
+ set_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
} else if (hci_is_le_conn_scanning(hdev)) {
window = hdev->le_scan_window_connect;
interval = hdev->le_scan_int_connect;
@@ -1170,19 +1172,6 @@ static void hci_req_set_event_filter(struct hci_request
*req)
hci_req_add(req, HCI_OP_WRITE_SCAN_ENABLE, 1, );
}
-static void hci_req_config_le_suspend_scan(struct hci_request *req)
-{
- /* Before changing params disable scan if enabled */
- if (hci_dev_test_flag(req->hdev, HCI_LE_SCAN))
- hci_req_add_le_scan_disable(req, false);
-
- /* Configure params and enable scanning */
- hci_req_add_le_passive_scan(req);
-
- /* Block suspend notifier on response */
- set_bit(SUSPEND_SCAN_ENABLE, req->hdev->suspend_tasks);
-}
-
static void cancel_adv_timeout(struct hci_dev *hdev)
{
if (hdev->adv_instance_timeout) {
@@ -1245,8 +1234,10 @@ static void suspend_req_complete(struct hci_dev *hdev,
u8 status, u16 opcode)
{
bt_dev_dbg(hdev, "Request complete opcode=0x%x, status=0x%x", opcode,
status);
- if (test_and_clear_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks) ||
- test_and_clear_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks)) {
+ if (test_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks) ||
+ test_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks)) {
+ clear_bit(SUSPEND_SCAN_ENABLE, hdev->suspend_tasks);
+ clear_bit(SUSPEND_SCAN_DISABLE, hdev->suspend_tasks);
wake_up(>suspend_wait_q);
}
}
@@ -1336,7 +1327,7 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum
suspended_state next)
/* Enable event filter for paired devices */
hci_req_set_event_filter();
/* Enable passive scan at lower duty cycle */
- hci_req_config_le_suspend_scan();
+ __hci_update_background_scan();
/* Pause scan changes again. */
hdev->scanning_paused = true;
hci_req_run(, suspend_req_complete);
@@ -1346,7 +1337,7 @@ void hci_req_prepare_suspend(struct hci_dev *hdev, enum
suspended_state next)
hci_req_clear_event_filter();
/* Reset passive/background scanning to normal */
- hci_req_config_le_suspend_scan();
+ __hci_update_background_scan();
/* Unpause directed advertising */
hdev->advertising_paused = false;
--
2.29.2.576.ga3fc446d84-goog
[PATCH v3] ath10k: add option for chip-id based BDF selection
In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.
The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Abhishek Kumar
---
Changes in v3:
- Resurreted Patch V1 because as per discussion we do need two
fallback board names and refactored ath10k_core_create_board_name.
drivers/net/wireless/ath/ath10k/core.c | 41 +++---
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..09d0fdfa6693 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1349,7 +1349,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
const char *boardname,
- const char *fallback_boardname,
+ const char *fallback_boardname1,
+ const char *fallback_boardname2,
const char *filename)
{
size_t len, magic_len;
@@ -1398,8 +1399,11 @@ static int ath10k_core_fetch_board_data_api_n(struct
ath10k *ar,
ret = ath10k_core_search_bd(ar, boardname, data, len);
/* if we didn't find it and have a fallback name, try that */
- if (ret == -ENOENT && fallback_boardname)
- ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+ if (ret == -ENOENT && fallback_boardname1)
+ ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+ if (ret == -ENOENT && fallback_boardname2)
+ ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
if (ret == -ENOENT) {
ath10k_err(ar,
@@ -1419,7 +1423,8 @@ static int ath10k_core_fetch_board_data_api_n(struct
ath10k *ar,
}
static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len, bool with_variant,
+bool with_chip_id)
{
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1438,7 +1443,7 @@ static int ath10k_core_create_board_name(struct ath10k
*ar, char *name,
}
if (ar->id.qmi_ids_valid) {
- if (with_variant && ar->id.bdf_ext[0] != '\0')
+ if (with_chip_id)
scnprintf(name, name_len,
"bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
ath10k_bus_str(ar->hif.bus),
@@ -1482,21 +1487,34 @@ static int ath10k_core_create_eboard_name(struct ath10k
*ar, char *name,
int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
{
- char boardname[100], fallback_boardname[100];
+ char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
int ret;
if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+ /* With variant and chip id */
ret = ath10k_core_create_board_name(ar, boardname,
- sizeof(boardname), true);
+ sizeof(boardname), true,
+ true);
if (ret) {
ath10k_err(ar, "failed to create board name: %d", ret);
return ret;
}
- ret = ath10k_core_create_board_name(ar, fallback_boardname,
- sizeof(boardname), false);
+ /* Without variant and only chip-id */
+ ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+ sizeof(boardname), false,
+ true);
+ if (ret) {
+ ath10k_err(ar, "failed to create 1st fallback board
name: %d", ret);
+ return ret;
+ }
+
+ /* Without variant and without chip-id */
+ ret = ath10k_core_create_board_name(ar, fallback_boardname2,
+ sizeof(boardname), false,
+ false);
if (ret) {
-
Re: [PATCH v2 1/1] ath10k: add option for chip-id based BDF selection
Hi, > > (no changes since v1) > > I think you need to work on the method you're using to generate your > patches. There are most definitely changes since v1. You described > them in your cover letter (which you don't really need for a singleton > patch) instead of here. I agree, this was not intentional, I will fix this in the upcoming patches. On Thu, Dec 3, 2020 at 7:34 AM Doug Anderson wrote: > > Hi, > > On Thu, Dec 3, 2020 at 3:33 AM Rakesh Pillai wrote: > > > > > What I'm trying to say is this. Imagine that: > > > > > > a) the device tree has the "variant" property. > > > > > > b) the BRD file has two entries, one for "board-id" (1) and one for > > > "board-id + chip-id" (2). It doesn't have one for "board-id + chip-id > > > + variant" (3). > > > > > > With your suggestion we'll see the "variant" property in the device > > > tree. That means we'll search for (1) and (3). (3) isn't there, so > > > we'll pick (1). ...but we really should have picked (2), right? > > > > Do we expect board-2.bin to not be populated with the bdf with variant > > field (if its necessary ?) > > The whole fact that there is a fallback to begin with implies that > there can be a mismatch between the board-2.bin and the device tree > file. Once we accept that there can be a mismatch, it seems good to > try all 3 fallbacks in order. > > > Seems fine for me, if we have 2 fallback names if that is needed. > OK, sounds good. So hopefully Abhishek can post a v3 based on what's > in <https://crrev.com/c/2556437> and you can confirm you're good with > it there? I agree, with this patch there can be mismatch between what's provided in the Board file and what required board name we are generating, so three calls are needed. So in a sense, we want to keep the V1 patch with fix to reuse the same BDF. I am making V3 changes and will address and push that out. Thanks Abhishek
Re: [PATCH 0/3] Bluetooth: Power down controller when suspending
Re-send to NXP email addresses for Chin-Ran Lo and Amitkumar Karwar (Marvell wireless IP acquired by NXP) On Tue, Nov 24, 2020 at 11:02 AM Abhishek Pandit-Subedi wrote: > > Hi Marcel, > > > On Mon, Nov 23, 2020 at 3:46 AM Marcel Holtmann wrote: > > > > Hi Abhishek, > > > > > This patch series adds support for a quirk that will power down the > > > Bluetooth controller when suspending and power it back up when resuming. > > > > > > On Marvell SDIO Bluetooth controllers (SD8897 and SD8997), we are seeing > > > a large number of suspend failures with the following log messages: > > > > > > [ 4764.773873] Bluetooth: hci_cmd_timeout() hci0 command 0x0c14 tx timeout > > > [ 4767.777897] Bluetooth: btmrvl_enable_hs() Host sleep enable command > > > failed > > > [ 4767.777920] Bluetooth: btmrvl_sdio_suspend() HS not actived, suspend > > > failed! > > > [ 4767.777946] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 > > > [ 4767.777963] call mmc2:0001:2+ returned -16 after 4882288 usecs > > > > > > The daily failure rate with this signature is quite significant and > > > users are likely facing this at least once a day (and some unlucky users > > > are likely facing it multiple times a day). > > > > > > Given the severity, we'd like to power off the controller during suspend > > > so the driver doesn't need to take any action (or block in any way) when > > > suspending and power on during resume. This will break wake-on-bt for > > > users but should improve the reliability of suspend. > > > > > > We don't want to force all users of MVL8897 and MVL8997 to encounter > > > this behavior if they're not affected (especially users that depend on > > > Bluetooth for keyboard/mouse input) so the new behavior is enabled via > > > module param. We are limiting this quirk to only Chromebooks (i.e. > > > laptop). Chromeboxes will continue to have the old behavior since users > > > may depend on BT HID to wake and use the system. > > > > I don’t have a super great feeling with this change. > > > > So historically only hciconfig hci0 up/down was doing a power cycle of the > > controller and when adding the mgmt interface we moved that to the mgmt > > interface. In addition we added a special case of power up via hdev->setup. > > We never had an intention that the kernel otherwise can power up/down the > > controller as it pleases. > > Aside from the powered setting, the stack is resilient to the > controller crashing (which would be akin to a power off and power on). > From the view of bluez, adapter lost and power down should be almost > equivalent right? ChromeOS has several platforms where Bluetooth has > been reset after suspend, usually due USB being powered off in S3, and > the stack is still well-behaving when that occurs. > > > > > Can we ask Marvell first to investigate why this is fundamentally broken > > with their hardware? > > +Chin-Ran Lo and +Amitkumar Karwar (added based on changes to > drivers/bluetooth/btmrvl_main.c) > > Could you please take a look at the original cover letter and comment > (or add others at Marvell who may be able to)? Is this a known issue > or a fix? > > >Since what you are proposing is a pretty heavy change that might has side > >affects. For example the state machine for the mgmt interface has no concept > >of a power down/up from the kernel. It is all triggered by bluetoothd. > > > > I am careful here since the whole power up/down path is already complicated > > enough. > > > > That sounds reasonable. I have landed this within ChromeOS so we can > test whether a) this improves stability enough and b) whether the > power off/on in the kernel has significant side effects. This will go > through our automated testing and dogfooding over the next few weeks > and hopefully identify those side-effects. I will re-raise this topic > with updates once we have more data. > > Also, in case it wasn't very clear, I put this behind a module param > that defaults to False because this is so heavy handed. We're only > using it on specific Chromebooks that are exhibiting the worst > behavior and not disabling it wholesale for all btmrvl controllers. > > Thanks > Abhishek > > > Regards > > > > Marcel > >
Re: [PATCH 0/3] Bluetooth: Power down controller when suspending
Hi Marcel, On Mon, Nov 23, 2020 at 3:46 AM Marcel Holtmann wrote: > > Hi Abhishek, > > > This patch series adds support for a quirk that will power down the > > Bluetooth controller when suspending and power it back up when resuming. > > > > On Marvell SDIO Bluetooth controllers (SD8897 and SD8997), we are seeing > > a large number of suspend failures with the following log messages: > > > > [ 4764.773873] Bluetooth: hci_cmd_timeout() hci0 command 0x0c14 tx timeout > > [ 4767.777897] Bluetooth: btmrvl_enable_hs() Host sleep enable command > > failed > > [ 4767.777920] Bluetooth: btmrvl_sdio_suspend() HS not actived, suspend > > failed! > > [ 4767.777946] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 > > [ 4767.777963] call mmc2:0001:2+ returned -16 after 4882288 usecs > > > > The daily failure rate with this signature is quite significant and > > users are likely facing this at least once a day (and some unlucky users > > are likely facing it multiple times a day). > > > > Given the severity, we'd like to power off the controller during suspend > > so the driver doesn't need to take any action (or block in any way) when > > suspending and power on during resume. This will break wake-on-bt for > > users but should improve the reliability of suspend. > > > > We don't want to force all users of MVL8897 and MVL8997 to encounter > > this behavior if they're not affected (especially users that depend on > > Bluetooth for keyboard/mouse input) so the new behavior is enabled via > > module param. We are limiting this quirk to only Chromebooks (i.e. > > laptop). Chromeboxes will continue to have the old behavior since users > > may depend on BT HID to wake and use the system. > > I don’t have a super great feeling with this change. > > So historically only hciconfig hci0 up/down was doing a power cycle of the > controller and when adding the mgmt interface we moved that to the mgmt > interface. In addition we added a special case of power up via hdev->setup. > We never had an intention that the kernel otherwise can power up/down the > controller as it pleases. Aside from the powered setting, the stack is resilient to the controller crashing (which would be akin to a power off and power on). >From the view of bluez, adapter lost and power down should be almost equivalent right? ChromeOS has several platforms where Bluetooth has been reset after suspend, usually due USB being powered off in S3, and the stack is still well-behaving when that occurs. > > Can we ask Marvell first to investigate why this is fundamentally broken with > their hardware? +Chin-Ran Lo and +Amitkumar Karwar (added based on changes to drivers/bluetooth/btmrvl_main.c) Could you please take a look at the original cover letter and comment (or add others at Marvell who may be able to)? Is this a known issue or a fix? >Since what you are proposing is a pretty heavy change that might has side >affects. For example the state machine for the mgmt interface has no concept >of a power down/up from the kernel. It is all triggered by bluetoothd. > > I am careful here since the whole power up/down path is already complicated > enough. > That sounds reasonable. I have landed this within ChromeOS so we can test whether a) this improves stability enough and b) whether the power off/on in the kernel has significant side effects. This will go through our automated testing and dogfooding over the next few weeks and hopefully identify those side-effects. I will re-raise this topic with updates once we have more data. Also, in case it wasn't very clear, I put this behind a module param that defaults to False because this is so heavy handed. We're only using it on specific Chromebooks that are exhibiting the worst behavior and not disabling it wholesale for all btmrvl controllers. Thanks Abhishek > Regards > > Marcel >
Re: [PATCH v3] ath10k: Fix the parsing error in service available event
Hi, The patch LGTM there is small nit, though. > +++ b/drivers/net/wireless/ath/ath10k/wmi.c > @@ -5751,8 +5751,13 @@ void ath10k_wmi_event_service_available(struct ath10k > *ar, struct sk_buff *skb) > ret); > } > > - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map, > - __le32_to_cpu(arg.service_map_ext_len)); > + /* > +* Initialization of "arg.service_map_ext_valid" to ZERO is necessary > +* for the below logic to work. > +*/ Nit: The comment will throw a checkpatch warning. "networking block comments don't use an empty /* line, use /* Comment..." -Abhishek
[PATCH 3/3] Bluetooth: btmrvl_sdio: Power down when suspending
After seeing a large number of suspend failures due to -EBUSY, the most
common cause for failure seems to be the log snippet below:
[ 4764.773873] Bluetooth: hci_cmd_timeout() hci0 command 0x0c14 tx timeout
[ 4767.777897] Bluetooth: btmrvl_enable_hs() Host sleep enable command failed
[ 4767.777920] Bluetooth: btmrvl_sdio_suspend() HS not actived, suspend failed!
[ 4767.777946] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16
[ 4767.777963] call mmc2:0001:2+ returned -16 after 4882288 usecs
Since the sleep command is timing out, this points to the firmware as
the most likely source of the problem and we don't have a way to address
the fix there (this is an old controller). So, to mitigate this issue,
we can simply power down the Bluetooth controller when entering suspend
and power it back up when exiting suspend. We control setting this quirk
via a module parameter, power_down_suspend (which defaults to false).
Signed-off-by: Abhishek Pandit-Subedi
---
drivers/bluetooth/btmrvl_sdio.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/bluetooth/btmrvl_sdio.c b/drivers/bluetooth/btmrvl_sdio.c
index 33d58b30c5acfc..e2e4917b4fe589 100644
--- a/drivers/bluetooth/btmrvl_sdio.c
+++ b/drivers/bluetooth/btmrvl_sdio.c
@@ -35,6 +35,12 @@
#define VERSION "1.0"
+/* Add module param to control whether the controller is powered down during
+ * suspend. Default is False.
+ */
+static bool power_down_suspend;
+module_param(power_down_suspend, bool, 0644);
+
static struct memory_type_mapping mem_type_mapping_tbl[] = {
{"ITCM", NULL, 0, 0xF0},
{"DTCM", NULL, 0, 0xF1},
@@ -1587,6 +1593,10 @@ static int btmrvl_sdio_probe(struct sdio_func *func,
goto disable_host_int;
}
+ if (power_down_suspend)
+ set_bit(HCI_QUIRK_POWER_DOWN_SYSTEM_SUSPEND,
+ >btmrvl_dev.hcidev->quirks);
+
return 0;
disable_host_int:
--
2.29.2.299.gdc1121823c-goog
[PATCH 2/3] Bluetooth: Add quirk to power down on suspend
Some older controllers fail to enter a quiescent state reliably when
supporting remote wake. For those cases, add a quirk that will power
down the controller when suspending and power it back up when resuming.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
include/net/bluetooth/hci.h | 7 +
include/net/bluetooth/hci_core.h | 4 +++
net/bluetooth/hci_core.c | 48 ++--
net/bluetooth/hci_request.c | 26 -
4 files changed, 82 insertions(+), 3 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c8e67042a3b14c..88d5c9554e4840 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -238,6 +238,13 @@ enum {
* during the hdev->setup vendor callback.
*/
HCI_QUIRK_BROKEN_ERR_DATA_REPORTING,
+
+ /* When this quirk is set, the adapter will be powered down during
+* system suspend and powerd up on resume. This should be used on
+* controllers that don't behave well during suspend, either causing
+* spurious wakeups or not entering a suspend state reliably.
+*/
+ HCI_QUIRK_POWER_DOWN_SYSTEM_SUSPEND,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index ff32d5a856f17f..e7dc6e3efbf246 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -90,6 +90,7 @@ struct discovery_state {
};
#define SUSPEND_NOTIFIER_TIMEOUT msecs_to_jiffies(2000) /* 2 seconds */
+#define SUSPEND_POWER_DOWN_TIMEOUT msecs_to_jiffies(1000)
enum suspend_tasks {
SUSPEND_PAUSE_DISCOVERY,
@@ -112,6 +113,9 @@ enum suspended_state {
BT_RUNNING = 0,
BT_SUSPEND_DISCONNECT,
BT_SUSPEND_CONFIGURE_WAKE,
+ BT_SUSPEND_DO_POWER_DOWN,
+ BT_SUSPEND_DO_POWER_UP,
+ BT_SUSPEND_POWERED_DOWN,/* Powered down prior to suspend */
};
struct hci_conn_hash {
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 8e90850d6d769e..d73e097d3ce16b 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3562,6 +3562,7 @@ static int hci_suspend_notifier(struct notifier_block
*nb, unsigned long action,
container_of(nb, struct hci_dev, suspend_notifier);
int ret = 0;
u8 state = BT_RUNNING;
+ bool powered;
/* If powering down, wait for completion. */
if (mgmt_powering_down(hdev)) {
@@ -3571,8 +3572,51 @@ static int hci_suspend_notifier(struct notifier_block
*nb, unsigned long action,
goto done;
}
- /* Suspend notifier should only act on events when powered. */
- if (!hdev_is_powered(hdev))
+ powered = hdev_is_powered(hdev);
+
+ /* Update the suspend state when entering suspend if the system is
+* currently powered off or if it is powered on but was previously
+* powered off.
+*/
+ if (action == PM_SUSPEND_PREPARE) {
+ /* Must hold dev lock when modifying suspend state. */
+ hci_dev_lock(hdev);
+ if (powered && hdev->suspend_state == BT_SUSPEND_POWERED_DOWN)
+ hdev->suspend_state = BT_RUNNING;
+ else if (!powered &&
+hdev->suspend_state != BT_SUSPEND_POWERED_DOWN)
+ hdev->suspend_state = BT_SUSPEND_POWERED_DOWN;
+
+ hci_dev_unlock(hdev);
+ }
+
+ /* When the power down quirk is set, we power down the adapter when
+* suspending and power it up when resuming. If the adapter was already
+* powered down before suspend, we don't do anything here.
+*/
+ if (test_bit(HCI_QUIRK_POWER_DOWN_SYSTEM_SUSPEND, >quirks) &&
+ hdev->suspend_state != BT_SUSPEND_POWERED_DOWN) {
+ if (action == PM_SUSPEND_PREPARE && powered) {
+ state = BT_SUSPEND_DO_POWER_DOWN;
+ ret = hci_change_suspend_state(hdev, state);
+
+ /* Emit that we're powering down for suspend */
+ hci_clear_wake_reason(hdev);
+ mgmt_suspending(hdev, state);
+ goto done;
+ } else if (action == PM_POST_SUSPEND && !powered) {
+ /* Emit that we're resuming before powering up. */
+ mgmt_resuming(hdev, hdev->wake_reason, >wake_addr,
+ hdev->wake_addr_type);
+
+ state = BT_SUSPEND_DO_POWER_UP;
+ ret = hci_change_suspend_state(hdev, state);
+ goto done;
+ }
+ }
+
+ /* Skip to end if we weren't powered. */
+ if (!powered)
goto done;
if (action == PM_SUSPEND_PREPARE) {
diff -
[PATCH 1/3] Bluetooth: Rename and move clean_up_hci_state
Rename clean_up_hci_state and move to the core header so that we can
power down the controller from within the kernel rather than just via
mgmt commands.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Daniel Winkler
Reviewed-by: Miao-chen Chou
---
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 45 +++
net/bluetooth/mgmt.c | 46 +---
3 files changed, 48 insertions(+), 45 deletions(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 9873e1c8cd163b..ff32d5a856f17f 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1072,6 +1072,8 @@ void hci_conn_enter_active_mode(struct hci_conn *conn,
__u8 force_active);
void hci_le_conn_failed(struct hci_conn *conn, u8 status);
+int hci_clean_up_state(struct hci_dev *hdev);
+
/*
* hci_conn_get() and hci_conn_put() are used to control the life-time of an
* "hci_conn" object. They do not guarantee that the hci_conn object is
running,
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 502552d6e9aff3..8e90850d6d769e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2290,6 +2290,51 @@ static void hci_power_on(struct work_struct *work)
}
}
+static void clean_up_hci_complete(struct hci_dev *hdev, u8 status, u16 opcode)
+{
+ BT_DBG("%s status 0x%02x", hdev->name, status);
+
+ if (hci_conn_count(hdev) == 0) {
+ cancel_delayed_work(>power_off);
+ queue_work(hdev->req_workqueue, >power_off.work);
+ }
+}
+
+/* This function requires the caller holds hdev->lock */
+int hci_clean_up_state(struct hci_dev *hdev)
+{
+ struct hci_request req;
+ struct hci_conn *conn;
+ bool discov_stopped;
+ int err;
+ u8 scan = 0x00;
+
+ hci_req_init(, hdev);
+
+ if (test_bit(HCI_ISCAN, >flags) ||
+ test_bit(HCI_PSCAN, >flags)) {
+ hci_req_add(, HCI_OP_WRITE_SCAN_ENABLE, 1, );
+ }
+
+ hci_req_clear_adv_instance(hdev, NULL, NULL, 0x00, false);
+
+ if (hci_dev_test_flag(hdev, HCI_LE_ADV))
+ __hci_req_disable_advertising();
+
+ discov_stopped = hci_req_stop_discovery();
+
+ list_for_each_entry(conn, >conn_hash.list, list) {
+ /* 0x15 == Terminated due to Power Off */
+ __hci_abort_conn(, conn, 0x15);
+ }
+
+ err = hci_req_run(, clean_up_hci_complete);
+ if (!err && discov_stopped)
+ hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
+
+ return err;
+}
+
static void hci_power_off(struct work_struct *work)
{
struct hci_dev *hdev = container_of(work, struct hci_dev,
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 12d7b368b428e8..ea136a6b730f9a 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -1122,16 +1122,6 @@ static int send_settings_rsp(struct sock *sk, u16
opcode, struct hci_dev *hdev)
sizeof(settings));
}
-static void clean_up_hci_complete(struct hci_dev *hdev, u8 status, u16 opcode)
-{
- bt_dev_dbg(hdev, "status 0x%02x", status);
-
- if (hci_conn_count(hdev) == 0) {
- cancel_delayed_work(>power_off);
- queue_work(hdev->req_workqueue, >power_off.work);
- }
-}
-
void mgmt_advertising_added(struct sock *sk, struct hci_dev *hdev, u8 instance)
{
struct mgmt_ev_advertising_added ev;
@@ -1159,40 +1149,6 @@ static void cancel_adv_timeout(struct hci_dev *hdev)
}
}
-static int clean_up_hci_state(struct hci_dev *hdev)
-{
- struct hci_request req;
- struct hci_conn *conn;
- bool discov_stopped;
- int err;
-
- hci_req_init(, hdev);
-
- if (test_bit(HCI_ISCAN, >flags) ||
- test_bit(HCI_PSCAN, >flags)) {
- u8 scan = 0x00;
- hci_req_add(, HCI_OP_WRITE_SCAN_ENABLE, 1, );
- }
-
- hci_req_clear_adv_instance(hdev, NULL, NULL, 0x00, false);
-
- if (hci_dev_test_flag(hdev, HCI_LE_ADV))
- __hci_req_disable_advertising();
-
- discov_stopped = hci_req_stop_discovery();
-
- list_for_each_entry(conn, >conn_hash.list, list) {
- /* 0x15 == Terminated due to Power Off */
- __hci_abort_conn(, conn, 0x15);
- }
-
- err = hci_req_run(, clean_up_hci_complete);
- if (!err && discov_stopped)
- hci_discovery_set_state(hdev, DISCOVERY_STOPPING);
-
- return err;
-}
-
static int set_powered(struct sock *sk, struct hci_dev *hdev, void *data,
u16 len)
{
@@ -1230,7 +1186,7 @@ static int set_powered(struct sock *sk, struct hci_dev
*hdev, void *data,
err = 0;
} else {
/* Disconnect connections, stop scans, etc */
[PATCH 0/3] Bluetooth: Power down controller when suspending
Hi Marcel and linux-bluetooth, This patch series adds support for a quirk that will power down the Bluetooth controller when suspending and power it back up when resuming. On Marvell SDIO Bluetooth controllers (SD8897 and SD8997), we are seeing a large number of suspend failures with the following log messages: [ 4764.773873] Bluetooth: hci_cmd_timeout() hci0 command 0x0c14 tx timeout [ 4767.777897] Bluetooth: btmrvl_enable_hs() Host sleep enable command failed [ 4767.777920] Bluetooth: btmrvl_sdio_suspend() HS not actived, suspend failed! [ 4767.777946] dpm_run_callback(): pm_generic_suspend+0x0/0x48 returns -16 [ 4767.777963] call mmc2:0001:2+ returned -16 after 4882288 usecs The daily failure rate with this signature is quite significant and users are likely facing this at least once a day (and some unlucky users are likely facing it multiple times a day). Given the severity, we'd like to power off the controller during suspend so the driver doesn't need to take any action (or block in any way) when suspending and power on during resume. This will break wake-on-bt for users but should improve the reliability of suspend. We don't want to force all users of MVL8897 and MVL8997 to encounter this behavior if they're not affected (especially users that depend on Bluetooth for keyboard/mouse input) so the new behavior is enabled via module param. We are limiting this quirk to only Chromebooks (i.e. laptop). Chromeboxes will continue to have the old behavior since users may depend on BT HID to wake and use the system. These changes were tested in the following ways on a Chromebook running the 4.19 kernel and a MVL-SD8897 chipset. We added the module param in /etc/modprobe.d/btmrvl_sdio.conf with the contents "options btmrvl_sdio power_down_suspend=Y". Tests run: With no devices paired: - suspend_stress_test --wake_min 10 --suspend_min 10 --count 500 With an LE keyboard paired: - suspend_stress_test --wake_min 10 --suspend_min 10 --count 500 Using the ChromeOS AVL test suite (stress tests are 25 iterations): - bluetooth_AdapterSRHealth (basic suite) - bluetooth_AdapterSRHealth.sr_reconnect_classic_hid_stress - bluetooth_AdapterSRHealth.sr_reconnect_le_hid_stress Thanks, Abhishek Abhishek Pandit-Subedi (3): Bluetooth: Rename and move clean_up_hci_state Bluetooth: Add quirk to power down on suspend Bluetooth: btmrvl_sdio: Power down when suspending drivers/bluetooth/btmrvl_sdio.c | 10 include/net/bluetooth/hci.h | 7 +++ include/net/bluetooth/hci_core.h | 6 +++ net/bluetooth/hci_core.c | 93 +++- net/bluetooth/hci_request.c | 26 - net/bluetooth/mgmt.c | 46 +--- 6 files changed, 140 insertions(+), 48 deletions(-) -- 2.29.2.299.gdc1121823c-goog
Re: [PATCH v1] Bluetooth: hci_qca: Handle spurious wakeup from SoC
Hi Venkata,
I think this code would be simplified by using a delayed_work struct
instead of a timer.
Based on your commit description:
On Sun, Nov 15, 2020 at 9:59 AM Venkata Lakshmi Narayana Gubba
wrote:
>
> Added timer to handle spurious wakeup from SoC.
> Timer is started when wake indicator is received from SoC.
> Timer is restarted when valid data is received from SoC.
> Timer is stopped when sleep indicator is received from SoC.
> SSR is triggered upon timer expiry.
>
> Signed-off-by: Venkata Lakshmi Narayana Gubba
in function qca_ibs_wake_ind: (timer started when wake indicator is
received from SoC)
queue_delayed_work(qca->workqueue, >spurious_wake,
IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS)
in function qca_ibs_sleep_ind: (Timer is stopped when sleep indicator
is received from SoC.)
cancel_delayed_work(qca->workqueue, >spurious_wake);
in qca_recv_acl_data, qca_recv_sco_data and qca_recv_event: (Timer is
restarted when valid data is received from SoC.)
if (!test_bit(QCA_IBS_DISABLED, >flags))
mod_delayed_work(qca->workqueue, >spurious_wake,
IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS)
and finally in qca_ibs_spurious_wake_timeout (originally named
hci_ibs_spurious_wake_timeout in your patch): (SSR is triggered upon
timer expiry.)
if (!test_bit(QCA_HW_ERROR_EVENT, >flags))
hci_reset_dev(hu->hdev);
That should trigger qca_hw_error so you don't need to duplicate the
crash dump triggering + waiting in multiple places (and if you get the
spurious wake bug WHILE doing SSR, it won't re-trigger the same
restart over and over).
Abhishek
> ---
> drivers/bluetooth/hci_qca.c | 99
> -
> 1 file changed, 97 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
> index 5cc7b16..6953001 100644
> --- a/drivers/bluetooth/hci_qca.c
> +++ b/drivers/bluetooth/hci_qca.c
> @@ -48,6 +48,7 @@
> #define IBS_WAKE_RETRANS_TIMEOUT_MS100
> #define IBS_BTSOC_TX_IDLE_TIMEOUT_MS 200
> #define IBS_HOST_TX_IDLE_TIMEOUT_MS2000
> +#define IBS_SOC_SPURIOUS_WAKE_TIMEOUT_MS 1
> #define CMD_TRANS_TIMEOUT_MS 100
> #define MEMDUMP_TIMEOUT_MS 8000
> #define IBS_DISABLE_SSR_TIMEOUT_MS (MEMDUMP_TIMEOUT_MS + 1000)
> @@ -147,7 +148,9 @@ struct qca_data {
> bool tx_vote; /* Clock must be on for TX */
> bool rx_vote; /* Clock must be on for RX */
> struct timer_list tx_idle_timer;
> + struct timer_list spurious_wake_timer;
> u32 tx_idle_delay;
> + u32 spurious_wake;
> struct timer_list wake_retrans_timer;
> u32 wake_retrans;
> struct workqueue_struct *workqueue;
> @@ -156,6 +159,7 @@ struct qca_data {
> struct work_struct ws_rx_vote_off;
> struct work_struct ws_tx_vote_off;
> struct work_struct ctrl_memdump_evt;
> + struct work_struct spurious_wake_timeout;
> struct delayed_work ctrl_memdump_timeout;
> struct qca_memdump_data *qca_memdump;
> unsigned long flags;
> @@ -229,6 +233,7 @@ static void qca_regulator_disable(struct qca_serdev
> *qcadev);
> static void qca_power_shutdown(struct hci_uart *hu);
> static int qca_power_off(struct hci_dev *hdev);
> static void qca_controller_memdump(struct work_struct *work);
> +static void qca_wq_spurious_wake_timeout(struct work_struct *work);
>
> static enum qca_btsoc_type qca_soc_type(struct hci_uart *hu)
> {
> @@ -530,6 +535,15 @@ static void hci_ibs_wake_retrans_timeout(struct
> timer_list *t)
> hci_uart_tx_wakeup(hu);
> }
>
> +static void hci_ibs_spurious_wake_timeout(struct timer_list *t)
> +{
> + struct qca_data *qca = from_timer(qca, t, spurious_wake_timer);
> + struct hci_uart *hu = qca->hu;
> +
> + bt_dev_warn(hu->hdev, "hu %p spurious wake timeout in %d state", hu,
> qca->rx_ibs_state);
> +
> + queue_work(qca->workqueue, >spurious_wake_timeout);
> +}
>
> static void qca_controller_memdump_timeout(struct work_struct *work)
> {
> @@ -584,6 +598,7 @@ static int qca_open(struct hci_uart *hu)
> INIT_WORK(>ws_rx_vote_off, qca_wq_serial_rx_clock_vote_off);
> INIT_WORK(>ws_tx_vote_off, qca_wq_serial_tx_clock_vote_off);
> INIT_WORK(>ctrl_memdump_evt, qca_controller_memdump);
> + INIT_WORK(>spurious_wake_timeout, qca_wq_spurious_wake_timeout);
> INIT_DELAYED_WORK(>ctrl_memdump_timeout,
> qca_controller_memdump_timeout);
> init_waitqueue_head(>suspend_wait_q);
> @@ -615,6 +630,9 @@ static int qca_open(struct hci_uart *hu)
> timer_setup(>tx_idle_timer, hci_ibs_tx_idle_timeou
Re: [PATCH v2] ath10k: Fix the parsing error in service available event
Hi All, The V2 patch now has good comments and probably spinning off a new V3 might be a good idea. Here are a few comments to the discussion. In response to Doug's comment > case WMI_TLV_TAG_FIRST_ARRAY_ENUM: > arg->service_map_ext_len = 0; > arg->service_map_ext = NULL; > return 0; Since the TLV messages are parsed iteratively for each tag, if WMI_TLV_TAG_FIRST_ARRAY_ENUM this comes as the last TLV tag then this might cause the map_len to be zero even if there is a valid tag like WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT , so having a "valid" flag seems to be a better and scalable approach. > > The TLV TAG " WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT" is the first > > and a mandatory TLV in the service available event. The subsequent > > TLVs are optional ones and may or may not be present (based on FW > > versions). > > From ath10k point of view never trust what the firmware sends you. Even > if WMI_TLV_TAG_STRUCT_SERVICE_AVAILABLE_EVENT is a mandatory TLV it > might be missing for whatever reasons. The same is with buffer lengths > etc and always confirm what you are receiving from the firmware. > Looks like the length for each tag is already being validated in ath10k_wmi_tlv_iter() and would return error if the length does not match against the wmi policy., so I think the tlv message validation is already being done. Kalle, Is the expectation here is to do anything additional ? -Abhishek On Thu, Nov 5, 2020 at 11:25 PM Kalle Valo wrote: > > Doug Anderson writes: > > >> static int ath10k_wmi_tlv_op_pull_svc_avail(struct ath10k *ar, > >> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c > >> b/drivers/net/wireless/ath/ath10k/wmi.c > >> index 1fa7107..2e4b561 100644 > >> --- a/drivers/net/wireless/ath/ath10k/wmi.c > >> +++ b/drivers/net/wireless/ath/ath10k/wmi.c > >> @@ -5751,8 +5751,9 @@ void ath10k_wmi_event_service_available(struct > >> ath10k *ar, struct sk_buff *skb) > >> ret); > >> } > >> > >> - ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, ar->wmi.svc_map, > >> - __le32_to_cpu(arg.service_map_ext_len)); > >> + if (arg.service_map_ext_valid) > >> + ath10k_wmi_map_svc_ext(ar, arg.service_map_ext, > >> ar->wmi.svc_map, > >> + > >> __le32_to_cpu(arg.service_map_ext_len)); > > > > Your new patch still requires the caller to init the > > "service_map_ext_valid" to false before calling, but I guess there's > > not a whole lot more we can do because we might be parsing more than > > one tag. It does seem nice that at least we now have a validity bit > > instead of just relying on a non-zero length to be valid. > > > > It might be nice to have a comment saying that it's up to us to init > > "arg.service_map_ext_valid" to false before calling > > ath10k_wmi_pull_svc_avail(), but I won't insist. Maybe that's obvious > > to everyone but me... > > It's not obvious to me either. Please add that comment. > > BTW, for some reason Doug's response didn't get to patchwork: > > https://patchwork.kernel.org/project/linux-wireless/patch/1603904469-598-1-git-send-email-pill...@codeaurora.org/ > > Though I do see it in linux-wireless, so most likely this was a > temporary glitch in patchwork. But it's just worrisome as nowadays I > only check the comments in patchwork before I apply the patch. > > -- > https://patchwork.kernel.org/project/linux-wireless/list/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
[PATCH v2 0/1] This patch address comments on patch v1
This patch extends ath10k_core_create_board_name function to support chip id based BDF selection and not add provision for fallback_boardname2, thus introducing lesser lines of code. (no changes since v1) Abhishek Kumar (1): ath10k: add option for chip-id based BDF selection drivers/net/wireless/ath/ath10k/core.c | 12 +--- 1 file changed, 9 insertions(+), 3 deletions(-) -- 2.29.2.222.g5d2a92d10f8-goog
[PATCH v2 1/1] ath10k: add option for chip-id based BDF selection
In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.
The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
Tested-on: WCN3990 hw1.0 SNOC WLAN.HL.3.2.2-00696-QCAHLSWMTPL-1
Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1
Signed-off-by: Abhishek Kumar
---
(no changes since v1)
drivers/net/wireless/ath/ath10k/core.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index d73ad60b571c..fa9e676b26d9 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1419,12 +1419,13 @@ static int ath10k_core_fetch_board_data_api_n(struct
ath10k *ar,
}
static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len,
+bool with_additional_params)
{
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
- if (with_variant && ar->id.bdf_ext[0] != '\0')
+ if (with_additional_params && ar->id.bdf_ext[0] != '\0')
scnprintf(variant, sizeof(variant), ",variant=%s",
ar->id.bdf_ext);
@@ -1438,12 +1439,17 @@ static int ath10k_core_create_board_name(struct ath10k
*ar, char *name,
}
if (ar->id.qmi_ids_valid) {
- if (with_variant && ar->id.bdf_ext[0] != '\0')
+ if (with_additional_params && ar->id.bdf_ext[0] != '\0')
scnprintf(name, name_len,
"bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
ath10k_bus_str(ar->hif.bus),
ar->id.qmi_board_id, ar->id.qmi_chip_id,
variant);
+ else if (with_additional_params)
+ scnprintf(name, name_len,
+ "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+ ath10k_bus_str(ar->hif.bus),
+ ar->id.qmi_board_id, ar->id.qmi_chip_id);
else
scnprintf(name, name_len,
"bus=%s,qmi-board-id=%x",
--
2.29.2.222.g5d2a92d10f8-goog
Re: [PATCH] ath10k: add option for chip-id based BDF selection
Apologies for the delay, was busy so could not work on V2 . I have started working on V2 patch. Will upload by today/tomorrow. Abhishek On Thu, Nov 5, 2020 at 11:11 PM Kalle Valo wrote: > > Abhishek Kumar wrote: > > > In some devices difference in chip-id should be enough to pick > > the right BDF. Add another support for chip-id based BDF selection. > > With this new option, ath10k supports 2 fallback options. > > > > The board name with chip-id as option looks as follows > > board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320' > > > > Signed-off-by: Abhishek Kumar > > Reviewed-by: Douglas Anderson > > Tested-by: Douglas Anderson > > Tested-by: Abhishek Kumar > > There were few checkpatch warnings which I fixed: > > $ ath10k-check > drivers/net/wireless/ath/ath10k/core.c:1501: Alignment should match open > parenthesis > drivers/net/wireless/ath/ath10k/core.c:1512: line length of 92 exceeds 90 > columns > drivers/net/wireless/ath/ath10k/core.c:1521: line length of 92 exceeds 90 > columns > > The first one was also what Doug commented. I also added Tested-on tags, > thanks for those. The updated patch is in pending branch (soon). > > But is this patch ok to take now? I didn't quite get the conclusion of the > discussion. > > -- > https://patchwork.kernel.org/project/linux-wireless/patch/2020102506.1.Ifbc28707942179f1cefc7491e995814564495270@changeid/ > > https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches >
[RFC v3] cpuidle : Add support for pseudo-cpuidle driver
This option adds support for a testing cpuidle driver, which allows
user to define custom idle states with their respective latencies and
residencies. This is useful for testing the behaviour of governors on
customized set of idle states.
This can be used as of now by specifying a customized set of cpuidle
states in the driver by the user. Will add the capability of this
driver to be used as a module in subsequent patches.
Original idea and discussion for this patch can be found at:
https://lkml.org/lkml/2019/12/17/655
Signed-off-by: Abhishek Goel
---
v1->v2 : Changes adapted from reviews of Randy, Dan and Nathan.
There was a divergence from expected behaviour of idle loop
in v1. That has been fixed.
v2->v3 : Added simulated latency while entering into idle states.
drivers/cpuidle/Kconfig| 9 +
drivers/cpuidle/Makefile | 1 +
drivers/cpuidle/cpuidle-test.c | 289 +
3 files changed, 299 insertions(+)
create mode 100644 drivers/cpuidle/cpuidle-test.c
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd66f02..1a0f227d61d6 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -71,6 +71,15 @@ config HALTPOLL_CPUIDLE
before halting in the guest (more efficient than polling in the
host via halt_poll_ns for some scenarios).
+config TEST_CPUIDLE
+ tristate "cpuidle test driver"
+ default n
+ help
+ This option enables a testing cpuidle driver, which allows the user
+ to define custom idle states with their respective latencies &
residencies.
+ This is useful for testing the behaviour of governors on different
+ sets of idle states.
+
endif
config ARCH_NEEDS_CPU_IDLE_COUPLED
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f07800cbb43f..68ea7dc257b5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
+obj-$(CONFIG_TEST_CPUIDLE) += cpuidle-test.o
##
# ARM SoC drivers
diff --git a/drivers/cpuidle/cpuidle-test.c b/drivers/cpuidle/cpuidle-test.c
new file mode 100644
index ..17ffce11cafc
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-test.c
@@ -0,0 +1,289 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cpuidle-test - Test driver for cpuidle.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CPUIDLE_STATE_MAX 10
+#define MAX_PARAM_LENGTH 100
+
+static unsigned int nr_states = 4;
+static unsigned int sim_type = 1;
+static char name[MAX_PARAM_LENGTH];
+static char latency_us[MAX_PARAM_LENGTH];
+static char residency_us[MAX_PARAM_LENGTH];
+
+
+module_param(nr_states, uint, 0644);
+module_param(sim_type, uint, 0644);
+module_param_string(name, name, MAX_PARAM_LENGTH, 0644);
+module_param_string(latency_us, latency_us, MAX_PARAM_LENGTH, 0644);
+module_param_string(residency_us, residency_us, MAX_PARAM_LENGTH, 0644);
+
+static struct cpuidle_driver test_cpuidle_driver = {
+ .name = "test_cpuidle",
+ .owner = THIS_MODULE,
+};
+
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+
+static struct cpuidle_device __percpu *test_cpuidle_devices;
+static enum cpuhp_state test_hp_idlestate;
+
+
+static int __cpuidle idle_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ u64 time_start;
+
+ local_irq_enable();
+
+ time_start = local_clock();
+ /*
+* Simulating entry latency into idle state.
+*/
+ while (local_clock() - time_start < drv->states[index].exit_latency) {
+ }
+
+ if (!current_set_polling_and_test()) {
+ while (!need_resched())
+ cpu_relax();
+ }
+
+ time_start = local_clock();
+ /*
+* Simulating exit latency from idle state.
+*/
+ while (local_clock() - time_start < drv->states[index].exit_latency) {
+ }
+
+ current_clr_polling();
+
+ return index;
+}
+
+ /*
+* Defining user specified custome set of idle states.
+*/
+static struct cpuidle_state cpuidle_states[CPUIDLE_STATE_MAX] = {
+ { .name = "snooze",
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = idle_loop },
+};
+
+static struct cpuidle_state cpuidle_states_ppc[] = {
+ { .name = "snooze",
+ .exit_latency = 0,
+
Re: [PATCH] ath10k: add option for chip-id based BDF selection
Additionally tested on ath10k based non-QMI platform Tested-on: QCA6174 HW3.2 WLAN.RM.4.4.1-00157-QCARMSWPZ-1 Tested-by: Abhishek Kumar -Abhishek
Re: [PATCH 1/1] kobject: Don't emit change events if not in sysfs
Hi Greg, I was debugging without a live repro and I was told this patch improved behavior but it's only by chance (someone bisected a Dell D6000 dock's displayport issue to this commit and this change seemed to help; udev logs later shows that's not the case). I took another look at device_init_wakeup and I can see that device_set_wakeup_capable does indeed check for device_is_registered before adding the wakeup attributes so the ordering of events I suspected cannot occur. Thanks for pushing back Greg. It made me take a deeper look at an assumption I hadn't challenged. Please consider this patch abandoned. Abhishek On Mon, Oct 19, 2020 at 10:56 PM Greg Kroah-Hartman wrote: > > On Mon, Oct 19, 2020 at 03:32:57PM -0700, Abhishek Pandit-Subedi wrote: > > Add a check to make sure the kobj is created and in sysfs before sending > > a change event notification. Otherwise, udev rules that depend on the > > change notification may find that the path that changed doesn't actually > > exist. > > Why is the user of the kobject trying to emit a uevent before it is > registered? Shouldn't we fix the root problem here instead? Otherwise > the event is still "gone", the caller will not know what to do about it. > > Please fix the root problem here. > > thanks, > > greg k-h
Re: Update WCN3991 FW file
It looks like we hadn't merged an earlier update to crnv32.bin in ChromeOS: ad1da95 - QCA : Updated firmware files for WCN3991 (3 weeks ago) After merging that commit, everything is working as expected. -- Tested-by: Abhishek Pandit-Subedi On Mon, Oct 19, 2020 at 2:37 PM Abhishek Pandit-Subedi wrote: > > Nack. > > This resulted in a boot loop on ChromeOS. It looks like only > 'crbtfw32.tlv' was changed and not 'crnv32.bin'. > > Abhishek > > On Sat, Oct 17, 2020 at 8:33 AM wrote: > > > > > > Hi Team, > > > > Please include updated firmware bin for WCN3991. > > > > Snapshot of pull request is as below, let me know if anything is > > missing. > > > > >>>>> > > > > The following changes since commit > > 58d41d0facca2478d3e45f6321224361519aee96: > > > >ice: Add comms package file for Intel E800 series driver (2020-10-05 > > 08:09:03 -0400) > > > > are available in the git repository at: > > > >https://github.com/shahasit/bt-linux-firmware/tree/master > > > > for you to fetch changes up to 8877322c1254f327f47c86ec02c46013b68b9a47: > > > >QCA : Updated firmware file for WCN3991 (2020-10-17 20:53:36 +0530) > > > > > > Asit Shah (1): > >QCA : Updated firmware file for WCN3991 > > > > qca/crbtfw32.tlv | Bin 126300 -> 126832 bytes > > 1 file changed, 0 insertions(+), 0 deletions(-) > > > > <<<<<< > > > > Regards, > > Asit
[PATCH] ath10k: add option for chip-id based BDF selection
In some devices difference in chip-id should be enough to pick
the right BDF. Add another support for chip-id based BDF selection.
With this new option, ath10k supports 2 fallback options.
The board name with chip-id as option looks as follows
board name 'bus=snoc,qmi-board-id=ff,qmi-chip-id=320'
Signed-off-by: Abhishek Kumar
---
drivers/net/wireless/ath/ath10k/core.c | 45 +++---
1 file changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/net/wireless/ath/ath10k/core.c
b/drivers/net/wireless/ath/ath10k/core.c
index a3f15cc89a10..767bb9d6a197 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1343,7 +1343,8 @@ static int ath10k_core_search_bd(struct ath10k *ar,
static int ath10k_core_fetch_board_data_api_n(struct ath10k *ar,
const char *boardname,
- const char *fallback_boardname,
+ const char *fallback_boardname1,
+ const char *fallback_boardname2,
const char *filename)
{
size_t len, magic_len;
@@ -1392,8 +1393,11 @@ static int ath10k_core_fetch_board_data_api_n(struct
ath10k *ar,
ret = ath10k_core_search_bd(ar, boardname, data, len);
/* if we didn't find it and have a fallback name, try that */
- if (ret == -ENOENT && fallback_boardname)
- ret = ath10k_core_search_bd(ar, fallback_boardname, data, len);
+ if (ret == -ENOENT && fallback_boardname1)
+ ret = ath10k_core_search_bd(ar, fallback_boardname1, data, len);
+
+ if (ret == -ENOENT && fallback_boardname2)
+ ret = ath10k_core_search_bd(ar, fallback_boardname2, data, len);
if (ret == -ENOENT) {
ath10k_err(ar,
@@ -1413,7 +1417,8 @@ static int ath10k_core_fetch_board_data_api_n(struct
ath10k *ar,
}
static int ath10k_core_create_board_name(struct ath10k *ar, char *name,
-size_t name_len, bool with_variant)
+size_t name_len, bool with_variant,
+bool with_chip_id)
{
/* strlen(',variant=') + strlen(ar->id.bdf_ext) */
char variant[9 + ATH10K_SMBIOS_BDF_EXT_STR_LENGTH] = { 0 };
@@ -1432,12 +1437,17 @@ static int ath10k_core_create_board_name(struct ath10k
*ar, char *name,
}
if (ar->id.qmi_ids_valid) {
- if (with_variant && ar->id.bdf_ext[0] != '\0')
+ if (with_variant && ar->id.bdf_ext[0] != '\0' && with_chip_id)
scnprintf(name, name_len,
"bus=%s,qmi-board-id=%x,qmi-chip-id=%x%s",
ath10k_bus_str(ar->hif.bus),
ar->id.qmi_board_id, ar->id.qmi_chip_id,
variant);
+ else if (with_chip_id)
+ scnprintf(name, name_len,
+ "bus=%s,qmi-board-id=%x,qmi-chip-id=%x",
+ ath10k_bus_str(ar->hif.bus),
+ ar->id.qmi_board_id, ar->id.qmi_chip_id);
else
scnprintf(name, name_len,
"bus=%s,qmi-board-id=%x",
@@ -1476,21 +1486,33 @@ static int ath10k_core_create_eboard_name(struct ath10k
*ar, char *name,
int ath10k_core_fetch_board_file(struct ath10k *ar, int bd_ie_type)
{
- char boardname[100], fallback_boardname[100];
+ char boardname[100], fallback_boardname1[100], fallback_boardname2[100];
int ret;
if (bd_ie_type == ATH10K_BD_IE_BOARD) {
+ /* With variant and chip id */
ret = ath10k_core_create_board_name(ar, boardname,
- sizeof(boardname), true);
+ sizeof(boardname), true, true);
if (ret) {
ath10k_err(ar, "failed to create board name: %d", ret);
return ret;
}
- ret = ath10k_core_create_board_name(ar, fallback_boardname,
- sizeof(boardname), false);
+ /* Without variant and only chip-id */
+ ret = ath10k_core_create_board_name(ar, fallback_boardname1,
+ sizeof(boardname), false,
+ true);
+ if (ret) {
+ ath10k_err(ar, "failed to create 1st fallback board
name: %d", ret);
+ return ret;
+
[PATCH 1/1] kobject: Don't emit change events if not in sysfs
Add a check to make sure the kobj is created and in sysfs before sending
a change event notification. Otherwise, udev rules that depend on the
change notification may find that the path that changed doesn't actually
exist.
Fixes: a45aca510b73b7 (PM: sleep: core: Emit changed uevent on
wakeup_sysfs_add/remove)
Signed-off-by: Abhishek Pandit-Subedi
---
lib/kobject_uevent.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/lib/kobject_uevent.c b/lib/kobject_uevent.c
index 7998affa45d49a..f08197e907d5ce 100644
--- a/lib/kobject_uevent.c
+++ b/lib/kobject_uevent.c
@@ -473,6 +473,11 @@ int kobject_uevent_env(struct kobject *kobj, enum
kobject_action action,
if (action == KOBJ_REMOVE)
kobj->state_remove_uevent_sent = 1;
+ if (action == KOBJ_CHANGE && !kobj->state_in_sysfs) {
+ pr_debug("kobject: can't emit KOBJ_CHANGE until in sysfs\n");
+ return -EINVAL;
+ }
+
pr_debug("kobject: '%s' (%p): %s\n",
kobject_name(kobj), kobj, __func__);
--
2.29.0.rc1.297.gfa9743e501-goog
[PATCH 0/1] kobject: Don't emit change events if not in sysfs
Hi maintainers, A little while ago, I got a bug report of a regression caused by a patch I submitted a45aca510b73b7 (PM: sleep: core: Emit changed uevent on wakeup_sysfs_add/remove) https://bugzilla.kernel.org/show_bug.cgi?id=209469 It seems possible for a "change" event to be sent before the device is added to the sysfs (so when the rule runs, it can't access the device path that emitted it). The bug report had the following log that made me identify this is possible: > Use global config file: /etc/usb_modeswitch.conf > Use top device dir /sys/bus/usb/devices/2-3 > Check class of first interface ... > No access to first interface. Exit I've added a patch to fix the former problem here and confirmed via udevadm monitor that no CHANGE requests are seen for devices before they emit the ADD event. Thanks Abhishek Abhishek Pandit-Subedi (1): kobject: Don't emit change events if not in sysfs lib/kobject_uevent.c | 5 + 1 file changed, 5 insertions(+) -- 2.29.0.rc1.297.gfa9743e501-goog
Re: Update WCN3991 FW file
Nack. This resulted in a boot loop on ChromeOS. It looks like only 'crbtfw32.tlv' was changed and not 'crnv32.bin'. Abhishek On Sat, Oct 17, 2020 at 8:33 AM wrote: > > > Hi Team, > > Please include updated firmware bin for WCN3991. > > Snapshot of pull request is as below, let me know if anything is > missing. > > >>>>> > > The following changes since commit > 58d41d0facca2478d3e45f6321224361519aee96: > >ice: Add comms package file for Intel E800 series driver (2020-10-05 > 08:09:03 -0400) > > are available in the git repository at: > >https://github.com/shahasit/bt-linux-firmware/tree/master > > for you to fetch changes up to 8877322c1254f327f47c86ec02c46013b68b9a47: > >QCA : Updated firmware file for WCN3991 (2020-10-17 20:53:36 +0530) > > > Asit Shah (1): >QCA : Updated firmware file for WCN3991 > > qca/crbtfw32.tlv | Bin 126300 -> 126832 bytes > 1 file changed, 0 insertions(+), 0 deletions(-) > > <<<<<< > > Regards, > Asit
[PATCH] Bluetooth: hci_h5: Add driver capabilities for RTL8822CS
Certain controller capabilities must be exposed by the driver because it
can't be queried from HCI (wideband speech support, for example). Update
the match data structure to set the supported capabilities and set the
proper quirks on hdev after registering the device.
Also update the 8822CS capabilities to show it supports wideband speech
and has valid le states (allows central peripheral role).
Signed-off-by: Abhishek Pandit-Subedi
---
drivers/bluetooth/hci_h5.c | 53 +++---
1 file changed, 44 insertions(+), 9 deletions(-)
diff --git a/drivers/bluetooth/hci_h5.c b/drivers/bluetooth/hci_h5.c
index a10d710fc3f13e..3833a2d276665f 100644
--- a/drivers/bluetooth/hci_h5.c
+++ b/drivers/bluetooth/hci_h5.c
@@ -97,6 +97,11 @@ struct h5 {
struct gpio_desc *device_wake_gpio;
};
+enum h5_capabilities {
+ H5_CAP_WIDEBAND_SPEECH = BIT(0),
+ H5_CAP_VALID_LE_STATES = BIT(1),
+};
+
struct h5_vnd {
int (*setup)(struct h5 *h5);
void (*open)(struct h5 *h5);
@@ -106,6 +111,11 @@ struct h5_vnd {
const struct acpi_gpio_mapping *acpi_gpio_map;
};
+struct h5_device_data {
+ uint32_t capabilities;
+ struct h5_vnd *vnd;
+};
+
static void h5_reset_rx(struct h5 *h5);
static void h5_link_control(struct hci_uart *hu, const void *data, size_t len)
@@ -791,7 +801,10 @@ static const struct hci_uart_proto h5p = {
static int h5_serdev_probe(struct serdev_device *serdev)
{
struct device *dev = >dev;
+ struct hci_dev *hdev;
struct h5 *h5;
+ const struct h5_device_data *data;
+ int err;
h5 = devm_kzalloc(dev, sizeof(*h5), GFP_KERNEL);
if (!h5)
@@ -808,23 +821,21 @@ static int h5_serdev_probe(struct serdev_device *serdev)
if (!match)
return -ENODEV;
- h5->vnd = (const struct h5_vnd *)match->driver_data;
+ data = (const struct h5_device_data *)match->driver_data;
+ h5->vnd = data->vnd;
h5->id = (char *)match->id;
if (h5->vnd->acpi_gpio_map)
devm_acpi_dev_add_driver_gpios(dev,
h5->vnd->acpi_gpio_map);
} else {
- const void *data;
-
data = of_device_get_match_data(dev);
if (!data)
return -ENODEV;
- h5->vnd = (const struct h5_vnd *)data;
+ h5->vnd = data->vnd;
}
-
h5->enable_gpio = devm_gpiod_get_optional(dev, "enable", GPIOD_OUT_LOW);
if (IS_ERR(h5->enable_gpio))
return PTR_ERR(h5->enable_gpio);
@@ -834,7 +845,20 @@ static int h5_serdev_probe(struct serdev_device *serdev)
if (IS_ERR(h5->device_wake_gpio))
return PTR_ERR(h5->device_wake_gpio);
- return hci_uart_register_device(>serdev_hu, );
+ err = hci_uart_register_device(>serdev_hu, );
+ if (err)
+ return err;
+
+ hdev = h5->serdev_hu.hdev;
+
+ /* Set match specific quirks */
+ if (data->capabilities & H5_CAP_WIDEBAND_SPEECH)
+ set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, >quirks);
+
+ if (data->capabilities & H5_CAP_VALID_LE_STATES)
+ set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
+
+ return 0;
}
static void h5_serdev_remove(struct serdev_device *serdev)
@@ -1000,12 +1024,21 @@ static struct h5_vnd rtl_vnd = {
.resume = h5_btrtl_resume,
.acpi_gpio_map = acpi_btrtl_gpios,
};
+
+static const struct h5_device_data h5_data_rtl8822cs = {
+ .capabilities = H5_CAP_WIDEBAND_SPEECH | H5_CAP_VALID_LE_STATES,
+ .vnd = _vnd,
+};
+
+static const struct h5_device_data h5_data_rtl8723bs = {
+ .vnd = _vnd,
+};
#endif
#ifdef CONFIG_ACPI
static const struct acpi_device_id h5_acpi_match[] = {
#ifdef CONFIG_BT_HCIUART_RTL
- { "OBDA8723", (kernel_ulong_t)_vnd },
+ { "OBDA8723", (kernel_ulong_t)_data_rtl8723bs},
#endif
{ },
};
@@ -1019,9 +1052,11 @@ static const struct dev_pm_ops h5_serdev_pm_ops = {
static const struct of_device_id rtl_bluetooth_of_match[] = {
#ifdef CONFIG_BT_HCIUART_RTL
{ .compatible = "realtek,rtl8822cs-bt",
- .data = (const void *)_vnd },
+ .data = _data_rtl8822cs,
+ },
{ .compatible = "realtek,rtl8723bs-bt",
- .data = (const void *)_vnd },
+ .data = _data_rtl8723bs,
+ },
#endif
{ },
};
--
2.28.0.1011.ga647a8990f-goog
Re: [PATCH v1] Bluetooth: hci_qca: Wait for timeout during suspend
Reviewed-by: Abhishek Pandit-Subedi On Tue, Oct 6, 2020 at 8:20 AM Balakrishna Godavarthi wrote: > > From: Venkata Lakshmi Narayana Gubba > > Currently qca_suspend() is relied on IBS mechanism. During > FW download and memory dump collections, IBS will be disabled. > In those cases, driver will allow suspend and still uses the > serdev port, which results to errors. Now added a wait timeout > if suspend is triggered during FW download and memory collections. > > Signed-off-by: Venkata Lakshmi Narayana Gubba > Signed-off-by: Balakrishna Godavarthi
Re: [PATCH v1] Bluetooth: hci_qca: Enhance retry logic in qca_setup
Reviewed-by: Abhishek Pandit-Subedi On Mon, Oct 5, 2020 at 11:33 PM Balakrishna Godavarthi wrote: > > Currently driver only retries to download FW if FW downloading > is failed. Sometimes observed command timeout for version request > command, if this happen on some platforms during boot time, then > a reboot is needed to turn ON BT. Instead to avoid a reboot, now > extended retry logic for version request command too. > > Signed-off-by: Balakrishna Godavarthi
[PATCH] Bluetooth: btqca: Add valid le states quirk
WCN3991 supports connectable advertisements so we need to add the valid
le states quirk so the 'central-peripheral' role is exposed in
userspace.
Signed-off-by: Abhishek Pandit-Subedi
---
Example result showing the central-peripheral role correctly.
localhost # bluetoothctl show
Controller 3C:28:6D:4F:A0:1F (public)
Name: BlueZ 5.54
Alias: Chromebook_63FC
Class: 0x0048
Powered: yes
Discoverable: no
DiscoverableTimeout: 0x00b4
Pairable: yes
UUID: A/V Remote Control(110e--1000-8000-00805f9b34fb)
UUID: Audio Source (110a--1000-8000-00805f9b34fb)
UUID: Handsfree Audio Gateway (111f--1000-8000-00805f9b34fb)
UUID: PnP Information (1200--1000-8000-00805f9b34fb)
UUID: A/V Remote Control Target (110c--1000-8000-00805f9b34fb)
UUID: Generic Access Profile(1800--1000-8000-00805f9b34fb)
UUID: Generic Attribute Profile (1801--1000-8000-00805f9b34fb)
UUID: Device Information(180a--1000-8000-00805f9b34fb)
Modalias: bluetooth:v00E0pC405d0057
Discovering: no
Roles: central
Roles: peripheral
Roles: central-peripheral
drivers/bluetooth/hci_qca.c | 19 +--
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
index 244b8feba52327..2d3f1f179a1e3d 100644
--- a/drivers/bluetooth/hci_qca.c
+++ b/drivers/bluetooth/hci_qca.c
@@ -78,6 +78,7 @@ enum qca_flags {
enum qca_capabilities {
QCA_CAP_WIDEBAND_SPEECH = BIT(0),
+ QCA_CAP_VALID_LE_STATES = BIT(1),
};
/* HCI_IBS transmit side sleep protocol states */
@@ -1780,7 +1781,7 @@ static const struct qca_device_data qca_soc_data_wcn3991
= {
{ "vddch0", 45 },
},
.num_vregs = 4,
- .capabilities = QCA_CAP_WIDEBAND_SPEECH,
+ .capabilities = QCA_CAP_WIDEBAND_SPEECH | QCA_CAP_VALID_LE_STATES,
};
static const struct qca_device_data qca_soc_data_wcn3998 = {
@@ -2017,11 +2018,17 @@ static int qca_serdev_probe(struct serdev_device
*serdev)
hdev->shutdown = qca_power_off;
}
- /* Wideband speech support must be set per driver since it can't be
-* queried via hci.
-*/
- if (data && (data->capabilities & QCA_CAP_WIDEBAND_SPEECH))
- set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED, >quirks);
+ if (data) {
+ /* Wideband speech support must be set per driver since it can't
+* be queried via hci. Same with the valid le states quirk.
+*/
+ if (data->capabilities & QCA_CAP_WIDEBAND_SPEECH)
+ set_bit(HCI_QUIRK_WIDEBAND_SPEECH_SUPPORTED,
+ >quirks);
+
+ if (data->capabilities & QCA_CAP_VALID_LE_STATES)
+ set_bit(HCI_QUIRK_VALID_LE_STATES, >quirks);
+ }
return 0;
}
--
2.28.0.709.gb0816b6eb0-goog
Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume
+ Alex Lu (who contributed the original change) Hi Kai-Heng, On Thu, Sep 24, 2020 at 12:10 AM Kai-Heng Feng wrote: > > [+Cc linux-usb] > > Hi Abhishek, > > > On Sep 24, 2020, at 04:41, Abhishek Pandit-Subedi > > wrote: > > > > Hi Kai-Heng, > > > > Which Realtek controller is this on?' > > The issue happens on 8821CE. > > > > > Specifically for RTL8822CE, we tested without reset_resume being set > > and that was causing the controller being reset without bluez ever > > learning about it (resulting in devices being unusable without > > toggling the BT power). > > The reset is done by the kernel, so how does that affect bluez? > > From what you described, it sounds more like runtime resume since bluez is > already running. > If we need reset resume for runtime resume, maybe it's another bug which > needs to be addressed? >From btusb.c: >https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/tree/drivers/bluetooth/btusb.c#n4189 /* Realtek devices lose their updated firmware over global * suspend that means host doesn't send SET_FEATURE * (DEVICE_REMOTE_WAKEUP) */ Runtime suspend always requires remote wakeup to be set and reset resume isn't used there. During system suspend, when remote wakeup is not set, RTL8822CE loses the FW loaded by the driver and any state currently in the controller. This causes the kernel and the controller state to go out of sync. One of the issues we observed on the Realtek controller without the reset resume quirk was that paired or connected devices would just stop working after resume. > > > If the firmware doesn't cut off power during suspend, maybe you > > shouldn't set the BTUSB_WAKEUP_DISABLE flag for that controller. > > We don't know beforehand if the platform firmware (BIOS for my case) will cut > power off or not. > > In general, laptops will cut off the USB power during S3. > When AC is plugged, some laptops cuts USB power off and some don't. This also > applies to many desktops. Not to mention there can be BIOS options to control > USB power under S3/S4/S5... > > So we don't know beforehand. > I think the confusion here stems from what is actually being turned off between our two boards and what we're referring to as firmware :) In your case, the Realtek controller retains firmware unless the platform cuts of power to USB (which it does during S3). In my case, the Realtek controller loses firmware when Remote Wakeup isn't set, even if the platform doesn't cut power to USB. In your case, since you don't need to enforce the 'Remote Wakeup' bit, if you unset the BTUSB_WAKEUP_DISABLE for that VID:PID, you should get the desirable behavior (which is actually the default behavior; remote wake will always be asserted instead of only during Runtime Suspend). @Alex -- What is the common behavior for Realtek controllers? Should we set BTUSB_WAKEUP_DISABLE only on RTL8822CE or should we unset it only on RTL8821CE? > > > > I would prefer this doesn't get accepted in its current state. > > Of course. > I think we need to find the root cause for your case before applying this one. > > Kai-Heng > > > > > Abhishek > > > > On Wed, Sep 23, 2020 at 10:56 AM Kai-Heng Feng > > wrote: > >> > >> Realtek bluetooth controller may fail to work after system sleep: > >> [ 1272.707670] Bluetooth: hci0: command 0x1001 tx timeout > >> [ 1280.835712] Bluetooth: hci0: RTL: HCI_OP_READ_LOCAL_VERSION failed > >> (-110) > >> > >> If platform firmware doesn't cut power off during suspend, the firmware > >> is considered retained in controller but the driver is still asking USB > >> core to perform a reset-resume. This can make bluetooth controller > >> unusable. > >> > >> So avoid unnecessary reset to resolve the issue. > >> > >> For devices that really lose power during suspend, USB core will detect > >> and handle reset-resume correctly. > >> > >> Signed-off-by: Kai-Heng Feng > >> --- > >> drivers/bluetooth/btusb.c | 8 +++- > >> 1 file changed, 3 insertions(+), 5 deletions(-) > >> > >> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c > >> index 8d2608ddfd08..de86ef4388f9 100644 > >> --- a/drivers/bluetooth/btusb.c > >> +++ b/drivers/bluetooth/btusb.c > >> @@ -4255,17 +4255,15 @@ static int btusb_suspend(struct usb_interface > >> *intf, pm_message_t message) > >>enable_irq(data->oob_wake_irq); > >>} > >> > >> - /* For global suspend, Realtek devices lose the loaded fw > &
Re: [PATCH] Bluetooth: btusb: Avoid unnecessary reset upon system resume
Hi Kai-Heng,
Which Realtek controller is this on?
Specifically for RTL8822CE, we tested without reset_resume being set
and that was causing the controller being reset without bluez ever
learning about it (resulting in devices being unusable without
toggling the BT power).
If the firmware doesn't cut off power during suspend, maybe you
shouldn't set the BTUSB_WAKEUP_DISABLE flag for that controller.
I would prefer this doesn't get accepted in its current state.
Abhishek
On Wed, Sep 23, 2020 at 10:56 AM Kai-Heng Feng
wrote:
>
> Realtek bluetooth controller may fail to work after system sleep:
> [ 1272.707670] Bluetooth: hci0: command 0x1001 tx timeout
> [ 1280.835712] Bluetooth: hci0: RTL: HCI_OP_READ_LOCAL_VERSION failed (-110)
>
> If platform firmware doesn't cut power off during suspend, the firmware
> is considered retained in controller but the driver is still asking USB
> core to perform a reset-resume. This can make bluetooth controller
> unusable.
>
> So avoid unnecessary reset to resolve the issue.
>
> For devices that really lose power during suspend, USB core will detect
> and handle reset-resume correctly.
>
> Signed-off-by: Kai-Heng Feng
> ---
> drivers/bluetooth/btusb.c | 8 +++-
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
> index 8d2608ddfd08..de86ef4388f9 100644
> --- a/drivers/bluetooth/btusb.c
> +++ b/drivers/bluetooth/btusb.c
> @@ -4255,17 +4255,15 @@ static int btusb_suspend(struct usb_interface *intf,
> pm_message_t message)
> enable_irq(data->oob_wake_irq);
> }
>
> - /* For global suspend, Realtek devices lose the loaded fw
> -* in them. But for autosuspend, firmware should remain.
> -* Actually, it depends on whether the usb host sends
> + /* For global suspend, Realtek devices lose the loaded fw in them if
> +* platform firmware cut power off. But for autosuspend, firmware
> +* should remain. Actually, it depends on whether the usb host sends
> * set feature (enable wakeup) or not.
> */
> if (test_bit(BTUSB_WAKEUP_DISABLE, >flags)) {
> if (PMSG_IS_AUTO(message) &&
> device_can_wakeup(>udev->dev))
> data->udev->do_remote_wakeup = 1;
> - else if (!PMSG_IS_AUTO(message))
> - data->udev->reset_resume = 1;
> }
>
> return 0;
> --
> 2.17.1
>
[RFC v2] cpuidle : Add support for pseudo-cpuidle driver
This option adds support for a testing cpuidle driver, which allows
user to define custom idle states with their respective latencies and
residencies. This is useful for testing the behaviour of governors on
customized set of idle states.
This can be used as of now by hard-coding the customized set of cpuidle
states in the driver. Will add the capability of this driver to be used
as a module in subsequent patches.
Original idea and discussion for this patch can be found at:
https://lkml.org/lkml/2019/12/17/655
Signed-off-by: Abhishek Goel
---
v1-> v2 : Includes minor changes adapted from reviews.
Also, there was a divergence from expected behaviour of idle
loop in v1. That has been fixed.
drivers/cpuidle/Kconfig| 9 ++
drivers/cpuidle/Makefile | 1 +
drivers/cpuidle/cpuidle-test.c | 283 +
3 files changed, 293 insertions(+)
create mode 100644 drivers/cpuidle/cpuidle-test.c
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd66f02..1a0f227d61d6 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -71,6 +71,15 @@ config HALTPOLL_CPUIDLE
before halting in the guest (more efficient than polling in the
host via halt_poll_ns for some scenarios).
+config TEST_CPUIDLE
+ tristate "cpuidle test driver"
+ default n
+ help
+ This option enables a testing cpuidle driver, which allows the user
+ to define custom idle states with their respective latencies &
residencies.
+ This is useful for testing the behaviour of governors on different
+ sets of idle states.
+
endif
config ARCH_NEEDS_CPU_IDLE_COUPLED
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f07800cbb43f..68ea7dc257b5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
+obj-$(CONFIG_TEST_CPUIDLE) += cpuidle-test.o
##
# ARM SoC drivers
diff --git a/drivers/cpuidle/cpuidle-test.c b/drivers/cpuidle/cpuidle-test.c
new file mode 100644
index ..45605f142dab
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-test.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cpuidle-test - Test driver for cpuidle.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CPUIDLE_STATE_MAX 10
+#define MAX_PARAM_LENGTH 100
+
+static unsigned int nr_states = 4;
+static unsigned int sim_type = 1;
+static char name[MAX_PARAM_LENGTH];
+static char latency_us[MAX_PARAM_LENGTH];
+static char residency_us[MAX_PARAM_LENGTH];
+
+
+module_param(nr_states, uint, 0644);
+module_param(sim_type, uint, 0644);
+module_param_string(name, name, MAX_PARAM_LENGTH, 0644);
+module_param_string(latency_us, latency_us, MAX_PARAM_LENGTH, 0644);
+module_param_string(residency_us, residency_us, MAX_PARAM_LENGTH, 0644);
+
+static struct cpuidle_driver test_cpuidle_driver = {
+ .name = "test_cpuidle",
+ .owner = THIS_MODULE,
+};
+
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+
+static struct cpuidle_device __percpu *test_cpuidle_devices;
+static enum cpuhp_state test_hp_idlestate;
+
+
+static int __cpuidle idle_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ u64 time_start;
+
+ local_irq_enable();
+ if (!current_set_polling_and_test()) {
+ while (!need_resched())
+ cpu_relax();
+ }
+
+ time_start = local_clock();
+
+ /*
+* Currently, this while loop which is intended to introduce latency
+* for corresponding idle states, may not necessarily execute
+* completely if there is a interrupt in betweeen. This need to be
+* worked upon to distinguish among idle states.
+*/
+ while (local_clock() - time_start < drv->states[index].exit_latency) {
+ }
+
+ current_clr_polling();
+
+ return index;
+}
+
+static struct cpuidle_state cpuidle_states[CPUIDLE_STATE_MAX] = {
+ { /* Snooze */
+ .name = "snooze",
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = idle_loop },
+};
+
+static struct cpuidle_state cpuidle_states_ppc[] = {
+ { .name = "snooze",
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = idle_loop },
+ {
+ .name = "stop0",
+
[RESEND PATCH] Bluetooth: Only mark socket zapped after unlocking
Since l2cap_sock_teardown_cb doesn't acquire the channel lock before setting the socket as zapped, it could potentially race with l2cap_sock_release which frees the socket. Thus, wait until the cleanup is complete before marking the socket as zapped. This race was reproduced on a JBL GO speaker after the remote device rejected L2CAP connection due to resource unavailability. Here is a dmesg log with debug logs from a repro of this bug: [ 3465.424086] Bluetooth: hci_core.c:hci_acldata_packet() hci0 len 16 handle 0x0003 flags 0x0002 [ 3465.424090] Bluetooth: hci_conn.c:hci_conn_enter_active_mode() hcon cfedd07d mode 0 [ 3465.424094] Bluetooth: l2cap_core.c:l2cap_recv_acldata() conn 7eae8952 len 16 flags 0x2 [ 3465.424098] Bluetooth: l2cap_core.c:l2cap_recv_frame() len 12, cid 0x0001 [ 3465.424102] Bluetooth: l2cap_core.c:l2cap_raw_recv() conn 7eae8952 [ 3465.424175] Bluetooth: l2cap_core.c:l2cap_sig_channel() code 0x03 len 8 id 0x0c [ 3465.424180] Bluetooth: l2cap_core.c:l2cap_connect_create_rsp() dcid 0x0045 scid 0x result 0x02 status 0x00 [ 3465.424189] Bluetooth: l2cap_core.c:l2cap_chan_put() chan 6acf9bff orig refcnt 4 [ 3465.424196] Bluetooth: l2cap_core.c:l2cap_chan_del() chan 6acf9bff, conn 7eae8952, err 111, state BT_CONNECT [ 3465.424203] Bluetooth: l2cap_sock.c:l2cap_sock_teardown_cb() chan 6acf9bff state BT_CONNECT [ 3465.424221] Bluetooth: l2cap_core.c:l2cap_chan_put() chan 6acf9bff orig refcnt 3 [ 3465.424226] Bluetooth: hci_core.h:hci_conn_drop() hcon cfedd07d orig refcnt 6 [ 3465.424234] BUG: spinlock bad magic on CPU#2, kworker/u17:0/159 [ 3465.425626] Bluetooth: hci_sock.c:hci_sock_sendmsg() sock 2bb0cb64 sk a7964053 [ 3465.430330] lock: 0xff804410aac0, .magic: , .owner: /-1, .owner_cpu: 0 [ 3465.430332] Causing a watchdog bite! Signed-off-by: Abhishek Pandit-Subedi Reported-by: Balakrishna Godavarthi Reviewed-by: Manish Mandlik --- We had some more data available (outside of dmesg and oops) that led us to suspect a race between l2cap_sock_teardown_cb and l2cap_sock_release. I've left this out of the commit message since it's not an oops or dmesg logs. Crash stack from CPU4: -- -24 |spin_bug( |[X19] lock = 0xFF810BDB1EC0, |[X20] msg = 0xFFD143FD7960) -25 |debug_spin_lock_before(inline) | [X19] lock = 0xFF810BDB1EC0 -25 |do_raw_spin_lock( |[X19] lock = 0xFF810BDB1EC0) -26 |raw_spin_lock_irqsave( |[X19] lock = 0xFF810BDB1EC0) -27 |skb_peek(inline) -27 |__skb_dequeue(inline) -27 |skb_dequeue( |[X20] list = 0xFF810BDB1EA8) | [locdesc] flags = 12297829382473034410 -28 |skb_queue_purge( |[X19] list = 0xFF810BDB1EA8 -> ( | [D:0xFF810BDB1EA8] next = 0x0, | [D:0xFF810BDB1EB0] prev = 0x0, | [D:0xFF810BDB1EB8] qlen = 0, | [D:0xFF810BDB1EC0] lock = ([D:0xFF810BDB1EC0] rlock = ([D:0xFF810BDB1EC0] raw_lock | [X0] skb = ??? -29 |l2cap_seq_list_free(inline) | [locdesc] seq_list = 0xFF810BDB1ED8 -> ( |[D:0xFF810BDB1ED8] head = 0, |[D:0xFF810BDB1EDA] tail = 0, |[D:0xFF810BDB1EDC] mask = 0, |[D:0xFF810BDB1EE0] list = 0x0) -29 |l2cap_chan_del( |[X19] chan = 0xFF810BDB1C00, | ?) -30 |l2cap_chan_unlock(inline) -30 |l2cap_connect_create_rsp(inline) | [X20] conn = 0xFF81231F2600 | [locdesc] err = 0 | [X27] chan = 0xFF810BDB1C00 -30 |l2cap_bredr_sig_cmd(inline) | [X20] conn = 0xFF81231F2600 | [locdesc] err = 0 -30 |l2cap_sig_channel(inline) | [X20] conn = 0xFF81231F2600 | [X19] skb = 0xFF813DE4C040 | [X28] data = 0xFF8131582014 | [locdesc] cmd_len = 43690 -30 |l2cap_recv_frame( |[X20] conn = 0xFF81231F2600, |[X19] skb = 0xFF813DE4C040) | [locdesc] psm = 43690 -31 |l2cap_recv_acldata( | ?, |[X19] skb = 0xFF813DE4C040, | ?) | [X21] len = 16 -32 |hci_rx_work( | ?) | [X21] hdev = 0xFF8133A02000 -33 |__read_once_size(inline) | [locdesc] size = 4 -33 |atomic_read(inline) | [locdesc] __u = ([locdesc] __val = -1431655766, [locdesc] __c = (170)) -33 |static_key_count(inline) -33 |static_key_false(inline) -33 |trace_workqueue_execute_end(inline) | [X22] work = 0xFF8133A02838 -33 |process_one_work( |[X19] worker = 0xFF8133FE4500, |[X22] work = 0xFF8133A02838) | [locdesc] work_color = -1431655766 -34 |__read_once_size(inline) | [locdesc] size = 8 -34 |list_empty(inline) | [locdesc] __u = ([locdesc] __val = 0x, [locdesc] __c = (170)) -34 |worker_thread( |[X19] __worker = 0xFF8133FE4500) | [X19] worker = 0xFF8133FE4500 -35 |kthread( |[X20] _create = 0xFF8133FB3A00) | [X20] create = 0xFF8133FB3A00 | [X0] ret = ?
[RESEND PATCH] bluetooth: Set ext scan response only when it exists
Only set extended scan response only when it exists. Otherwise, clear the scan response data. Per the core spec v5.2, Vol 4, Part E, 7.8.55 If the advertising set is non-scannable and the Host uses this command other than to discard existing data, the Controller shall return the error code Invalid HCI Command Parameters (0x12). On WCN3991, the controller correctly responds with Invalid Parameters when this is sent. That error causes __hci_req_hci_power_on to fail with -EINVAL and LE devices can't connect because background scanning isn't configured. Here is an hci trace of where this issue occurs during power on: < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) plen 25 Handle: 0x00 Properties: 0x0010 Use legacy advertising PDUs: ADV_NONCONN_IND Min advertising interval: 181.250 msec (0x0122) Max advertising interval: 181.250 msec (0x0122) Channel map: 37, 38, 39 (0x07) Own address type: Random (0x01) Peer address type: Public (0x00) Peer address: 00:00:00:00:00:00 (OUI 00-00-00) Filter policy: Allow Scan Request from Any, Allow Connect... TX power: 127 dbm (0x7f) Primary PHY: LE 1M (0x01) Secondary max skip: 0x00 Secondary PHY: LE 1M (0x01) SID: 0x00 Scan request notifications: Disabled (0x00) > HCI Event: Command Complete (0x0e) plen 5 LE Set Extended Advertising Parameters (0x08|0x0036) ncmd 1 Status: Success (0x00) TX power (selected): 9 dbm (0x09) < HCI Command: LE Set Advertising Set Random Address (0x08|0x0035) plen 7 Advertising handle: 0x00 Advertising random address: 08:FD:55:ED:22:28 (OUI 08-FD-55) > HCI Event: Command Complete (0x0e) plen 4 LE Set Advertising Set Random Address (0x08|0x0035) ncmd Status: Success (0x00) < HCI Command: LE Set Extended Scan Response Data (0x08|0x0038) plen 35 Handle: 0x00 Operation: Complete scan response data (0x03) Fragment preference: Minimize fragmentation (0x01) Data length: 0x0d Name (short): Chromebook > HCI Event: Command Complete (0x0e) plen 4 LE Set Extended Scan Response Data (0x08|0x0038) ncmd 1 Status: Invalid HCI Command Parameters (0x12) Signed-off-by: Abhishek Pandit-Subedi Reviewed-by: Daniel Winkler --- net/bluetooth/hci_request.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index e0269192f2e536..e17bc8a1c66ddd 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -1533,11 +1533,14 @@ void __hci_req_update_scan_rsp_data(struct hci_request *req, u8 instance) memset(, 0, sizeof(cp)); - if (instance) + /* Extended scan response data doesn't allow a response to be +* set if the instance isn't scannable. +*/ + if (get_adv_instance_scan_rsp_len(hdev, instance)) len = create_instance_scan_rsp_data(hdev, instance, cp.data); else - len = create_default_scan_rsp_data(hdev, cp.data); + len = 0; if (hdev->scan_rsp_data_len == len && !memcmp(cp.data, hdev->scan_rsp_data, len)) -- 2.28.0.618.gf4bc123cb7-goog
[PATCH v2 1/3] Bluetooth: Add mgmt suspend and resume events
Add the controller suspend and resume events, which will signal when
Bluetooth has completed preparing for suspend and when it's ready for
resume.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
Changes in v2:
- Added suspend/resume events to list of mgmt events
include/net/bluetooth/hci_core.h | 3 +++
include/net/bluetooth/mgmt.h | 11 +++
net/bluetooth/mgmt.c | 26 ++
3 files changed, 40 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8caac20556b499..02a6ee056b2374 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1750,6 +1750,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
*bdaddr, u8 link_type,
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, s8 rssi, u8 *name, u8 name_len);
void mgmt_discovering(struct hci_dev *hdev, u8 discovering);
+void mgmt_suspending(struct hci_dev *hdev, u8 state);
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type);
bool mgmt_powering_down(struct hci_dev *hdev);
void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent);
void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 9ad505b9e694e4..e19e33c7b65c34 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1030,3 +1030,14 @@ struct mgmt_ev_adv_monitor_added {
struct mgmt_ev_adv_monitor_removed {
__le16 monitor_handle;
} __packed;
+
+#define MGMT_EV_CONTROLLER_SUSPEND 0x002d
+struct mgmt_ev_controller_suspend {
+ __u8suspend_state;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_RESUME 0x002e
+struct mgmt_ev_controller_resume {
+ __u8wake_reason;
+ struct mgmt_addr_info addr;
+} __packed;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e1d12494d16e14..db48ee3c213cbd 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -163,6 +163,8 @@ static const u16 mgmt_events[] = {
MGMT_EV_PHY_CONFIGURATION_CHANGED,
MGMT_EV_EXP_FEATURE_CHANGED,
MGMT_EV_DEVICE_FLAGS_CHANGED,
+ MGMT_EV_CONTROLLER_SUSPEND,
+ MGMT_EV_CONTROLLER_RESUME,
};
static const u16 mgmt_untrusted_commands[] = {
@@ -8874,6 +8876,30 @@ void mgmt_discovering(struct hci_dev *hdev, u8
discovering)
mgmt_event(MGMT_EV_DISCOVERING, hdev, , sizeof(ev), NULL);
}
+void mgmt_suspending(struct hci_dev *hdev, u8 state)
+{
+ struct mgmt_ev_controller_suspend ev;
+
+ ev.suspend_state = state;
+ mgmt_event(MGMT_EV_CONTROLLER_SUSPEND, hdev, , sizeof(ev), NULL);
+}
+
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type)
+{
+ struct mgmt_ev_controller_resume ev;
+
+ ev.wake_reason = reason;
+ if (bdaddr) {
+ bacpy(, bdaddr);
+ ev.addr.type = addr_type;
+ } else {
+ memset(, 0, sizeof(ev.addr));
+ }
+
+ mgmt_event(MGMT_EV_CONTROLLER_RESUME, hdev, , sizeof(ev), NULL);
+}
+
static struct hci_mgmt_chan chan = {
.channel= HCI_CHANNEL_CONTROL,
.handler_count = ARRAY_SIZE(mgmt_handlers),
--
2.28.0.618.gf4bc123cb7-goog
[PATCH v2 2/3] Bluetooth: Add suspend reason for device disconnect
Update device disconnect event with reason 0x5 to indicate that device
disconnected because the controller is suspending.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
Changes in v2: None
include/net/bluetooth/mgmt.h | 1 +
net/bluetooth/mgmt.c | 4
2 files changed, 5 insertions(+)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index e19e33c7b65c34..a4b8935e0db97a 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -842,6 +842,7 @@ struct mgmt_ev_device_connected {
#define MGMT_DEV_DISCONN_LOCAL_HOST0x02
#define MGMT_DEV_DISCONN_REMOTE0x03
#define MGMT_DEV_DISCONN_AUTH_FAILURE 0x04
+#define MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND0x05
#define MGMT_EV_DEVICE_DISCONNECTED0x000C
struct mgmt_ev_device_disconnected {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index db48ee3c213cbd..0b711ad80f6bd1 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8270,6 +8270,10 @@ void mgmt_device_disconnected(struct hci_dev *hdev,
bdaddr_t *bdaddr,
ev.addr.type = link_to_bdaddr(link_type, addr_type);
ev.reason = reason;
+ /* Report disconnects due to suspend */
+ if (hdev->suspended)
+ ev.reason = MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND;
+
mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, , sizeof(ev), sk);
if (sk)
--
2.28.0.618.gf4bc123cb7-goog
[PATCH v2 3/3] Bluetooth: Emit controller suspend and resume events
Emit controller suspend and resume events when we are ready for suspend
and we've resumed from suspend.
The controller suspend event will report whatever suspend state was
successfully entered. The controller resume event will check the first
HCI event that was received after we finished preparing for suspend and,
if it was a connection event, store the address of the peer that caused
the event. If it was not a connection event, we mark the wake reason as
an unexpected event.
Here is a sample btmon trace with these events:
@ MGMT Event: Controller Suspended (0x002d) plen 1
Suspend state: Page scanning and/or passive scanning (2)
@ MGMT Event: Controller Resumed (0x002e) plen 8
Wake reason: Remote wake due to peer device connection (2)
LE Address: CD:F3:CD:13:C5:9A (OUI CD-F3-CD)
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
Changes in v2: None
include/net/bluetooth/hci_core.h | 3 ++
include/net/bluetooth/mgmt.h | 4 ++
net/bluetooth/hci_core.c | 26 +++-
net/bluetooth/hci_event.c| 73
4 files changed, 105 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 02a6ee056b2374..9873e1c8cd163b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -484,6 +484,9 @@ struct hci_dev {
enum suspended_statesuspend_state;
boolscanning_paused;
boolsuspended;
+ u8 wake_reason;
+ bdaddr_twake_addr;
+ u8 wake_addr_type;
wait_queue_head_t suspend_wait_q;
DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index a4b8935e0db97a..6b55155e05e977 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1042,3 +1042,7 @@ struct mgmt_ev_controller_resume {
__u8wake_reason;
struct mgmt_addr_info addr;
} __packed;
+
+#define MGMT_WAKE_REASON_NON_BT_WAKE 0x0
+#define MGMT_WAKE_REASON_UNEXPECTED0x1
+#define MGMT_WAKE_REASON_REMOTE_WAKE 0x2
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ef32b12f150cd1..8a2645a8330137 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3497,12 +3497,24 @@ static int hci_change_suspend_state(struct hci_dev
*hdev,
return hci_suspend_wait_event(hdev);
}
+static void hci_clear_wake_reason(struct hci_dev *hdev)
+{
+ hci_dev_lock(hdev);
+
+ hdev->wake_reason = 0;
+ bacpy(>wake_addr, BDADDR_ANY);
+ hdev->wake_addr_type = 0;
+
+ hci_dev_unlock(hdev);
+}
+
static int hci_suspend_notifier(struct notifier_block *nb, unsigned long
action,
void *data)
{
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)) {
@@ -3523,15 +3535,27 @@ static int hci_suspend_notifier(struct notifier_block
*nb, unsigned long action,
* - Second, program event filter/whitelist and enable scan
*/
ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
+ if (!ret)
+ state = BT_SUSPEND_DISCONNECT;
/* Only configure whitelist if disconnect succeeded and wake
* isn't being prevented.
*/
- if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev)))
+ 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);
+
} else if (action == PM_POST_SUSPEND) {
ret = hci_change_suspend_state(hdev, BT_RUNNING);
+
+ mgmt_resuming(hdev, hdev->wake_reason, >wake_addr,
+ hdev->wake_addr_type);
}
done:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 33d8458fdd4adc..a68be50097ac35 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6000,6 +6000,76 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev,
u16 opcode,
return true;
}
+static void hci_store_wake_reason(struct hci_dev *hdev, u8 event,
+ struct sk_buff *skb)
+{
+ struct hci_ev_le_advertising_info *adv;
+
[PATCH v2 0/3] Bluetooth: Emit events for suspend/resume
Hi Marcel, This series adds the suspend/resume events suggested in https://patchwork.kernel.org/patch/11771001/. I have tested it with some userspace changes that monitors the controller resumed event to trigger audio device reconnection and verified that the events are correctly emitted. Patch for btmon changes: https://patchwork.kernel.org/patch/11743863/ Please take a look. Abhishek Changes in v2: - Added suspend/resume events to list of mgmt events Abhishek Pandit-Subedi (3): Bluetooth: Add mgmt suspend and resume events Bluetooth: Add suspend reason for device disconnect Bluetooth: Emit controller suspend and resume events include/net/bluetooth/hci_core.h | 6 +++ include/net/bluetooth/mgmt.h | 16 +++ net/bluetooth/hci_core.c | 26 +++- net/bluetooth/hci_event.c| 73 net/bluetooth/mgmt.c | 30 + 5 files changed, 150 insertions(+), 1 deletion(-) -- 2.28.0.618.gf4bc123cb7-goog
[RESEND PATCH 2/3] Bluetooth: Add suspend reason for device disconnect
Update device disconnect event with reason 0x5 to indicate that device
disconnected because the controller is suspending.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
include/net/bluetooth/mgmt.h | 1 +
net/bluetooth/mgmt.c | 4
2 files changed, 5 insertions(+)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index e19e33c7b65c34..a4b8935e0db97a 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -842,6 +842,7 @@ struct mgmt_ev_device_connected {
#define MGMT_DEV_DISCONN_LOCAL_HOST0x02
#define MGMT_DEV_DISCONN_REMOTE0x03
#define MGMT_DEV_DISCONN_AUTH_FAILURE 0x04
+#define MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND0x05
#define MGMT_EV_DEVICE_DISCONNECTED0x000C
struct mgmt_ev_device_disconnected {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1475a47edb080b..e33f45e20ed1e7 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8268,6 +8268,10 @@ void mgmt_device_disconnected(struct hci_dev *hdev,
bdaddr_t *bdaddr,
ev.addr.type = link_to_bdaddr(link_type, addr_type);
ev.reason = reason;
+ /* Report disconnects due to suspend */
+ if (hdev->suspended)
+ ev.reason = MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND;
+
mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, , sizeof(ev), sk);
if (sk)
--
2.28.0.618.gf4bc123cb7-goog
[RESEND PATCH 3/3] Bluetooth: Emit controller suspend and resume events
Emit controller suspend and resume events when we are ready for suspend
and we've resumed from suspend.
The controller suspend event will report whatever suspend state was
successfully entered. The controller resume event will check the first
HCI event that was received after we finished preparing for suspend and,
if it was a connection event, store the address of the peer that caused
the event. If it was not a connection event, we mark the wake reason as
an unexpected event.
Here is a sample btmon trace with these events:
@ MGMT Event: Controller Suspended (0x002d) plen 1
Suspend state: Page scanning and/or passive scanning (2)
@ MGMT Event: Controller Resumed (0x002e) plen 8
Wake reason: Remote wake due to peer device connection (2)
LE Address: CD:F3:CD:13:C5:9A (OUI CD-F3-CD)
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
include/net/bluetooth/hci_core.h | 3 ++
include/net/bluetooth/mgmt.h | 4 ++
net/bluetooth/hci_core.c | 26 +++-
net/bluetooth/hci_event.c| 73
4 files changed, 105 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 02a6ee056b2374..9873e1c8cd163b 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -484,6 +484,9 @@ struct hci_dev {
enum suspended_statesuspend_state;
boolscanning_paused;
boolsuspended;
+ u8 wake_reason;
+ bdaddr_twake_addr;
+ u8 wake_addr_type;
wait_queue_head_t suspend_wait_q;
DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index a4b8935e0db97a..6b55155e05e977 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1042,3 +1042,7 @@ struct mgmt_ev_controller_resume {
__u8wake_reason;
struct mgmt_addr_info addr;
} __packed;
+
+#define MGMT_WAKE_REASON_NON_BT_WAKE 0x0
+#define MGMT_WAKE_REASON_UNEXPECTED0x1
+#define MGMT_WAKE_REASON_REMOTE_WAKE 0x2
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index ef32b12f150cd1..8a2645a8330137 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3497,12 +3497,24 @@ static int hci_change_suspend_state(struct hci_dev
*hdev,
return hci_suspend_wait_event(hdev);
}
+static void hci_clear_wake_reason(struct hci_dev *hdev)
+{
+ hci_dev_lock(hdev);
+
+ hdev->wake_reason = 0;
+ bacpy(>wake_addr, BDADDR_ANY);
+ hdev->wake_addr_type = 0;
+
+ hci_dev_unlock(hdev);
+}
+
static int hci_suspend_notifier(struct notifier_block *nb, unsigned long
action,
void *data)
{
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)) {
@@ -3523,15 +3535,27 @@ static int hci_suspend_notifier(struct notifier_block
*nb, unsigned long action,
* - Second, program event filter/whitelist and enable scan
*/
ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
+ if (!ret)
+ state = BT_SUSPEND_DISCONNECT;
/* Only configure whitelist if disconnect succeeded and wake
* isn't being prevented.
*/
- if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev)))
+ 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);
+
} else if (action == PM_POST_SUSPEND) {
ret = hci_change_suspend_state(hdev, BT_RUNNING);
+
+ mgmt_resuming(hdev, hdev->wake_reason, >wake_addr,
+ hdev->wake_addr_type);
}
done:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 33d8458fdd4adc..a68be50097ac35 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -6000,6 +6000,76 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev,
u16 opcode,
return true;
}
+static void hci_store_wake_reason(struct hci_dev *hdev, u8 event,
+ struct sk_buff *skb)
+{
+ struct hci_ev_le_advertising_info *adv;
+ struct hci_ev_le_direct
[RESEND PATCH 0/3] Bluetooth: Emit events for suspend/resume
Hi Marcel, This series adds the suspend/resume events suggested in https://patchwork.kernel.org/patch/11771001/. I have tested it with some userspace changes that monitors the controller resumed event to trigger audio device reconnection and verified that the events are correctly emitted. Patch for btmon changes: https://patchwork.kernel.org/patch/11743863/ Please take a look. Abhishek Abhishek Pandit-Subedi (3): Bluetooth: Add mgmt suspend and resume events Bluetooth: Add suspend reason for device disconnect Bluetooth: Emit controller suspend and resume events include/net/bluetooth/hci_core.h | 6 +++ include/net/bluetooth/mgmt.h | 16 +++ net/bluetooth/hci_core.c | 26 +++- net/bluetooth/hci_event.c| 73 net/bluetooth/mgmt.c | 28 5 files changed, 148 insertions(+), 1 deletion(-) -- 2.28.0.618.gf4bc123cb7-goog
[RESEND PATCH 1/3] Bluetooth: Add mgmt suspend and resume events
Add the controller suspend and resume events, which will signal when
Bluetooth has completed preparing for suspend and when it's ready for
resume.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
include/net/bluetooth/hci_core.h | 3 +++
include/net/bluetooth/mgmt.h | 11 +++
net/bluetooth/mgmt.c | 24
3 files changed, 38 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 8caac20556b499..02a6ee056b2374 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1750,6 +1750,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
*bdaddr, u8 link_type,
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, s8 rssi, u8 *name, u8 name_len);
void mgmt_discovering(struct hci_dev *hdev, u8 discovering);
+void mgmt_suspending(struct hci_dev *hdev, u8 state);
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type);
bool mgmt_powering_down(struct hci_dev *hdev);
void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent);
void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 9ad505b9e694e4..e19e33c7b65c34 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1030,3 +1030,14 @@ struct mgmt_ev_adv_monitor_added {
struct mgmt_ev_adv_monitor_removed {
__le16 monitor_handle;
} __packed;
+
+#define MGMT_EV_CONTROLLER_SUSPEND 0x002d
+struct mgmt_ev_controller_suspend {
+ __u8suspend_state;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_RESUME 0x002e
+struct mgmt_ev_controller_resume {
+ __u8wake_reason;
+ struct mgmt_addr_info addr;
+} __packed;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index e1d12494d16e14..1475a47edb080b 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8874,6 +8874,30 @@ void mgmt_discovering(struct hci_dev *hdev, u8
discovering)
mgmt_event(MGMT_EV_DISCOVERING, hdev, , sizeof(ev), NULL);
}
+void mgmt_suspending(struct hci_dev *hdev, u8 state)
+{
+ struct mgmt_ev_controller_suspend ev;
+
+ ev.suspend_state = state;
+ mgmt_event(MGMT_EV_CONTROLLER_SUSPEND, hdev, , sizeof(ev), NULL);
+}
+
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type)
+{
+ struct mgmt_ev_controller_resume ev;
+
+ ev.wake_reason = reason;
+ if (bdaddr) {
+ bacpy(, bdaddr);
+ ev.addr.type = addr_type;
+ } else {
+ memset(, 0, sizeof(ev.addr));
+ }
+
+ mgmt_event(MGMT_EV_CONTROLLER_RESUME, hdev, , sizeof(ev), NULL);
+}
+
static struct hci_mgmt_chan chan = {
.channel= HCI_CHANNEL_CONTROL,
.handler_count = ARRAY_SIZE(mgmt_handlers),
--
2.28.0.618.gf4bc123cb7-goog
[PATCH] Bluetooth: Re-order clearing suspend tasks
Unregister_pm_notifier is a blocking call so suspend tasks should be cleared beforehand. Otherwise, the notifier will wait for completion before returning (and we encounter a 2s timeout on resume). Fixes: 0e9952804ec9c8 (Bluetooth: Clear suspend tasks on unregister) Signed-off-by: Abhishek Pandit-Subedi --- Should have caught that unregister_pm_notifier was blocking last time but when testing the earlier patch, I got unlucky and saw that the error message was never hit (the suspend timeout). When re-testing this patch on the same device, I was able to reproduce the problem on an older build with the 0e9952804ec9c8 but not on a newer build with the same patch. Changing the order correctly fixes it everywhere. Confirmed this by adding debug logs in btusb_disconnect and hci_suspend_notifier to confirm what order things were getting called. Sorry about the churn. Next I'm going try to do something about the palm shaped indentation on my forehead... net/bluetooth/hci_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index efc0fe2b47dac2..be9cdf5dabe5dc 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3794,8 +3794,8 @@ void hci_unregister_dev(struct hci_dev *hdev) cancel_work_sync(>power_on); - unregister_pm_notifier(>suspend_notifier); hci_suspend_clear_tasks(hdev); + unregister_pm_notifier(>suspend_notifier); cancel_work_sync(>suspend_prepare); hci_dev_do_close(hdev); -- 2.28.0.618.gf4bc123cb7-goog
[PATCH] Bluetooth: Re-order clearing suspend tasks
Unregister_pm_notifier is a blocking call so suspend tasks should be cleared beforehand. Otherwise, the notifier will wait for completion before returning (and we encounter a 2s timeout on resume). Fixes: 0e9952804ec9c8 (Bluetooth: Clear suspend tasks on unregister) Signed-off-by: Abhishek Pandit-Subedi --- Should have caught that unregister_pm_notifier was blocking last time but when testing the earlier patch, I got unlucky and saw that the error message was never hit (the suspend timeout). When re-testing this patch on the same device, I was able to reproduce the problem on an older build with the 0e9952804ec9c8 but not on a newer build with the same patch. Changing the order correctly fixes it everywhere. Confirmed this by adding debug logs in btusb_disconnect and hci_suspend_notifier to confirm what order things were getting called. Sorry about the churn. Next I'm going try to do something about the palm shaped indentation on my forehead... net/bluetooth/hci_core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index efc0fe2b47dac2..be9cdf5dabe5dc 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3794,8 +3794,8 @@ void hci_unregister_dev(struct hci_dev *hdev) cancel_work_sync(>power_on); - unregister_pm_notifier(>suspend_notifier); hci_suspend_clear_tasks(hdev); + unregister_pm_notifier(>suspend_notifier); cancel_work_sync(>suspend_prepare); hci_dev_do_close(hdev); -- 2.28.0.618.gf4bc123cb7-goog
Re: [PATCH] Bluetooth: Clear suspend tasks on unregister
v2 sent with fix.
On Mon, Aug 31, 2020 at 8:49 AM Marcel Holtmann wrote:
>
> Hi Abhishek,
>
> > While unregistering, make sure to clear the suspend tasks before
> > cancelling the work. If the unregister is called during resume from
> > suspend, this will unnecessarily add 2s to the resume time otherwise.
> >
> > Fixes: 4e8c36c3b0d73d (Bluetooth: Fix suspend notifier race)
> > Signed-off-by: Abhishek Pandit-Subedi
> > ---
> > This was discovered with RT8822CE using the btusb driver. This chipset
> > will reset on resume during system suspend and was unnecessarily adding
> > 2s to every resume. Since we're unregistering anyway, there's no harm in
> > just clearing the pending events.
> >
> > net/bluetooth/hci_core.c | 11 +++
> > 1 file changed, 11 insertions(+)
> >
> > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> > index 68bfe57b66250f..ed4cb3479433c0 100644
> > --- a/net/bluetooth/hci_core.c
> > +++ b/net/bluetooth/hci_core.c
> > @@ -3442,6 +3442,16 @@ void hci_copy_identity_address(struct hci_dev *hdev,
> > bdaddr_t *bdaddr,
> > }
> > }
> >
> > +static void hci_suspend_clear_tasks(struct hci_dev *hdev)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < __SUSPEND_NUM_TASKS; ++i)
> > + clear_bit(i, hdev->suspend_tasks);
>
> I prefer i++ instead of ++i.
>
> Regards
>
> Marcel
>
[PATCH v2] Bluetooth: Clear suspend tasks on unregister
While unregistering, make sure to clear the suspend tasks before
cancelling the work. If the unregister is called during resume from
suspend, this will unnecessarily add 2s to the resume time otherwise.
Fixes: 4e8c36c3b0d73d (Bluetooth: Fix suspend notifier race)
Signed-off-by: Abhishek Pandit-Subedi
---
This was discovered with RT8822CE using the btusb driver. This chipset
will reset on resume during system suspend and was unnecessarily adding
2s to every resume. Since we're unregistering anyway, there's no harm in
just clearing the pending events.
Changes in v2:
- ++i to i++
net/bluetooth/hci_core.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 68bfe57b66250f..efc0fe2b47dac2 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3442,6 +3442,16 @@ void hci_copy_identity_address(struct hci_dev *hdev,
bdaddr_t *bdaddr,
}
}
+static void hci_suspend_clear_tasks(struct hci_dev *hdev)
+{
+ int i;
+
+ for (i = 0; i < __SUSPEND_NUM_TASKS; i++)
+ clear_bit(i, hdev->suspend_tasks);
+
+ wake_up(>suspend_wait_q);
+}
+
static int hci_suspend_wait_event(struct hci_dev *hdev)
{
#define WAKE_COND
\
@@ -3785,6 +3795,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
cancel_work_sync(>power_on);
unregister_pm_notifier(>suspend_notifier);
+ hci_suspend_clear_tasks(hdev);
cancel_work_sync(>suspend_prepare);
hci_dev_do_close(hdev);
--
2.28.0.402.g5ffc5be6b7-goog
Re: [PATCH] Bluetooth: Clear suspend tasks on unregister
Please disregard this earlier email without the Fixes tag.
On Wed, Aug 26, 2020 at 3:26 PM Abhishek Pandit-Subedi
wrote:
>
> While unregistering, make sure to clear the suspend tasks before
> cancelling the work. If the unregister is called during resume from
> suspend, this will unnecessarily add 2s to the resume time otherwise.
>
> Signed-off-by: Abhishek Pandit-Subedi
> ---
> This was discovered with RT8822CE using the btusb driver. This chipset
> will reset on resume during system suspend and was unnecessarily adding
> 2s to every resume. Since we're unregistering anyway, there's no harm in
> just clearing the pending events.
>
> net/bluetooth/hci_core.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
> index 68bfe57b66250f..ed4cb3479433c0 100644
> --- a/net/bluetooth/hci_core.c
> +++ b/net/bluetooth/hci_core.c
> @@ -3442,6 +3442,16 @@ void hci_copy_identity_address(struct hci_dev *hdev,
> bdaddr_t *bdaddr,
> }
> }
>
> +static void hci_suspend_clear_tasks(struct hci_dev *hdev)
> +{
> + int i;
> +
> + for (i = 0; i < __SUSPEND_NUM_TASKS; ++i)
> + clear_bit(i, hdev->suspend_tasks);
> +
> + wake_up(>suspend_wait_q);
> +}
> +
> static int hci_suspend_wait_event(struct hci_dev *hdev)
> {
> #define WAKE_COND
> \
> @@ -3785,6 +3795,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
> cancel_work_sync(>power_on);
>
> unregister_pm_notifier(>suspend_notifier);
> + hci_suspend_clear_tasks(hdev);
> cancel_work_sync(>suspend_prepare);
>
> hci_dev_do_close(hdev);
> --
> 2.28.0.297.g1956fa8f8d-goog
>
[PATCH] Bluetooth: Clear suspend tasks on unregister
While unregistering, make sure to clear the suspend tasks before
cancelling the work. If the unregister is called during resume from
suspend, this will unnecessarily add 2s to the resume time otherwise.
Fixes: 4e8c36c3b0d73d (Bluetooth: Fix suspend notifier race)
Signed-off-by: Abhishek Pandit-Subedi
---
This was discovered with RT8822CE using the btusb driver. This chipset
will reset on resume during system suspend and was unnecessarily adding
2s to every resume. Since we're unregistering anyway, there's no harm in
just clearing the pending events.
net/bluetooth/hci_core.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 68bfe57b66250f..ed4cb3479433c0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3442,6 +3442,16 @@ void hci_copy_identity_address(struct hci_dev *hdev,
bdaddr_t *bdaddr,
}
}
+static void hci_suspend_clear_tasks(struct hci_dev *hdev)
+{
+ int i;
+
+ for (i = 0; i < __SUSPEND_NUM_TASKS; ++i)
+ clear_bit(i, hdev->suspend_tasks);
+
+ wake_up(>suspend_wait_q);
+}
+
static int hci_suspend_wait_event(struct hci_dev *hdev)
{
#define WAKE_COND
\
@@ -3785,6 +3795,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
cancel_work_sync(>power_on);
unregister_pm_notifier(>suspend_notifier);
+ hci_suspend_clear_tasks(hdev);
cancel_work_sync(>suspend_prepare);
hci_dev_do_close(hdev);
--
2.28.0.297.g1956fa8f8d-goog
[PATCH] Bluetooth: Clear suspend tasks on unregister
While unregistering, make sure to clear the suspend tasks before
cancelling the work. If the unregister is called during resume from
suspend, this will unnecessarily add 2s to the resume time otherwise.
Signed-off-by: Abhishek Pandit-Subedi
---
This was discovered with RT8822CE using the btusb driver. This chipset
will reset on resume during system suspend and was unnecessarily adding
2s to every resume. Since we're unregistering anyway, there's no harm in
just clearing the pending events.
net/bluetooth/hci_core.c | 11 +++
1 file changed, 11 insertions(+)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 68bfe57b66250f..ed4cb3479433c0 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3442,6 +3442,16 @@ void hci_copy_identity_address(struct hci_dev *hdev,
bdaddr_t *bdaddr,
}
}
+static void hci_suspend_clear_tasks(struct hci_dev *hdev)
+{
+ int i;
+
+ for (i = 0; i < __SUSPEND_NUM_TASKS; ++i)
+ clear_bit(i, hdev->suspend_tasks);
+
+ wake_up(>suspend_wait_q);
+}
+
static int hci_suspend_wait_event(struct hci_dev *hdev)
{
#define WAKE_COND
\
@@ -3785,6 +3795,7 @@ void hci_unregister_dev(struct hci_dev *hdev)
cancel_work_sync(>power_on);
unregister_pm_notifier(>suspend_notifier);
+ hci_suspend_clear_tasks(hdev);
cancel_work_sync(>suspend_prepare);
hci_dev_do_close(hdev);
--
2.28.0.297.g1956fa8f8d-goog
Re: [PATCH 0/3] Bluetooth: Emit events for suspend/resume
Hi Marcel, Please review this patch. A newer series for how the controller resume event will be used is available at https://patchwork.kernel.org/project/bluetooth/list/?series=334811 Besides usage in bluez, these events will also make debugging and testing suspend/resume for Bluez easier (i.e. finding spurious wakes due to BT in suspend stress tests, asserting that wakeup from peers occurred as expected in tests) Abhishek On Tue, Aug 4, 2020 at 10:11 AM Abhishek Pandit-Subedi wrote: > > Hi, > > Gentle reminder that this is waiting for feedback. Related userspace > changes are here to see how we plan on using it: > https://patchwork.kernel.org/project/bluetooth/list/?series=325777 > > Thanks > Abhishek > > On Tue, Jul 28, 2020 at 6:42 PM Abhishek Pandit-Subedi > wrote: > > > > > > Hi Marcel, > > > > This series adds the suspend/resume events suggested in > > https://patchwork.kernel.org/patch/11663455/. > > > > I have tested it with some userspace changes that monitors the > > controller resumed event to trigger audio device reconnection and > > verified that the events are correctly emitted. > > > > Please take a look. > > Abhishek > > > > > > Abhishek Pandit-Subedi (3): > > Bluetooth: Add mgmt suspend and resume events > > Bluetooth: Add suspend reason for device disconnect > > Bluetooth: Emit controller suspend and resume events > > > > include/net/bluetooth/hci_core.h | 6 +++ > > include/net/bluetooth/mgmt.h | 16 +++ > > net/bluetooth/hci_core.c | 26 +++- > > net/bluetooth/hci_event.c| 73 > > net/bluetooth/mgmt.c | 28 > > 5 files changed, 148 insertions(+), 1 deletion(-) > > > > -- > > 2.28.0.rc0.142.g3c755180ce-goog > >
[PATCH] Bluetooth: Only mark socket zapped after unlocking
Since l2cap_sock_teardown_cb doesn't acquire the channel lock before setting the socket as zapped, it could potentially race with l2cap_sock_release which frees the socket. Thus, wait until the cleanup is complete before marking the socket as zapped. This race was reproduced on a JBL GO speaker after the remote device rejected L2CAP connection due to resource unavailability. Here is a dmesg log with debug logs from a repro of this bug: [ 3465.424086] Bluetooth: hci_core.c:hci_acldata_packet() hci0 len 16 handle 0x0003 flags 0x0002 [ 3465.424090] Bluetooth: hci_conn.c:hci_conn_enter_active_mode() hcon cfedd07d mode 0 [ 3465.424094] Bluetooth: l2cap_core.c:l2cap_recv_acldata() conn 7eae8952 len 16 flags 0x2 [ 3465.424098] Bluetooth: l2cap_core.c:l2cap_recv_frame() len 12, cid 0x0001 [ 3465.424102] Bluetooth: l2cap_core.c:l2cap_raw_recv() conn 7eae8952 [ 3465.424175] Bluetooth: l2cap_core.c:l2cap_sig_channel() code 0x03 len 8 id 0x0c [ 3465.424180] Bluetooth: l2cap_core.c:l2cap_connect_create_rsp() dcid 0x0045 scid 0x result 0x02 status 0x00 [ 3465.424189] Bluetooth: l2cap_core.c:l2cap_chan_put() chan 6acf9bff orig refcnt 4 [ 3465.424196] Bluetooth: l2cap_core.c:l2cap_chan_del() chan 6acf9bff, conn 7eae8952, err 111, state BT_CONNECT [ 3465.424203] Bluetooth: l2cap_sock.c:l2cap_sock_teardown_cb() chan 6acf9bff state BT_CONNECT [ 3465.424221] Bluetooth: l2cap_core.c:l2cap_chan_put() chan 6acf9bff orig refcnt 3 [ 3465.424226] Bluetooth: hci_core.h:hci_conn_drop() hcon cfedd07d orig refcnt 6 [ 3465.424234] BUG: spinlock bad magic on CPU#2, kworker/u17:0/159 [ 3465.425626] Bluetooth: hci_sock.c:hci_sock_sendmsg() sock 2bb0cb64 sk a7964053 [ 3465.430330] lock: 0xff804410aac0, .magic: , .owner: /-1, .owner_cpu: 0 [ 3465.430332] Causing a watchdog bite! Signed-off-by: Abhishek Pandit-Subedi Reported-by: Balakrishna Godavarthi Reviewed-by: Manish Mandlik --- We had some more data available (outside of dmesg and oops) that led us to suspect a race between l2cap_sock_teardown_cb and l2cap_sock_release. I've left this out of the commit message since it's not an oops or dmesg logs. Crash stack from CPU4: -- -24 |spin_bug( |[X19] lock = 0xFF810BDB1EC0, |[X20] msg = 0xFFD143FD7960) -25 |debug_spin_lock_before(inline) | [X19] lock = 0xFF810BDB1EC0 -25 |do_raw_spin_lock( |[X19] lock = 0xFF810BDB1EC0) -26 |raw_spin_lock_irqsave( |[X19] lock = 0xFF810BDB1EC0) -27 |skb_peek(inline) -27 |__skb_dequeue(inline) -27 |skb_dequeue( |[X20] list = 0xFF810BDB1EA8) | [locdesc] flags = 12297829382473034410 -28 |skb_queue_purge( |[X19] list = 0xFF810BDB1EA8 -> ( | [D:0xFF810BDB1EA8] next = 0x0, | [D:0xFF810BDB1EB0] prev = 0x0, | [D:0xFF810BDB1EB8] qlen = 0, | [D:0xFF810BDB1EC0] lock = ([D:0xFF810BDB1EC0] rlock = ([D:0xFF810BDB1EC0] raw_lock | [X0] skb = ??? -29 |l2cap_seq_list_free(inline) | [locdesc] seq_list = 0xFF810BDB1ED8 -> ( |[D:0xFF810BDB1ED8] head = 0, |[D:0xFF810BDB1EDA] tail = 0, |[D:0xFF810BDB1EDC] mask = 0, |[D:0xFF810BDB1EE0] list = 0x0) -29 |l2cap_chan_del( |[X19] chan = 0xFF810BDB1C00, | ?) -30 |l2cap_chan_unlock(inline) -30 |l2cap_connect_create_rsp(inline) | [X20] conn = 0xFF81231F2600 | [locdesc] err = 0 | [X27] chan = 0xFF810BDB1C00 -30 |l2cap_bredr_sig_cmd(inline) | [X20] conn = 0xFF81231F2600 | [locdesc] err = 0 -30 |l2cap_sig_channel(inline) | [X20] conn = 0xFF81231F2600 | [X19] skb = 0xFF813DE4C040 | [X28] data = 0xFF8131582014 | [locdesc] cmd_len = 43690 -30 |l2cap_recv_frame( |[X20] conn = 0xFF81231F2600, |[X19] skb = 0xFF813DE4C040) | [locdesc] psm = 43690 -31 |l2cap_recv_acldata( | ?, |[X19] skb = 0xFF813DE4C040, | ?) | [X21] len = 16 -32 |hci_rx_work( | ?) | [X21] hdev = 0xFF8133A02000 -33 |__read_once_size(inline) | [locdesc] size = 4 -33 |atomic_read(inline) | [locdesc] __u = ([locdesc] __val = -1431655766, [locdesc] __c = (170)) -33 |static_key_count(inline) -33 |static_key_false(inline) -33 |trace_workqueue_execute_end(inline) | [X22] work = 0xFF8133A02838 -33 |process_one_work( |[X19] worker = 0xFF8133FE4500, |[X22] work = 0xFF8133A02838) | [locdesc] work_color = -1431655766 -34 |__read_once_size(inline) | [locdesc] size = 8 -34 |list_empty(inline) | [locdesc] __u = ([locdesc] __val = 0x, [locdesc] __c = (170)) -34 |worker_thread( |[X19] __worker = 0xFF8133FE4500) | [X19] worker = 0xFF8133FE4500 -35 |kthread( |[X20] _create = 0xFF8133FB3A00) | [X20] create = 0xFF8133FB3A00 | [X0] ret = ?
[PATCH] bluetooth: Set ext scan response only when it exists
Only set extended scan response only when it exists. Otherwise, clear the scan response data. Per the core spec v5.2, Vol 4, Part E, 7.8.55 If the advertising set is non-scannable and the Host uses this command other than to discard existing data, the Controller shall return the error code Invalid HCI Command Parameters (0x12). On WCN3991, the controller correctly responds with Invalid Parameters when this is sent. That error causes __hci_req_hci_power_on to fail with -EINVAL and LE devices can't connect because background scanning isn't configured. Here is an hci trace of where this issue occurs during power on: < HCI Command: LE Set Extended Advertising Parameters (0x08|0x0036) plen 25 Handle: 0x00 Properties: 0x0010 Use legacy advertising PDUs: ADV_NONCONN_IND Min advertising interval: 181.250 msec (0x0122) Max advertising interval: 181.250 msec (0x0122) Channel map: 37, 38, 39 (0x07) Own address type: Random (0x01) Peer address type: Public (0x00) Peer address: 00:00:00:00:00:00 (OUI 00-00-00) Filter policy: Allow Scan Request from Any, Allow Connect... TX power: 127 dbm (0x7f) Primary PHY: LE 1M (0x01) Secondary max skip: 0x00 Secondary PHY: LE 1M (0x01) SID: 0x00 Scan request notifications: Disabled (0x00) > HCI Event: Command Complete (0x0e) plen 5 LE Set Extended Advertising Parameters (0x08|0x0036) ncmd 1 Status: Success (0x00) TX power (selected): 9 dbm (0x09) < HCI Command: LE Set Advertising Set Random Address (0x08|0x0035) plen 7 Advertising handle: 0x00 Advertising random address: 08:FD:55:ED:22:28 (OUI 08-FD-55) > HCI Event: Command Complete (0x0e) plen 4 LE Set Advertising Set Random Address (0x08|0x0035) ncmd Status: Success (0x00) < HCI Command: LE Set Extended Scan Response Data (0x08|0x0038) plen 35 Handle: 0x00 Operation: Complete scan response data (0x03) Fragment preference: Minimize fragmentation (0x01) Data length: 0x0d Name (short): Chromebook > HCI Event: Command Complete (0x0e) plen 4 LE Set Extended Scan Response Data (0x08|0x0038) ncmd 1 Status: Invalid HCI Command Parameters (0x12) Signed-off-by: Abhishek Pandit-Subedi Reviewed-by: Daniel Winkler --- net/bluetooth/hci_request.c | 7 +-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index e0269192f2e536..e17bc8a1c66ddd 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -1533,11 +1533,14 @@ void __hci_req_update_scan_rsp_data(struct hci_request *req, u8 instance) memset(, 0, sizeof(cp)); - if (instance) + /* Extended scan response data doesn't allow a response to be +* set if the instance isn't scannable. +*/ + if (get_adv_instance_scan_rsp_len(hdev, instance)) len = create_instance_scan_rsp_data(hdev, instance, cp.data); else - len = create_default_scan_rsp_data(hdev, cp.data); + len = 0; if (hdev->scan_rsp_data_len == len && !memcmp(cp.data, hdev->scan_rsp_data, len)) -- 2.28.0.220.ged08abb693-goog
Re: [PATCH 0/3] Bluetooth: Emit events for suspend/resume
Hi, Gentle reminder that this is waiting for feedback. Related userspace changes are here to see how we plan on using it: https://patchwork.kernel.org/project/bluetooth/list/?series=325777 Thanks Abhishek On Tue, Jul 28, 2020 at 6:42 PM Abhishek Pandit-Subedi wrote: > > > Hi Marcel, > > This series adds the suspend/resume events suggested in > https://patchwork.kernel.org/patch/11663455/. > > I have tested it with some userspace changes that monitors the > controller resumed event to trigger audio device reconnection and > verified that the events are correctly emitted. > > Please take a look. > Abhishek > > > Abhishek Pandit-Subedi (3): > Bluetooth: Add mgmt suspend and resume events > Bluetooth: Add suspend reason for device disconnect > Bluetooth: Emit controller suspend and resume events > > include/net/bluetooth/hci_core.h | 6 +++ > include/net/bluetooth/mgmt.h | 16 +++ > net/bluetooth/hci_core.c | 26 +++- > net/bluetooth/hci_event.c| 73 > net/bluetooth/mgmt.c | 28 > 5 files changed, 148 insertions(+), 1 deletion(-) > > -- > 2.28.0.rc0.142.g3c755180ce-goog >
[PATCH] Revert "Bluetooth: btusb: Disable runtime suspend on Realtek devices"
This reverts commit 7ecacafc240638148567742cca41aa7144b4fe1e. Testing this change on a board with RTL8822CE, I found that enabling autosuspend has no effect on the stability of the system. The board continued working after autosuspend, suspend and reboot. The original commit makes it impossible to enable autosuspend on working systems so it should be reverted. Disabling autosuspend should be done via module param or udev in userspace instead. Signed-off-by: Abhishek Pandit-Subedi --- We have a few Chromebooks using the RTL 8822CE part over USB and they are running without problems with autosuspend enabled. While bringing up a new board, I found some power regressions that I was able to narrow down to this change so I'm requesting a revert. I tested this on Hp Chromebook 14a (running 4.14 kernel and 5.4 kernel) with this revert: * Enabled autosuspend, used it normally with a HID device * Suspended the Chromebook and verified it worked normally on resume * Rebooted the Chromebook and verified Bluetooth was working on next boot I didn't see the issue that was originally reported with this fix. For the original reporter, if you're still seeing this issue, there are other ways to disable autosuspend for your device: * set module param: enable_autosuspend=0 * change your kconfig so BT_HCIBTUSB_AUTOSUSPEND=n * use a udev rule to disable autosuspend for specific vid:pid Keeping this change in the kernel makes it impossible to enable autosuspend so it should be reverted. drivers/bluetooth/btusb.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c index 1f51494f581812..8d2608ddfd0875 100644 --- a/drivers/bluetooth/btusb.c +++ b/drivers/bluetooth/btusb.c @@ -4086,10 +4086,6 @@ static int btusb_probe(struct usb_interface *intf, set_bit(BTUSB_USE_ALT1_FOR_WBS, >flags); else bt_dev_err(hdev, "Device does not support ALT setting 1"); - - err = usb_autopm_get_interface(intf); - if (err < 0) - goto out_free_dev; } if (!reset) -- 2.28.0.rc0.142.g3c755180ce-goog
[PATCH 3/3] Bluetooth: Emit controller suspend and resume events
Emit controller suspend and resume events when we are ready for suspend
and we've resumed from suspend.
The controller suspend event will report whatever suspend state was
successfully entered. The controller resume event will check the first
HCI event that was received after we finished preparing for suspend and,
if it was a connection event, store the address of the peer that caused
the event. If it was not a connection event, we mark the wake reason as
an unexpected event.
Here is a sample btmon trace with these events:
@ MGMT Event: Controller Suspended (0x002d) plen 1
Suspend state: Page scanning and/or passive scanning (2)
@ MGMT Event: Controller Resumed (0x002e) plen 8
Wake reason: Remote wake due to peer device connection (2)
LE Address: CD:F3:CD:13:C5:9A (OUI CD-F3-CD)
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
include/net/bluetooth/hci_core.h | 3 ++
include/net/bluetooth/mgmt.h | 4 ++
net/bluetooth/hci_core.c | 26 +++-
net/bluetooth/hci_event.c| 73
4 files changed, 105 insertions(+), 1 deletion(-)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index 1b336e6ebe66aa..7314798e47c3d6 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -484,6 +484,9 @@ struct hci_dev {
enum suspended_statesuspend_state;
boolscanning_paused;
boolsuspended;
+ u8 wake_reason;
+ bdaddr_twake_addr;
+ u8 wake_addr_type;
wait_queue_head_t suspend_wait_q;
DECLARE_BITMAP(suspend_tasks, __SUSPEND_NUM_TASKS);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index 1a98f836aad126..052fe443484112 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1040,3 +1040,7 @@ struct mgmt_ev_controller_resume {
__u8wake_reason;
struct mgmt_addr_info addr;
} __packed;
+
+#define MGMT_WAKE_REASON_NON_BT_WAKE 0x0
+#define MGMT_WAKE_REASON_UNEXPECTED0x1
+#define MGMT_WAKE_REASON_REMOTE_WAKE 0x2
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 4ba23b821cbf4a..2cc121731d6a7e 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3470,12 +3470,24 @@ static int hci_change_suspend_state(struct hci_dev
*hdev,
return hci_suspend_wait_event(hdev);
}
+static void hci_clear_wake_reason(struct hci_dev *hdev)
+{
+ hci_dev_lock(hdev);
+
+ hdev->wake_reason = 0;
+ bacpy(>wake_addr, BDADDR_ANY);
+ hdev->wake_addr_type = 0;
+
+ hci_dev_unlock(hdev);
+}
+
static int hci_suspend_notifier(struct notifier_block *nb, unsigned long
action,
void *data)
{
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)) {
@@ -3496,15 +3508,27 @@ static int hci_suspend_notifier(struct notifier_block
*nb, unsigned long action,
* - Second, program event filter/whitelist and enable scan
*/
ret = hci_change_suspend_state(hdev, BT_SUSPEND_DISCONNECT);
+ if (!ret)
+ state = BT_SUSPEND_DISCONNECT;
/* Only configure whitelist if disconnect succeeded and wake
* isn't being prevented.
*/
- if (!ret && !(hdev->prevent_wake && hdev->prevent_wake(hdev)))
+ 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);
+
} else if (action == PM_POST_SUSPEND) {
ret = hci_change_suspend_state(hdev, BT_RUNNING);
+
+ mgmt_resuming(hdev, hdev->wake_reason, >wake_addr,
+ hdev->wake_addr_type);
}
done:
diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c
index 61f8c4d1202823..e92311ead456e5 100644
--- a/net/bluetooth/hci_event.c
+++ b/net/bluetooth/hci_event.c
@@ -5979,6 +5979,76 @@ static bool hci_get_cmd_complete(struct hci_dev *hdev,
u16 opcode,
return true;
}
+static void hci_store_wake_reason(struct hci_dev *hdev, u8 event,
+ struct sk_buff *skb)
+{
+ struct hci_ev_le_advertising_info *adv;
+ struct hci_ev_le_direct
[PATCH 1/3] Bluetooth: Add mgmt suspend and resume events
Add the controller suspend and resume events, which will signal when
Bluetooth has completed preparing for suspend and when it's ready for
resume.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
include/net/bluetooth/hci_core.h | 3 +++
include/net/bluetooth/mgmt.h | 11 +++
net/bluetooth/mgmt.c | 24
3 files changed, 38 insertions(+)
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index bee1b4778ccc96..1b336e6ebe66aa 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -1747,6 +1747,9 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t
*bdaddr, u8 link_type,
void mgmt_remote_name(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type,
u8 addr_type, s8 rssi, u8 *name, u8 name_len);
void mgmt_discovering(struct hci_dev *hdev, u8 discovering);
+void mgmt_suspending(struct hci_dev *hdev, u8 state);
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type);
bool mgmt_powering_down(struct hci_dev *hdev);
void mgmt_new_ltk(struct hci_dev *hdev, struct smp_ltk *key, bool persistent);
void mgmt_new_irk(struct hci_dev *hdev, struct smp_irk *irk, bool persistent);
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index beae5c3980f03b..d9a88cab379555 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -1028,3 +1028,14 @@ struct mgmt_ev_adv_monitor_added {
struct mgmt_ev_adv_monitor_removed {
__le16 monitor_handle;
} __packed;
+
+#define MGMT_EV_CONTROLLER_SUSPEND 0x002d
+struct mgmt_ev_controller_suspend {
+ __u8suspend_state;
+} __packed;
+
+#define MGMT_EV_CONTROLLER_RESUME 0x002e
+struct mgmt_ev_controller_resume {
+ __u8wake_reason;
+ struct mgmt_addr_info addr;
+} __packed;
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index f45105d2de7722..1c89ae819207ac 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8730,6 +8730,30 @@ void mgmt_discovering(struct hci_dev *hdev, u8
discovering)
mgmt_event(MGMT_EV_DISCOVERING, hdev, , sizeof(ev), NULL);
}
+void mgmt_suspending(struct hci_dev *hdev, u8 state)
+{
+ struct mgmt_ev_controller_suspend ev;
+
+ ev.suspend_state = state;
+ mgmt_event(MGMT_EV_CONTROLLER_SUSPEND, hdev, , sizeof(ev), NULL);
+}
+
+void mgmt_resuming(struct hci_dev *hdev, u8 reason, bdaddr_t *bdaddr,
+ u8 addr_type)
+{
+ struct mgmt_ev_controller_resume ev;
+
+ ev.wake_reason = reason;
+ if (bdaddr) {
+ bacpy(, bdaddr);
+ ev.addr.type = addr_type;
+ } else {
+ memset(, 0, sizeof(ev.addr));
+ }
+
+ mgmt_event(MGMT_EV_CONTROLLER_RESUME, hdev, , sizeof(ev), NULL);
+}
+
static struct hci_mgmt_chan chan = {
.channel= HCI_CHANNEL_CONTROL,
.handler_count = ARRAY_SIZE(mgmt_handlers),
--
2.28.0.rc0.142.g3c755180ce-goog
[PATCH 0/3] Bluetooth: Emit events for suspend/resume
Hi Marcel, This series adds the suspend/resume events suggested in https://patchwork.kernel.org/patch/11663455/. I have tested it with some userspace changes that monitors the controller resumed event to trigger audio device reconnection and verified that the events are correctly emitted. Please take a look. Abhishek Abhishek Pandit-Subedi (3): Bluetooth: Add mgmt suspend and resume events Bluetooth: Add suspend reason for device disconnect Bluetooth: Emit controller suspend and resume events include/net/bluetooth/hci_core.h | 6 +++ include/net/bluetooth/mgmt.h | 16 +++ net/bluetooth/hci_core.c | 26 +++- net/bluetooth/hci_event.c| 73 net/bluetooth/mgmt.c | 28 5 files changed, 148 insertions(+), 1 deletion(-) -- 2.28.0.rc0.142.g3c755180ce-goog
[PATCH 2/3] Bluetooth: Add suspend reason for device disconnect
Update device disconnect event with reason 0x5 to indicate that device
disconnected because the controller is suspending.
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
Reviewed-by: Sonny Sasaka
---
include/net/bluetooth/mgmt.h | 1 +
net/bluetooth/mgmt.c | 4
2 files changed, 5 insertions(+)
diff --git a/include/net/bluetooth/mgmt.h b/include/net/bluetooth/mgmt.h
index d9a88cab379555..1a98f836aad126 100644
--- a/include/net/bluetooth/mgmt.h
+++ b/include/net/bluetooth/mgmt.h
@@ -840,6 +840,7 @@ struct mgmt_ev_device_connected {
#define MGMT_DEV_DISCONN_LOCAL_HOST0x02
#define MGMT_DEV_DISCONN_REMOTE0x03
#define MGMT_DEV_DISCONN_AUTH_FAILURE 0x04
+#define MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND0x05
#define MGMT_EV_DEVICE_DISCONNECTED0x000C
struct mgmt_ev_device_disconnected {
diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c
index 1c89ae819207ac..fcda479134c756 100644
--- a/net/bluetooth/mgmt.c
+++ b/net/bluetooth/mgmt.c
@@ -8124,6 +8124,10 @@ void mgmt_device_disconnected(struct hci_dev *hdev,
bdaddr_t *bdaddr,
ev.addr.type = link_to_bdaddr(link_type, addr_type);
ev.reason = reason;
+ /* Report disconnects due to suspend */
+ if (hdev->suspended)
+ ev.reason = MGMT_DEV_DISCONN_LOCAL_HOST_SUSPEND;
+
mgmt_event(MGMT_EV_DEVICE_DISCONNECTED, hdev, , sizeof(ev), sk);
if (sk)
--
2.28.0.rc0.142.g3c755180ce-goog
[PATCH v3] Bluetooth: Fix suspend notifier race
Unregister from suspend notifications and cancel suspend preparations
before running hci_dev_do_close. Otherwise, the suspend notifier may
race with unregister and cause cmd_timeout even after hdev has been
freed.
Below is the trace from when this panic was seen:
[ 832.578518] Bluetooth: hci_core.c:hci_cmd_timeout() hci0: command 0x0c05 tx
timeout
[ 832.586200] BUG: kernel NULL pointer dereference, address:
[ 832.586203] #PF: supervisor read access in kernel mode
[ 832.586205] #PF: error_code(0x) - not-present page
[ 832.586206] PGD 0 P4D 0
[ 832.586210] PM: suspend exit
[ 832.608870] Oops: [#1] PREEMPT SMP NOPTI
[ 832.613232] CPU: 3 PID: 10755 Comm: kworker/3:7 Not tainted
5.4.44-04894-g1e9dbb96a161 #1
[ 832.630036] Workqueue: events hci_cmd_timeout [bluetooth]
[ 832.630046] RIP: 0010:__queue_work+0xf0/0x374
[ 832.630051] RSP: 0018:9b5285f1fdf8 EFLAGS: 00010046
[ 832.674033] RAX: 8a97681bac00 RBX: RCX: 8a976a000600
[ 832.681162] RDX: RSI: 0009 RDI: 8a976a000748
[ 832.688289] RBP: 9b5285f1fe38 R08: R09: 8a97681bac00
[ 832.695418] R10: 0002 R11: 8a976a0006d8 R12: 8a9745107600
[ 832.698045] usb 1-6: new full-speed USB device number 119 using xhci_hcd
[ 832.702547] R13: 8a9673658850 R14: 0040 R15: 001e
[ 832.702549] FS: () GS:8a976af8()
knlGS:
[ 832.702550] CS: 0010 DS: ES: CR0: 80050033
[ 832.702550] CR2: CR3: 00010415a000 CR4: 003406e0
[ 832.702551] Call Trace:
[ 832.702558] queue_work_on+0x3f/0x68
[ 832.702562] process_one_work+0x1db/0x396
[ 832.747397] worker_thread+0x216/0x375
[ 832.751147] kthread+0x138/0x140
[ 832.754377] ? pr_cont_work+0x58/0x58
[ 832.758037] ? kthread_blkcg+0x2e/0x2e
[ 832.761787] ret_from_fork+0x22/0x40
[ 832.846191] ---[ end trace fa93f466da517212 ]---
Fixes: 9952d90ea2885 ("Bluetooth: Handle PM_SUSPEND_PREPARE and
PM_POST_SUSPEND")
Signed-off-by: Abhishek Pandit-Subedi
Reviewed-by: Miao-chen Chou
---
Hi Marcel,
This fixes a race between hci_unregister_dev and the suspend notifier.
The suspend notifier handler seemed to be scheduling commands even after
it was cleaned up and this was resulting in a panic in cmd_timeout (when
it tries to requeue the cmd_timer).
This was tested on 5.4 kernel with a suspend+resume stress test for 500+
iterations. I also confirmed that after a usb disconnect, the suspend
notifier times out before the USB device is probed again (fixing the
original race between the usb_disconnect + probe and the notifier).
Thanks
Abhishek
Changes in v3:
* Added fixes tag
Changes in v2:
* Moved oops into commit message
net/bluetooth/hci_core.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 5394ab56c915a9..4ba23b821cbf4a 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -3767,9 +3767,10 @@ void hci_unregister_dev(struct hci_dev *hdev)
cancel_work_sync(>power_on);
- hci_dev_do_close(hdev);
-
unregister_pm_notifier(>suspend_notifier);
+ cancel_work_sync(>suspend_prepare);
+
+ hci_dev_do_close(hdev);
if (!test_bit(HCI_INIT, >flags) &&
!hci_dev_test_flag(hdev, HCI_SETUP) &&
--
2.28.0.rc0.142.g3c755180ce-goog
Re: [PATCH v2] Bluetooth: Fix suspend notifier race
I sent this a bit too quick without a Fixes tag. Please disregard. v3 coming up. On Tue, Jul 28, 2020 at 9:53 AM Abhishek Pandit-Subedi wrote: > > Unregister from suspend notifications and cancel suspend preparations > before running hci_dev_do_close. Otherwise, the suspend notifier may > race with unregister and cause cmd_timeout even after hdev has been > freed. > > Below is the trace from when this panic was seen: > > [ 832.578518] Bluetooth: hci_core.c:hci_cmd_timeout() hci0: command 0x0c05 > tx timeout > [ 832.586200] BUG: kernel NULL pointer dereference, address: > [ 832.586203] #PF: supervisor read access in kernel mode > [ 832.586205] #PF: error_code(0x) - not-present page > [ 832.586206] PGD 0 P4D 0 > [ 832.586210] PM: suspend exit > [ 832.608870] Oops: [#1] PREEMPT SMP NOPTI > [ 832.613232] CPU: 3 PID: 10755 Comm: kworker/3:7 Not tainted > 5.4.44-04894-g1e9dbb96a161 #1 > [ 832.630036] Workqueue: events hci_cmd_timeout [bluetooth] > [ 832.630046] RIP: 0010:__queue_work+0xf0/0x374 > [ 832.630051] RSP: 0018:9b5285f1fdf8 EFLAGS: 00010046 > [ 832.674033] RAX: 8a97681bac00 RBX: RCX: > 8a976a000600 > [ 832.681162] RDX: RSI: 0009 RDI: > 8a976a000748 > [ 832.688289] RBP: 9b5285f1fe38 R08: R09: > 8a97681bac00 > [ 832.695418] R10: 0002 R11: 8a976a0006d8 R12: > 8a9745107600 > [ 832.698045] usb 1-6: new full-speed USB device number 119 using xhci_hcd > [ 832.702547] R13: 8a9673658850 R14: 0040 R15: > 001e > [ 832.702549] FS: () GS:8a976af8() > knlGS: > [ 832.702550] CS: 0010 DS: ES: CR0: 80050033 > [ 832.702550] CR2: CR3: 00010415a000 CR4: > 003406e0 > [ 832.702551] Call Trace: > [ 832.702558] queue_work_on+0x3f/0x68 > [ 832.702562] process_one_work+0x1db/0x396 > [ 832.747397] worker_thread+0x216/0x375 > [ 832.751147] kthread+0x138/0x140 > [ 832.754377] ? pr_cont_work+0x58/0x58 > [ 832.758037] ? kthread_blkcg+0x2e/0x2e > [ 832.761787] ret_from_fork+0x22/0x40 > [ 832.846191] ---[ end trace fa93f466da517212 ]--- > > Signed-off-by: Abhishek Pandit-Subedi > Reviewed-by: Miao-chen Chou > --- > Hi Marcel, > > This fixes a race between hci_unregister_dev and the suspend notifier. > > The suspend notifier handler seemed to be scheduling commands even after > it was cleaned up and this was resulting in a panic in cmd_timeout (when > it tries to requeue the cmd_timer). > > This was tested on 5.4 kernel with a suspend+resume stress test for 500+ > iterations. I also confirmed that after a usb disconnect, the suspend > notifier times out before the USB device is probed again (fixing the > original race between the usb_disconnect + probe and the notifier). > > Thanks > Abhishek > > > Changes in v2: > * Moved oops into commit message > > net/bluetooth/hci_core.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c > index 5394ab56c915a9..4ba23b821cbf4a 100644 > --- a/net/bluetooth/hci_core.c > +++ b/net/bluetooth/hci_core.c > @@ -3767,9 +3767,10 @@ void hci_unregister_dev(struct hci_dev *hdev) > > cancel_work_sync(>power_on); > > - hci_dev_do_close(hdev); > - > unregister_pm_notifier(>suspend_notifier); > + cancel_work_sync(>suspend_prepare); > + > + hci_dev_do_close(hdev); > > if (!test_bit(HCI_INIT, >flags) && > !hci_dev_test_flag(hdev, HCI_SETUP) && > -- > 2.28.0.rc0.142.g3c755180ce-goog >
[PATCH v2] Bluetooth: Fix suspend notifier race
Unregister from suspend notifications and cancel suspend preparations before running hci_dev_do_close. Otherwise, the suspend notifier may race with unregister and cause cmd_timeout even after hdev has been freed. Below is the trace from when this panic was seen: [ 832.578518] Bluetooth: hci_core.c:hci_cmd_timeout() hci0: command 0x0c05 tx timeout [ 832.586200] BUG: kernel NULL pointer dereference, address: [ 832.586203] #PF: supervisor read access in kernel mode [ 832.586205] #PF: error_code(0x) - not-present page [ 832.586206] PGD 0 P4D 0 [ 832.586210] PM: suspend exit [ 832.608870] Oops: [#1] PREEMPT SMP NOPTI [ 832.613232] CPU: 3 PID: 10755 Comm: kworker/3:7 Not tainted 5.4.44-04894-g1e9dbb96a161 #1 [ 832.630036] Workqueue: events hci_cmd_timeout [bluetooth] [ 832.630046] RIP: 0010:__queue_work+0xf0/0x374 [ 832.630051] RSP: 0018:9b5285f1fdf8 EFLAGS: 00010046 [ 832.674033] RAX: 8a97681bac00 RBX: RCX: 8a976a000600 [ 832.681162] RDX: RSI: 0009 RDI: 8a976a000748 [ 832.688289] RBP: 9b5285f1fe38 R08: R09: 8a97681bac00 [ 832.695418] R10: 0002 R11: 8a976a0006d8 R12: 8a9745107600 [ 832.698045] usb 1-6: new full-speed USB device number 119 using xhci_hcd [ 832.702547] R13: 8a9673658850 R14: 0040 R15: 001e [ 832.702549] FS: () GS:8a976af8() knlGS: [ 832.702550] CS: 0010 DS: ES: CR0: 80050033 [ 832.702550] CR2: CR3: 00010415a000 CR4: 003406e0 [ 832.702551] Call Trace: [ 832.702558] queue_work_on+0x3f/0x68 [ 832.702562] process_one_work+0x1db/0x396 [ 832.747397] worker_thread+0x216/0x375 [ 832.751147] kthread+0x138/0x140 [ 832.754377] ? pr_cont_work+0x58/0x58 [ 832.758037] ? kthread_blkcg+0x2e/0x2e [ 832.761787] ret_from_fork+0x22/0x40 [ 832.846191] ---[ end trace fa93f466da517212 ]--- Signed-off-by: Abhishek Pandit-Subedi Reviewed-by: Miao-chen Chou --- Hi Marcel, This fixes a race between hci_unregister_dev and the suspend notifier. The suspend notifier handler seemed to be scheduling commands even after it was cleaned up and this was resulting in a panic in cmd_timeout (when it tries to requeue the cmd_timer). This was tested on 5.4 kernel with a suspend+resume stress test for 500+ iterations. I also confirmed that after a usb disconnect, the suspend notifier times out before the USB device is probed again (fixing the original race between the usb_disconnect + probe and the notifier). Thanks Abhishek Changes in v2: * Moved oops into commit message net/bluetooth/hci_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 5394ab56c915a9..4ba23b821cbf4a 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3767,9 +3767,10 @@ void hci_unregister_dev(struct hci_dev *hdev) cancel_work_sync(>power_on); - hci_dev_do_close(hdev); - unregister_pm_notifier(>suspend_notifier); + cancel_work_sync(>suspend_prepare); + + hci_dev_do_close(hdev); if (!test_bit(HCI_INIT, >flags) && !hci_dev_test_flag(hdev, HCI_SETUP) && -- 2.28.0.rc0.142.g3c755180ce-goog
[PATCH] Bluetooth: Fix suspend notifier race
Unregister from suspend notifications and cancel suspend preparations before running hci_dev_do_close. Otherwise, the suspend notifier may race with unregister and cause cmd_timeout even after hdev has been freed. Signed-off-by: Abhishek Pandit-Subedi Reviewed-by: Miao-chen Chou --- Hi Marcel, This fixes a race between hci_unregister_dev and the suspend notifier. Without these changes, we encountered the following kernel panic when a USB disconnect (with btusb) occurred on resume: [ 832.578518] Bluetooth: hci_core.c:hci_cmd_timeout() hci0: command 0x0c05 tx timeout [ 832.586200] BUG: kernel NULL pointer dereference, address: [ 832.586203] #PF: supervisor read access in kernel mode [ 832.586205] #PF: error_code(0x) - not-present page [ 832.586206] PGD 0 P4D 0 [ 832.586210] PM: suspend exit [ 832.608870] Oops: [#1] PREEMPT SMP NOPTI [ 832.613232] CPU: 3 PID: 10755 Comm: kworker/3:7 Not tainted 5.4.44-04894-g1e9dbb96a161 #1 [ 832.630036] Workqueue: events hci_cmd_timeout [bluetooth] [ 832.630046] RIP: 0010:__queue_work+0xf0/0x374 [ 832.630051] RSP: 0018:9b5285f1fdf8 EFLAGS: 00010046 [ 832.674033] RAX: 8a97681bac00 RBX: RCX: 8a976a000600 [ 832.681162] RDX: RSI: 0009 RDI: 8a976a000748 [ 832.688289] RBP: 9b5285f1fe38 R08: R09: 8a97681bac00 [ 832.695418] R10: 0002 R11: 8a976a0006d8 R12: 8a9745107600 [ 832.698045] usb 1-6: new full-speed USB device number 119 using xhci_hcd [ 832.702547] R13: 8a9673658850 R14: 0040 R15: 001e [ 832.702549] FS: () GS:8a976af8() knlGS: [ 832.702550] CS: 0010 DS: ES: CR0: 80050033 [ 832.702550] CR2: CR3: 00010415a000 CR4: 003406e0 [ 832.702551] Call Trace: [ 832.702558] queue_work_on+0x3f/0x68 [ 832.702562] process_one_work+0x1db/0x396 [ 832.747397] worker_thread+0x216/0x375 [ 832.751147] kthread+0x138/0x140 [ 832.754377] ? pr_cont_work+0x58/0x58 [ 832.758037] ? kthread_blkcg+0x2e/0x2e [ 832.761787] ret_from_fork+0x22/0x40 [ 832.846191] ---[ end trace fa93f466da517212 ]--- The suspend notifier handler seemed to be scheduling commands even after it was cleaned up and this was resulting in a panic in cmd_timeout (when it tries to requeue the cmd_timer). This was tested on 5.4 kernel with a suspend+resume stress test for 500+ iterations. I also confirmed that after a usb disconnect, the suspend notifier times out before the USB device is probed again (fixing the original race between the usb_disconnect + probe and the notifier). Thanks Abhishek net/bluetooth/hci_core.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 6509f785dd1481..97221d1fa883d1 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3765,9 +3765,10 @@ void hci_unregister_dev(struct hci_dev *hdev) cancel_work_sync(>power_on); - hci_dev_do_close(hdev); - unregister_pm_notifier(>suspend_notifier); + cancel_work_sync(>suspend_prepare); + + hci_dev_do_close(hdev); if (!test_bit(HCI_INIT, >flags) && !hci_dev_test_flag(hdev, HCI_SETUP) && -- 2.28.0.rc0.142.g3c755180ce-goog
[RFC] cpuidle : Add support for pseudo-cpuidle driver
This option adds support for a testing cpuidle driver, which allows
user to define custom idle states with their respective latencies and
residencies. This is useful for testing the behaviour of governors on
customized set of idle states.
This can be used as of now by hard-coding the customized set of cpuidle
states in the driver. Will add the capability of this driver to be used
as a module in subsequent patches.
Original idea and discussion for this patch can be found at:
https://lkml.org/lkml/2019/12/17/655
Signed-off-by: Abhishek Goel
---
drivers/cpuidle/Kconfig| 9 ++
drivers/cpuidle/Makefile | 1 +
drivers/cpuidle/cpuidle-test.c | 276 +
3 files changed, 286 insertions(+)
create mode 100644 drivers/cpuidle/cpuidle-test.c
diff --git a/drivers/cpuidle/Kconfig b/drivers/cpuidle/Kconfig
index c0aeedd66f02..1d73153a0e35 100644
--- a/drivers/cpuidle/Kconfig
+++ b/drivers/cpuidle/Kconfig
@@ -71,6 +71,15 @@ config HALTPOLL_CPUIDLE
before halting in the guest (more efficient than polling in the
host via halt_poll_ns for some scenarios).
+config TEST_CPUIDLE
+ tristate "cpuidle test driver"
+ default m
+ help
+This option enables a testing cpuidle driver, which allows to user
+to define custom idle states with their respective latencies and
residencies.
+This is useful for testing the behaviour of governors on different
+set of idle states.
+
endif
config ARCH_NEEDS_CPU_IDLE_COUPLED
diff --git a/drivers/cpuidle/Makefile b/drivers/cpuidle/Makefile
index f07800cbb43f..68ea7dc257b5 100644
--- a/drivers/cpuidle/Makefile
+++ b/drivers/cpuidle/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED) += coupled.o
obj-$(CONFIG_DT_IDLE_STATES) += dt_idle_states.o
obj-$(CONFIG_ARCH_HAS_CPU_RELAX) += poll_state.o
obj-$(CONFIG_HALTPOLL_CPUIDLE) += cpuidle-haltpoll.o
+obj-$(CONFIG_TEST_CPUIDLE) += cpuidle-test.o
##
# ARM SoC drivers
diff --git a/drivers/cpuidle/cpuidle-test.c b/drivers/cpuidle/cpuidle-test.c
new file mode 100644
index ..399729440569
--- /dev/null
+++ b/drivers/cpuidle/cpuidle-test.c
@@ -0,0 +1,276 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cpuidle-test - Test driver for cpuidle.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CPUIDLE_STATE_MAX 10
+#define MAX_PARAM_LENGTH 100
+
+static unsigned int nr_states = 4;
+static unsigned int sim_type = 1;
+static char name[MAX_PARAM_LENGTH];
+static char latency_us[MAX_PARAM_LENGTH];
+static char residency_us[MAX_PARAM_LENGTH];
+
+
+module_param(nr_states, uint, 0644);
+module_param(sim_type, uint, 0644);
+module_param_string(name, name, MAX_PARAM_LENGTH, 0644);
+module_param_string(latency_us, latency_us, MAX_PARAM_LENGTH, 0644);
+module_param_string(residency_us, residency_us, MAX_PARAM_LENGTH, 0644);
+
+static struct cpuidle_driver test_cpuidle_driver = {
+ .name = "test_cpuidle",
+ .owner = THIS_MODULE,
+};
+
+static struct cpuidle_state *cpuidle_state_table __read_mostly;
+
+static struct cpuidle_device __percpu *test_cpuidle_devices;
+static enum cpuhp_state test_hp_idlestate;
+
+
+static int __cpuidle idle_loop(struct cpuidle_device *dev,
+ struct cpuidle_driver *drv,
+ int index)
+{
+ u64 time_start;
+
+ local_irq_enable();
+ if (!current_set_polling_and_test()) {
+ while (!need_resched())
+ cpu_relax();
+ }
+
+ time_start = local_clock();
+
+ while (local_clock() - time_start < drv->states[index].exit_latency)
+
+ current_clr_polling();
+
+ return index;
+}
+
+static struct cpuidle_state cpuidle_states[CPUIDLE_STATE_MAX] = {
+ { /* Snooze */
+ .name = "snooze",
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = idle_loop },
+};
+
+static struct cpuidle_state cpuidle_states_ppc[] = {
+ { .name = "snooze",
+ .exit_latency = 0,
+ .target_residency = 0,
+ .enter = idle_loop },
+ {
+ .name = "stop0",
+ .exit_latency = 2,
+ .target_residency = 20,
+ .enter = idle_loop },
+ {
+ .name = "stop1",
+ .exit_latency = 5,
+ .target_residency = 50,
+ .enter = idle_loop },
+ {
+ .name = "stop2",
+ .exit_latency = 10,
+ .target_residency = 100,
+ .enter = idle_loop },
+};
+
+static struct cpuidle_state cpuidle_states_i
Re: [power] 47b918cf9a: kmsg.power_supply_ADP1:Error_in_uevent_for_wakeup_sysfs_add
This version of the patch was not merged and the message above doesn't
exist in the merged patch:
https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git/commit/?h=bleeding-edge=9a3e9e6ff6d7f6b8ce7903893962d50adcbe82d2
The err log was emitted during boot as well and is innocuous since the
power_supply initializes fully in the next line:
kern :err : [5.918034] power_supply ADP1: Error in uevent for
wakeup_sysfs_add: -11
kern :info : [5.918300] ACPI: AC Adapter [ADP1] (on-line)
Abhishek
On Mon, Jul 13, 2020 at 10:30 PM kernel test robot
wrote:
>
> Greeting,
>
> FYI, we noticed the following commit (built with gcc-9):
>
> commit: 47b918cf9a1d2b6e36706fd2be2b91e65f490146 ("[PATCH v2 1/1] power: Emit
> changed uevent on wakeup_sysfs_add/remove")
> url:
> https://github.com/0day-ci/linux/commits/Abhishek-Pandit-Subedi/power-Emit-changed-uevent-on-wakeup_sysfs_add-remove/20200707-050912
> base: https://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git
> linux-next
>
> in testcase: suspend-stress
> with following parameters:
>
> mode: freeze
> iterations: 10
>
>
>
> on test machine: 4 threads Ivy Bridge with 4G memory
>
> caused below changes (please refer to attached dmesg/kmsg for entire
> log/backtrace):
>
>
>
>
> If you fix the issue, kindly add following tag
> Reported-by: kernel test robot
>
>
>
> kern :debug : [5.917685] calling acpi_ac_init+0x0/0xa3 @ 1
> kern :err : [5.918034] power_supply ADP1: Error in uevent for
> wakeup_sysfs_add: -11
> kern :info : [5.918300] ACPI: AC Adapter [ADP1] (on-line)
> kern :debug : [5.918500] initcall acpi_ac_init+0x0/0xa3 returned 0 after
> 609 usecs
> kern :debug : [5.918725] calling acpi_button_driver_init+0x0/0x53 @ 1
> kern :info : [5.919006] input: Power Button as
> /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0C:00/input/input0
> kern :info : [5.919367] ACPI: Power Button [PWRB]
> kern :info : [5.919580] input: Lid Switch as
> /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0C0D:00/input/input1
> kern :info : [5.919927] ACPI: Lid Switch [LID]
> kern :info : [5.920131] input: Power Button as
> /devices/LNXSYSTM:00/LNXPWRBN:00/input/input2
> kern :info : [5.920455] ACPI: Power Button [PWRF]
> kern :debug : [5.920644] initcall acpi_button_driver_init+0x0/0x53
> returned 0 after 1669 usecs
> kern :debug : [5.920944] calling acpi_fan_driver_init+0x0/0x13 @ 1
> kern :debug : [5.921155] initcall acpi_fan_driver_init+0x0/0x13 returned
> 0 after 11 usecs
> kern :debug : [5.921388] calling acpi_processor_driver_init+0x0/0xb7 @ 1
> kern :debug : [5.921905] initcall acpi_processor_driver_init+0x0/0xb7
> returned 0 after 299 usecs
> kern :debug : [5.922203] calling acpi_thermal_init+0x0/0x82 @ 1
> kern :info : [5.922755] thermal LNXTHERM:00: registered as thermal_zone0
> kern :info : [5.922977] ACPI: Thermal Zone [TZ01] (16 C)
> kern :debug : [5.923177] initcall acpi_thermal_init+0x0/0x82 returned 0
> after 759 usecs
> kern :debug : [5.923409] calling acpi_battery_init+0x0/0x39 @ 1
> kern :debug : [5.923606] initcall acpi_battery_init+0x0/0x39 returned 0
> after 4 usecs
> kern :debug : [5.923841] calling acpi_hed_driver_init+0x0/0x11 @ 1
> kern :debug : [5.924075] initcall acpi_hed_driver_init+0x0/0x11 returned
> 0 after 32 usecs
> kern :info : [5.924178] battery: ACPI: Battery Slot [BAT1] (battery
> present)
> kern :debug : [5.924309] calling bgrt_init+0x0/0xbe @ 1
> kern :debug : [5.924312] initcall bgrt_init+0x0/0xbe returned -19 after
> 0 usecs
> kern :debug : [5.924928] calling erst_init+0x0/0x309 @ 1
> kern :debug : [5.925110] initcall erst_init+0x0/0x309 returned 0 after 0
> usecs
> kern :debug : [5.925325] calling ghes_init+0x0/0xe5 @ 1
> kern :debug : [5.925504] initcall ghes_init+0x0/0xe5 returned -19 after
> 0 usecs
> kern :debug : [5.925721] calling erst_dbg_init+0x0/0x2c @ 1
> kern :info : [5.925912] ERST DBG: ERST support is disabled.
>
>
>
> To reproduce:
>
> git clone https://github.com/intel/lkp-tests.git
> cd lkp-tests
> bin/lkp install job.yaml # job file is attached in this email
> bin/lkp run job.yaml
>
>
>
> Thanks,
> Rong Chen
>
Re: [PATCH v4] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode
On Wed, Jul 8, 2020 at 12:34 PM Randy Dunlap wrote: > > Hi again, > > On 7/8/20 12:25 PM, Abhishek Bhardwaj wrote: > > diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig > > index b277a2db62676..1f85374a0b812 100644 > > --- a/arch/x86/kvm/Kconfig > > +++ b/arch/x86/kvm/Kconfig > > @@ -107,4 +107,17 @@ config KVM_MMU_AUDIT > >This option adds a R/W kVM module parameter 'mmu_audit', which allows > >auditing of KVM MMU events at runtime. > > > > +config KVM_VMENTRY_L1D_FLUSH > > + int "L1D cache flush settings (1-3)" > > + range 1 3 > > + default "2" > > + depends on KVM && X86_64 > > + help > > + This setting determines the L1D cache flush behavior before a > > VMENTER. > > + This is similar to setting the option / parameter to > > + kvm-intel.vmentry_l1d_flush. > > + 1 - Never flush. > > + 2 - Conditionally flush. > > + 3 - Always flush. > > + > > endif # VIRTUALIZATION > > If you do a v5, the help text lines (under "help") should be indented > with one tab + 2 spaces according to Documentation/process/coding-style.rst. Apologies for missing this. Fixed in v5 - https://lkml.org/lkml/2020/7/8/1369 > > -- > ~Randy > -- Abhishek
[PATCH v5] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode
This change adds a new kernel configuration that sets the l1d cache flush setting at compile time rather than at run time. The reasons for this change are as follows - - Kernel command line arguments are getting unwieldy. These parameters are not a scalable way to set the kernel config. They're intended as a super limited way for the bootloader to pass info to the kernel and also as a way for end users who are not compiling the kernel themselves to tweak the kernel behavior. - Also, if a user wants this setting from the start. It's a definite smell that it deserves to be a compile time thing rather than adding extra code plus whatever miniscule time at runtime to pass an extra argument. - Finally, it doesn't preclude the runtime / kernel command line way. Users are free to use those as well. Signed-off-by: Abhishek Bhardwaj --- Changes in v5: - Fix indentation of the help text in the KConfig. Changes in v4: - Add motivation for the change in the commit message. Changes in v3: - Change depends on to only x86_64. - Remove copy paste errors at the end of the KConfig. Changes in v2: - Fix typo in the help of the new KConfig. arch/x86/kernel/cpu/bugs.c | 8 arch/x86/kvm/Kconfig | 13 + 2 files changed, 21 insertions(+) diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 0b71970d2d3d2..1dcc875cf5547 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1406,7 +1406,15 @@ enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH; #if IS_ENABLED(CONFIG_KVM_INTEL) EXPORT_SYMBOL_GPL(l1tf_mitigation); #endif +#if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_NEVER; +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_COND; +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_ALWAYS; +#else enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO; +#endif EXPORT_SYMBOL_GPL(l1tf_vmx_mitigation); /* diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index b277a2db62676..cb2639dfb6da1 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -107,4 +107,17 @@ config KVM_MMU_AUDIT This option adds a R/W kVM module parameter 'mmu_audit', which allows auditing of KVM MMU events at runtime. +config KVM_VMENTRY_L1D_FLUSH + int "L1D cache flush settings (1-3)" + range 1 3 + default "2" + depends on KVM && X86_64 + help + This setting determines the L1D cache flush behavior before a VMENTER. + This is similar to setting the option / parameter to + kvm-intel.vmentry_l1d_flush. + 1 - Never flush. + 2 - Conditionally flush. + 3 - Always flush. + endif # VIRTUALIZATION -- 2.27.0.383.g050319c2ae-goog
Re: [PATCH v3] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode
On Mon, Jul 6, 2020 at 8:14 AM Waiman Long wrote:
>
> On 7/5/20 5:51 PM, Abhishek Bhardwaj wrote:
>
> That is why I said a comment will have to be added to highlight this
> dependency. For instance,
>
> +/*
> + * Three of the enums are explicitly assigned as the KVM_VMENTRY_L1D_FLUSH
> + * config entry in arch/x86/kvm/Kconfig depends on these values.
> + */
> enum vmx_l1d_flush_state {
> VMENTER_L1D_FLUSH_AUTO,
> - VMENTER_L1D_FLUSH_NEVER,
> - VMENTER_L1D_FLUSH_COND,
> - VMENTER_L1D_FLUSH_ALWAYS,
> + VMENTER_L1D_FLUSH_NEVER = 1,
> + VMENTER_L1D_FLUSH_COND = 2,
> + VMENTER_L1D_FLUSH_ALWAYS = 3,
> VMENTER_L1D_FLUSH_EPT_DISABLED,
> VMENTER_L1D_FLUSH_NOT_REQUIRED,
> };
>
> Of course, this is just a suggestion.
>
> I'd rather avoid this dependency. However, if people are okay with the
> CONFIG option then I am happy to oblige with whatever people agree on.
> Can a maintainer chime in ? Waiman if you're the final authority on
> this, will you accept the patch if I incorporated your suggestion ?
>
> It is fine if you think this is not what you want.
>
> BTW, I am not a maintainer. However, no maintainer will give pre-approval. At
> least, you have to give the reason why this patch is needed in the patch
> itself. Before that happens, I don't see how it will get merged upstream.
I just updated the patch with the reasoning in the commit message -
https://lkml.org/lkml/2020/7/8/1325
>
> Cheers,
> Longman
--
Abhishek
[PATCH v4] x86/speculation/l1tf: Add KConfig for setting the L1D cache flush mode
This change adds a new kernel configuration that sets the l1d cache flush setting at compile time rather than at run time. The reasons for this change are as follows - - Kernel command line arguments are getting unwieldy. These parameters are not a scalable way to set the kernel config. They're intended as a super limited way for the bootloader to pass info to the kernel and also as a way for end users who are not compiling the kernel themselves to tweak the kernel behavior. - Also, if a user wants this setting from the start. It's a definite smell that it deserves to be a compile time thing rather than adding extra code plus whatever miniscule time at runtime to pass an extra argument. - Finally, it doesn't preclude the runtime / kernel command line way. Users are free to use those as well. Signed-off-by: Abhishek Bhardwaj --- Changes in v4: - Add motivation for the change in the commit message. Changes in v3: - Change depends on to only x86_64. - Remove copy paste errors at the end of the KConfig. Changes in v2: - Fix typo in the help of the new KConfig. arch/x86/kernel/cpu/bugs.c | 8 arch/x86/kvm/Kconfig | 13 + 2 files changed, 21 insertions(+) diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c index 0b71970d2d3d2..1dcc875cf5547 100644 --- a/arch/x86/kernel/cpu/bugs.c +++ b/arch/x86/kernel/cpu/bugs.c @@ -1406,7 +1406,15 @@ enum l1tf_mitigations l1tf_mitigation __ro_after_init = L1TF_MITIGATION_FLUSH; #if IS_ENABLED(CONFIG_KVM_INTEL) EXPORT_SYMBOL_GPL(l1tf_mitigation); #endif +#if (CONFIG_KVM_VMENTRY_L1D_FLUSH == 1) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_NEVER; +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 2) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_COND; +#elif (CONFIG_KVM_VMENTRY_L1D_FLUSH == 3) +enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_ALWAYS; +#else enum vmx_l1d_flush_state l1tf_vmx_mitigation = VMENTER_L1D_FLUSH_AUTO; +#endif EXPORT_SYMBOL_GPL(l1tf_vmx_mitigation); /* diff --git a/arch/x86/kvm/Kconfig b/arch/x86/kvm/Kconfig index b277a2db62676..1f85374a0b812 100644 --- a/arch/x86/kvm/Kconfig +++ b/arch/x86/kvm/Kconfig @@ -107,4 +107,17 @@ config KVM_MMU_AUDIT This option adds a R/W kVM module parameter 'mmu_audit', which allows auditing of KVM MMU events at runtime. +config KVM_VMENTRY_L1D_FLUSH + int "L1D cache flush settings (1-3)" + range 1 3 + default "2" + depends on KVM && X86_64 + help +This setting determines the L1D cache flush behavior before a VMENTER. +This is similar to setting the option / parameter to +kvm-intel.vmentry_l1d_flush. +1 - Never flush. +2 - Conditionally flush. +3 - Always flush. + endif # VIRTUALIZATION -- 2.27.0.383.g050319c2ae-goog
[PATCH v5 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
Udev rules that depend on the power/wakeup attribute don't get triggered
correctly if device_set_wakeup_capable is called after the device is
created. This can happen for several reasons (driver sets wakeup after
device is created, wakeup is changed on parent device, etc) and it seems
reasonable to emit a changed event when adding or removing attributes on
the device.
Signed-off-by: Abhishek Pandit-Subedi
---
Changes in v5:
- Ignore return from kobject_uevent when adding to sysfs
Changes in v4:
- Fix warning where returning from void and tested on device
Changes in v3:
- Simplified error handling
Changes in v2:
- Add newline at end of bt_dev_err
drivers/base/power/sysfs.c | 9 -
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
index 24d25cf8ab1487..c7b24812523c9e 100644
--- a/drivers/base/power/sysfs.c
+++ b/drivers/base/power/sysfs.c
@@ -1,6 +1,7 @@
// SPDX-License-Identifier: GPL-2.0
/* sysfs entries for device PM */
#include
+#include
#include
#include
#include
@@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t
kuid, kgid_t kgid)
int wakeup_sysfs_add(struct device *dev)
{
- return sysfs_merge_group(>kobj, _wakeup_attr_group);
+ int ret = sysfs_merge_group(>kobj, _wakeup_attr_group);
+
+ if (!ret)
+ kobject_uevent(>kobj, KOBJ_CHANGE);
+
+ return ret;
}
void wakeup_sysfs_remove(struct device *dev)
{
sysfs_unmerge_group(>kobj, _wakeup_attr_group);
+ kobject_uevent(>kobj, KOBJ_CHANGE);
}
int pm_qos_sysfs_add_resume_latency(struct device *dev)
--
2.27.0.212.ge8ba1cc988-goog
[PATCH v5 0/1] power: Emit change uevent when updating sysfs
Hi linux-pm, ChromeOS has a udev rule to chown the `power/wakeup` attribute so that the power manager can modify it during runtime. (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/platform2/power_manager/udev/99-powerd-permissions.rules) In our automated tests, we found that the `power/wakeup` attributes weren't being chown-ed for some boards. On investigating, I found that when the drivers probe and call device_set_wakeup_capable, no uevent was being emitted for the newly added power/wakeup attribute. This was manifesting at boot on some boards (Marvell SDIO bluetooth and Broadcom Serial bluetooth drivers) or during usb disconnects during resume (Realtek btusb driver with reset resume quirk). It seems reasonable to me that changes to the attributes of a device should cause a changed uevent so I have added that here. Here's an example of the kernel events after toggling the authorized bit of /sys/bus/usb/devices/1-3/ $ echo 0 > /sys/bus/usb/devices/1-3/authorized KERNEL[27.357994] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill1 (rfkill) KERNEL[27.358049] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[27.358458] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[27.358486] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) KERNEL[27.358529] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) $ echo 1 > /sys/bus/usb/devices/1-3/authorized KERNEL[36.415749] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[36.415798] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[36.417414] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[36.417447] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill) KERNEL[36.417481] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) Series-version 4 update: Tested again on device and verified it's working as expected: KERNEL[52.678174] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill) KERNEL[52.678211] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[52.678251] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[52.679721] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) KERNEL[52.679772] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[56.022259] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[56.022306] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[56.022488] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[56.022531] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill3 (rfkill) KERNEL[56.023392] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) Thanks Abhishek Changes in v5: - Ignore return from kobject_uevent when adding to sysfs Changes in v4: - Fix warning where returning from void and tested on device Changes in v3: - Simplified error handling Changes in v2: - Add newline at end of bt_dev_err Abhishek Pandit-Subedi (1): power: Emit changed uevent on wakeup_sysfs_add/remove drivers/base/power/sysfs.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) -- 2.27.0.212.ge8ba1cc988-goog
Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
Sounds good to me. Patch v5 incoming after compile-test.
Thanks
Abhishek
On Tue, Jul 7, 2020 at 10:16 AM Rafael J. Wysocki wrote:
>
> On Tue, Jul 7, 2020 at 6:48 PM Abhishek Pandit-Subedi
> wrote:
> >
> > Hi Rafael,
> >
> > (resent in plain text)
> >
> > On Tue, Jul 7, 2020 at 9:28 AM Rafael J. Wysocki wrote:
> > >
> > > On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
> > > wrote:
> > > >
> > > > Udev rules that depend on the power/wakeup attribute don't get triggered
> > > > correctly if device_set_wakeup_capable is called after the device is
> > > > created. This can happen for several reasons (driver sets wakeup after
> > > > device is created, wakeup is changed on parent device, etc) and it seems
> > > > reasonable to emit a changed event when adding or removing attributes on
> > > > the device.
> > > >
> > > > Signed-off-by: Abhishek Pandit-Subedi
> > > > ---
> > > >
> > > > Changes in v4:
> > > > - Fix warning where returning from void and tested on device
> > > >
> > > > Changes in v3:
> > > > - Simplified error handling
> > > >
> > > > Changes in v2:
> > > > - Add newline at end of bt_dev_err
> > > >
> > > > drivers/base/power/sysfs.c | 9 -
> > > > 1 file changed, 8 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > > > index 24d25cf8ab1487..aeb58d40aac8de 100644
> > > > --- a/drivers/base/power/sysfs.c
> > > > +++ b/drivers/base/power/sysfs.c
> > > > @@ -1,6 +1,7 @@
> > > > // SPDX-License-Identifier: GPL-2.0
> > > > /* sysfs entries for device PM */
> > > > #include
> > > > +#include
> > > > #include
> > > > #include
> > > > #include
> > > > @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev,
> > > > kuid_t kuid, kgid_t kgid)
> > > >
> > > > int wakeup_sysfs_add(struct device *dev)
> > > > {
> > > > - return sysfs_merge_group(>kobj, _wakeup_attr_group);
> > > > + int ret = sysfs_merge_group(>kobj, _wakeup_attr_group);
> > > > +
> > > > + if (ret)
> > > > + return ret;
> > > > +
> > > > + return kobject_uevent(>kobj, KOBJ_CHANGE);
> > >
> > > So let me repeat the previous comment:
> > >
> > > If you return an error here, it may confuse the caller to think that
> > > the operation has failed completely, whereas the merging of the
> > > attribute group has been successful already.
> > >
> > > I don't think that an error can be returned at this point.
> > >
> >
> > The caller looks at the return code and just logs that an error
> > occurred (no other action). It's also unlikely for kobject_uevent to
> > fail (I saw mostly -ENOMEM and an -ENOENT when the kobj wasn't in the
> > correct set).
> >
> > Call site:
> > int ret = wakeup_sysfs_add(dev);
> >
> > if (ret)
> > dev_info(dev, "Wakeup sysfs attributes not added\n");
>
> Yes, which is confusing, because the wakeup attributes may in fact
> have been added. Which is my point.
>
> >
> > So I'm ok with either keeping this as-is (caller isn't getting
> > confused, just logging) or swallowing the return of kobject_uevent.
>
> I would just ignore the return value of kobject_uevent() along the
> lines of wakeup_sysfs_remove() below.
>
> Thanks!
>
> > > > }
> > > >
> > > > void wakeup_sysfs_remove(struct device *dev)
> > > > {
> > > > sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> > > > + kobject_uevent(>kobj, KOBJ_CHANGE);
> > > > }
> > > >
> > > > int pm_qos_sysfs_add_resume_latency(struct device *dev)
> > > > --
> > > > 2.27.0.212.ge8ba1cc988-goog
> > > >
Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
Hi Rafael,
(resent in plain text)
On Tue, Jul 7, 2020 at 9:28 AM Rafael J. Wysocki wrote:
>
> On Tue, Jul 7, 2020 at 6:24 PM Abhishek Pandit-Subedi
> wrote:
> >
> > Udev rules that depend on the power/wakeup attribute don't get triggered
> > correctly if device_set_wakeup_capable is called after the device is
> > created. This can happen for several reasons (driver sets wakeup after
> > device is created, wakeup is changed on parent device, etc) and it seems
> > reasonable to emit a changed event when adding or removing attributes on
> > the device.
> >
> > Signed-off-by: Abhishek Pandit-Subedi
> > ---
> >
> > Changes in v4:
> > - Fix warning where returning from void and tested on device
> >
> > Changes in v3:
> > - Simplified error handling
> >
> > Changes in v2:
> > - Add newline at end of bt_dev_err
> >
> > drivers/base/power/sysfs.c | 9 -
> > 1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> > index 24d25cf8ab1487..aeb58d40aac8de 100644
> > --- a/drivers/base/power/sysfs.c
> > +++ b/drivers/base/power/sysfs.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> > /* sysfs entries for device PM */
> > #include
> > +#include
> > #include
> > #include
> > #include
> > @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t
> > kuid, kgid_t kgid)
> >
> > int wakeup_sysfs_add(struct device *dev)
> > {
> > - return sysfs_merge_group(>kobj, _wakeup_attr_group);
> > + int ret = sysfs_merge_group(>kobj, _wakeup_attr_group);
> > +
> > + if (ret)
> > + return ret;
> > +
> > + return kobject_uevent(>kobj, KOBJ_CHANGE);
>
> So let me repeat the previous comment:
>
> If you return an error here, it may confuse the caller to think that
> the operation has failed completely, whereas the merging of the
> attribute group has been successful already.
>
> I don't think that an error can be returned at this point.
>
The caller looks at the return code and just logs that an error
occurred (no other action). It's also unlikely for kobject_uevent to
fail (I saw mostly -ENOMEM and an -ENOENT when the kobj wasn't in the
correct set).
Call site:
int ret = wakeup_sysfs_add(dev);
if (ret)
dev_info(dev, "Wakeup sysfs attributes not added\n");
So I'm ok with either keeping this as-is (caller isn't getting
confused, just logging) or swallowing the return of kobject_uevent.
> > }
> >
> > void wakeup_sysfs_remove(struct device *dev)
> > {
> > sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> > + kobject_uevent(>kobj, KOBJ_CHANGE);
> > }
> >
> > int pm_qos_sysfs_add_resume_latency(struct device *dev)
> > --
> > 2.27.0.212.ge8ba1cc988-goog
> >
Re: [PATCH v4 1/1] power: Emit changed uevent on wakeup_sysfs_add/remove
Built and tested on Chromebook w/ 5.4 kernel.
Sorry about the churn -- will start building with warnings = errors
before I send patches upstream.
Thanks
Abhishek
On Tue, Jul 7, 2020 at 9:24 AM Abhishek Pandit-Subedi
wrote:
>
> Udev rules that depend on the power/wakeup attribute don't get triggered
> correctly if device_set_wakeup_capable is called after the device is
> created. This can happen for several reasons (driver sets wakeup after
> device is created, wakeup is changed on parent device, etc) and it seems
> reasonable to emit a changed event when adding or removing attributes on
> the device.
>
> Signed-off-by: Abhishek Pandit-Subedi
> ---
>
> Changes in v4:
> - Fix warning where returning from void and tested on device
>
> Changes in v3:
> - Simplified error handling
>
> Changes in v2:
> - Add newline at end of bt_dev_err
>
> drivers/base/power/sysfs.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/sysfs.c b/drivers/base/power/sysfs.c
> index 24d25cf8ab1487..aeb58d40aac8de 100644
> --- a/drivers/base/power/sysfs.c
> +++ b/drivers/base/power/sysfs.c
> @@ -1,6 +1,7 @@
> // SPDX-License-Identifier: GPL-2.0
> /* sysfs entries for device PM */
> #include
> +#include
> #include
> #include
> #include
> @@ -739,12 +740,18 @@ int dpm_sysfs_change_owner(struct device *dev, kuid_t
> kuid, kgid_t kgid)
>
> int wakeup_sysfs_add(struct device *dev)
> {
> - return sysfs_merge_group(>kobj, _wakeup_attr_group);
> + int ret = sysfs_merge_group(>kobj, _wakeup_attr_group);
> +
> + if (ret)
> + return ret;
> +
> + return kobject_uevent(>kobj, KOBJ_CHANGE);
> }
>
> void wakeup_sysfs_remove(struct device *dev)
> {
> sysfs_unmerge_group(>kobj, _wakeup_attr_group);
> + kobject_uevent(>kobj, KOBJ_CHANGE);
> }
>
> int pm_qos_sysfs_add_resume_latency(struct device *dev)
> --
> 2.27.0.212.ge8ba1cc988-goog
>
[PATCH v4 0/1] power: Emit change uevent when updating sysfs
Hi linux-pm, ChromeOS has a udev rule to chown the `power/wakeup` attribute so that the power manager can modify it during runtime. (https://source.chromium.org/chromiumos/chromiumos/codesearch/+/master:src/platform2/power_manager/udev/99-powerd-permissions.rules) In our automated tests, we found that the `power/wakeup` attributes weren't being chown-ed for some boards. On investigating, I found that when the drivers probe and call device_set_wakeup_capable, no uevent was being emitted for the newly added power/wakeup attribute. This was manifesting at boot on some boards (Marvell SDIO bluetooth and Broadcom Serial bluetooth drivers) or during usb disconnects during resume (Realtek btusb driver with reset resume quirk). It seems reasonable to me that changes to the attributes of a device should cause a changed uevent so I have added that here. Here's an example of the kernel events after toggling the authorized bit of /sys/bus/usb/devices/1-3/ $ echo 0 > /sys/bus/usb/devices/1-3/authorized KERNEL[27.357994] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill1 (rfkill) KERNEL[27.358049] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[27.358458] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[27.358486] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) KERNEL[27.358529] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) $ echo 1 > /sys/bus/usb/devices/1-3/authorized KERNEL[36.415749] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[36.415798] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[36.417414] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[36.417447] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill) KERNEL[36.417481] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) Series-version 4 update: Tested again on device and verified it's working as expected: KERNEL[52.678174] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill2 (rfkill) KERNEL[52.678211] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[52.678251] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[52.679721] remove /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) KERNEL[52.679772] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[56.022259] change /devices/pci:00/:00:15.0/usb1/1-3 (usb) KERNEL[56.022306] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0 (usb) KERNEL[56.022488] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0 (bluetooth) KERNEL[56.022531] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.0/bluetooth/hci0/rfkill3 (rfkill) KERNEL[56.023392] add /devices/pci:00/:00:15.0/usb1/1-3/1-3:1.1 (usb) Thanks Abhishek Changes in v4: - Fix warning where returning from void and tested on device Changes in v3: - Simplified error handling Changes in v2: - Add newline at end of bt_dev_err Abhishek Pandit-Subedi (1): power: Emit changed uevent on wakeup_sysfs_add/remove drivers/base/power/sysfs.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) -- 2.27.0.212.ge8ba1cc988-goog
