Re: [PATCH v3] Bluetooth: virtio_bt: fix device removal

["Michael S. Tsirkin" <mst@redhat.com>]

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