Bluetooth: virtio_bt: fix device removal
PATCH v3 1/1 Bluetooth: virtio_bt: fix device removal
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the virtbt_close() callback we should deactivate all interrupts. To be fixed.
squashed fixup: bluetooth: virtio_bt: fix an error code in probe()
Message-Id: <[email protected]>
---
changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handling
drivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)
drivers/bluetooth/virtio_bt.c +17 -2
@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;
@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);
@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;
@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ;
Signature · 1 line
MST
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=666822
---Test result---
Test Summary:
CheckPatch PASS 1.28 seconds GitLint FAIL 0.72 seconds SubjectPrefix PASS 0.59 seconds BuildKernel PASS 38.37 seconds BuildKernel32 PASS 34.59 seconds Incremental Build with patchesPASS 48.74 seconds TestRunner: Setup PASS 566.46 seconds TestRunner: l2cap-tester PASS 19.52 seconds TestRunner: bnep-tester PASS 7.72 seconds TestRunner: mgmt-tester PASS 116.56 seconds TestRunner: rfcomm-tester PASS 11.25 seconds TestRunner: sco-tester PASS 11.05 seconds TestRunner: smp-tester PASS 11.04 seconds TestRunner: userchan-tester PASS 7.86 seconds
Details
##############################
Test: GitLint - FAIL - 0.72 seconds
Run gitlint with rule in .gitlint
[v3] Bluetooth: virtio_bt: fix device removal
36: B3 Line contains hard tab characters (\t): " tkeaked commit log to make lines shorter"
38: B3 Line contains hard tab characters (\t): " fixed error handling"
---
Regards,
Linux Bluetooth
Quoted 26 lines · On Thu, Aug 11, 2022 at 03:09:43AM -0700, [email protected] wrote:
This is automated email and please do not reply to this email!
Dear submitter,
Thank you for submitting the patches to the linux bluetooth mailing list. This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=666822---Test result---
Test Summary:
CheckPatch PASS 1.28 seconds GitLint FAIL 0.72 seconds SubjectPrefix PASS 0.59 seconds BuildKernel PASS 38.37 seconds BuildKernel32 PASS 34.59 seconds Incremental Build with patchesPASS 48.74 seconds TestRunner: Setup PASS 566.46 seconds TestRunner: l2cap-tester PASS 19.52 seconds TestRunner: bnep-tester PASS 7.72 seconds TestRunner: mgmt-tester PASS 116.56 seconds TestRunner: rfcomm-tester PASS 11.25 seconds TestRunner: sco-tester PASS 11.05 seconds TestRunner: smp-tester PASS 11.04 seconds TestRunner: userchan-tester PASS 7.86 seconds
Quoted 7 lines
Details
##############################
Test: GitLint - FAIL - 0.72 seconds
Run gitlint with rule in .gitlint
[v3] Bluetooth:virtio_bt: fix device removal
36: B3 Line contains hard tab characters (\t): " tkeaked commit log to make lines shorter"
38: B3 Line contains hard tab characters (\t): " fixed error handling"
False positive: this is text after ---, it really should not be linted at all.
Quoted 7 lines
---
Regards,
Linux Bluetooth
Luiz Augusto von Dentz
maintainerHi Michael,
Quoted 17 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
Quoted 8 lines
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()
Shouldn't the line above be a Fixes tag with the commit hash you are attempting to fix, other than that I'd be fine to apply these changes.
Quoted 72 lines
Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +17
-3@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ; --MST
Signature · 1 line
Luiz Augusto von Dentz
Quoted 27 lines · Luiz Augusto von Dentz
Hi Michael,
Quoted 24 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.
Quoted 4 lines
Quoted 1 line
squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()Shouldn't the line above be a Fixes tag with the commit hash you are attempting to fix, other than that I'd be fine to apply these changes.
No - what I tried to say here is that I posted the original patch and then Dan and Yang posted fixups, I squashed them in but wanted to keep contribution.
Quoted 76 lines
Quoted 72 lines
Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +17
-3@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ; --MST
Signature · 1 line
Luiz Augusto von Dentz
Quoted 27 lines · Luiz Augusto von Dentz
Hi Michael,
Quoted 24 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.
Quoted 4 lines
Quoted 1 line
squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()Shouldn't the line above be a Fixes tag with the commit hash you are attempting to fix, other than that I'd be fine to apply these changes.
ping? what's going on?
Quoted 76 lines
Quoted 72 lines
Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +17
-3@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ; --MST
Signature · 1 line
Luiz Augusto von Dentz
Luiz Augusto von Dentz
maintainerHi Michael,
Quoted 29 lines · Michael S. Tsirkin
Quoted 27 lines · Luiz Augusto von Dentz
Hi Michael,
Quoted 24 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()Shouldn't the line above be a Fixes tag with the commit hash you are attempting to fix, other than that I'd be fine to apply these changes.
ping? what's going on?
Please resend, our patchwork only retain the patches for 30 days.
Quoted 77 lines
Quoted 76 lines
Quoted 70 lines
Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +17
-3@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ; --Quoted 2 lines
MST
--
Luiz Augusto von Dentz
Signature · 1 line
Luiz Augusto von Dentz
Quoted 34 lines · Luiz Augusto von Dentz
Hi Michael,
Quoted 31 lines · Michael S. Tsirkin
Quoted 29 lines · Luiz Augusto von Dentz
Hi Michael,
Quoted 25 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
Shouldn't the line above be a Fixes tag with the commit hash you are attempting to fix, other than that I'd be fine to apply these changes.
ping? what's going on?
Please resend, our patchwork only retain the patches for 30 days.
I did but what is going on here? Why was the patch dropped?
Quoted 81 lines
Quoted 77 lines
Quoted 76 lines
Quoted 70 lines
Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +17
-3@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ; --Quoted 2 lines
MST
--
Luiz Augusto von DentzSignature · 1 line
Luiz Augusto von Dentz
Tested, works fine in our projects.
Quoted 88 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
---resending due to v3 having been dropped
changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +16
-2@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;
drivers/bluetooth/virtio_bt.c static void virtbt_remove(struct virtio_device *vdev) · line 388
Quoted 7 lines
drivers/bluetooth/virtio_bt.c +1
-0@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ;
Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>
Luiz Augusto von Dentz
maintainerHi Michael,
Quoted 36 lines · Igor Skalkin
Tested, works fine in our projects.
Tested-by Igor Skalkin <[email protected]>Quoted 30 lines · Michael S. Tsirkin
Device removal is clearly out of virtio spec: it attempts to remove unused buffers from a VQ before invoking device reset. To fix, make open/close NOPs and do all cleanup/setup in probe/remove.
NB: This is a hacky way to handle this - virtbt_{open,close} as NOP is not really what a driver is supposed to be doing. These are transport enable/disable callbacks from the BT core towards the driver. It maps to a device being enabled/disabled by something like bluetoothd for example. So if disabled, users expect that no resources/queues are in use. It does work with all other transports like USB, SDIO, UART etc. There should be no buffer used if the device is powered off. We also don’t have any USB URBs in-flight if the transport is not active.
The way to implement a proper fix would be using vq reset if supported, or even using a full device reset.
The cost of the hack is a single skb wasted on an unused bt device.
NB2: with this fix in place driver still suffers from a race condition if an interrupt triggers while device is being reset. To fix, in the
virtbt_close()callback we should deactivate all interrupts. To be fixed.squashed fixup: bluetooth:
virtio_bt: fix an error code inprobe()Signed-off-by Dan Carpenter <[email protected]>Reported-by Hulk Robot <[email protected]>Signed-off-by Yang Yingliang <[email protected]>Signed-off-by Michael S. Tsirkin <[email protected]>Message-Id: <[email protected]>
Ive pushed this one.
Quoted 67 lines
Quoted 58 lines
---
resending due to v3 having been dropped
changes from v2:
tkeaked commit log to make lines shorter
changes from v1:
fixed error handlingdrivers/bluetooth/virtio_bt.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)drivers/bluetooth/virtio_bt.c +16
-2@@ -50,8 +50,11 @@ static int virtbt_add_inbuf(struct virtio_bluetooth *vbt)static int virtbt_open (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} +static int virtbt_open_vdev (struct virtio_bluetooth *vbt) +{if (virtbt_add_inbuf (vbt) <0 )return -EIO ;@@ -61,7 +64,11 @@ static int virtbt_open(struct hci_dev *hdev)static int virtbt_close (struct hci_dev *hdev) { -struct virtio_bluetooth *vbt =hci_get_drvdata (hdev); +return 0 ; +} + +static int virtbt_close_vdev (struct virtio_bluetooth *vbt) +{int i;cancel_work_sync (&vbt->rx);@@ -354,8 +361,15 @@ static int virtbt_probe(struct virtio_device *vdev) goto failed; } +virtio_device_ready (vdev); + err =virtbt_open_vdev (vbt); +if (err) + goto open_failed; +return 0 ; +open_failed: +hci_free_dev (hdev); failed: vdev->config->del_vqs (vdev);return err;drivers/bluetooth/virtio_bt.c static void virtbt_remove(struct virtio_device *vdev) · line 388
Quoted 7 lines
drivers/bluetooth/virtio_bt.c +1
-0@@ -368,6 +382,7 @@ static void virtbt_remove(struct virtio_device *vdev)hci_unregister_dev (hdev);virtio_reset_device (vdev); +virtbt_close_vdev (vbt);hci_free_dev (hdev); vbt->hdev =NULL ;Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>
Signature · 1 line
Luiz Augusto von Dentz