[dpdk-dev,v5,01/13] librte_ether: modify internal callback function
diff mbox

Message ID 1475753191-17391-2-git-send-email-bernard.iremonger@intel.com
State Superseded, archived
Headers show

Commit Message

Bernard Iremonger Oct. 6, 2016, 11:26 a.m. UTC 
add parameter to the _rte_eth_dev_callback_process function.

Adding a parameter to this function allows passing information
to the application when an eth device event occurs such as
a VF to PF message.
This allows the application to decide if a particular function
is permitted.

Signed-off-by: Alex Zelezniak <az5157@att.com>
Signed-off-by: Bernard Iremonger <bernard.iremonger@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 5 ++++-
 lib/librte_ether/rte_ethdev.h | 7 ++++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

Comments

Thomas Monjalon Oct. 6, 2016, 12:56 p.m. UTC | #1 
2016-10-06 12:26, Bernard Iremonger:
>  void
>  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> -	enum rte_eth_event_type event)
> +	enum rte_eth_event_type event, void *param)

You need to squash the patches updating the calls to this function.
Otherwise, this patch does not compile.

[...]
> +		if (param != NULL)
> +			dev_cb.cb_arg = (void *) param;

You are overriding the user parameter.
As it is only for a new event, it can be described in the register API
that the user param won't be returned for this event.
But a better design would be to add a new parameter to the callback.
However it will be an API breakage.

> +	RTE_ETH_EVENT_VF_MBOX,  /**< PF mailbox processing callback */

Sorry I do not parse well this line.
The event name is VF_MBOX and the comment is about the callback
processing this event on PF side?
I would suggest this kind of comment: "message from VF received by PF"

[...]
>   *  Pointer to struct rte_eth_dev.
>   * @param event
>   *  Eth device interrupt event type.
> + * @param param
> + *  Parameter to pass back to user application.
> + *  Allows the user application to decide if a particular function
> + *  is permitted.

In a more generic case, the parameter gives some details about the event.
Bernard Iremonger Oct. 6, 2016, 2:33 p.m. UTC | #2 
Hi Thomas,

> Subject: Re: [dpdk-dev] [PATCH v5 01/13] librte_ether: modify internal
> callback function
> 
> 2016-10-06 12:26, Bernard Iremonger:
> >  void
> >  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > -	enum rte_eth_event_type event)
> > +	enum rte_eth_event_type event, void *param)
> 
> You need to squash the patches updating the calls to this function.
> Otherwise, this patch does not compile.

I will have to squash everything into one patch, separate patches will not compile.
 
> [...]
> > +		if (param != NULL)
> > +			dev_cb.cb_arg = (void *) param;
> 
> You are overriding the user parameter.


Yes, we want to update the user parameter for the RTE_ETH_EVENT_VF_MBOX

> As it is only for a new event, it can be described in the register API that the
> user param won't be returned for this event.

I will add a description in the rte_eth_dev_callback_register  function.

> But a better design would be to add a new parameter to the callback.
> However it will be an API breakage.

I do not want to break the ABI at this point.

> 
> > +	RTE_ETH_EVENT_VF_MBOX,  /**< PF mailbox processing callback */
> 
> Sorry I do not parse well this line.
> The event name is VF_MBOX and the comment is about the callback
> processing this event on PF side?
> I would suggest this kind of comment: "message from VF received by PF"

Ok.

> 
> [...]
> >   *  Pointer to struct rte_eth_dev.
> >   * @param event
> >   *  Eth device interrupt event type.
> > + * @param param
> > + *  Parameter to pass back to user application.
> > + *  Allows the user application to decide if a particular function
> > + *  is permitted.
> 
> In a more generic case, the parameter gives some details about the event.
Thomas Monjalon Oct. 6, 2016, 2:56 p.m. UTC | #3 
2016-10-06 14:33, Iremonger, Bernard:
> Hi Thomas,
> 
> > Subject: Re: [dpdk-dev] [PATCH v5 01/13] librte_ether: modify internal
> > callback function
> > 
> > 2016-10-06 12:26, Bernard Iremonger:
> > >  void
> > >  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > > -	enum rte_eth_event_type event)
> > > +	enum rte_eth_event_type event, void *param)
> > 
> > You need to squash the patches updating the calls to this function.
> > Otherwise, this patch does not compile.
> 
> I will have to squash everything into one patch, separate patches will not compile.

No you can keep a separate patch for the VF event in ixgbe.

> > [...]
> > > +		if (param != NULL)
> > > +			dev_cb.cb_arg = (void *) param;
> > 
> > You are overriding the user parameter.
> 
> Yes, we want to update the user parameter for the RTE_ETH_EVENT_VF_MBOX
> 
> > As it is only for a new event, it can be described in the register API that the
> > user param won't be returned for this event.
> 
> I will add a description in the rte_eth_dev_callback_register  function.
> 
> > But a better design would be to add a new parameter to the callback.
> > However it will be an API breakage.
> 
> I do not want to break the ABI at this point.

Yes, but it can be considered for a later change.

> > > +	RTE_ETH_EVENT_VF_MBOX,  /**< PF mailbox processing callback */
> > 
> > Sorry I do not parse well this line.
> > The event name is VF_MBOX and the comment is about the callback
> > processing this event on PF side?
> > I would suggest this kind of comment: "message from VF received by PF"
> 
> Ok.
> 
> > 
> > [...]
> > >   *  Pointer to struct rte_eth_dev.
> > >   * @param event
> > >   *  Eth device interrupt event type.
> > > + * @param param
> > > + *  Parameter to pass back to user application.
> > > + *  Allows the user application to decide if a particular function
> > > + *  is permitted.
> > 
> > In a more generic case, the parameter gives some details about the event.

Please consider a rewording here, thanks.
Bernard Iremonger Oct. 6, 2016, 3:32 p.m. UTC | #4 
Hi Thomas,

<snip>

> > > Subject: Re: [dpdk-dev] [PATCH v5 01/13] librte_ether: modify
> > > internal callback function
> > >
> > > 2016-10-06 12:26, Bernard Iremonger:
> > > >  void
> > > >  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > > > -	enum rte_eth_event_type event)
> > > > +	enum rte_eth_event_type event, void *param)
> > >
> > > You need to squash the patches updating the calls to this function.
> > > Otherwise, this patch does not compile.
> >
> > I will have to squash everything into one patch, separate patches will not
> compile.
> 
> No you can keep a separate patch for the VF event in ixgbe.

I have 4 patches at present

librte_ether
net/ixgbe
drivers/net
app/test

Would this be acceptable or do you just want  everything squashed into librte_ether except for net/ixgbe?
 
 
> > > [...]
> > > > +		if (param != NULL)
> > > > +			dev_cb.cb_arg = (void *) param;
> > >
> > > You are overriding the user parameter.
> >
> > Yes, we want to update the user parameter for the
> > RTE_ETH_EVENT_VF_MBOX

I have renamed param to cb_arg to make it clearer what is happening.


> > > As it is only for a new event, it can be described in the register
> > > API that the user param won't be returned for this event.
> >
> > I will add a description in the rte_eth_dev_callback_register  function.
> >
> > > But a better design would be to add a new parameter to the callback.
> > > However it will be an API breakage.
> >
> > I do not want to break the ABI at this point.
> 
> Yes, but it can be considered for a later change.

Yes, ok
 
> > > > +	RTE_ETH_EVENT_VF_MBOX,  /**< PF mailbox processing callback */
> > >
> > > Sorry I do not parse well this line.
> > > The event name is VF_MBOX and the comment is about the callback
> > > processing this event on PF side?
> > > I would suggest this kind of comment: "message from VF received by PF"
> >
> > Ok.
> >
> > >
> > > [...]
> > > >   *  Pointer to struct rte_eth_dev.
> > > >   * @param event
> > > >   *  Eth device interrupt event type.
> > > > + * @param param
> > > > + *  Parameter to pass back to user application.
> > > > + *  Allows the user application to decide if a particular
> > > > + function
> > > > + *  is permitted.
> > >
> > > In a more generic case, the parameter gives some details about the
> event.
> 
> Please consider a rewording here, thanks.
 
I have reworded here, I hope it is clearer.

Regards,

Bernard.
Thomas Monjalon Oct. 6, 2016, 3:41 p.m. UTC | #5 
2016-10-06 15:32, Iremonger, Bernard:
> > > > 2016-10-06 12:26, Bernard Iremonger:
> > > > >  void
> > > > >  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > > > > -	enum rte_eth_event_type event)
> > > > > +	enum rte_eth_event_type event, void *param)
> > > >
> > > > You need to squash the patches updating the calls to this function.
> > > > Otherwise, this patch does not compile.
> > >
> > > I will have to squash everything into one patch, separate patches will not
> > compile.
> > 
> > No you can keep a separate patch for the VF event in ixgbe.
> 
> I have 4 patches at present
> 
> librte_ether
> net/ixgbe
> drivers/net
> app/test
> 
> Would this be acceptable or do you just want  everything squashed into librte_ether except for net/ixgbe?

You can test the compilation of each patch with this command:
	git rebase -i origin/master -x scripts/test-build.sh
or just:
	git rebase -i origin/master -x make

I think you need to squash drivers/net (adding just NULL param) and
app/test in ethdev patch.
Bernard Iremonger Oct. 6, 2016, 3:45 p.m. UTC | #6 
Hi Thomas,

> Subject: Re: [dpdk-dev] [PATCH v5 01/13] librte_ether: modify internal
> callback function
> 
> 2016-10-06 15:32, Iremonger, Bernard:
> > > > > 2016-10-06 12:26, Bernard Iremonger:
> > > > > >  void
> > > > > >  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
> > > > > > -	enum rte_eth_event_type event)
> > > > > > +	enum rte_eth_event_type event, void *param)
> > > > >
> > > > > You need to squash the patches updating the calls to this function.
> > > > > Otherwise, this patch does not compile.
> > > >
> > > > I will have to squash everything into one patch, separate patches
> > > > will not
> > > compile.
> > >
> > > No you can keep a separate patch for the VF event in ixgbe.
> >
> > I have 4 patches at present
> >
> > librte_ether
> > net/ixgbe
> > drivers/net
> > app/test
> >
> > Would this be acceptable or do you just want  everything squashed into
> librte_ether except for net/ixgbe?
> 
> You can test the compilation of each patch with this command:
> 	git rebase -i origin/master -x scripts/test-build.sh or just:
> 	git rebase -i origin/master -x make
> 
> I think you need to squash drivers/net (adding just NULL param) and
> app/test in ethdev patch.

They do not compile if applied one by one.
Ok, I will squash drivers/net and app/test into the librte_ether patch.

Regards,

Bernard.

Patch
diff mbox 

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index c517e88..416ca77 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -2508,7 +2508,7 @@  rte_eth_dev_callback_unregister(uint8_t port_id,
 
 void
 _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
-	enum rte_eth_event_type event)
+	enum rte_eth_event_type event, void *param)
 {
 	struct rte_eth_dev_callback *cb_lst;
 	struct rte_eth_dev_callback dev_cb;
@@ -2519,6 +2519,9 @@  _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
 			continue;
 		dev_cb = *cb_lst;
 		cb_lst->active = 1;
+		if (param != NULL)
+			dev_cb.cb_arg = (void *) param;
+
 		rte_spinlock_unlock(&rte_eth_dev_cb_lock);
 		dev_cb.cb_fn(dev->data->port_id, dev_cb.event,
 						dev_cb.cb_arg);
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 7218b6f..9fb34ca 100644
--- 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 */
 };
 
@@ -3085,12 +3086,16 @@  int rte_eth_dev_callback_unregister(uint8_t port_id,
  *  Pointer to struct rte_eth_dev.
  * @param event
  *  Eth device interrupt event type.
+ * @param param
+ *  Parameter to pass back to user application.
+ *  Allows the user application to decide if a particular function
+ *  is permitted.
  *
  * @return
  *  void
  */
 void _rte_eth_dev_callback_process(struct rte_eth_dev *dev,
-				enum rte_eth_event_type event);
+				enum rte_eth_event_type event, void *param);
 
 /**
  * When there is no rx packet coming in Rx Queue for a long time, we can