Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vdpa/virtio: Move doorbell relay disable to dev close #54

Merged
merged 1 commit into from
Jan 8, 2024

Conversation

kailiangz1
Copy link
Collaborator

Doorbell relay is enabled when dev config, set vring state should not disble doorbell, only in dev close or rpc remove device, we need disable relay

RM:3725995

@kailiangz1 kailiangz1 requested a review from yajwu January 4, 2024 03:59
@@ -474,6 +474,25 @@ virtio_vdpa_dev_notifier_work(void *arg)
return ret;
}

static void virtio_vdpa_doorbell_relay_disable(struct virtio_vdpa_priv *priv)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DPDK coding style, should be:
static void
virtio_vdpa...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -474,6 +474,25 @@ virtio_vdpa_dev_notifier_work(void *arg)
return ret;
}

static void virtio_vdpa_doorbell_relay_disable(struct virtio_vdpa_priv *priv)
{

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this line can be deleted

uint16_t nr_virtqs = priv->hw_nr_virtqs;
int vq_idx, ret;

for(vq_idx = 0; vq_idx < nr_virtqs; vq_idx++) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need a space after 'for'

ret = virtio_vdpa_virtq_doorbell_relay_disable(priv, vq_idx);
if (ret) {
DRV_LOG(ERR, "%s doorbell relay disable failed ret:%d",
priv->vdev->device->name, ret);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to add vq_idx to log

DRV_LOG(ERR, "%s doorbell relay disable failed ret:%d",
priv->vdev->device->name, ret);
}
priv->vrings[vq_idx]->notifier_state = VIRTIO_VDPA_NOTIFIER_RELAY_DISABLED;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the state seems to be set in virtio_vdpa_virtq_doorbell_relay_disable already. so this is not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -1630,6 +1642,7 @@ virtio_vdpa_dev_do_remove(struct rte_pci_device *pci_dev, struct virtio_vdpa_pri

if (priv->configured)
virtio_vdpa_dev_close(priv->vid);
virtio_vdpa_doorbell_relay_disable(priv);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? if configured, it will be done in close. if not configured, it seems relay will not be enabled. Is there any case that this function call is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just prevent if config fail, still check doorbell relay setting

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

priv->configured flag is set in the end of config, just after relay_enable. relay enable fail will not return in current code. so if config fail, it's failing before enable. right?

Doorbell relay is enabled when dev config, set vring state
should not disble doorbell, only in dev close or rpc remove
device, we need disable relay

RM:3725995

Signed-off-by: Kailiang <kailiangz@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants