| Message ID | 20220603103356.26564-1-gautam.dawar@amd.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | vdpa: allow vdpa dev_del management operation to return failure | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
> From: Gautam Dawar <gautam.dawar@amd.com> > Sent: Friday, June 3, 2022 1:34 PM > To: netdev@vger.kernel.org > Cc: linux-net-drivers@amd.com; hanand@amd.com; Gautam Dawar <gautam.dawar@amd.com>; Michael S. Tsirkin > <mst@redhat.com>; Jason Wang <jasowang@redhat.com>; Zhu Lingshan <lingshan.zhu@intel.com>; Xie Yongji > <xieyongji@bytedance.com>; Eli Cohen <elic@nvidia.com>; Parav Pandit <parav@nvidia.com>; Si-Wei Liu <si-wei.liu@oracle.com>; > Stefano Garzarella <sgarzare@redhat.com>; Wan Jiabing <wanjiabing@vivo.com>; Dan Carpenter <dan.carpenter@oracle.com>; > virtualization@lists.linux-foundation.org; linux-kernel@vger.kernel.org > Subject: [PATCH] vdpa: allow vdpa dev_del management operation to return failure > > Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows > returning a value to depict the operation status but the return type > of dev_del() callback is void. So, any error while deleting the vdpa > device in the vdpa parent driver can't be returned to the management > layer. > This patch changes the return type of dev_del() callback to int to > allow returning an error code in case of failure. > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > --- This patch is not changing anything except for planning on some future device returning error upon delete. Until that happens I don't see much reason for making this change. > drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- > drivers/vdpa/vdpa.c | 11 ++++++++--- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > include/linux/vdpa.h | 5 +++-- > 7 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 4366320fb68d..6a967935478b 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -800,13 +800,14 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > +static int ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > { > struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; > > ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev); > _vdpa_unregister_device(dev); > ifcvf_mgmt_dev->adapter = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops ifcvf_vdpa_mgmt_dev_ops = { > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index e0de44000d92..b06204c2f3e8 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -2775,7 +2775,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > return err; > } > > -static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) > +static int mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) > { > struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev); > struct mlx5_vdpa_dev *mvdev = to_mvdev(dev); > @@ -2788,6 +2788,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device * > destroy_workqueue(wq); > _vdpa_unregister_device(dev); > mgtdev->ndev = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops mdev_ops = { > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 2b75c00b1005..65dc8bf2f37f 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -363,10 +363,11 @@ static int vdpa_match_remove(struct device *dev, void *data) > { > struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev); > struct vdpa_mgmt_dev *mdev = vdev->mdev; > + int err = 0; > > if (mdev == data) > - mdev->ops->dev_del(mdev, vdev); > - return 0; > + err = mdev->ops->dev_del(mdev, vdev); > + return err; > } > > void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev) > @@ -673,7 +674,11 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i > goto mdev_err; > } > mdev = vdev->mdev; > - mdev->ops->dev_del(mdev, vdev); > + err = mdev->ops->dev_del(mdev, vdev); > + if (err) { > + NL_SET_ERR_MSG_MOD(info->extack, "ops->dev_del failed"); > + goto dev_err; > + } > mdev_err: > put_device(dev); > dev_err: > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > index 42d401d43911..443d4b94268f 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > @@ -280,12 +280,13 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) > { > struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_blk_mgmtdev_ops = { > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index d5324f6fd8c7..9e5a5ad34e65 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -167,12 +167,13 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) > { > struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index f85d1a08ed87..33ff45e70ff7 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -1540,9 +1540,10 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return 0; > } > > -static void vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > +static int vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > { > _vdpa_unregister_device(dev); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = { > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > index 8943a209202e..e547c9dfdfce 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -443,12 +443,13 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status); > * @mdev: parent device to use for device removal > * @dev: vdpa device to remove > * Driver need to remove the specified device by calling > - * _vdpa_unregister_device(). > + * _vdpa_unregister_device(). Driver must return 0 > + * on success or appropriate error code in failure case. > */ > struct vdpa_mgmtdev_ops { > int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name, > const struct vdpa_dev_set_config *config); > - void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); > + int (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); > }; > > /** > -- > 2.30.1
Hi Gautam: On Fri, Jun 3, 2022 at 6:34 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows > returning a value to depict the operation status but the return type > of dev_del() callback is void. So, any error while deleting the vdpa > device in the vdpa parent driver can't be returned to the management > layer. I wonder under which cognition we can hit an error in dev_del()? Thanks > This patch changes the return type of dev_del() callback to int to > allow returning an error code in case of failure. > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- > drivers/vdpa/vdpa.c | 11 ++++++++--- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > include/linux/vdpa.h | 5 +++-- > 7 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c > index 4366320fb68d..6a967935478b 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -800,13 +800,14 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > +static int ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > { > struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; > > ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev); > _vdpa_unregister_device(dev); > ifcvf_mgmt_dev->adapter = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops ifcvf_vdpa_mgmt_dev_ops = { > diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index e0de44000d92..b06204c2f3e8 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -2775,7 +2775,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > return err; > } > > -static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) > +static int mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) > { > struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev); > struct mlx5_vdpa_dev *mvdev = to_mvdev(dev); > @@ -2788,6 +2788,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device * > destroy_workqueue(wq); > _vdpa_unregister_device(dev); > mgtdev->ndev = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops mdev_ops = { > diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c > index 2b75c00b1005..65dc8bf2f37f 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -363,10 +363,11 @@ static int vdpa_match_remove(struct device *dev, void *data) > { > struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev); > struct vdpa_mgmt_dev *mdev = vdev->mdev; > + int err = 0; > > if (mdev == data) > - mdev->ops->dev_del(mdev, vdev); > - return 0; > + err = mdev->ops->dev_del(mdev, vdev); > + return err; > } > > void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev) > @@ -673,7 +674,11 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i > goto mdev_err; > } > mdev = vdev->mdev; > - mdev->ops->dev_del(mdev, vdev); > + err = mdev->ops->dev_del(mdev, vdev); > + if (err) { > + NL_SET_ERR_MSG_MOD(info->extack, "ops->dev_del failed"); > + goto dev_err; > + } > mdev_err: > put_device(dev); > dev_err: > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > index 42d401d43911..443d4b94268f 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > @@ -280,12 +280,13 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) > { > struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_blk_mgmtdev_ops = { > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index d5324f6fd8c7..9e5a5ad34e65 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -167,12 +167,13 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) > { > struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { > diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c > index f85d1a08ed87..33ff45e70ff7 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -1540,9 +1540,10 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return 0; > } > > -static void vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > +static int vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) > { > _vdpa_unregister_device(dev); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = { > diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h > index 8943a209202e..e547c9dfdfce 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -443,12 +443,13 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status); > * @mdev: parent device to use for device removal > * @dev: vdpa device to remove > * Driver need to remove the specified device by calling > - * _vdpa_unregister_device(). > + * _vdpa_unregister_device(). Driver must return 0 > + * on success or appropriate error code in failure case. > */ > struct vdpa_mgmtdev_ops { > int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name, > const struct vdpa_dev_set_config *config); > - void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); > + int (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); > }; > > /** > -- > 2.30.1 >
[AMD Official Use Only - General] Hi Gautam: [GD>>] Hi Jason, On Fri, Jun 3, 2022 at 6:34 PM Gautam Dawar <gautam.dawar@amd.com> wrote: > > Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows > returning a value to depict the operation status but the return type > of dev_del() callback is void. So, any error while deleting the vdpa > device in the vdpa parent driver can't be returned to the management > layer. I wonder under which cognition we can hit an error in dev_del()? [GD>>] In the AMD-Xilinx vDPA driver, on receiving vdpa device deletion request, I try to identify if the vdpa device is in use by any virtio-net driver (through any vdpa bus driver) by looking at the vdpa device status value. In case the vdpa device status is >= VIRTIO_CONFIG_S_DRIVER, -EBUSY is returned. This is to avoid side-effects as noted in https://bugzilla.kernel.org/show_bug.cgi?id=213179 caused by deleting the vdpa device when it is being used. > Thanks > This patch changes the return type of dev_del() callback to int to > allow returning an error code in case of failure. > > Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> > --- > drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- > drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- > drivers/vdpa/vdpa.c | 11 ++++++++--- > drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- > drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- > drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- > include/linux/vdpa.h | 5 +++-- > 7 files changed, 21 insertions(+), 10 deletions(-) > > diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c > b/drivers/vdpa/ifcvf/ifcvf_main.c index 4366320fb68d..6a967935478b > 100644 > --- a/drivers/vdpa/ifcvf/ifcvf_main.c > +++ b/drivers/vdpa/ifcvf/ifcvf_main.c > @@ -800,13 +800,14 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct > vdpa_device *dev) > +static int ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct > +vdpa_device *dev) > { > struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; > > ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev); > _vdpa_unregister_device(dev); > ifcvf_mgmt_dev->adapter = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops ifcvf_vdpa_mgmt_dev_ops = { diff > --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c > b/drivers/vdpa/mlx5/net/mlx5_vnet.c > index e0de44000d92..b06204c2f3e8 100644 > --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c > +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c > @@ -2775,7 +2775,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, > return err; > } > > -static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct > vdpa_device *dev) > +static int mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct > +vdpa_device *dev) > { > struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev); > struct mlx5_vdpa_dev *mvdev = to_mvdev(dev); @@ -2788,6 > +2788,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device * > destroy_workqueue(wq); > _vdpa_unregister_device(dev); > mgtdev->ndev = NULL; > + return 0; > } > > static const struct vdpa_mgmtdev_ops mdev_ops = { diff --git > a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index > 2b75c00b1005..65dc8bf2f37f 100644 > --- a/drivers/vdpa/vdpa.c > +++ b/drivers/vdpa/vdpa.c > @@ -363,10 +363,11 @@ static int vdpa_match_remove(struct device *dev, > void *data) { > struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev); > struct vdpa_mgmt_dev *mdev = vdev->mdev; > + int err = 0; > > if (mdev == data) > - mdev->ops->dev_del(mdev, vdev); > - return 0; > + err = mdev->ops->dev_del(mdev, vdev); > + return err; > } > > void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev) @@ -673,7 > +674,11 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i > goto mdev_err; > } > mdev = vdev->mdev; > - mdev->ops->dev_del(mdev, vdev); > + err = mdev->ops->dev_del(mdev, vdev); > + if (err) { > + NL_SET_ERR_MSG_MOD(info->extack, "ops->dev_del failed"); > + goto dev_err; > + } > mdev_err: > put_device(dev); > dev_err: > diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > index 42d401d43911..443d4b94268f 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c > @@ -280,12 +280,13 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) { > struct vdpasim *simdev = container_of(dev, struct vdpasim, > vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_blk_mgmtdev_ops = { diff > --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > index d5324f6fd8c7..9e5a5ad34e65 100644 > --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c > @@ -167,12 +167,13 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return ret; > } > > -static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > +static int vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, > struct vdpa_device *dev) { > struct vdpasim *simdev = container_of(dev, struct vdpasim, > vdpa); > > _vdpa_unregister_device(&simdev->vdpa); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { diff > --git a/drivers/vdpa/vdpa_user/vduse_dev.c > b/drivers/vdpa/vdpa_user/vduse_dev.c > index f85d1a08ed87..33ff45e70ff7 100644 > --- a/drivers/vdpa/vdpa_user/vduse_dev.c > +++ b/drivers/vdpa/vdpa_user/vduse_dev.c > @@ -1540,9 +1540,10 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, > return 0; > } > > -static void vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct > vdpa_device *dev) > +static int vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct > +vdpa_device *dev) > { > _vdpa_unregister_device(dev); > + return 0; > } > > static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = { diff > --git a/include/linux/vdpa.h b/include/linux/vdpa.h index > 8943a209202e..e547c9dfdfce 100644 > --- a/include/linux/vdpa.h > +++ b/include/linux/vdpa.h > @@ -443,12 +443,13 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status); > * @mdev: parent device to use for device removal > * @dev: vdpa device to remove > * Driver need to remove the specified device by calling > - * _vdpa_unregister_device(). > + * _vdpa_unregister_device(). Driver must return 0 > + * on success or appropriate error code in failure case. > */ > struct vdpa_mgmtdev_ops { > int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name, > const struct vdpa_dev_set_config *config); > - void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); > + int (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device > + *dev); > }; > > /** > -- > 2.30.1 >
> From: Dawar, Gautam <gautam.dawar@amd.com> > Sent: Wednesday, June 8, 2022 6:30 AM > To: Jason Wang <jasowang@redhat.com> > Cc: netdev <netdev@vger.kernel.org>; linux-net-drivers (AMD-Xilinx) <linux- > net-drivers@amd.com>; Anand, Harpreet <harpreet.anand@amd.com>; > Michael S. Tsirkin <mst@redhat.com>; Zhu Lingshan > <lingshan.zhu@intel.com>; Xie Yongji <xieyongji@bytedance.com>; Eli > Cohen <elic@nvidia.com>; Parav Pandit <parav@nvidia.com>; Si-Wei Liu <si- > wei.liu@oracle.com>; Stefano Garzarella <sgarzare@redhat.com>; Wan > Jiabing <wanjiabing@vivo.com>; Dan Carpenter > <dan.carpenter@oracle.com>; virtualization <virtualization@lists.linux- > foundation.org>; linux-kernel <linux-kernel@vger.kernel.org> > Subject: RE: [PATCH] vdpa: allow vdpa dev_del management operation to > return failure > > [AMD Official Use Only - General] > > Hi Gautam: > [GD>>] Hi Jason, > > On Fri, Jun 3, 2022 at 6:34 PM Gautam Dawar <gautam.dawar@amd.com> > wrote: > > > > Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows > > returning a value to depict the operation status but the return type > > of dev_del() callback is void. So, any error while deleting the vdpa > > device in the vdpa parent driver can't be returned to the management > > layer. > > I wonder under which cognition we can hit an error in dev_del()? > [GD>>] In the AMD-Xilinx vDPA driver, on receiving vdpa device deletion > request, I try to identify if the vdpa device is in use by any virtio-net driver > (through any vdpa bus driver) by looking at the vdpa device status value. In > case the vdpa device status is >= VIRTIO_CONFIG_S_DRIVER, -EBUSY is > returned. > This is to avoid side-effects as noted in > https://bugzilla.kernel.org/show_bug.cgi?id=213179 caused by deleting the > vdpa device when it is being used. > > User should be able to delete the device anytime. Upper layers who are unable to perform teardown sequence should be fixed.
On Wed, Jun 8, 2022 at 6:43 PM Parav Pandit <parav@nvidia.com> wrote: > > > > From: Dawar, Gautam <gautam.dawar@amd.com> > > Sent: Wednesday, June 8, 2022 6:30 AM > > To: Jason Wang <jasowang@redhat.com> > > Cc: netdev <netdev@vger.kernel.org>; linux-net-drivers (AMD-Xilinx) <linux- > > net-drivers@amd.com>; Anand, Harpreet <harpreet.anand@amd.com>; > > Michael S. Tsirkin <mst@redhat.com>; Zhu Lingshan > > <lingshan.zhu@intel.com>; Xie Yongji <xieyongji@bytedance.com>; Eli > > Cohen <elic@nvidia.com>; Parav Pandit <parav@nvidia.com>; Si-Wei Liu <si- > > wei.liu@oracle.com>; Stefano Garzarella <sgarzare@redhat.com>; Wan > > Jiabing <wanjiabing@vivo.com>; Dan Carpenter > > <dan.carpenter@oracle.com>; virtualization <virtualization@lists.linux- > > foundation.org>; linux-kernel <linux-kernel@vger.kernel.org> > > Subject: RE: [PATCH] vdpa: allow vdpa dev_del management operation to > > return failure > > > > [AMD Official Use Only - General] > > > > Hi Gautam: > > [GD>>] Hi Jason, > > > > On Fri, Jun 3, 2022 at 6:34 PM Gautam Dawar <gautam.dawar@amd.com> > > wrote: > > > > > > Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows > > > returning a value to depict the operation status but the return type > > > of dev_del() callback is void. So, any error while deleting the vdpa > > > device in the vdpa parent driver can't be returned to the management > > > layer. > > > > I wonder under which cognition we can hit an error in dev_del()? > > [GD>>] In the AMD-Xilinx vDPA driver, on receiving vdpa device deletion > > request, I try to identify if the vdpa device is in use by any virtio-net driver > > (through any vdpa bus driver) by looking at the vdpa device status value. In > > case the vdpa device status is >= VIRTIO_CONFIG_S_DRIVER, -EBUSY is > > returned. > > This is to avoid side-effects as noted in > > https://bugzilla.kernel.org/show_bug.cgi?id=213179 caused by deleting the > > vdpa device when it is being used. > > > > User should be able to delete the device anytime. It requires a poll event to user space and then Qemu can release the vhost-vDPA device. This is how VFIO works. We probably need to implement something like this. But notice that, at the worst case, usersapce may not respond to this event, so there's nothing more kenrel can do execpt for waiting. We need to consider something different. I used to have an idea to make vhost-vdpa couple with vDPA loosely with SRCU/RCU. We might consider implementing that. > Upper layers who are unable to perform teardown sequence should be fixed. I think we probably don't need to bother with failing the dev_del(). We can consider to fix/workaround the waiting first. Thanks
> From: Jason Wang <jasowang@redhat.com> > Sent: Thursday, June 9, 2022 3:19 AM [..] > > > This is to avoid side-effects as noted in > > > https://bugzilla.kernel.org/show_bug.cgi?id=213179 caused by > > > deleting the vdpa device when it is being used. > > > > > > User should be able to delete the device anytime. > > It requires a poll event to user space and then Qemu can release the vhost- > vDPA device. This is how VFIO works. We probably need to implement > something like this. > Yes, I remember hang with either vfio or vfio-mdev. > But notice that, at the worst case, usersapce may not respond to this event, > so there's nothing more kenrel can do execpt for waiting. > > We need to consider something different. I used to have an idea to make > vhost-vdpa couple with vDPA loosely with SRCU/RCU. We might consider > implementing that. > Right. It needs a different solution. As you described, vhost-vdpa cannot rely on the user space involvement in freeing. Rdma uverbs does that for device removal cases to detach the user space ioctl() context with low level device. > > Upper layers who are unable to perform teardown sequence should be > fixed. > > I think we probably don't need to bother with failing the dev_del(). > We can consider to fix/workaround the waiting first. > Lets fix it as you hinted with S/RCU. We don’t need to add UAPI for the fix. UAPI anyway doesn’t work, when driver/system level event occurs such as device fatal error, sriov disablement, sf removal and more. Vhost-vdpa should be able to detach it from the underlying device.
diff --git a/drivers/vdpa/ifcvf/ifcvf_main.c b/drivers/vdpa/ifcvf/ifcvf_main.c index 4366320fb68d..6a967935478b 100644 --- a/drivers/vdpa/ifcvf/ifcvf_main.c +++ b/drivers/vdpa/ifcvf/ifcvf_main.c @@ -800,13 +800,14 @@ static int ifcvf_vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, return ret; } -static void ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) +static int ifcvf_vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) { struct ifcvf_vdpa_mgmt_dev *ifcvf_mgmt_dev; ifcvf_mgmt_dev = container_of(mdev, struct ifcvf_vdpa_mgmt_dev, mdev); _vdpa_unregister_device(dev); ifcvf_mgmt_dev->adapter = NULL; + return 0; } static const struct vdpa_mgmtdev_ops ifcvf_vdpa_mgmt_dev_ops = { diff --git a/drivers/vdpa/mlx5/net/mlx5_vnet.c b/drivers/vdpa/mlx5/net/mlx5_vnet.c index e0de44000d92..b06204c2f3e8 100644 --- a/drivers/vdpa/mlx5/net/mlx5_vnet.c +++ b/drivers/vdpa/mlx5/net/mlx5_vnet.c @@ -2775,7 +2775,7 @@ static int mlx5_vdpa_dev_add(struct vdpa_mgmt_dev *v_mdev, const char *name, return err; } -static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) +static int mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device *dev) { struct mlx5_vdpa_mgmtdev *mgtdev = container_of(v_mdev, struct mlx5_vdpa_mgmtdev, mgtdev); struct mlx5_vdpa_dev *mvdev = to_mvdev(dev); @@ -2788,6 +2788,7 @@ static void mlx5_vdpa_dev_del(struct vdpa_mgmt_dev *v_mdev, struct vdpa_device * destroy_workqueue(wq); _vdpa_unregister_device(dev); mgtdev->ndev = NULL; + return 0; } static const struct vdpa_mgmtdev_ops mdev_ops = { diff --git a/drivers/vdpa/vdpa.c b/drivers/vdpa/vdpa.c index 2b75c00b1005..65dc8bf2f37f 100644 --- a/drivers/vdpa/vdpa.c +++ b/drivers/vdpa/vdpa.c @@ -363,10 +363,11 @@ static int vdpa_match_remove(struct device *dev, void *data) { struct vdpa_device *vdev = container_of(dev, struct vdpa_device, dev); struct vdpa_mgmt_dev *mdev = vdev->mdev; + int err = 0; if (mdev == data) - mdev->ops->dev_del(mdev, vdev); - return 0; + err = mdev->ops->dev_del(mdev, vdev); + return err; } void vdpa_mgmtdev_unregister(struct vdpa_mgmt_dev *mdev) @@ -673,7 +674,11 @@ static int vdpa_nl_cmd_dev_del_set_doit(struct sk_buff *skb, struct genl_info *i goto mdev_err; } mdev = vdev->mdev; - mdev->ops->dev_del(mdev, vdev); + err = mdev->ops->dev_del(mdev, vdev); + if (err) { + NL_SET_ERR_MSG_MOD(info->extack, "ops->dev_del failed"); + goto dev_err; + } mdev_err: put_device(dev); dev_err: diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c index 42d401d43911..443d4b94268f 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_blk.c @@ -280,12 +280,13 @@ static int vdpasim_blk_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, return ret; } -static void vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, +static int vdpasim_blk_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) { struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); _vdpa_unregister_device(&simdev->vdpa); + return 0; } static const struct vdpa_mgmtdev_ops vdpasim_blk_mgmtdev_ops = { diff --git a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c index d5324f6fd8c7..9e5a5ad34e65 100644 --- a/drivers/vdpa/vdpa_sim/vdpa_sim_net.c +++ b/drivers/vdpa/vdpa_sim/vdpa_sim_net.c @@ -167,12 +167,13 @@ static int vdpasim_net_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, return ret; } -static void vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, +static int vdpasim_net_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) { struct vdpasim *simdev = container_of(dev, struct vdpasim, vdpa); _vdpa_unregister_device(&simdev->vdpa); + return 0; } static const struct vdpa_mgmtdev_ops vdpasim_net_mgmtdev_ops = { diff --git a/drivers/vdpa/vdpa_user/vduse_dev.c b/drivers/vdpa/vdpa_user/vduse_dev.c index f85d1a08ed87..33ff45e70ff7 100644 --- a/drivers/vdpa/vdpa_user/vduse_dev.c +++ b/drivers/vdpa/vdpa_user/vduse_dev.c @@ -1540,9 +1540,10 @@ static int vdpa_dev_add(struct vdpa_mgmt_dev *mdev, const char *name, return 0; } -static void vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) +static int vdpa_dev_del(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev) { _vdpa_unregister_device(dev); + return 0; } static const struct vdpa_mgmtdev_ops vdpa_dev_mgmtdev_ops = { diff --git a/include/linux/vdpa.h b/include/linux/vdpa.h index 8943a209202e..e547c9dfdfce 100644 --- a/include/linux/vdpa.h +++ b/include/linux/vdpa.h @@ -443,12 +443,13 @@ void vdpa_set_status(struct vdpa_device *vdev, u8 status); * @mdev: parent device to use for device removal * @dev: vdpa device to remove * Driver need to remove the specified device by calling - * _vdpa_unregister_device(). + * _vdpa_unregister_device(). Driver must return 0 + * on success or appropriate error code in failure case. */ struct vdpa_mgmtdev_ops { int (*dev_add)(struct vdpa_mgmt_dev *mdev, const char *name, const struct vdpa_dev_set_config *config); - void (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); + int (*dev_del)(struct vdpa_mgmt_dev *mdev, struct vdpa_device *dev); }; /**
Currently, the vdpa_nl_cmd_dev_del_set_doit() implementation allows returning a value to depict the operation status but the return type of dev_del() callback is void. So, any error while deleting the vdpa device in the vdpa parent driver can't be returned to the management layer. This patch changes the return type of dev_del() callback to int to allow returning an error code in case of failure. Signed-off-by: Gautam Dawar <gautam.dawar@amd.com> --- drivers/vdpa/ifcvf/ifcvf_main.c | 3 ++- drivers/vdpa/mlx5/net/mlx5_vnet.c | 3 ++- drivers/vdpa/vdpa.c | 11 ++++++++--- drivers/vdpa/vdpa_sim/vdpa_sim_blk.c | 3 ++- drivers/vdpa/vdpa_sim/vdpa_sim_net.c | 3 ++- drivers/vdpa/vdpa_user/vduse_dev.c | 3 ++- include/linux/vdpa.h | 5 +++-- 7 files changed, 21 insertions(+), 10 deletions(-)