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

On Thu, Nov 25, 2021 at 10:58:56PM +0100, Marcel Holtmann wrote:

Hi Michael,

quoted

quoted

quoted

quoted

quoted

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.

so the virtbt_{open,close} as NOP is not really what a driver is suppose
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, I expect that no
resources/queues are in use.

Maybe I misunderstand the virtio spec in that regard, but I would like
to keep this fundamental concept of a Bluetooth driver. It does work
with all other transports like USB, SDIO, UART etc.

quoted

The cost here is a single skb wasted on an unused bt device - which
seems modest.

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.

quoted

NB: with this fix in place driver still suffers from a race condition if
an interrupt triggers while device is being reset. Work on a fix for
that issue is in progress.

In the virtbt_close() callback we should deactivate all interrupts.

If you want to do that then device has to be reset on close,
and fully reinitialized on open.
Can you work on a patch like that?
Given I don't have the device such a rework is probably more
than I can undertake.

so you mean move virtio_find_vqs() into virtbt_open() and del_vqs() into
virtbt_close()?

And reset before del_vqs.

quoted

Or is there are way to set up the queues without starting them?

However I am failing to understand your initial concern, we do reset()
before del_vqs() in virtbt_remove(). Should we be doing something different
in virtbt_close() other than virtqueue_detach_unused_buf(). Why would I
keep buffers attached if they are not used.

They are not used at that point but until device is reset can use them.
Also, if you then proceed to open without a reset, and kick,
device will start by processing the original buffers, crashing
or corrupting memory.

so the only valid usage is like this:

	vdev->config->reset(vdev);

	while ((.. = virtqueue_detach_unused_buf(vq))) {
	}

	vdev->config->del_vqs(vdev);

If I make virtbt_{open,close} a NOP, then I keep adding an extra SKB to inbuf on
every power cycle (ifup/ifdown).

So make sure you don't :)

How does netdev handle this?

Regards

Marcel

For net, open adds buffers to vq. close does not free them up -
they stay in the vq until device is removed.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization