Re: [PATCH v2] Bluetooth: virtio_bt: fix cleanup paths

On Thu, Jun 25, 2026 at 11:06:20AM +0300, Dan Carpenter wrote:
> On Thu, Jun 25, 2026 at 10:01:59AM +0800, Haoxiang Li wrote:
> > virtbt_probe() registers the HCI device before opening the virtio
> > Bluetooth device. If virtbt_open_vdev() fails, the error path frees
> > the HCI device without unregistering it first. The probe error paths
> > also leak the virtio_bluetooth structure after it has been allocated.
> > 
> > Rework the probe error handling into an unwind ladder so each failure
> > path releases the resources acquired earlier. Also close the virtio
> > device before unregistering the HCI device in virtbt_remove(), matching
> > the cleanup order used by the probe failure path.
> > 
> > Fixes: afd2daa26c7a ("Bluetooth: Add support for virtio transport driver")
> > Fixes: dc65b4b0f90a ("Bluetooth: virtio_bt: fix device removal")
> > Cc: [email protected]
> > Signed-off-by: Haoxiang Li <[email protected]>
> > ---
> > Changes in v2:
> >  - Rework virtbt_probe() error paths into an unwind ladder.
> >  - Free vbt on probe failures.
> >  - Reset the virtio device and unregister the HCI device before freeing it
> >    when virtbt_open_vdev() fails.
> >  - Close the virtio device before unregistering the HCI device in remove().
> > 
> >    Thanks Dan for the suggestions. The blog is very helpful.
> > ---
> >  drivers/bluetooth/virtio_bt.c | 23 ++++++++++++++---------
> >  1 file changed, 14 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/bluetooth/virtio_bt.c b/drivers/bluetooth/virtio_bt.c
> > index 140ab55c9fc5..4ca9b76f6410 100644
> > --- a/drivers/bluetooth/virtio_bt.c
> > +++ b/drivers/bluetooth/virtio_bt.c
> > @@ -311,12 +311,12 @@ static int virtbt_probe(struct virtio_device *vdev)
> >  
> >  	err = virtio_find_vqs(vdev, VIRTBT_NUM_VQS, vbt->vqs, vqs_info, NULL);
> >  	if (err)
> > -		return err;
> > +		goto err_free_vbt;
> >  
> >  	hdev = hci_alloc_dev();
> >  	if (!hdev) {
> >  		err = -ENOMEM;
> > -		goto failed;
> > +		goto err_del_vqs;
> >  	}
> >  
> >  	vbt->hdev = hdev;
> > @@ -383,23 +383,28 @@ static int virtbt_probe(struct virtio_device *vdev)
> >  	if (virtio_has_feature(vdev, VIRTIO_BT_F_AOSP_EXT))
> >  		hci_set_aosp_capable(hdev);
> >  
> > -	if (hci_register_dev(hdev) < 0) {
> > -		hci_free_dev(hdev);
> > +	err = hci_register_dev(hdev);
> > +	if (err < 0) {
> >  		err = -EBUSY;
> > -		goto failed;
> > +		goto err_free_hdev;
> >  	}
> >  
> >  	virtio_device_ready(vdev);
> >  	err = virtbt_open_vdev(vbt);
> >  	if (err)
> > -		goto open_failed;
> > +		goto err_reset_vdev;
> >  
> >  	return 0;
> >  
> > -open_failed:
> > +err_reset_vdev:
> > +	virtio_reset_device(vdev);
> 
> I'm not sure that this reset is necessary.  I suspect that it isn't,
> but I don't know for sure.  In a situation like this, I'd probably
> err on the side of only fixing things which I know and leaving the
> rest as a leak or whatever.

if u called virtio_device_ready then yes you must reset.

> Otherwise it looks correct to me.
> 
> regards,
> dan carpenter