* [PATCH v1] raw/ifpga: remove virtual device unplug operation
@ 2023-03-16 20:44 Wei Huang
2023-03-20 6:51 ` Xu, Rosen
2023-03-20 16:06 ` Thomas Monjalon
0 siblings, 2 replies; 14+ messages in thread
From: Wei Huang @ 2023-03-16 20:44 UTC (permalink / raw)
To: dev, thomas, david.marchand
Cc: stable, rosen.xu, tianfei.zhang, qi.z.zhang, Wei Huang
VDEV bus has implemented cleanup() function to perform cleanup for
devices on the bus during eal_cleanup(), so there is no need for
ifpga driver to record virtual devices and unplug them.
Signed-off-by: Wei Huang <wei.huang@intel.com>
---
drivers/raw/ifpga/ifpga_rawdev.c | 99 ++++------------------------------------
drivers/raw/ifpga/ifpga_rawdev.h | 2 -
2 files changed, 9 insertions(+), 92 deletions(-)
diff --git a/drivers/raw/ifpga/ifpga_rawdev.c b/drivers/raw/ifpga/ifpga_rawdev.c
index 1020adc..8e6e70f 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.c
+++ b/drivers/raw/ifpga/ifpga_rawdev.c
@@ -140,8 +140,6 @@ struct ifpga_rawdev *
for (i = 0; i < IFPGA_MAX_IRQ; i++)
dev->intr_handle[i] = NULL;
dev->poll_enabled = 0;
- for (i = 0; i < IFPGA_MAX_VDEV; i++)
- dev->vdev_name[i] = NULL;
return dev;
}
@@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
struct ifpga_rawdev *ifpga_rdev = NULL;
struct opae_adapter *adapter;
struct opae_manager *mgr;
- char *vdev_name = NULL;
- int i, ret = 0;
+ int ret = 0;
if (dev) {
ifpga_rdev = ifpga_rawdev_get(dev);
if (ifpga_rdev) {
- for (i = 0; i < IFPGA_MAX_VDEV; i++) {
- vdev_name = ifpga_rdev->vdev_name[i];
- if (vdev_name)
- rte_vdev_uninit(vdev_name);
- }
ifpga_monitor_stop_func(ifpga_rdev);
ifpga_rdev->rawdev = NULL;
}
@@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const char *key __rte_unused,
static int
ifpga_cfg_probe(struct rte_vdev_device *vdev)
{
- struct rte_rawdev *rawdev = NULL;
- struct ifpga_rawdev *ifpga_dev;
struct ifpga_vdev_args args;
char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
- const char *vdev_name = NULL;
- int i, n, ret = 0;
-
- vdev_name = rte_vdev_device_name(vdev);
- if (!vdev_name)
- return -EINVAL;
+ int ret = 0;
- IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s", vdev_name);
+ IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
+ rte_vdev_device_name(vdev));
ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
if (ret)
return ret;
memset(dev_name, 0, sizeof(dev_name));
- snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s", args.bdf);
- rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
- if (!rawdev)
- return -ENODEV;
- ifpga_dev = ifpga_rawdev_get(rawdev);
- if (!ifpga_dev)
- return -ENODEV;
-
- for (i = 0; i < IFPGA_MAX_VDEV; i++) {
- if (ifpga_dev->vdev_name[i] == NULL) {
- n = strlen(vdev_name) + 1;
- ifpga_dev->vdev_name[i] = rte_malloc(NULL, n, 0);
- if (ifpga_dev->vdev_name[i] == NULL)
- return -ENOMEM;
- strlcpy(ifpga_dev->vdev_name[i], vdev_name, n);
- break;
- }
- }
-
- if (i >= IFPGA_MAX_VDEV) {
- IFPGA_RAWDEV_PMD_ERR("Can't create more virtual device!");
- return -ENOENT;
- }
-
snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
args.port, args.bdf);
- ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
- dev_name, vdev->device.devargs->args);
- if (ret) {
- rte_free(ifpga_dev->vdev_name[i]);
- ifpga_dev->vdev_name[i] = NULL;
- }
-
- return ret;
+ return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME), dev_name,
+ vdev->device.devargs->args);
}
static int
ifpga_cfg_remove(struct rte_vdev_device *vdev)
{
- struct rte_rawdev *rawdev = NULL;
- struct ifpga_rawdev *ifpga_dev;
- struct ifpga_vdev_args args;
- char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
- const char *vdev_name = NULL;
- char *tmp_vdev = NULL;
- int i, ret = 0;
-
- vdev_name = rte_vdev_device_name(vdev);
- if (!vdev_name)
- return -EINVAL;
+ IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
+ rte_vdev_device_name(vdev));
- IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s", vdev_name);
-
- ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
- if (ret)
- return ret;
-
- memset(dev_name, 0, sizeof(dev_name));
- snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s", args.bdf);
- rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
- if (!rawdev)
- return -ENODEV;
- ifpga_dev = ifpga_rawdev_get(rawdev);
- if (!ifpga_dev)
- return -ENODEV;
-
- snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
- args.port, args.bdf);
- ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME), dev_name);
-
- for (i = 0; i < IFPGA_MAX_VDEV; i++) {
- tmp_vdev = ifpga_dev->vdev_name[i];
- if (tmp_vdev && !strcmp(tmp_vdev, vdev_name)) {
- free(tmp_vdev);
- ifpga_dev->vdev_name[i] = NULL;
- break;
- }
- }
-
- return ret;
+ return 0;
}
static struct rte_vdev_driver ifpga_cfg_driver = {
diff --git a/drivers/raw/ifpga/ifpga_rawdev.h b/drivers/raw/ifpga/ifpga_rawdev.h
index 0fb66cb..1c128c7 100644
--- a/drivers/raw/ifpga/ifpga_rawdev.h
+++ b/drivers/raw/ifpga/ifpga_rawdev.h
@@ -65,8 +65,6 @@ struct ifpga_rawdev {
void *intr_handle[IFPGA_MAX_IRQ];
/* enable monitor thread poll device's sensors or not */
int poll_enabled;
- /* name of virtual devices created on raw device */
- char *vdev_name[IFPGA_MAX_VDEV];
};
struct ifpga_vdev_args {
--
1.8.3.1
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-16 20:44 [PATCH v1] raw/ifpga: remove virtual device unplug operation Wei Huang
@ 2023-03-20 6:51 ` Xu, Rosen
2023-03-20 12:58 ` Zhang, Qi Z
2023-03-20 16:06 ` Thomas Monjalon
1 sibling, 1 reply; 14+ messages in thread
From: Xu, Rosen @ 2023-03-20 6:51 UTC (permalink / raw)
To: Huang, Wei, dev, thomas, david.marchand
Cc: stable, Zhang, Tianfei, Zhang, Qi Z
Hi,
> -----Original Message-----
> From: Huang, Wei <wei.huang@intel.com>
> Sent: Friday, March 17, 2023 4:45 AM
> To: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> Cc: stable@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> <tianfei.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Huang, Wei
> <wei.huang@intel.com>
> Subject: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> VDEV bus has implemented cleanup() function to perform cleanup for
> devices on the bus during eal_cleanup(), so there is no need for ifpga driver
> to record virtual devices and unplug them.
>
> Signed-off-by: Wei Huang <wei.huang@intel.com>
> ---
> drivers/raw/ifpga/ifpga_rawdev.c | 99 ++++------------------------------------
> drivers/raw/ifpga/ifpga_rawdev.h | 2 -
> 2 files changed, 9 insertions(+), 92 deletions(-)
>
> diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> b/drivers/raw/ifpga/ifpga_rawdev.c
> index 1020adc..8e6e70f 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.c
> +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> @@ -140,8 +140,6 @@ struct ifpga_rawdev *
> for (i = 0; i < IFPGA_MAX_IRQ; i++)
> dev->intr_handle[i] = NULL;
> dev->poll_enabled = 0;
> - for (i = 0; i < IFPGA_MAX_VDEV; i++)
> - dev->vdev_name[i] = NULL;
>
> return dev;
> }
> @@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
> struct ifpga_rawdev *ifpga_rdev = NULL;
> struct opae_adapter *adapter;
> struct opae_manager *mgr;
> - char *vdev_name = NULL;
> - int i, ret = 0;
> + int ret = 0;
>
> if (dev) {
> ifpga_rdev = ifpga_rawdev_get(dev);
> if (ifpga_rdev) {
> - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> - vdev_name = ifpga_rdev->vdev_name[i];
> - if (vdev_name)
> - rte_vdev_uninit(vdev_name);
> - }
> ifpga_monitor_stop_func(ifpga_rdev);
> ifpga_rdev->rawdev = NULL;
> }
> @@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const
> char *key __rte_unused, static int ifpga_cfg_probe(struct rte_vdev_device
> *vdev) {
> - struct rte_rawdev *rawdev = NULL;
> - struct ifpga_rawdev *ifpga_dev;
> struct ifpga_vdev_args args;
> char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> - const char *vdev_name = NULL;
> - int i, n, ret = 0;
> -
> - vdev_name = rte_vdev_device_name(vdev);
> - if (!vdev_name)
> - return -EINVAL;
> + int ret = 0;
>
> - IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> vdev_name);
> + IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> + rte_vdev_device_name(vdev));
>
> ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> if (ret)
> return ret;
>
> memset(dev_name, 0, sizeof(dev_name));
> - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> args.bdf);
> - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> - if (!rawdev)
> - return -ENODEV;
> - ifpga_dev = ifpga_rawdev_get(rawdev);
> - if (!ifpga_dev)
> - return -ENODEV;
> -
> - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> - if (ifpga_dev->vdev_name[i] == NULL) {
> - n = strlen(vdev_name) + 1;
> - ifpga_dev->vdev_name[i] = rte_malloc(NULL, n, 0);
> - if (ifpga_dev->vdev_name[i] == NULL)
> - return -ENOMEM;
> - strlcpy(ifpga_dev->vdev_name[i], vdev_name, n);
> - break;
> - }
> - }
> -
> - if (i >= IFPGA_MAX_VDEV) {
> - IFPGA_RAWDEV_PMD_ERR("Can't create more virtual
> device!");
> - return -ENOENT;
> - }
> -
> snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> args.port, args.bdf);
> - ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> - dev_name, vdev->device.devargs->args);
> - if (ret) {
> - rte_free(ifpga_dev->vdev_name[i]);
> - ifpga_dev->vdev_name[i] = NULL;
> - }
> -
> - return ret;
> + return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> dev_name,
> + vdev->device.devargs->args);
> }
>
> static int
> ifpga_cfg_remove(struct rte_vdev_device *vdev) {
> - struct rte_rawdev *rawdev = NULL;
> - struct ifpga_rawdev *ifpga_dev;
> - struct ifpga_vdev_args args;
> - char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> - const char *vdev_name = NULL;
> - char *tmp_vdev = NULL;
> - int i, ret = 0;
> -
> - vdev_name = rte_vdev_device_name(vdev);
> - if (!vdev_name)
> - return -EINVAL;
> + IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> + rte_vdev_device_name(vdev));
>
> - IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> vdev_name);
> -
> - ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> - if (ret)
> - return ret;
> -
> - memset(dev_name, 0, sizeof(dev_name));
> - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> args.bdf);
> - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> - if (!rawdev)
> - return -ENODEV;
> - ifpga_dev = ifpga_rawdev_get(rawdev);
> - if (!ifpga_dev)
> - return -ENODEV;
> -
> - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> - args.port, args.bdf);
> - ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
> dev_name);
> -
> - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> - tmp_vdev = ifpga_dev->vdev_name[i];
> - if (tmp_vdev && !strcmp(tmp_vdev, vdev_name)) {
> - free(tmp_vdev);
> - ifpga_dev->vdev_name[i] = NULL;
> - break;
> - }
> - }
> -
> - return ret;
> + return 0;
> }
>
> static struct rte_vdev_driver ifpga_cfg_driver = { diff --git
> a/drivers/raw/ifpga/ifpga_rawdev.h b/drivers/raw/ifpga/ifpga_rawdev.h
> index 0fb66cb..1c128c7 100644
> --- a/drivers/raw/ifpga/ifpga_rawdev.h
> +++ b/drivers/raw/ifpga/ifpga_rawdev.h
> @@ -65,8 +65,6 @@ struct ifpga_rawdev {
> void *intr_handle[IFPGA_MAX_IRQ];
> /* enable monitor thread poll device's sensors or not */
> int poll_enabled;
> - /* name of virtual devices created on raw device */
> - char *vdev_name[IFPGA_MAX_VDEV];
> };
>
> struct ifpga_vdev_args {
> --
> 1.8.3.1
Acked-by: Rosen Xu <rosen.xu@intel.com>
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-20 6:51 ` Xu, Rosen
@ 2023-03-20 12:58 ` Zhang, Qi Z
2023-03-20 13:00 ` Zhang, Qi Z
0 siblings, 1 reply; 14+ messages in thread
From: Zhang, Qi Z @ 2023-03-20 12:58 UTC (permalink / raw)
To: Xu, Rosen, Huang, Wei, dev, thomas, david.marchand; +Cc: stable, Zhang, Tianfei
> -----Original Message-----
> From: Xu, Rosen <rosen.xu@intel.com>
> Sent: Monday, March 20, 2023 2:52 PM
> To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org;
> thomas@monjalon.net; david.marchand@redhat.com
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Zhang, Qi Z
> <qi.z.zhang@intel.com>
> Subject: RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> Hi,
>
> > -----Original Message-----
> > From: Huang, Wei <wei.huang@intel.com>
> > Sent: Friday, March 17, 2023 4:45 AM
> > To: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> > Cc: stable@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> > <tianfei.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Huang,
> > Wei <wei.huang@intel.com>
> > Subject: [PATCH v1] raw/ifpga: remove virtual device unplug operation
> >
> > VDEV bus has implemented cleanup() function to perform cleanup for
> > devices on the bus during eal_cleanup(), so there is no need for ifpga
> > driver to record virtual devices and unplug them.
> >
> > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > ---
> > drivers/raw/ifpga/ifpga_rawdev.c | 99
> > ++++------------------------------------
> > drivers/raw/ifpga/ifpga_rawdev.h | 2 -
> > 2 files changed, 9 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > b/drivers/raw/ifpga/ifpga_rawdev.c
> > index 1020adc..8e6e70f 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > @@ -140,8 +140,6 @@ struct ifpga_rawdev *
> > for (i = 0; i < IFPGA_MAX_IRQ; i++)
> > dev->intr_handle[i] = NULL;
> > dev->poll_enabled = 0;
> > - for (i = 0; i < IFPGA_MAX_VDEV; i++)
> > - dev->vdev_name[i] = NULL;
> >
> > return dev;
> > }
> > @@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
> > struct ifpga_rawdev *ifpga_rdev = NULL;
> > struct opae_adapter *adapter;
> > struct opae_manager *mgr;
> > - char *vdev_name = NULL;
> > - int i, ret = 0;
> > + int ret = 0;
> >
> > if (dev) {
> > ifpga_rdev = ifpga_rawdev_get(dev);
> > if (ifpga_rdev) {
> > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > - vdev_name = ifpga_rdev->vdev_name[i];
> > - if (vdev_name)
> > - rte_vdev_uninit(vdev_name);
> > - }
> > ifpga_monitor_stop_func(ifpga_rdev);
> > ifpga_rdev->rawdev = NULL;
> > }
> > @@ -1778,104 +1770,31 @@ static int ifpga_rawdev_get_string_arg(const
> > char *key __rte_unused, static int ifpga_cfg_probe(struct
> > rte_vdev_device
> > *vdev) {
> > - struct rte_rawdev *rawdev = NULL;
> > - struct ifpga_rawdev *ifpga_dev;
> > struct ifpga_vdev_args args;
> > char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> > - const char *vdev_name = NULL;
> > - int i, n, ret = 0;
> > -
> > - vdev_name = rte_vdev_device_name(vdev);
> > - if (!vdev_name)
> > - return -EINVAL;
> > + int ret = 0;
> >
> > - IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> > vdev_name);
> > + IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> > + rte_vdev_device_name(vdev));
> >
> > ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> > if (ret)
> > return ret;
> >
> > memset(dev_name, 0, sizeof(dev_name));
> > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> > args.bdf);
> > - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> > - if (!rawdev)
> > - return -ENODEV;
> > - ifpga_dev = ifpga_rawdev_get(rawdev);
> > - if (!ifpga_dev)
> > - return -ENODEV;
> > -
> > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > - if (ifpga_dev->vdev_name[i] == NULL) {
> > - n = strlen(vdev_name) + 1;
> > - ifpga_dev->vdev_name[i] = rte_malloc(NULL, n, 0);
> > - if (ifpga_dev->vdev_name[i] == NULL)
> > - return -ENOMEM;
> > - strlcpy(ifpga_dev->vdev_name[i], vdev_name, n);
> > - break;
> > - }
> > - }
> > -
> > - if (i >= IFPGA_MAX_VDEV) {
> > - IFPGA_RAWDEV_PMD_ERR("Can't create more virtual
> > device!");
> > - return -ENOENT;
> > - }
> > -
> > snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> > args.port, args.bdf);
> > - ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> > - dev_name, vdev->device.devargs->args);
> > - if (ret) {
> > - rte_free(ifpga_dev->vdev_name[i]);
> > - ifpga_dev->vdev_name[i] = NULL;
> > - }
> > -
> > - return ret;
> > + return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> > dev_name,
> > + vdev->device.devargs->args);
> > }
> >
> > static int
> > ifpga_cfg_remove(struct rte_vdev_device *vdev) {
> > - struct rte_rawdev *rawdev = NULL;
> > - struct ifpga_rawdev *ifpga_dev;
> > - struct ifpga_vdev_args args;
> > - char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> > - const char *vdev_name = NULL;
> > - char *tmp_vdev = NULL;
> > - int i, ret = 0;
> > -
> > - vdev_name = rte_vdev_device_name(vdev);
> > - if (!vdev_name)
> > - return -EINVAL;
> > + IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> > + rte_vdev_device_name(vdev));
> >
> > - IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> > vdev_name);
> > -
> > - ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> > - if (ret)
> > - return ret;
> > -
> > - memset(dev_name, 0, sizeof(dev_name));
> > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> > args.bdf);
> > - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> > - if (!rawdev)
> > - return -ENODEV;
> > - ifpga_dev = ifpga_rawdev_get(rawdev);
> > - if (!ifpga_dev)
> > - return -ENODEV;
> > -
> > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> > - args.port, args.bdf);
> > - ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
> > dev_name);
> > -
> > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > - tmp_vdev = ifpga_dev->vdev_name[i];
> > - if (tmp_vdev && !strcmp(tmp_vdev, vdev_name)) {
> > - free(tmp_vdev);
> > - ifpga_dev->vdev_name[i] = NULL;
> > - break;
> > - }
> > - }
> > -
> > - return ret;
> > + return 0;
> > }
> >
> > static struct rte_vdev_driver ifpga_cfg_driver = { diff --git
> > a/drivers/raw/ifpga/ifpga_rawdev.h b/drivers/raw/ifpga/ifpga_rawdev.h
> > index 0fb66cb..1c128c7 100644
> > --- a/drivers/raw/ifpga/ifpga_rawdev.h
> > +++ b/drivers/raw/ifpga/ifpga_rawdev.h
> > @@ -65,8 +65,6 @@ struct ifpga_rawdev {
> > void *intr_handle[IFPGA_MAX_IRQ];
> > /* enable monitor thread poll device's sensors or not */
> > int poll_enabled;
> > - /* name of virtual devices created on raw device */
> > - char *vdev_name[IFPGA_MAX_VDEV];
> > };
> >
> > struct ifpga_vdev_args {
> > --
> > 1.8.3.1
>
> Acked-by: Rosen Xu <rosen.xu@intel.com>
Applied to dpdk-next-net-intel.
Thanks
Qi
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-20 12:58 ` Zhang, Qi Z
@ 2023-03-20 13:00 ` Zhang, Qi Z
0 siblings, 0 replies; 14+ messages in thread
From: Zhang, Qi Z @ 2023-03-20 13:00 UTC (permalink / raw)
To: Xu, Rosen, Huang, Wei, dev, thomas, david.marchand; +Cc: stable, Zhang, Tianfei
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Monday, March 20, 2023 8:59 PM
> To: Xu, Rosen <rosen.xu@intel.com>; Huang, Wei <wei.huang@intel.com>;
> dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>
> Subject: RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
>
>
> > -----Original Message-----
> > From: Xu, Rosen <rosen.xu@intel.com>
> > Sent: Monday, March 20, 2023 2:52 PM
> > To: Huang, Wei <wei.huang@intel.com>; dev@dpdk.org;
> > thomas@monjalon.net; david.marchand@redhat.com
> > Cc: stable@dpdk.org; Zhang, Tianfei <tianfei.zhang@intel.com>; Zhang,
> > Qi Z <qi.z.zhang@intel.com>
> > Subject: RE: [PATCH v1] raw/ifpga: remove virtual device unplug
> > operation
> >
> > Hi,
> >
> > > -----Original Message-----
> > > From: Huang, Wei <wei.huang@intel.com>
> > > Sent: Friday, March 17, 2023 4:45 AM
> > > To: dev@dpdk.org; thomas@monjalon.net; david.marchand@redhat.com
> > > Cc: stable@dpdk.org; Xu, Rosen <rosen.xu@intel.com>; Zhang, Tianfei
> > > <tianfei.zhang@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>;
> > > Huang, Wei <wei.huang@intel.com>
> > > Subject: [PATCH v1] raw/ifpga: remove virtual device unplug
> > > operation
> > >
> > > VDEV bus has implemented cleanup() function to perform cleanup for
> > > devices on the bus during eal_cleanup(), so there is no need for
> > > ifpga driver to record virtual devices and unplug them.
> > >
> > > Signed-off-by: Wei Huang <wei.huang@intel.com>
> > > ---
> > > drivers/raw/ifpga/ifpga_rawdev.c | 99
> > > ++++------------------------------------
> > > drivers/raw/ifpga/ifpga_rawdev.h | 2 -
> > > 2 files changed, 9 insertions(+), 92 deletions(-)
> > >
> > > diff --git a/drivers/raw/ifpga/ifpga_rawdev.c
> > > b/drivers/raw/ifpga/ifpga_rawdev.c
> > > index 1020adc..8e6e70f 100644
> > > --- a/drivers/raw/ifpga/ifpga_rawdev.c
> > > +++ b/drivers/raw/ifpga/ifpga_rawdev.c
> > > @@ -140,8 +140,6 @@ struct ifpga_rawdev *
> > > for (i = 0; i < IFPGA_MAX_IRQ; i++)
> > > dev->intr_handle[i] = NULL;
> > > dev->poll_enabled = 0;
> > > - for (i = 0; i < IFPGA_MAX_VDEV; i++)
> > > - dev->vdev_name[i] = NULL;
> > >
> > > return dev;
> > > }
> > > @@ -749,17 +747,11 @@ static int set_surprise_link_check_aer(
> > > struct ifpga_rawdev *ifpga_rdev = NULL;
> > > struct opae_adapter *adapter;
> > > struct opae_manager *mgr;
> > > - char *vdev_name = NULL;
> > > - int i, ret = 0;
> > > + int ret = 0;
> > >
> > > if (dev) {
> > > ifpga_rdev = ifpga_rawdev_get(dev);
> > > if (ifpga_rdev) {
> > > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > > - vdev_name = ifpga_rdev->vdev_name[i];
> > > - if (vdev_name)
> > > - rte_vdev_uninit(vdev_name);
> > > - }
> > > ifpga_monitor_stop_func(ifpga_rdev);
> > > ifpga_rdev->rawdev = NULL;
> > > }
> > > @@ -1778,104 +1770,31 @@ static int
> > > ifpga_rawdev_get_string_arg(const char *key __rte_unused, static
> > > int ifpga_cfg_probe(struct rte_vdev_device
> > > *vdev) {
> > > - struct rte_rawdev *rawdev = NULL;
> > > - struct ifpga_rawdev *ifpga_dev;
> > > struct ifpga_vdev_args args;
> > > char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> > > - const char *vdev_name = NULL;
> > > - int i, n, ret = 0;
> > > -
> > > - vdev_name = rte_vdev_device_name(vdev);
> > > - if (!vdev_name)
> > > - return -EINVAL;
> > > + int ret = 0;
> > >
> > > - IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> > > vdev_name);
> > > + IFPGA_RAWDEV_PMD_INFO("probe ifpga virtual device %s",
> > > + rte_vdev_device_name(vdev));
> > >
> > > ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> > > if (ret)
> > > return ret;
> > >
> > > memset(dev_name, 0, sizeof(dev_name));
> > > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> > > args.bdf);
> > > - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> > > - if (!rawdev)
> > > - return -ENODEV;
> > > - ifpga_dev = ifpga_rawdev_get(rawdev);
> > > - if (!ifpga_dev)
> > > - return -ENODEV;
> > > -
> > > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > > - if (ifpga_dev->vdev_name[i] == NULL) {
> > > - n = strlen(vdev_name) + 1;
> > > - ifpga_dev->vdev_name[i] = rte_malloc(NULL, n, 0);
> > > - if (ifpga_dev->vdev_name[i] == NULL)
> > > - return -ENOMEM;
> > > - strlcpy(ifpga_dev->vdev_name[i], vdev_name, n);
> > > - break;
> > > - }
> > > - }
> > > -
> > > - if (i >= IFPGA_MAX_VDEV) {
> > > - IFPGA_RAWDEV_PMD_ERR("Can't create more virtual
> > > device!");
> > > - return -ENOENT;
> > > - }
> > > -
> > > snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> > > args.port, args.bdf);
> > > - ret = rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> > > - dev_name, vdev->device.devargs->args);
> > > - if (ret) {
> > > - rte_free(ifpga_dev->vdev_name[i]);
> > > - ifpga_dev->vdev_name[i] = NULL;
> > > - }
> > > -
> > > - return ret;
> > > + return rte_eal_hotplug_add(RTE_STR(IFPGA_BUS_NAME),
> > > dev_name,
> > > + vdev->device.devargs->args);
> > > }
> > >
> > > static int
> > > ifpga_cfg_remove(struct rte_vdev_device *vdev) {
> > > - struct rte_rawdev *rawdev = NULL;
> > > - struct ifpga_rawdev *ifpga_dev;
> > > - struct ifpga_vdev_args args;
> > > - char dev_name[RTE_RAWDEV_NAME_MAX_LEN];
> > > - const char *vdev_name = NULL;
> > > - char *tmp_vdev = NULL;
> > > - int i, ret = 0;
> > > -
> > > - vdev_name = rte_vdev_device_name(vdev);
> > > - if (!vdev_name)
> > > - return -EINVAL;
> > > + IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> > > + rte_vdev_device_name(vdev));
> > >
> > > - IFPGA_RAWDEV_PMD_INFO("remove ifpga virtual device %s",
> > > vdev_name);
> > > -
> > > - ret = ifpga_vdev_parse_devargs(vdev->device.devargs, &args);
> > > - if (ret)
> > > - return ret;
> > > -
> > > - memset(dev_name, 0, sizeof(dev_name));
> > > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "IFPGA:%s",
> > > args.bdf);
> > > - rawdev = rte_rawdev_pmd_get_named_dev(dev_name);
> > > - if (!rawdev)
> > > - return -ENODEV;
> > > - ifpga_dev = ifpga_rawdev_get(rawdev);
> > > - if (!ifpga_dev)
> > > - return -ENODEV;
> > > -
> > > - snprintf(dev_name, RTE_RAWDEV_NAME_MAX_LEN, "%d|%s",
> > > - args.port, args.bdf);
> > > - ret = rte_eal_hotplug_remove(RTE_STR(IFPGA_BUS_NAME),
> > > dev_name);
> > > -
> > > - for (i = 0; i < IFPGA_MAX_VDEV; i++) {
> > > - tmp_vdev = ifpga_dev->vdev_name[i];
> > > - if (tmp_vdev && !strcmp(tmp_vdev, vdev_name)) {
> > > - free(tmp_vdev);
> > > - ifpga_dev->vdev_name[i] = NULL;
> > > - break;
> > > - }
> > > - }
> > > -
> > > - return ret;
> > > + return 0;
> > > }
> > >
> > > static struct rte_vdev_driver ifpga_cfg_driver = { diff --git
> > > a/drivers/raw/ifpga/ifpga_rawdev.h
> > > b/drivers/raw/ifpga/ifpga_rawdev.h
> > > index 0fb66cb..1c128c7 100644
> > > --- a/drivers/raw/ifpga/ifpga_rawdev.h
> > > +++ b/drivers/raw/ifpga/ifpga_rawdev.h
> > > @@ -65,8 +65,6 @@ struct ifpga_rawdev {
> > > void *intr_handle[IFPGA_MAX_IRQ];
> > > /* enable monitor thread poll device's sensors or not */
> > > int poll_enabled;
> > > - /* name of virtual devices created on raw device */
> > > - char *vdev_name[IFPGA_MAX_VDEV];
> > > };
> > >
> > > struct ifpga_vdev_args {
> > > --
> > > 1.8.3.1
> >
> > Acked-by: Rosen Xu <rosen.xu@intel.com>
>
> Applied to dpdk-next-net-intel.
>
> Thanks
> Qi
Sorry, this didnt happen, I replied on the wrong email.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-16 20:44 [PATCH v1] raw/ifpga: remove virtual device unplug operation Wei Huang
2023-03-20 6:51 ` Xu, Rosen
@ 2023-03-20 16:06 ` Thomas Monjalon
2023-03-21 0:11 ` Huang, Wei
1 sibling, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2023-03-20 16:06 UTC (permalink / raw)
To: Wei Huang
Cc: dev, david.marchand, stable, rosen.xu, tianfei.zhang, qi.z.zhang
16/03/2023 21:44, Wei Huang:
> VDEV bus has implemented cleanup() function to perform cleanup for
> devices on the bus during eal_cleanup(), so there is no need for
> ifpga driver to record virtual devices and unplug them.
Why no need?
If the application wants to explicitly remove a device, what happens?
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-20 16:06 ` Thomas Monjalon
@ 2023-03-21 0:11 ` Huang, Wei
2023-03-21 8:14 ` Thomas Monjalon
0 siblings, 1 reply; 14+ messages in thread
From: Huang, Wei @ 2023-03-21 0:11 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
Hi Tomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, March 21, 2023 00:06
> To: Huang, Wei <wei.huang@intel.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; stable@dpdk.org; Xu,
> Rosen <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> 16/03/2023 21:44, Wei Huang:
> > VDEV bus has implemented cleanup() function to perform cleanup for
> > devices on the bus during eal_cleanup(), so there is no need for ifpga
> > driver to record virtual devices and unplug them.
>
> Why no need?
> If the application wants to explicitly remove a device, what happens?
>
>
EAL will output an error information "Cannot find plugged device (%s)".
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-21 0:11 ` Huang, Wei
@ 2023-03-21 8:14 ` Thomas Monjalon
2023-03-21 8:41 ` Huang, Wei
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2023-03-21 8:14 UTC (permalink / raw)
To: Huang, Wei
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
21/03/2023 01:11, Huang, Wei:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 16/03/2023 21:44, Wei Huang:
> > > VDEV bus has implemented cleanup() function to perform cleanup for
> > > devices on the bus during eal_cleanup(), so there is no need for ifpga
> > > driver to record virtual devices and unplug them.
> >
> > Why no need?
> > If the application wants to explicitly remove a device, what happens?
> >
> >
> EAL will output an error information "Cannot find plugged device (%s)".
It does not look what we expect.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-21 8:14 ` Thomas Monjalon
@ 2023-03-21 8:41 ` Huang, Wei
2023-03-21 10:30 ` Thomas Monjalon
0 siblings, 1 reply; 14+ messages in thread
From: Huang, Wei @ 2023-03-21 8:41 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
Hi Thomas,
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, March 21, 2023 16:14
> To: Huang, Wei <wei.huang@intel.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; stable@dpdk.org; Xu,
> Rosen <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> 21/03/2023 01:11, Huang, Wei:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 16/03/2023 21:44, Wei Huang:
> > > > VDEV bus has implemented cleanup() function to perform cleanup for
> > > > devices on the bus during eal_cleanup(), so there is no need for
> > > > ifpga driver to record virtual devices and unplug them.
> > >
> > > Why no need?
> > > If the application wants to explicitly remove a device, what happens?
> > >
> > >
> > EAL will output an error information "Cannot find plugged device (%s)".
>
> It does not look what we expect.
>
Let me clear it.
With this patch, no error information will be outputted.
Without this patch, error information will be outputted. Because bus cleanup action will unplug virtual device, then ifpga PMD unplug the virtual device which is already be cleanup, bus->find_device() returns NULL, EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-21 8:41 ` Huang, Wei
@ 2023-03-21 10:30 ` Thomas Monjalon
2023-03-22 1:26 ` Huang, Wei
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2023-03-21 10:30 UTC (permalink / raw)
To: Huang, Wei
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
21/03/2023 09:41, Huang, Wei:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/03/2023 01:11, Huang, Wei:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 16/03/2023 21:44, Wei Huang:
> > > > > VDEV bus has implemented cleanup() function to perform cleanup for
> > > > > devices on the bus during eal_cleanup(), so there is no need for
> > > > > ifpga driver to record virtual devices and unplug them.
> > > >
> > > > Why no need?
> > > > If the application wants to explicitly remove a device, what happens?
> > > >
> > > >
> > > EAL will output an error information "Cannot find plugged device (%s)".
> >
> > It does not look what we expect.
> >
> Let me clear it.
> With this patch, no error information will be outputted.
> Without this patch, error information will be outputted.
> Because bus cleanup action will unplug virtual device,
> then ifpga PMD unplug the virtual device which is already be cleanup,
Why ipfga unplug the device after the bus cleanup?
I'm not following.
> bus->find_device() returns NULL,
> EAL output "Cannot find plugged device (%s)\n" at line 302 in eal_common_dev.c
Anyway, the good answer is not to completely remove the "remove" operation.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-21 10:30 ` Thomas Monjalon
@ 2023-03-22 1:26 ` Huang, Wei
2023-03-22 11:54 ` Thomas Monjalon
0 siblings, 1 reply; 14+ messages in thread
From: Huang, Wei @ 2023-03-22 1:26 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Tuesday, March 21, 2023 18:30
> To: Huang, Wei <wei.huang@intel.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; stable@dpdk.org; Xu,
> Rosen <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> 21/03/2023 09:41, Huang, Wei:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 21/03/2023 01:11, Huang, Wei:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 16/03/2023 21:44, Wei Huang:
> > > > > > VDEV bus has implemented cleanup() function to perform cleanup
> > > > > > for devices on the bus during eal_cleanup(), so there is no
> > > > > > need for ifpga driver to record virtual devices and unplug them.
> > > > >
> > > > > Why no need?
> > > > > If the application wants to explicitly remove a device, what happens?
> > > > >
> > > > >
> > > > EAL will output an error information "Cannot find plugged device (%s)".
> > >
> > > It does not look what we expect.
> > >
> > Let me clear it.
> > With this patch, no error information will be outputted.
> > Without this patch, error information will be outputted.
> > Because bus cleanup action will unplug virtual device, then ifpga PMD
> > unplug the virtual device which is already be cleanup,
>
> Why ipfga unplug the device after the bus cleanup?
> I'm not following.
>
The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup, ifpga has the responsibility to unplug these virtual devices.
> > bus->find_device() returns NULL,
> > EAL output "Cannot find plugged device (%s)\n" at line 302 in
> > eal_common_dev.c
>
> Anyway, the good answer is not to completely remove the "remove"
> operation.
>
If not to completely remove the "remove", the same virtual device will be unplug twice, is it reasonable?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-22 1:26 ` Huang, Wei
@ 2023-03-22 11:54 ` Thomas Monjalon
2023-03-23 3:26 ` Huang, Wei
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2023-03-22 11:54 UTC (permalink / raw)
To: Huang, Wei
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
22/03/2023 02:26, Huang, Wei:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 21/03/2023 09:41, Huang, Wei:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 21/03/2023 01:11, Huang, Wei:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 16/03/2023 21:44, Wei Huang:
> > > > > > > VDEV bus has implemented cleanup() function to perform cleanup
> > > > > > > for devices on the bus during eal_cleanup(), so there is no
> > > > > > > need for ifpga driver to record virtual devices and unplug them.
> > > > > >
> > > > > > Why no need?
> > > > > > If the application wants to explicitly remove a device, what happens?
> > > > > >
> > > > > >
> > > > > EAL will output an error information "Cannot find plugged device (%s)".
> > > >
> > > > It does not look what we expect.
> > > >
> > > Let me clear it.
> > > With this patch, no error information will be outputted.
> > > Without this patch, error information will be outputted.
> > > Because bus cleanup action will unplug virtual device, then ifpga PMD
> > > unplug the virtual device which is already be cleanup,
> >
> > Why ipfga unplug the device after the bus cleanup?
> > I'm not following.
> >
> The virtual device is created upon ifpga, if VDEV bus doesn't perform cleanup,
> ifpga has the responsibility to unplug these virtual devices.
Really I don't understand the flow.
Are you talking about EAL cleanup case?
What happens first? Do you need ifpga to be called first?
I think you need the correct checks to allow any order of cleanup.
> > > bus->find_device() returns NULL,
> > > EAL output "Cannot find plugged device (%s)\n" at line 302 in
> > > eal_common_dev.c
> >
> > Anyway, the good answer is not to completely remove the "remove"
> > operation.
> >
> If not to completely remove the "remove", the same virtual device will be unplug twice, is it reasonable?
You need to add a check to not unplug something already unplugged.
But you must allow the user calling "remove" directly.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-22 11:54 ` Thomas Monjalon
@ 2023-03-23 3:26 ` Huang, Wei
2023-03-23 8:52 ` Thomas Monjalon
0 siblings, 1 reply; 14+ messages in thread
From: Huang, Wei @ 2023-03-23 3:26 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Wednesday, March 22, 2023 19:54
> To: Huang, Wei <wei.huang@intel.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; stable@dpdk.org; Xu,
> Rosen <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> 22/03/2023 02:26, Huang, Wei:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 21/03/2023 09:41, Huang, Wei:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 21/03/2023 01:11, Huang, Wei:
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > 16/03/2023 21:44, Wei Huang:
> > > > > > > > VDEV bus has implemented cleanup() function to perform
> > > > > > > > cleanup for devices on the bus during eal_cleanup(), so
> > > > > > > > there is no need for ifpga driver to record virtual devices and
> unplug them.
> > > > > > >
> > > > > > > Why no need?
> > > > > > > If the application wants to explicitly remove a device, what
> happens?
> > > > > > >
> > > > > > >
> > > > > > EAL will output an error information "Cannot find plugged device
> (%s)".
> > > > >
> > > > > It does not look what we expect.
> > > > >
> > > > Let me clear it.
> > > > With this patch, no error information will be outputted.
> > > > Without this patch, error information will be outputted.
> > > > Because bus cleanup action will unplug virtual device, then ifpga
> > > > PMD unplug the virtual device which is already be cleanup,
> > >
> > > Why ipfga unplug the device after the bus cleanup?
> > > I'm not following.
> > >
> > The virtual device is created upon ifpga, if VDEV bus doesn't perform
> > cleanup, ifpga has the responsibility to unplug these virtual devices.
>
> Really I don't understand the flow.
> Are you talking about EAL cleanup case?
Yes, it's about EAL cleanup.
> What happens first? Do you need ifpga to be called first?
The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup()
eal_bus_cleanup() will call each bus's cleanup method to complete cleanup work.
There are three types of device related to ifpga PMD: PCI device, VDEV device and AFU device.
VDEV device is created on PCI device, it's a mediate device which purpose is to plug a AFU device on IFPGA bus.
Before eal_bus_cleanup() is implemented, application will call rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be removed when PCI device is removed, AFU device will be removed when VDEV device is removed, it works fine.
Now eal_bus_cleanup() takes the job, application has no need to call rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.
> I think you need the correct checks to allow any order of cleanup.
When this patch is committed, no order dependent on cleanup.
> > > > bus->find_device() returns NULL,
> > > > EAL output "Cannot find plugged device (%s)\n" at line 302 in
> > > > eal_common_dev.c
> > >
> > > Anyway, the good answer is not to completely remove the "remove"
> > > operation.
> > >
> > If not to completely remove the "remove", the same virtual device will be
> unplug twice, is it reasonable?
>
> You need to add a check to not unplug something already unplugged.
> But you must allow the user calling "remove" directly.
>
rte_pmd_ifpga_cleanup() is the only one interface for user to calling "remove" directly, when this patch is committed, VDEV and AFU device will not be unplugged twice.
For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like below
for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
dev = &ifpga_rawdevices[i];
if (dev->rawdev) {
rte_rawdev_pmd_release(dev->rawdev);
dev->rawdev = NULL;
}
}
If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be unplugged again.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-23 3:26 ` Huang, Wei
@ 2023-03-23 8:52 ` Thomas Monjalon
2023-03-24 1:27 ` Huang, Wei
0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2023-03-23 8:52 UTC (permalink / raw)
To: Huang, Wei
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
23/03/2023 04:26, Huang, Wei:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 22/03/2023 02:26, Huang, Wei:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 21/03/2023 09:41, Huang, Wei:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 21/03/2023 01:11, Huang, Wei:
> > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > 16/03/2023 21:44, Wei Huang:
> > > > > > > > > VDEV bus has implemented cleanup() function to perform
> > > > > > > > > cleanup for devices on the bus during eal_cleanup(), so
> > > > > > > > > there is no need for ifpga driver to record virtual devices and
> > unplug them.
> > > > > > > >
> > > > > > > > Why no need?
> > > > > > > > If the application wants to explicitly remove a device, what
> > happens?
> > > > > > > >
> > > > > > > >
> > > > > > > EAL will output an error information "Cannot find plugged device
> > (%s)".
> > > > > >
> > > > > > It does not look what we expect.
> > > > > >
> > > > > Let me clear it.
> > > > > With this patch, no error information will be outputted.
> > > > > Without this patch, error information will be outputted.
> > > > > Because bus cleanup action will unplug virtual device, then ifpga
> > > > > PMD unplug the virtual device which is already be cleanup,
> > > >
> > > > Why ipfga unplug the device after the bus cleanup?
> > > > I'm not following.
> > > >
> > > The virtual device is created upon ifpga, if VDEV bus doesn't perform
> > > cleanup, ifpga has the responsibility to unplug these virtual devices.
> >
> > Really I don't understand the flow.
> > Are you talking about EAL cleanup case?
> Yes, it's about EAL cleanup.
> > What happens first? Do you need ifpga to be called first?
> The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup()
> eal_bus_cleanup() will call each bus's cleanup method to complete cleanup work.
> There are three types of device related to ifpga PMD: PCI device, VDEV device and AFU device.
> VDEV device is created on PCI device, it's a mediate device which purpose is to plug a AFU device on IFPGA bus.
> Before eal_bus_cleanup() is implemented, application will call rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be removed when PCI device is removed, AFU device will be removed when VDEV device is removed, it works fine.
> Now eal_bus_cleanup() takes the job, application has no need to call rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.
That's the problem in your approach.
You need to keep the code removing children devices.
And if all children are removed, then the parent should remove itself.
If you implement these 2 conditions, the cleanup can happen in any order.
> > I think you need the correct checks to allow any order of cleanup.
> When this patch is committed, no order dependent on cleanup.
>
> > > > > bus->find_device() returns NULL,
> > > > > EAL output "Cannot find plugged device (%s)\n" at line 302 in
> > > > > eal_common_dev.c
> > > >
> > > > Anyway, the good answer is not to completely remove the "remove"
> > > > operation.
> > > >
> > > If not to completely remove the "remove", the same virtual device will be
> > unplug twice, is it reasonable?
> >
> > You need to add a check to not unplug something already unplugged.
> > But you must allow the user calling "remove" directly.
> >
> rte_pmd_ifpga_cleanup() is the only one interface for user to calling "remove" directly,
No, there are functions in EAL to "remove" devices, like rte_dev_remove(),
and we must make sure it works effectively.
> when this patch is committed, VDEV and AFU device will not be unplugged twice.
> For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like below
> for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> dev = &ifpga_rawdevices[i];
> if (dev->rawdev) {
> rte_rawdev_pmd_release(dev->rawdev);
> dev->rawdev = NULL;
> }
> }
> If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be unplugged again.
^ permalink raw reply [flat|nested] 14+ messages in thread
* RE: [PATCH v1] raw/ifpga: remove virtual device unplug operation
2023-03-23 8:52 ` Thomas Monjalon
@ 2023-03-24 1:27 ` Huang, Wei
0 siblings, 0 replies; 14+ messages in thread
From: Huang, Wei @ 2023-03-24 1:27 UTC (permalink / raw)
To: Thomas Monjalon
Cc: dev, david.marchand, stable, Xu, Rosen, Zhang, Tianfei, Zhang, Qi Z
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, March 23, 2023 16:52
> To: Huang, Wei <wei.huang@intel.com>
> Cc: dev@dpdk.org; david.marchand@redhat.com; stable@dpdk.org; Xu,
> Rosen <rosen.xu@intel.com>; Zhang, Tianfei <tianfei.zhang@intel.com>;
> Zhang, Qi Z <qi.z.zhang@intel.com>
> Subject: Re: [PATCH v1] raw/ifpga: remove virtual device unplug operation
>
> 23/03/2023 04:26, Huang, Wei:
> > From: Thomas Monjalon <thomas@monjalon.net>
> > > 22/03/2023 02:26, Huang, Wei:
> > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > 21/03/2023 09:41, Huang, Wei:
> > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > 21/03/2023 01:11, Huang, Wei:
> > > > > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > > > > 16/03/2023 21:44, Wei Huang:
> > > > > > > > > > VDEV bus has implemented cleanup() function to perform
> > > > > > > > > > cleanup for devices on the bus during eal_cleanup(),
> > > > > > > > > > so there is no need for ifpga driver to record virtual
> > > > > > > > > > devices and
> > > unplug them.
> > > > > > > > >
> > > > > > > > > Why no need?
> > > > > > > > > If the application wants to explicitly remove a device,
> > > > > > > > > what
> > > happens?
> > > > > > > > >
> > > > > > > > >
> > > > > > > > EAL will output an error information "Cannot find plugged
> > > > > > > > device
> > > (%s)".
> > > > > > >
> > > > > > > It does not look what we expect.
> > > > > > >
> > > > > > Let me clear it.
> > > > > > With this patch, no error information will be outputted.
> > > > > > Without this patch, error information will be outputted.
> > > > > > Because bus cleanup action will unplug virtual device, then
> > > > > > ifpga PMD unplug the virtual device which is already be
> > > > > > cleanup,
> > > > >
> > > > > Why ipfga unplug the device after the bus cleanup?
> > > > > I'm not following.
> > > > >
> > > > The virtual device is created upon ifpga, if VDEV bus doesn't
> > > > perform cleanup, ifpga has the responsibility to unplug these virtual
> devices.
> > >
> > > Really I don't understand the flow.
> > > Are you talking about EAL cleanup case?
> > Yes, it's about EAL cleanup.
> > > What happens first? Do you need ifpga to be called first?
> > The cleanup flow is rte_eal_cleanup() -> eal_bus_cleanup()
> > eal_bus_cleanup() will call each bus's cleanup method to complete cleanup
> work.
> > There are three types of device related to ifpga PMD: PCI device, VDEV
> device and AFU device.
> > VDEV device is created on PCI device, it's a mediate device which purpose
> is to plug a AFU device on IFPGA bus.
> > Before eal_bus_cleanup() is implemented, application will call
> rte_pmd_ifpga_cleanup() to remove PCI device, VDEV device will be
> removed when PCI device is removed, AFU device will be removed when
> VDEV device is removed, it works fine.
> > Now eal_bus_cleanup() takes the job, application has no need to call
> rte_pmd_ifpga_cleanup(), ifpga PCI device has no need to remove ifpga
> VDEV device and ifpga VDEV device has no need to remove ifpga AFU device.
>
> That's the problem in your approach.
> You need to keep the code removing children devices.
> And if all children are removed, then the parent should remove itself.
> If you implement these 2 conditions, the cleanup can happen in any order.
>
OK, this patch should be rejected, I will commit another patch according to your comments.
> > > I think you need the correct checks to allow any order of cleanup.
> > When this patch is committed, no order dependent on cleanup.
> >
> > > > > > bus->find_device() returns NULL,
> > > > > > EAL output "Cannot find plugged device (%s)\n" at line 302 in
> > > > > > eal_common_dev.c
> > > > >
> > > > > Anyway, the good answer is not to completely remove the "remove"
> > > > > operation.
> > > > >
> > > > If not to completely remove the "remove", the same virtual device
> > > > will be
> > > unplug twice, is it reasonable?
> > >
> > > You need to add a check to not unplug something already unplugged.
> > > But you must allow the user calling "remove" directly.
> > >
> > rte_pmd_ifpga_cleanup() is the only one interface for user to calling
> > "remove" directly,
>
> No, there are functions in EAL to "remove" devices, like rte_dev_remove(),
> and we must make sure it works effectively.
>
> > when this patch is committed, VDEV and AFU device will not be unplugged
> twice.
> > For PCI device, the implementation of rte_pmd_ifpga_cleanup() is like
> below
> > for (i = 0; i < IFPGA_RAWDEV_NUM; i++) {
> > dev = &ifpga_rawdevices[i];
> > if (dev->rawdev) {
> > rte_rawdev_pmd_release(dev->rawdev);
> > dev->rawdev = NULL;
> > }
> > }
> > If ifpga PCI device is already removed, dev->rawdev is NULL, it will not be
> unplugged again.
>
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2023-03-24 1:27 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-16 20:44 [PATCH v1] raw/ifpga: remove virtual device unplug operation Wei Huang
2023-03-20 6:51 ` Xu, Rosen
2023-03-20 12:58 ` Zhang, Qi Z
2023-03-20 13:00 ` Zhang, Qi Z
2023-03-20 16:06 ` Thomas Monjalon
2023-03-21 0:11 ` Huang, Wei
2023-03-21 8:14 ` Thomas Monjalon
2023-03-21 8:41 ` Huang, Wei
2023-03-21 10:30 ` Thomas Monjalon
2023-03-22 1:26 ` Huang, Wei
2023-03-22 11:54 ` Thomas Monjalon
2023-03-23 3:26 ` Huang, Wei
2023-03-23 8:52 ` Thomas Monjalon
2023-03-24 1:27 ` Huang, Wei
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).