Hi Thomas, <snip>
> Subject: Re: [dpdk-dev] [PATCH v4 1/2] librte_ether: add internal callback > functions > > 2016-10-04 15:52, Bernard Iremonger: > > add _rte_eth_dev_callback_process_vf function. > > add _rte_eth_dev_callback_process_generic function > > > > Adding a callback to the user application on VF to PF mailbox message, > > allows passing information to the application controlling the PF when > > a VF mailbox event message is received, such as VF reset. > > I have some difficulties to parse this explanation. > Please could you reword it and precise the direction of the message and the > use case context? I will reword the explanation and add use case context. > > --- a/lib/librte_ether/rte_ethdev.c > > +++ b/lib/librte_ether/rte_ethdev.c > > @@ -2510,6 +2510,20 @@ void > > _rte_eth_dev_callback_process(struct rte_eth_dev *dev, > > enum rte_eth_event_type event) > > { > > + return _rte_eth_dev_callback_process_generic(dev, event, NULL); } > > + > > +void > > +_rte_eth_dev_callback_process_vf(struct rte_eth_dev *dev, > > + enum rte_eth_event_type event, void *param) { > > + return _rte_eth_dev_callback_process_generic(dev, event, param); > } > > This function is just adding a parameter, compared to the legacy > _rte_eth_dev_callback_process. > Why calling it process_vf? The parameter is just being added for the VF event, the handling of the other events is unchanged. > And by the way, why not just replacing the legacy function? > As it is a driver interface, there is no ABI restriction. I thought there would be an ABI issue if the legacy function is replaced. The _rte_eth_dev_callback_process is exported in DPDK 2.2 and used in the following PMD's, lib and app: app/test/virtual_pmd drivers/net/e1000 drivers/net/ixgbe drivers/net/mlx5 drivers/net/vhost drivers/net/virtio lib/librte_ether Adding a parameter to _rte_eth_dev_callback_process() will impact all of the above. Will this cause an ABI issue? > > + > > +void > > +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev, > > + enum rte_eth_event_type event, void *param) { > [...] > > --- a/lib/librte_ether/rte_ethdev.h > > +++ b/lib/librte_ether/rte_ethdev.h > > @@ -3026,6 +3026,7 @@ enum rte_eth_event_type { > > /**< queue state event (enabled/disabled) > */ > > RTE_ETH_EVENT_INTR_RESET, > > /**< reset interrupt event, sent to VF on PF reset */ > > + RTE_ETH_EVENT_VF_MBOX, /**< PF mailbox processing callback */ > > RTE_ETH_EVENT_MAX /**< max value of this enum */ > > }; > > Either we choose to have a "generic" VF event well documented, or it is just > a specific event with a tip on where to find the doc. > Here we need at least to know how to handle the argument. It is a specific event for VF to PF messages, details on the function and arguments are in the rte_ethdev.h file. > > +/** > > + * @internal Executes all the user application registered callbacks. Used > by: > > + * _rte_eth_dev_callback_process and > _rte_eth_dev_callback_process_vf > > + * It is for DPDK internal user only. User application should not > > +call it > > + * directly. > > + * > > + * @param dev > > + * Pointer to struct rte_eth_dev. > > + * @param event > > + * Eth device interrupt event type. > > + * > > + * @param param > > + * parameters to pass back to user application. > > + * > > + * @return > > + * void > > + */ > > +void > > +_rte_eth_dev_callback_process_generic(struct rte_eth_dev *dev, > > + enum rte_eth_event_type event, void > *param); > > This is really an internal function and should not be exported at all. Both new functions are internal I will make them static and remove them from the map file. When the functions are made static, should the function declarations be moved from rte_ethdev.h to rte_ethdev.c ? Thanks for the review. Regards, Bernard.