[dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support device reset

[Jerin Jacob]

On Wed, Jun 22, 2016 at 06:42:43AM +0000, Lu, Wenzhuo wrote:
> Hi Jerin,
> 
> 
> > -----Original Message-----
> > From: Jerin Jacob [mailto:jerin.jacob at caviumnetworks.com]
> > Sent: Wednesday, June 22, 2016 2:10 PM
> > To: Lu, Wenzhuo
> > Cc: Ananyev, Konstantin; Stephen Hemminger; dev at dpdk.org; Richardson,
> > Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing; Zhang, Helin;
> > thomas.monjalon at 6wind.com
> > Subject: Re: [dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support device reset
> > 
> > On Wed, Jun 22, 2016 at 05:05:14AM +0000, Lu, Wenzhuo wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Jerin Jacob [mailto:jerin.jacob at caviumnetworks.com]
> > > > Sent: Wednesday, June 22, 2016 12:15 PM
> > > > To: Lu, Wenzhuo
> > > > Cc: Ananyev, Konstantin; Stephen Hemminger; dev at dpdk.org;
> > > > Richardson, Bruce; Chen, Jing D; Liang, Cunming; Wu, Jingjing;
> > > > Zhang, Helin; thomas.monjalon at 6wind.com
> > > > Subject: Re: [dpdk-dev] [PATCH v6 1/4] lib/librte_ether: support
> > > > device reset
> > > >
> > > > On Wed, Jun 22, 2016 at 03:32:16AM +0000, Lu, Wenzhuo wrote:
> > > > > Lost here. I think these RTE_ETH_EVENTs are used to connect the
> > > > > APP call
> > > > back functions with the events.
> > > > > Actually I want the APP to register a callback function
> > > > > reset_event_callback for
> > > > the reset event. Like this,
> > > > > 		/* register reset interrupt callback */
> > > > > 		rte_eth_dev_callback_register(portid,
> > > > > 			RTE_ETH_EVENT_INTR_RESET, reset_event_callback,
> > > > NULL); And when the
> > > > > VF driver finds PF link down/up, it  should  use
> > > > _rte_eth_dev_callback_process(dev, RTE_ETH_EVENT_INTR_RESET) to run
> > > > into the callback which is provided by APP. Means reset_event_callback here.
> > > >
> > > > me too. Their is existing RTE_ETH_EVENT_INTR_RESET event to notify
> > > > the PF reset.I guess it is not for the PF link change or it isfor
> > > > generic VF reset request initiated by PF for everything.
> > > I think this event is for device reset not only for PF but also can for VF. I think
> > we can use this event when the driver want the APP to reset the device. The PF
> > link down/up caused VF reset is one of the cases.
> > 
> > Then please correct description for the RTE_ETH_EVENT_INTR_RESET in
> > lib/librte_ether/rte_ethdev.h "/**< reset interrupt event, sent to VF on PF reset
> > */"
> > 
> > >
> > > >
> > > > file: lib/librte_ether/rte_ethdev.h
> > > >         RTE_ETH_EVENT_INTR_RESET,
> > > > 		/**< reset interrupt event, sent to VF on PF reset */
> > > >
> > > > ^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >
> > > > if application need to call rte_ethdev_reset() on
> > > > RTE_ETH_EVENT_INTR_RESET event then please mention it commit log or
> > API description.
> > > Good suggestion. I'll try to find where's the good place to add more
> > explanation.
> > 
> > I guess then reset API can be changed to rte_ethdev_process_reset_intr() or
> > similar to reflect the use case(API called by application on reset event from PF)
> > 
> > The PMDs were PF does not generate the RTE_ETH_EVENT_INTR_RESET to VF
> > then VF's reset PMD callback shall be a 'nop'
> > 
> > Jerin
> But I don't think it's appropriate to bind the RTE_ETH_EVENTs with the APIs. This patch set provide a reset API to reset the device. Don't mean this reset API only can be used when the APP hit the event RTE_ETH_EVENT_INTR_RESET. I can add some comments to suggest the user to call the reset API at that time. But I think APP can call the reset API anytime when it thinks it's necessary. So I don't like the name *process_reset_intr*, it hints that this API is only for the INTR_RESET event.

That's where scope of API and PMD implementation its not getting clear.
Can you tell me any other use case where we need to call this API from application.
The name rte_ethdev_reset() is too generic. If you are going with that
generic name then you may need add lot of details in API description.

Thomas,
As a librte_ether maintainer any comments on this?


> 
> > 
> > > >
> > >