On Wed, Nov 06, 2024 at 05:14:24PM +0530, Prasad Pandit wrote:
On Wed, 6 Nov 2024 at 16:05, Stefano Garzarella <sgarz...@redhat.com> wrote:
>+fail_iotlb:
>+ hdev->vhost_ops->vhost_set_iotlb_callback(hdev, false);
> fail_start:
>+ hdev->vhost_ops->vhost_dev_start(hdev, false);
This should go before the fail_start label, since it should not be
called when vhost_dev_start() fails.
* I see, okay.
Also we need to check if that callback is defined.
* That check is already done while reaching the 'goto fail_iotlb;'
statement, no? OR maybe we only check for:
hdev->vhost_ops->vhost_dev_start() function?
For vhost_set_iotlb_callback() that is true because for now we go to
that label only if the callback is defined, but this is not the case for
hdev->vhost_ops->vhost_dev_start().
Anyway if in the future we add a new step that need to go on that label
we may forgot to remember that, so since it's not a hot path, I'd add
both checks as we do in vhost_dev_stop().
>* Looking at the vhost_vdpa_dev_start(), when it is called with
>'started=false' parameter, it calls the vdpa_suspend, vdpa_stop etc.
>functions. ie. probably other ->vhost_dev_start() and
>->vhost_set_iotlb_callback() functions need to take appropriate action
>when called with 'started/enabled=false' parameter.
We already call them in vhost_dev_stop(), so I guess we are fine.
* I meant vhost device functions like vhost_user_dev_start() and
vhost_user_set_iotlb_callback() need to take action when called with a
'false' parameter.
IIUC in vhost_dev_stop() we already call both of them with a 'false'
parameter, so that functions should be already prepared or am I missing
something?
Thanks,
Stefano
