Re: [dpdk-dev] [PATCH v3 1/4] ethdev: Add eal device event callback

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 09.07.2018 14:46, Jeff Guo wrote:
> Implement a eal device event callback "rte_eth_dev_event_callback"
> in ethdev, it could let pmd driver have chance to manage the eal
> device event, such as process hotplug event.
  >
> Signed-off-by: Jeff Guo <jia.guo@intel.com>
> ---
> v3->v2:
> add new callback in ethdev
> ---
>   doc/guides/rel_notes/release_18_08.rst |  8 ++++++++
>   lib/librte_ethdev/rte_ethdev.c         | 37 ++++++++++++++++++++++++++++++++++
>   lib/librte_ethdev/rte_ethdev_driver.h  | 20 ++++++++++++++++++
>   3 files changed, 65 insertions(+)
>
> diff --git a/doc/guides/rel_notes/release_18_08.rst b/doc/guides/rel_notes/release_18_08.rst
> index bc01242..2326058 100644
> --- a/doc/guides/rel_notes/release_18_08.rst
> +++ b/doc/guides/rel_notes/release_18_08.rst
> @@ -46,6 +46,14 @@ New Features
>     Flow API support has been added to CXGBE Poll Mode Driver to offload
>     flows to Chelsio T5/T6 NICs.
>   
> +* **Added eal device event callback in ethdev for hotplug.**
> +
> +  Implement a eal device event callback in ethdev, it could let pmd driver

"pmd driver" sounds strange since PMD stands for poll-mode driver.

> +  have chance to manage the eal device event, such as process hotplug event.
> +
> +  * ``rte_eth_dev_event_callback`` for driver use to register it and process
> +    eal device event.
> +
>   
>   API Changes
>   -----------
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index a9977df..36f218a 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -4518,6 +4518,43 @@ rte_eth_devargs_parse(const char *dargs, struct rte_eth_devargs *eth_da)
>   	return result;
>   }
>   
> +void __rte_experimental
> +rte_eth_dev_event_callback(char *device_name, enum rte_dev_event_type type,
> +			     void *arg)
> +{
> +	struct rte_eth_dev *eth_dev = (struct rte_eth_dev *)arg;
> +
> +	if (type >= RTE_DEV_EVENT_MAX) {
> +		fprintf(stderr, "%s called upon invalid event %d\n",
> +			__func__, type);
> +		fflush(stderr);

I'd like to understand why fprintf() is used here for logging instead of 
rte_log
mechanisms.
Also if we really want the log, may be it make sense to move the if to 
default
case below.

> +	}
> +
> +	switch (type) {
> +	case RTE_DEV_EVENT_REMOVE:
> +		ethdev_log(INFO, "The device: %s has been removed!\n",
> +			    device_name);
> +
> +		if (!device_name || !eth_dev)
> +			return;
> +
> +		if (!(eth_dev->data->dev_flags & RTE_ETH_EVENT_INTR_RMV))
> +			return;
> +
> +		if (!strcmp(device_name, eth_dev->device->name))

Do we really need to check it? The callback is registered for devices
with such name, so it should be always true. May be it is OK to double-check
I just want to be sure that I understand it properly.

> +			_rte_eth_dev_callback_process(eth_dev,
> +						      RTE_ETH_EVENT_INTR_RMV,
> +						      NULL);
> +		break;
> +	case RTE_DEV_EVENT_ADD:
> +		ethdev_log(INFO, "The device: %s has been added!\n",
> +			device_name);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>   RTE_INIT(ethdev_init_log);
>   static void
>   ethdev_init_log(void)
> diff --git a/lib/librte_ethdev/rte_ethdev_driver.h b/lib/librte_ethdev/rte_ethdev_driver.h
> index c9c825e..fed5afa 100644
> --- a/lib/librte_ethdev/rte_ethdev_driver.h
> +++ b/lib/librte_ethdev/rte_ethdev_driver.h
> @@ -82,6 +82,26 @@ int rte_eth_dev_release_port(struct rte_eth_dev *eth_dev);
>   void _rte_eth_dev_reset(struct rte_eth_dev *dev);
>   
>   /**
> + * @warning
> + * @b EXPERIMENTAL: this API may change without prior notice.
> + *
> + * Implement a rte eth eal device event callbacks for the specific device.
> + *
> + * @param device_name
> + *  Pointer to the name of the rte device.

Is it name of the device which generates the event? If so, it should be 
highlighted.

> + * @param event
> + *  Eal device event type.
> + * @param ret_param
> + *  To pass data back to user application.
> + *
> + * @return
> + *  void
> + */
> +void __rte_experimental
> +rte_eth_dev_event_callback(char *device_name,
> +		enum rte_dev_event_type event, void *cb_arg);
> +
> +/**
>    * @internal Executes all the user application registered callbacks for
>    * the specific device. It is for DPDK internal user only. User
>    * application should not call it directly.