Hello Ferruh,
> On Dec 15, 2020, at 8:42 AM, Ferruh Yigit <ferruh.yi...@intel.com> wrote:
>
> On 12/10/2020 2:22 PM, Andrew Boyer wrote:
>> This is now required behavior for a PMD.
>> Remove the UNMAINTAINED flag.
>> Signed-off-by: Andrew Boyer <abo...@pensando.io>
>> ---
>> MAINTAINERS | 2 +-
>> drivers/net/ionic/ionic_ethdev.c | 41 ++++++++++++++++----------------
>> drivers/net/ionic/ionic_lif.c | 15 ++++++++++++
>> drivers/net/ionic/ionic_lif.h | 1 +
>> 4 files changed, 38 insertions(+), 21 deletions(-)
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 7bc0010f2..fc1b09923 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -840,7 +840,7 @@ F: doc/guides/nics/pfe.rst
>> F: drivers/net/pfe/
>> F: doc/guides/nics/features/pfe.ini
>> -Pensando ionic - UNMAINTAINED
>> +Pensando ionic
>> M: Andrew Boyer <abo...@pensando.io>
>> F: drivers/net/ionic/
>> F: doc/guides/nics/ionic.rst
>> diff --git a/drivers/net/ionic/ionic_ethdev.c
>> b/drivers/net/ionic/ionic_ethdev.c
>> index 5a360ac08..629d7068b 100644
>> --- a/drivers/net/ionic/ionic_ethdev.c
>> +++ b/drivers/net/ionic/ionic_ethdev.c
>> @@ -958,6 +958,8 @@ ionic_dev_stop(struct rte_eth_dev *eth_dev)
>> return err;
>> }
>> +static void ionic_unconfigure_intr(struct ionic_adapter *adapter);
>> +
>> /*
>> * Reset and stop device.
>> */
>> @@ -965,6 +967,8 @@ static int
>> ionic_dev_close(struct rte_eth_dev *eth_dev)
>> {
>> struct ionic_lif *lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>> + struct ionic_adapter *adapter = lif->adapter;
>> + uint32_t i;
>> int err;
>> IONIC_PRINT_CALL();
>> @@ -977,12 +981,21 @@ ionic_dev_close(struct rte_eth_dev *eth_dev)
>> return -1;
>> }
>> - err = eth_ionic_dev_uninit(eth_dev);
>> - if (err) {
>> - IONIC_PRINT(ERR, "Cannot destroy LIF: %d", err);
>> - return -1;
>> + ionic_lif_free_queues(lif);
>> +
>> + IONIC_PRINT(NOTICE, "Removing device %s", eth_dev->device->name);
>> + ionic_unconfigure_intr(adapter);
>> +
>> + for (i = 0; i < adapter->nlifs; i++) {
>> + lif = adapter->lifs[i];
>> + rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit);
>
> Should 'ionic_lif_stop()' & 'ionic_lif_free_queues()' be called per 'lif'
> here?
You are correct. A future patch removes multi-lif support - it is unused. So
this is a result of how I broke up the changes. See below.
>> }
>> + ionic_port_reset(adapter);
>> + ionic_reset(adapter);
>> +
>> + rte_free(adapter);
>> +
>
> In ionic, an ethdev is created per 'lif' during probe and when one of the
> ethdev closed, all 'lif' destroyed and 'adapter' freed, so all ethdev should
> be unusable at this stage.
> 1) First better to document this behavior in the commit log, and as overall
> can you please prefer more descriptive commit logs.
Sure, I will add more detail to commit logs.
> 2) What happens to the ethdev statuses, 'lif' destroyed and ethdev are not
> usable but ethdev status not updated or freed?
> What happens if the application tries to access to ethdev of another 'lif'
> after 'ionic_dev_close()'?
I see what you’re saying, we get here from eth_dev_ops.dev_close ->
ionic_dev_close(), so the first ethdev to get closed brings down the adapter
etc.
In reality we are going to have one adapter <-> one lif <-> one ethdev, so
closing the ethdev will be the end of everything. (Just for this PF/VF; they
are independent.)
Rather than doing any design work to fix this I will reorder the patches to
take out multi-lif support first.
-Andrew
>
>> return 0;
>> }
>> @@ -1280,10 +1293,7 @@ static int
>> eth_ionic_pci_remove(struct rte_pci_device *pci_dev __rte_unused)
>> {
>> char name[RTE_ETH_NAME_MAX_LEN];
>> - struct ionic_adapter *adapter = NULL;
>> struct rte_eth_dev *eth_dev;
>> - struct ionic_lif *lif;
>> - uint32_t i;
>> /* Adapter lookup is using (the first) eth_dev name */
>> snprintf(name, sizeof(name), "net_%s_lif_0",
>> @@ -1291,19 +1301,10 @@ eth_ionic_pci_remove(struct rte_pci_device *pci_dev
>> __rte_unused)
>>
>
> Should remove '__rte_unused'
OK
>
>> eth_dev = rte_eth_dev_allocated(name);
>> if (eth_dev) {
>> - lif = IONIC_ETH_DEV_TO_LIF(eth_dev);
>> - adapter = lif->adapter;
>> - }
>> -
>> - if (adapter) {
>> - ionic_unconfigure_intr(adapter);
>> -
>> - for (i = 0; i < adapter->nlifs; i++) {
>> - lif = adapter->lifs[i];
>> - rte_eth_dev_destroy(lif->eth_dev, eth_ionic_dev_uninit);
>> - }
>> -
>> - rte_free(adapter);
>> + ionic_dev_close(eth_dev);
>> + } else {
>> + IONIC_PRINT(WARNING, "Cannot find device %s",
>> + pci_dev->device.name);
>
> Not sure if this warning is correct, if there is no allocated ethdev this is
> not an error, this means there is nothing to remove and function can continue
> with 'return 0'
OK, I will reduce it to DEBUG level.
>> }
>> return 0;
>> diff --git a/drivers/net/ionic/ionic_lif.c b/drivers/net/ionic/ionic_lif.c
>> index 875c7e585..e213597ee 100644
>> --- a/drivers/net/ionic/ionic_lif.c
>> +++ b/drivers/net/ionic/ionic_lif.c
>> @@ -927,6 +927,21 @@ ionic_lif_free(struct ionic_lif *lif)
>> }
>> }
>> +void
>> +ionic_lif_free_queues(struct ionic_lif *lif)
>> +{
>> + uint32_t i;
>> +
>> + for (i = 0; i < lif->ntxqcqs; i++) {
>> + ionic_dev_tx_queue_release(lif->eth_dev->data->tx_queues[i]);
>> + lif->eth_dev->data->tx_queues[i] = NULL;
>> + }
>> + for (i = 0; i < lif->nrxqcqs; i++) {
>> + ionic_dev_rx_queue_release(lif->eth_dev->data->rx_queues[i]);
>> + lif->eth_dev->data->rx_queues[i] = NULL;
>> + }
>> +}
>> +
>> int
>> ionic_lif_rss_config(struct ionic_lif *lif,
>> const uint16_t types, const uint8_t *key, const uint32_t *indir)
>> diff --git a/drivers/net/ionic/ionic_lif.h b/drivers/net/ionic/ionic_lif.h
>> index b80931c61..bf010716e 100644
>> --- a/drivers/net/ionic/ionic_lif.h
>> +++ b/drivers/net/ionic/ionic_lif.h
>> @@ -122,6 +122,7 @@ int ionic_lifs_size(struct ionic_adapter *ionic);
>> int ionic_lif_alloc(struct ionic_lif *lif);
>> void ionic_lif_free(struct ionic_lif *lif);
>> +void ionic_lif_free_queues(struct ionic_lif *lif);
>> int ionic_lif_init(struct ionic_lif *lif);
>> void ionic_lif_deinit(struct ionic_lif *lif);
