[dpdk-dev,v3,2/2] net/i40e: add hot plug monitor in i40e
Checks
Commit Message
From: "Guo, Jia" <jia.guo@intel.com>
This patch enable the hot plug feature in i40e, by monitoring the
hot plug uevent of the device. When remove event got, call the app
callback function to handle the detach process.
Signed-off-by: Guo, Jia <jia.guo@intel.com>
---
v3->v2: refine the return issue if device remove
---
drivers/net/i40e/i40e_ethdev.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
Comments
> -----Original Message-----
> From: Guo, Jia
> Sent: Thursday, June 29, 2017 1:02 PM
> To: Zhang, Helin <helin.zhang@intel.com>; Wu, Jingjing
> <jingjing.wu@intel.com>
> Cc: dev@dpdk.org; Guo, Jia <jia.guo@intel.com>; Richardson, Bruce
> <bruce.richardson@intel.com>; Ananyev, Konstantin
> <konstantin.ananyev@intel.com>; gaetan.rivet@6wind.com;
> thomas.monjalon@6wind.com; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: [PATCH v3 2/2] net/i40e: add hot plug monitor in i40e
>
> From: "Guo, Jia" <jia.guo@intel.com>
>
> This patch enable the hot plug feature in i40e, by monitoring the hot plug
> uevent of the device. When remove event got, call the app callback function to
> handle the detach process.
>
> Signed-off-by: Guo, Jia <jia.guo@intel.com>
Acked-by: Jingjing Wu <jingjing.wu@intel.com>
29/06/2017 07:01, Jeff Guo:
> --- a/drivers/net/i40e/i40e_ethdev.c
> +++ b/drivers/net/i40e/i40e_ethdev.c
> @@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
>
> /* enable uio intr after callback register */
> rte_intr_enable(intr_handle);
> +
> /*
> * Add an ethertype filter to drop all flow control frames transmitted
> * from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
> @@ -5832,11 +5833,29 @@ struct i40e_vsi *
> {
> struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> + struct rte_uevent event;
> uint32_t icr0;
> + struct rte_pci_device *pci_dev;
> + struct rte_intr_handle *intr_handle;
> +
> + pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> + intr_handle = &pci_dev->intr_handle;
>
> /* Disable interrupt */
> i40e_pf_disable_irq0(hw);
>
> + /* check device uevent */
> + if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
> + if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
> + if (event.action == RTE_UEVENT_REMOVE) {
> + _rte_eth_dev_callback_process(dev,
> + RTE_ETH_EVENT_INTR_RMV, NULL);
> + return;
> + }
> + }
> + goto done;
> + }
There is nothing specific to i40e in this patch.
It seems wrong to add such generic code in every drivers.
07/07/2017 09:56, Thomas Monjalon:
> 29/06/2017 07:01, Jeff Guo:
> > --- a/drivers/net/i40e/i40e_ethdev.c
> > +++ b/drivers/net/i40e/i40e_ethdev.c
> > @@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
> >
> > /* enable uio intr after callback register */
> > rte_intr_enable(intr_handle);
> > +
> > /*
> > * Add an ethertype filter to drop all flow control frames transmitted
> > * from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
> > @@ -5832,11 +5833,29 @@ struct i40e_vsi *
> > {
> > struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> > struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> > + struct rte_uevent event;
> > uint32_t icr0;
> > + struct rte_pci_device *pci_dev;
> > + struct rte_intr_handle *intr_handle;
> > +
> > + pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> > + intr_handle = &pci_dev->intr_handle;
> >
> > /* Disable interrupt */
> > i40e_pf_disable_irq0(hw);
> >
> > + /* check device uevent */
> > + if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
> > + if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
> > + if (event.action == RTE_UEVENT_REMOVE) {
> > + _rte_eth_dev_callback_process(dev,
> > + RTE_ETH_EVENT_INTR_RMV, NULL);
> > + return;
> > + }
> > + }
> > + goto done;
> > + }
>
> There is nothing specific to i40e in this patch.
> It seems wrong to add such generic code in every drivers.
It should be managed at bus layer and not be specific to ethdev only.
On 7/7/2017 6:17 PM, Thomas Monjalon wrote:
> 07/07/2017 09:56, Thomas Monjalon:
>> 29/06/2017 07:01, Jeff Guo:
>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>> @@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
>>>
>>> /* enable uio intr after callback register */
>>> rte_intr_enable(intr_handle);
>>> +
>>> /*
>>> * Add an ethertype filter to drop all flow control frames transmitted
>>> * from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
>>> @@ -5832,11 +5833,29 @@ struct i40e_vsi *
>>> {
>>> struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
>>> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>> + struct rte_uevent event;
>>> uint32_t icr0;
>>> + struct rte_pci_device *pci_dev;
>>> + struct rte_intr_handle *intr_handle;
>>> +
>>> + pci_dev = RTE_ETH_DEV_TO_PCI(dev);
>>> + intr_handle = &pci_dev->intr_handle;
>>>
>>> /* Disable interrupt */
>>> i40e_pf_disable_irq0(hw);
>>>
>>> + /* check device uevent */
>>> + if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
>>> + if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
>>> + if (event.action == RTE_UEVENT_REMOVE) {
>>> + _rte_eth_dev_callback_process(dev,
>>> + RTE_ETH_EVENT_INTR_RMV, NULL);
>>> + return;
>>> + }
>>> + }if
>>> + goto done;
>>> + }
>> There is nothing specific to i40e in this patch.
>> It seems wrong to add such generic code in every drivers.
> It should be managed at bus layer and not be specific to ethdev only.
if all could managed at bus layer might also be what i want to see,
but that would not so economical at currently. because of the event need
to exposure to driver to use app's callback to handle it by
detach/attach device. mlx driver also go through this path to show the
rmv event usege. we just add uevent for pci uio device. Anyway, i think
the uevent must be useful for future PF/VFIO live migration. if there
are not other concern about the other patch that added uevent api in
eal([PATCH v3 1/2] eal: add uevent api for hot plug), i suggest to merge
it at first. Then we could go next to enhancement it with the 6wind hot
plug feature.
07/07/2017 16:08, Guo, Jia:
>
> On 7/7/2017 6:17 PM, Thomas Monjalon wrote:
> > 07/07/2017 09:56, Thomas Monjalon:
> >> 29/06/2017 07:01, Jeff Guo:
> >>> --- a/drivers/net/i40e/i40e_ethdev.c
> >>> +++ b/drivers/net/i40e/i40e_ethdev.c
> >>> @@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
> >>>
> >>> /* enable uio intr after callback register */
> >>> rte_intr_enable(intr_handle);
> >>> +
> >>> /*
> >>> * Add an ethertype filter to drop all flow control frames transmitted
> >>> * from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
> >>> @@ -5832,11 +5833,29 @@ struct i40e_vsi *
> >>> {
> >>> struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
> >>> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> >>> + struct rte_uevent event;
> >>> uint32_t icr0;
> >>> + struct rte_pci_device *pci_dev;
> >>> + struct rte_intr_handle *intr_handle;
> >>> +
> >>> + pci_dev = RTE_ETH_DEV_TO_PCI(dev);
> >>> + intr_handle = &pci_dev->intr_handle;
> >>>
> >>> /* Disable interrupt */
> >>> i40e_pf_disable_irq0(hw);
> >>>
> >>> + /* check device uevent */
> >>> + if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
> >>> + if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
> >>> + if (event.action == RTE_UEVENT_REMOVE) {
> >>> + _rte_eth_dev_callback_process(dev,
> >>> + RTE_ETH_EVENT_INTR_RMV, NULL);
> >>> + return;
> >>> + }
> >>> + }if
> >>> + goto done;
> >>> + }
> >> There is nothing specific to i40e in this patch.
> >> It seems wrong to add such generic code in every drivers.
> > It should be managed at bus layer and not be specific to ethdev only.
> if all could managed at bus layer might also be what i want to see,
> but that would not so economical at currently. because of the event need
> to exposure to driver to use app's callback to handle it by
> detach/attach device. mlx driver also go through this path to show the
> rmv event usege. we just add uevent for pci uio device. Anyway, i think
> the uevent must be useful for future PF/VFIO live migration. if there
> are not other concern about the other patch that added uevent api in
> eal([PATCH v3 1/2] eal: add uevent api for hot plug), i suggest to merge
> it at first. Then we could go next to enhancement it with the 6wind hot
> plug feature.
Sorry it looks wrong to apply half of this patchset, given we are not
sure how the remaining part should be implemented.
Let's take time for a better solution and try to gather more opinions.
It will be highlighted as one of the next priorities, after the bus rework
in progress.
On 7/10/2017 6:35 AM, Thomas Monjalon wrote:
> 07/07/2017 16:08, Guo, Jia:
>> On 7/7/2017 6:17 PM, Thomas Monjalon wrote:
>>> 07/07/2017 09:56, Thomas Monjalon:
>>>> 29/06/2017 07:01, Jeff Guo:
>>>>> --- a/drivers/net/i40e/i40e_ethdev.c
>>>>> +++ b/drivers/net/i40e/i40e_ethdev.c
>>>>> @@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
>>>>>
>>>>> /* enable uio intr after callback register */
>>>>> rte_intr_enable(intr_handle);
>>>>> +
>>>>> /*
>>>>> * Add an ethertype filter to drop all flow control frames transmitted
>>>>> * from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
>>>>> @@ -5832,11 +5833,29 @@ struct i40e_vsi *
>>>>> {
>>>>> struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
>>>>> struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
>>>>> + struct rte_uevent event;
>>>>> uint32_t icr0;
>>>>> + struct rte_pci_device *pci_dev;
>>>>> + struct rte_intr_handle *intr_handle;
>>>>> +
>>>>> + pci_dev = RTE_ETH_DEV_TO_PCI(dev);
>>>>> + intr_handle = &pci_dev->intr_handle;
>>>>>
>>>>> /* Disable interrupt */
>>>>> i40e_pf_disable_irq0(hw);
>>>>>
>>>>> + /* check device uevent */
>>>>> + if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
>>>>> + if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
>>>>> + if (event.action == RTE_UEVENT_REMOVE) {
>>>>> + _rte_eth_dev_callback_process(dev,
>>>>> + RTE_ETH_EVENT_INTR_RMV, NULL);
>>>>> + return;
>>>>> + }
>>>>> + }if
>>>>> + goto done;
>>>>> + }
>>>> There is nothing specific to i40e in this patch.
>>>> It seems wrong to add such generic code in every drivers.
>>> It should be managed at bus layer and not be specific to ethdev only.
>> if all could managed at bus layer might also be what i want to see,
>> but that would not so economical at currently. because of the event need
>> to exposure to driver to use app's callback to handle it by
>> detach/attach device. mlx driver also go through this path to show the
>> rmv event usege. we just add uevent for pci uio device. Anyway, i think
>> the uevent must be useful for future PF/VFIO live migration. if there
>> are not other concern about the other patch that added uevent api in
>> eal([PATCH v3 1/2] eal: add uevent api for hot plug), i suggest to merge
>> it at first. Then we could go next to enhancement it with the 6wind hot
>> plug feature.
> Sorry it looks wrong to apply half of this patchset, given we are not
> sure how the remaining part should be implemented.
> Let's take time for a better solution and try to gather more opinions.
> It will be highlighted as one of the next priorities, after the bus rework
> in progress.
some how i agree with your concern, maybe we could find a better option
to handle that.
1) about bus layer , as we know , recently the pci uio api be moved out
of the eal bus, but not for the eal interrupt. so that would not affect
if we add uio api in, just like the exist api "uio_intr_enable" , i
could modify the api name into "uio_uevent_get", "uio_uevent_connnect"
for better identify. But it still in eal interrupt layer before we
decide to modify the eal interrupt layer.
2) about the uevent callback handler, fail-safe driver solution said
that fail-safe driver register the callback into device, and device
interrupt handler identify the event and callback fail-safe to switch
sub-device to process hot plug out case. so if the event process not
place in device(mlx driver handle it in device driver), it maybe place
in eal. To be generic for all pci uio driver. i could add
"uio_uevent_handler" api into eal interrupt. but it will bring some
ethdev process into eal. other wise we still need to add the handler in
all device driver like this patch here. Any comment about it?
3) the patch set here is about uevent monitoring for pci uio driver. the
hot plug remove event processing is depend on the failsave driver.
bellow for ref.
http://dpdk.org/dev/patchwork/patch/26842/
@@ -1283,6 +1283,7 @@ static inline void i40e_GLQF_reg_init(struct i40e_hw *hw)
/* enable uio intr after callback register */
rte_intr_enable(intr_handle);
+
/*
* Add an ethertype filter to drop all flow control frames transmitted
* from VSIs. By doing so, we stop VF from sending out PAUSE or PFC
@@ -5832,11 +5833,29 @@ struct i40e_vsi *
{
struct rte_eth_dev *dev = (struct rte_eth_dev *)param;
struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+ struct rte_uevent event;
uint32_t icr0;
+ struct rte_pci_device *pci_dev;
+ struct rte_intr_handle *intr_handle;
+
+ pci_dev = RTE_ETH_DEV_TO_PCI(dev);
+ intr_handle = &pci_dev->intr_handle;
/* Disable interrupt */
i40e_pf_disable_irq0(hw);
+ /* check device uevent */
+ if (rte_uevent_get(intr_handle->uevent_fd, &event) == 0) {
+ if (event.subsystem == RTE_UEVENT_SUBSYSTEM_UIO) {
+ if (event.action == RTE_UEVENT_REMOVE) {
+ _rte_eth_dev_callback_process(dev,
+ RTE_ETH_EVENT_INTR_RMV, NULL);
+ return;
+ }
+ }
+ goto done;
+ }
+
/* read out interrupt causes */
icr0 = I40E_READ_REG(hw, I40E_PFINT_ICR0);