Re: [PATCH] net/bonding: make bonded device configure method re-entrant

[Chas Williams <3chas3@gmail.com>]

On 10/17/22 10:10, Andrew Rybchenko wrote:
> On 10/17/22 15:32, Chas Williams wrote:
>> This appears to be correct. A minor comment inline.
>>
>> On 10/17/22 04:42, Andrew Rybchenko wrote:
>>> Chas, Cornor, could you review the patch, please.
>>>
>>> Thanks,
>>> Andrew.
>>>
>>> On 9/11/22 15:24, Ivan Malov wrote:
>>>> According to the documentation, rte_eth_dev_configure()
>>>> can be invoked repeatedly while in stopped state.
>>>> The current implementation in the bonding driver
>>>> allows for that (technically), but the user sees
>>>> warnings which say that back-end devices have
>>>> already been harnessed. Re-factor the code
>>>> to have cleanup before each (re-)configure.
>>>>
>>>> Signed-off-by: Ivan Malov <ivan.malov@oktetlabs.ru>
>>>> Reviewed-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> ---
>>>>   drivers/net/bonding/rte_eth_bond_pmd.c | 27 
>>>> ++++++++++++++++++--------
>>>>   1 file changed, 19 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/net/bonding/rte_eth_bond_pmd.c 
>>>> b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> index d01c954296..92a33b45bd 100644
>>>> --- a/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> +++ b/drivers/net/bonding/rte_eth_bond_pmd.c
>>>> @@ -2143,18 +2143,13 @@ bond_ethdev_stop(struct rte_eth_dev *eth_dev)
>>>>       return 0;
>>>>   }
>>>> -int
>>>> -bond_ethdev_close(struct rte_eth_dev *dev)
>>>> +static void
>>>> +bond_ethdev_cfg_cleanup(struct rte_eth_dev *dev)
>>>>   {
>>>>       struct bond_dev_private *internals = dev->data->dev_private;
>>>>       uint16_t bond_port_id = internals->port_id;
>>>> -    int skipped = 0;
>>>>       struct rte_flow_error ferror;
>>>> -
>>>> -    if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>> -        return 0;
>>>> -
>>>> -    RTE_BOND_LOG(INFO, "Closing bonded device %s", dev->device->name);
>>>> +    int skipped = 0;
>>>>       /* Flush flows in all back-end devices before removing them */
>>>>       bond_flow_ops.flush(dev, &ferror);
>>>> @@ -2176,6 +2171,20 @@ bond_ethdev_close(struct rte_eth_dev *dev)
>>>>               skipped++;
>>>>           }
>>>>       }
>>>> +}
>>>> +
>>>> +int
>>>> +bond_ethdev_close(struct rte_eth_dev *dev)
>>>> +{
>>>> +    struct bond_dev_private *internals = dev->data->dev_private;
>>>> +
>>>> +    if (rte_eal_process_type() != RTE_PROC_PRIMARY)
>>>> +        return 0;
>>>> +
>>>> +    RTE_BOND_LOG(INFO, "Closing bonded device %s", dev->device->name);
>>>> +
>>>> +    bond_ethdev_cfg_cleanup(dev);
>>>> +
>>>>       bond_ethdev_free_queues(dev);
>>>>       rte_bitmap_reset(internals->vlan_filter_bmp);
>>>>       rte_bitmap_free(internals->vlan_filter_bmp);
>>>> @@ -3606,6 +3615,8 @@ bond_ethdev_configure(struct rte_eth_dev *dev)
>>>>       unsigned i, j;
>>>> +    bond_ethdev_cfg_cleanup(dev);
>>>> +
>>
>> You might want a space after the variable declaration section. It makes
>> it easier to read.
> 
> I can fix it on applying. May I add your ack?

Acked-by: Chas Williams <3chas3@gmail.com>

>>
>>>>       /*
>>>>        * If RSS is enabled, fill table with default values and
>>>>        * set key to the value specified in port RSS configuration.
>>>
>