| Message ID | 4c8aeca04ed20e2776cadd9bdb57a7a3632d622c.1592404644.git.marcel@holtmann.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Combination of pending patches | expand |
Hi On 17.06.2020 16:39, Marcel Holtmann wrote: > From: Miao-chen Chou <mcchou@chromium.org> > > This calls hci_update_background_scan() when there is any update on the > advertisement monitors. If there is at least one advertisement monitor, > the filtering policy of scan parameters should be 0x00. This also reports > device found mgmt events if there is at least one monitor. > > The following cases were tested with btmgmt advmon-* commands. > (1) add a ADV monitor and observe that the passive scanning is > triggered. > (2) remove the last ADV monitor and observe that the passive scanning is > terminated. > (3) with a LE peripheral paired, repeat (1) and observe the passive > scanning continues. > (4) with a LE peripheral paired, repeat (2) and observe the passive > scanning continues. > (5) with a ADV monitor, suspend/resume the host and observe the passive > scanning continues. > > Signed-off-by: Miao-chen Chou <mcchou@chromium.org> > Signed-off-by: Marcel Holtmann <marcel@holtmann.org> This patch landed recently in linux-next as commit 8208f5a9d435 ("Bluetooth: Update background scan and report device based on advertisement monitors"). It causes a regression, a kernel oops during system suspend/resume cycle on Samsung Exynos5250 based Snow Chromebook: 8<--- cut here --- Unable to handle kernel NULL pointer dereference at virtual address 00000000 pgd = 86c149f5 [00000000] *pgd=00000000 Internal error: Oops: 5 [#1] SMP ARM Modules linked in: cmac cros_ec_sysfs cros_ec_lightbar cros_ec_debugfs cros_ec_chardev cros_ec_keyb cros_ec_dev snd_soc_hdmi_codec cros_ec_i2c cros_ec snd_soc_snow snd_soc_i2s snd_soc_idma snd_soc_s3c_dma exynosdrm analogix_dp exynos_gsc v4l2_mem2mem snd_soc_max98095 snd_soc_core ac97_bus snd_pcm_dmaengine snd_pcm snd_timer nxp_ptn3460 snd soundcore pwm_samsung spi_s3c64xx cyapatp crc_itu_t mwifiex_sdio mwifiex sha256_generic libsha256 sha256_arm btmrvl_sdio btmrvl cfg80211 bluetooth s5p_mfc governor_simpleondemand videobuf2_dma_contig videobuf2_memops videobuf2_v4l2 ecdh_generic ecc videobuf2_common videodev phy_exynos_usb2 ohci_exynos panfrost gpu_sched mc s3c2410_wdt s5p_sss s5p_cec exynos_rng rtc_s3c i2c_arb_gpio_challenge CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.7.0-rc7-02995-g8208f5a9d435 #8564 Hardware name: Samsung Exynos (Flattened Device Tree) Workqueue: events_freezable mmc_rescan PC is at __queue_work+0x6c/0x4e8 LR is at __queue_work+0x68/0x4e8 pc : [<c03619d8>] lr : [<c03619d4>] psr: 60000093 ... Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none Control: 10c5387d Table: 6be2406a DAC: 00000051 Process kworker/1:0 (pid: 16, stack limit = 0xf9898f75) Stack: (0xee117de0 to 0xee118000) ... [<c03619d8>] (__queue_work) from [<c0361e94>] (queue_work_on+0x40/0x4c) [<c0361e94>] (queue_work_on) from [<bf17dd0c>] (hci_adv_monitors_clear+0x74/0x90 [bluetooth]) [<bf17dd0c>] (hci_adv_monitors_clear [bluetooth]) from [<bf17e138>] (hci_unregister_dev+0x158/0x224 [bluetooth]) [<bf17e138>] (hci_unregister_dev [bluetooth]) from [<bf1d1638>] (btmrvl_remove_card+0x58/0x7c [btmrvl]) [<bf1d1638>] (btmrvl_remove_card [btmrvl]) from [<c0d09698>] (sdio_bus_remove+0x30/0x11c) [<c0d09698>] (sdio_bus_remove) from [<c09d534c>] (device_release_driver_internal+0xe8/0x1ac) [<c09d534c>] (device_release_driver_internal) from [<c09d3e18>] (bus_remove_device+0xcc/0xf8) [<c09d3e18>] (bus_remove_device) from [<c09cfa3c>] (device_del+0x15c/0x384) [<c09cfa3c>] (device_del) from [<c0d098b8>] (sdio_remove_func+0x20/0x34) [<c0d098b8>] (sdio_remove_func) from [<c0d075bc>] (mmc_sdio_remove+0x38/0x64) [<c0d075bc>] (mmc_sdio_remove) from [<c0d08858>] (mmc_sdio_detect+0x6c/0xf8) [<c0d08858>] (mmc_sdio_detect) from [<c0cff6f0>] (mmc_rescan+0x1d0/0x42c) [<c0cff6f0>] (mmc_rescan) from [<c0362454>] (process_one_work+0x178/0x4ac) [<c0362454>] (process_one_work) from [<c0362b44>] (worker_thread+0x2c/0x530) [<c0362b44>] (worker_thread) from [<c0368610>] (kthread+0x12c/0x158) [<c0368610>] (kthread) from [<c03001a8>] (ret_from_fork+0x14/0x2c) Exception stack(0xee117fb0 to 0xee117ff8) ... ---[ end trace 0ec00d142e0a49cf ]--- This board uses btmrvl_sdio bluetooth driver if that helps. Reverting this commit in linux-next 20200629 'fixes' the issue. I can do more tests if needed on this hardware, just let me know how can I help and what to do. > --- > include/net/bluetooth/hci_core.h | 1 + > net/bluetooth/hci_core.c | 13 +++++++++++++ > net/bluetooth/hci_event.c | 5 +++-- > net/bluetooth/hci_request.c | 17 ++++++++++++++--- > net/bluetooth/mgmt.c | 5 ++++- > 5 files changed, 35 insertions(+), 6 deletions(-) > ... Best regards
Hi Marek, >> This calls hci_update_background_scan() when there is any update on the >> advertisement monitors. If there is at least one advertisement monitor, >> the filtering policy of scan parameters should be 0x00. This also reports >> device found mgmt events if there is at least one monitor. >> >> The following cases were tested with btmgmt advmon-* commands. >> (1) add a ADV monitor and observe that the passive scanning is >> triggered. >> (2) remove the last ADV monitor and observe that the passive scanning is >> terminated. >> (3) with a LE peripheral paired, repeat (1) and observe the passive >> scanning continues. >> (4) with a LE peripheral paired, repeat (2) and observe the passive >> scanning continues. >> (5) with a ADV monitor, suspend/resume the host and observe the passive >> scanning continues. >> >> Signed-off-by: Miao-chen Chou <mcchou@chromium.org> >> Signed-off-by: Marcel Holtmann <marcel@holtmann.org> > > This patch landed recently in linux-next as commit 8208f5a9d435 > ("Bluetooth: Update background scan and report device based on > advertisement monitors"). > > It causes a regression, a kernel oops during system suspend/resume cycle > on Samsung Exynos5250 based Snow Chromebook: > > 8<--- cut here --- > Unable to handle kernel NULL pointer dereference at virtual address 00000000 > pgd = 86c149f5 > [00000000] *pgd=00000000 > Internal error: Oops: 5 [#1] SMP ARM > Modules linked in: cmac cros_ec_sysfs cros_ec_lightbar cros_ec_debugfs > cros_ec_chardev cros_ec_keyb cros_ec_dev snd_soc_hdmi_codec cros_ec_i2c > cros_ec snd_soc_snow snd_soc_i2s snd_soc_idma snd_soc_s3c_dma exynosdrm > analogix_dp exynos_gsc v4l2_mem2mem snd_soc_max98095 snd_soc_core > ac97_bus snd_pcm_dmaengine snd_pcm snd_timer nxp_ptn3460 snd soundcore > pwm_samsung spi_s3c64xx cyapatp crc_itu_t mwifiex_sdio mwifiex > sha256_generic libsha256 sha256_arm btmrvl_sdio btmrvl cfg80211 > bluetooth s5p_mfc governor_simpleondemand videobuf2_dma_contig > videobuf2_memops videobuf2_v4l2 ecdh_generic ecc videobuf2_common > videodev phy_exynos_usb2 ohci_exynos panfrost gpu_sched mc s3c2410_wdt > s5p_sss s5p_cec exynos_rng rtc_s3c i2c_arb_gpio_challenge > CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted > 5.7.0-rc7-02995-g8208f5a9d435 #8564 > Hardware name: Samsung Exynos (Flattened Device Tree) > Workqueue: events_freezable mmc_rescan > PC is at __queue_work+0x6c/0x4e8 > LR is at __queue_work+0x68/0x4e8 > pc : [<c03619d8>] lr : [<c03619d4>] psr: 60000093 > ... > Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none > Control: 10c5387d Table: 6be2406a DAC: 00000051 > Process kworker/1:0 (pid: 16, stack limit = 0xf9898f75) > Stack: (0xee117de0 to 0xee118000) > ... > [<c03619d8>] (__queue_work) from [<c0361e94>] (queue_work_on+0x40/0x4c) > [<c0361e94>] (queue_work_on) from [<bf17dd0c>] > (hci_adv_monitors_clear+0x74/0x90 [bluetooth]) > [<bf17dd0c>] (hci_adv_monitors_clear [bluetooth]) from [<bf17e138>] > (hci_unregister_dev+0x158/0x224 [bluetooth]) > [<bf17e138>] (hci_unregister_dev [bluetooth]) from [<bf1d1638>] > (btmrvl_remove_card+0x58/0x7c [btmrvl]) > [<bf1d1638>] (btmrvl_remove_card [btmrvl]) from [<c0d09698>] > (sdio_bus_remove+0x30/0x11c) > [<c0d09698>] (sdio_bus_remove) from [<c09d534c>] > (device_release_driver_internal+0xe8/0x1ac) > [<c09d534c>] (device_release_driver_internal) from [<c09d3e18>] > (bus_remove_device+0xcc/0xf8) > [<c09d3e18>] (bus_remove_device) from [<c09cfa3c>] (device_del+0x15c/0x384) > [<c09cfa3c>] (device_del) from [<c0d098b8>] (sdio_remove_func+0x20/0x34) > [<c0d098b8>] (sdio_remove_func) from [<c0d075bc>] > (mmc_sdio_remove+0x38/0x64) > [<c0d075bc>] (mmc_sdio_remove) from [<c0d08858>] (mmc_sdio_detect+0x6c/0xf8) > [<c0d08858>] (mmc_sdio_detect) from [<c0cff6f0>] (mmc_rescan+0x1d0/0x42c) > [<c0cff6f0>] (mmc_rescan) from [<c0362454>] (process_one_work+0x178/0x4ac) > [<c0362454>] (process_one_work) from [<c0362b44>] (worker_thread+0x2c/0x530) > [<c0362b44>] (worker_thread) from [<c0368610>] (kthread+0x12c/0x158) > [<c0368610>] (kthread) from [<c03001a8>] (ret_from_fork+0x14/0x2c) > Exception stack(0xee117fb0 to 0xee117ff8) > ... > ---[ end trace 0ec00d142e0a49cf ]--- > > This board uses btmrvl_sdio bluetooth driver if that helps. Reverting > this commit in linux-next 20200629 'fixes' the issue. > > I can do more tests if needed on this hardware, just let me know how can > I help and what to do. can you check latest bluetooth-next. I think that we applied a fix for it. Regards Marcel
Hi Marcel, On 30.06.2020 08:49, Marcel Holtmann wrote: >>> This calls hci_update_background_scan() when there is any update on the >>> advertisement monitors. If there is at least one advertisement monitor, >>> the filtering policy of scan parameters should be 0x00. This also reports >>> device found mgmt events if there is at least one monitor. >>> >>> The following cases were tested with btmgmt advmon-* commands. >>> (1) add a ADV monitor and observe that the passive scanning is >>> triggered. >>> (2) remove the last ADV monitor and observe that the passive scanning is >>> terminated. >>> (3) with a LE peripheral paired, repeat (1) and observe the passive >>> scanning continues. >>> (4) with a LE peripheral paired, repeat (2) and observe the passive >>> scanning continues. >>> (5) with a ADV monitor, suspend/resume the host and observe the passive >>> scanning continues. >>> >>> Signed-off-by: Miao-chen Chou <mcchou@chromium.org> >>> Signed-off-by: Marcel Holtmann <marcel@holtmann.org> >> This patch landed recently in linux-next as commit 8208f5a9d435 >> ("Bluetooth: Update background scan and report device based on >> advertisement monitors"). >> >> It causes a regression, a kernel oops during system suspend/resume cycle >> on Samsung Exynos5250 based Snow Chromebook: >> >> 8<--- cut here --- >> Unable to handle kernel NULL pointer dereference at virtual address 00000000 >> pgd = 86c149f5 >> [00000000] *pgd=00000000 >> Internal error: Oops: 5 [#1] SMP ARM >> Modules linked in: cmac cros_ec_sysfs cros_ec_lightbar cros_ec_debugfs >> cros_ec_chardev cros_ec_keyb cros_ec_dev snd_soc_hdmi_codec cros_ec_i2c >> cros_ec snd_soc_snow snd_soc_i2s snd_soc_idma snd_soc_s3c_dma exynosdrm >> analogix_dp exynos_gsc v4l2_mem2mem snd_soc_max98095 snd_soc_core >> ac97_bus snd_pcm_dmaengine snd_pcm snd_timer nxp_ptn3460 snd soundcore >> pwm_samsung spi_s3c64xx cyapatp crc_itu_t mwifiex_sdio mwifiex >> sha256_generic libsha256 sha256_arm btmrvl_sdio btmrvl cfg80211 >> bluetooth s5p_mfc governor_simpleondemand videobuf2_dma_contig >> videobuf2_memops videobuf2_v4l2 ecdh_generic ecc videobuf2_common >> videodev phy_exynos_usb2 ohci_exynos panfrost gpu_sched mc s3c2410_wdt >> s5p_sss s5p_cec exynos_rng rtc_s3c i2c_arb_gpio_challenge >> CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted >> 5.7.0-rc7-02995-g8208f5a9d435 #8564 >> Hardware name: Samsung Exynos (Flattened Device Tree) >> Workqueue: events_freezable mmc_rescan >> PC is at __queue_work+0x6c/0x4e8 >> LR is at __queue_work+0x68/0x4e8 >> pc : [<c03619d8>] lr : [<c03619d4>] psr: 60000093 >> ... >> Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none >> Control: 10c5387d Table: 6be2406a DAC: 00000051 >> Process kworker/1:0 (pid: 16, stack limit = 0xf9898f75) >> Stack: (0xee117de0 to 0xee118000) >> ... >> [<c03619d8>] (__queue_work) from [<c0361e94>] (queue_work_on+0x40/0x4c) >> [<c0361e94>] (queue_work_on) from [<bf17dd0c>] >> (hci_adv_monitors_clear+0x74/0x90 [bluetooth]) >> [<bf17dd0c>] (hci_adv_monitors_clear [bluetooth]) from [<bf17e138>] >> (hci_unregister_dev+0x158/0x224 [bluetooth]) >> [<bf17e138>] (hci_unregister_dev [bluetooth]) from [<bf1d1638>] >> (btmrvl_remove_card+0x58/0x7c [btmrvl]) >> [<bf1d1638>] (btmrvl_remove_card [btmrvl]) from [<c0d09698>] >> (sdio_bus_remove+0x30/0x11c) >> [<c0d09698>] (sdio_bus_remove) from [<c09d534c>] >> (device_release_driver_internal+0xe8/0x1ac) >> [<c09d534c>] (device_release_driver_internal) from [<c09d3e18>] >> (bus_remove_device+0xcc/0xf8) >> [<c09d3e18>] (bus_remove_device) from [<c09cfa3c>] (device_del+0x15c/0x384) >> [<c09cfa3c>] (device_del) from [<c0d098b8>] (sdio_remove_func+0x20/0x34) >> [<c0d098b8>] (sdio_remove_func) from [<c0d075bc>] >> (mmc_sdio_remove+0x38/0x64) >> [<c0d075bc>] (mmc_sdio_remove) from [<c0d08858>] (mmc_sdio_detect+0x6c/0xf8) >> [<c0d08858>] (mmc_sdio_detect) from [<c0cff6f0>] (mmc_rescan+0x1d0/0x42c) >> [<c0cff6f0>] (mmc_rescan) from [<c0362454>] (process_one_work+0x178/0x4ac) >> [<c0362454>] (process_one_work) from [<c0362b44>] (worker_thread+0x2c/0x530) >> [<c0362b44>] (worker_thread) from [<c0368610>] (kthread+0x12c/0x158) >> [<c0368610>] (kthread) from [<c03001a8>] (ret_from_fork+0x14/0x2c) >> Exception stack(0xee117fb0 to 0xee117ff8) >> ... >> ---[ end trace 0ec00d142e0a49cf ]--- >> >> This board uses btmrvl_sdio bluetooth driver if that helps. Reverting >> this commit in linux-next 20200629 'fixes' the issue. >> >> I can do more tests if needed on this hardware, just let me know how can >> I help and what to do. > can you check latest bluetooth-next. I think that we applied a fix for it. Indeed. Commit f2790f025a0e ("Bluetooth: fix kernel null pointer dereference error on suspend") fixed the issue. Best regards
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h index c54f9295892e..524057598ffd 100644 --- a/include/net/bluetooth/hci_core.h +++ b/include/net/bluetooth/hci_core.h @@ -1284,6 +1284,7 @@ void hci_adv_monitors_clear(struct hci_dev *hdev); void hci_free_adv_monitor(struct adv_monitor *monitor); int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor); int hci_remove_adv_monitor(struct hci_dev *hdev, u16 handle); +bool hci_is_adv_monitoring(struct hci_dev *hdev); void hci_event_packet(struct hci_dev *hdev, struct sk_buff *skb); diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c index 59132b3e2cde..7959b851cc63 100644 --- a/net/bluetooth/hci_core.c +++ b/net/bluetooth/hci_core.c @@ -3005,6 +3005,8 @@ void hci_adv_monitors_clear(struct hci_dev *hdev) hci_free_adv_monitor(monitor); idr_destroy(&hdev->adv_monitors_idr); + + hci_update_background_scan(hdev); } void hci_free_adv_monitor(struct adv_monitor *monitor) @@ -3038,6 +3040,9 @@ int hci_add_adv_monitor(struct hci_dev *hdev, struct adv_monitor *monitor) hdev->adv_monitors_cnt++; monitor->handle = handle; + + hci_update_background_scan(hdev); + return 0; } @@ -3069,9 +3074,17 @@ int hci_remove_adv_monitor(struct hci_dev *hdev, u16 handle) idr_for_each(&hdev->adv_monitors_idr, &free_adv_monitor, hdev); } + hci_update_background_scan(hdev); + return 0; } +/* This function requires the caller holds hdev->lock */ +bool hci_is_adv_monitoring(struct hci_dev *hdev) +{ + return !idr_is_empty(&hdev->adv_monitors_idr); +} + struct bdaddr_list *hci_bdaddr_list_lookup(struct list_head *bdaddr_list, bdaddr_t *bdaddr, u8 type) { diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c index 8981954ff4c4..e08d4dd9a24e 100644 --- a/net/bluetooth/hci_event.c +++ b/net/bluetooth/hci_event.c @@ -5447,14 +5447,15 @@ static void process_adv_report(struct hci_dev *hdev, u8 type, bdaddr_t *bdaddr, /* Passive scanning shouldn't trigger any device found events, * except for devices marked as CONN_REPORT for which we do send - * device found events. + * device found events, or advertisement monitoring requested. */ if (hdev->le_scan_type == LE_SCAN_PASSIVE) { if (type == LE_ADV_DIRECT_IND) return; if (!hci_pend_le_action_lookup(&hdev->pend_le_reports, - bdaddr, bdaddr_type)) + bdaddr, bdaddr_type) && + idr_is_empty(&hdev->adv_monitors_idr)) return; if (type == LE_ADV_NONCONN_IND || type == LE_ADV_SCAN_IND) diff --git a/net/bluetooth/hci_request.c b/net/bluetooth/hci_request.c index eee9c007a5fb..29decd7e8051 100644 --- a/net/bluetooth/hci_request.c +++ b/net/bluetooth/hci_request.c @@ -413,11 +413,15 @@ static void __hci_update_background_scan(struct hci_request *req) */ hci_discovery_filter_clear(hdev); + BT_DBG("%s ADV monitoring is %s", hdev->name, + hci_is_adv_monitoring(hdev) ? "on" : "off"); + if (list_empty(&hdev->pend_le_conns) && - list_empty(&hdev->pend_le_reports)) { + list_empty(&hdev->pend_le_reports) && + !hci_is_adv_monitoring(hdev)) { /* If there is no pending LE connections or devices - * to be scanned for, we should stop the background - * scanning. + * to be scanned for or no ADV monitors, we should stop the + * background scanning. */ /* If controller is not scanning we are done. */ @@ -794,6 +798,13 @@ static u8 update_white_list(struct hci_request *req) return 0x00; } + /* Once the controller offloading of advertisement monitor is in place, + * the if condition should include the support of MSFT extension + * support. + */ + if (!idr_is_empty(&hdev->adv_monitors_idr)) + return 0x00; + /* Select filter policy to use white list */ return 0x01; } diff --git a/net/bluetooth/mgmt.c b/net/bluetooth/mgmt.c index b194da4de2d7..ec66160a673c 100644 --- a/net/bluetooth/mgmt.c +++ b/net/bluetooth/mgmt.c @@ -8575,8 +8575,11 @@ void mgmt_device_found(struct hci_dev *hdev, bdaddr_t *bdaddr, u8 link_type, if (!hci_discovery_active(hdev)) { if (link_type == ACL_LINK) return; - if (link_type == LE_LINK && list_empty(&hdev->pend_le_reports)) + if (link_type == LE_LINK && + list_empty(&hdev->pend_le_reports) && + !hci_is_adv_monitoring(hdev)) { return; + } } if (hdev->discovery.result_filtering) {