Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220811094542.268519-1-mst@redhat.com>
In-Reply-To: <20220811094542.268519-1-mst@redhat.com>
From:   Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date:   Mon, 15 Aug 2022 11:07:05 -0700
Message-ID: <CABBYNZLeszHHi2TVcQsOtPWs1u=s8gjOVAhL+Q=O-ThgpxvpOw@mail.gmail.com>
Subject: Re: [PATCH v3] Bluetooth: virtio_bt: fix device removal
To:     "Michael S. Tsirkin" <mst@redhat.com>
Cc:     Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Dan Carpenter <dan.carpenter@oracle.com>,
        Hulk Robot <hulkci@huawei.com>,
        Yang Yingliang <yangyingliang@huawei.com>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        "linux-bluetooth@vger.kernel.org" <linux-bluetooth@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

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=E2=80=99t 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 *v=
bt)
>
>  static int virtbt_open(struct hci_dev *hdev)
>  {
> -       struct virtio_bluetooth *vbt =3D 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 =3D 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 =3D 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 =3D NULL;
> --
> MST
>


--=20
Luiz Augusto von Dentz