| Message ID | 20220811094542.268519-1-mst@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v3] Bluetooth: virtio_bt: fix device removal | expand |
Hi Michael, On Thu, Aug 11, 2022 at 2:46 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > 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() 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. > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Message-Id: <20220811080943.198245-1-mst@redhat.com> > --- > > 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(-) > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > index 67c21263f9e0..f6d699fed139 100644 > --- a/drivers/bluetooth/virtio_bt.c > +++ b/drivers/bluetooth/virtio_bt.c > @@ -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 >
On Mon, Aug 15, 2022 at 11:07:05AM -0700, Luiz Augusto von Dentz wrote: > Hi Michael, > > On Thu, Aug 11, 2022 at 2:46 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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() > > 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. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Message-Id: <20220811080943.198245-1-mst@redhat.com> > > --- > > > > 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(-) > > > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > > index 67c21263f9e0..f6d699fed139 100644 > > --- a/drivers/bluetooth/virtio_bt.c > > +++ b/drivers/bluetooth/virtio_bt.c > > @@ -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 > > > > > -- > Luiz Augusto von Dentz
On Mon, Aug 15, 2022 at 11:07:05AM -0700, Luiz Augusto von Dentz wrote: > Hi Michael, > > On Thu, Aug 11, 2022 at 2:46 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > 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() > > 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? > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Message-Id: <20220811080943.198245-1-mst@redhat.com> > > --- > > > > 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(-) > > > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > > index 67c21263f9e0..f6d699fed139 100644 > > --- a/drivers/bluetooth/virtio_bt.c > > +++ b/drivers/bluetooth/virtio_bt.c > > @@ -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 > > > > > -- > Luiz Augusto von Dentz
Hi Michael, On Fri, Oct 7, 2022 at 6:01 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Aug 15, 2022 at 11:07:05AM -0700, Luiz Augusto von Dentz wrote: > > Hi Michael, > > > > On Thu, Aug 11, 2022 at 2:46 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > 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() > > > > 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. > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > Message-Id: <20220811080943.198245-1-mst@redhat.com> > > > --- > > > > > > 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(-) > > > > > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > > > index 67c21263f9e0..f6d699fed139 100644 > > > --- a/drivers/bluetooth/virtio_bt.c > > > +++ b/drivers/bluetooth/virtio_bt.c > > > @@ -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 > > > > > > > > > -- > > Luiz Augusto von Dentz >
On Fri, Oct 07, 2022 at 12:33:03PM -0700, Luiz Augusto von Dentz wrote: > Hi Michael, > > On Fri, Oct 7, 2022 at 6:01 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Aug 15, 2022 at 11:07:05AM -0700, Luiz Augusto von Dentz wrote: > > > Hi Michael, > > > > > > On Thu, Aug 11, 2022 at 2:46 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > 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() > > > > > > 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? > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > > > Reported-by: Hulk Robot <hulkci@huawei.com> > > > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > > > Message-Id: <20220811080943.198245-1-mst@redhat.com> > > > > --- > > > > > > > > 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(-) > > > > > > > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > > > > index 67c21263f9e0..f6d699fed139 100644 > > > > --- a/drivers/bluetooth/virtio_bt.c > > > > +++ b/drivers/bluetooth/virtio_bt.c > > > > @@ -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 > > > > > > > > > > > > > -- > > > Luiz Augusto von Dentz > > > > > -- > Luiz Augusto von Dentz
Tested, works fine in our projects. Tested-by: Igor Skalkin <Igor.Skalkin@opensynergy.com> On 10/10/22 19:14, Michael S. Tsirkin wrote: > 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() > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > Reported-by: Hulk Robot <hulkci@huawei.com> > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > Message-Id: <20220811080943.198245-1-mst@redhat.com> > --- > > resending due to v3 having been dropped > 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(-) > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > index 67c21263f9e0..f6d699fed139 100644 > --- a/drivers/bluetooth/virtio_bt.c > +++ b/drivers/bluetooth/virtio_bt.c > @@ -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; 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/>
Hi Michael, On Tue, Oct 11, 2022 at 5:18 AM Igor Skalkin <igor.skalkin@opensynergy.com> wrote: > > Tested, works fine in our projects. > > Tested-by: Igor Skalkin <Igor.Skalkin@opensynergy.com> > > On 10/10/22 19:14, Michael S. Tsirkin wrote: > > 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() > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Reported-by: Hulk Robot <hulkci@huawei.com> > > Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com> > > Message-Id: <20220811080943.198245-1-mst@redhat.com> Ive pushed this one. > > --- > > > > resending due to v3 having been dropped > > 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(-) > > > > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c > > index 67c21263f9e0..f6d699fed139 100644 > > --- a/drivers/bluetooth/virtio_bt.c > > +++ b/drivers/bluetooth/virtio_bt.c > > @@ -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; > > 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/>
diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c index 67c21263f9e0..f6d699fed139 100644 --- a/drivers/bluetooth/virtio_bt.c +++ b/drivers/bluetooth/virtio_bt.c @@ -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;
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() Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reported-by: Hulk Robot <hulkci@huawei.com> Signed-off-by: Yang Yingliang <yangyingliang@huawei.com> Signed-off-by: Michael S. Tsirkin <mst@redhat.com> Message-Id: <20220811080943.198245-1-mst@redhat.com> --- 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(-)