RE: [PATCH v7 04/12] net/nfp: add initial flower firmware support

[Chaoyong He <chaoyong.he@corigine.com>]

> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@xilinx.com>
> Sent: Monday, September 5, 2022 11:40 PM
> To: Chaoyong He <chaoyong.he@corigine.com>; dev@dpdk.org
> Cc: oss-drivers <oss-drivers@corigine.com>; Niklas Soderlund
> <niklas.soderlund@corigine.com>; Heinrich Kuhn
> <heinrich.kuhn@corigine.com>
> Subject: Re: [PATCH v7 04/12] net/nfp: add initial flower firmware support
> 
> On 8/12/2022 11:22 AM, Chaoyong He wrote:
> > Adds the basic probing infrastructure to support the flower firmware.
> > This firmware is geared towards offloading OVS and can generally be
> > found in /lib/firmware/netronome/flower. It is also used by the NFP
> > kernel driver when OVS offload with TC is desired.
> 
> '/lib/firmware/netronome/flower' FW is loaded (automatically ?) by kernel
> driver, right?
> Is there anything related to the DPDK with the name/path of the FW?
> 
> 
> And I wonder if these kind of information should be part of driver
> documentation?
> 
> <...>
> 
> > @@ -965,6 +968,16 @@
> >   			goto hwqueues_cleanup;
> >   		}
> >   		break;
> > +	case NFP_APP_FLOWER_NIC:
> > +		PMD_INIT_LOG(INFO, "Initializing Flower");
> > +		pci_dev->device.driver = &pci_drv->driver;
> 
> Why this assignment is required? Driver shouldn't need to update this
> bus related data struct.

Following the framework's logic: 
function 'rte_pci_probe_one_driver()' in file drivers/bus/pci/pci_common.c

```
...
ret = dr->probe(dr, dev);    // here call our nfp_pf_pci_probe()
if (ret) {
...
} else {
        dev->device.driver = &dr->driver;
}

```

Here the framework will do the assignment.

But in our logic, if we won't add this assignment, we will run into a coredump:
```
Program terminated with signal SIGSEGV, Segmentation fault.
#0  rte_eth_dev_info_get (port_id=0, dev_info=0x7ffe9c3d3f70) at ../lib/ethdev/rte_ethdev.c:3138
3138            dev_info->driver_name = dev->device->driver->name;
(gdb) bt
#0  rte_eth_dev_info_get (port_id=0, dev_info=0x7ffe9c3d3f70) at ../lib/ethdev/rte_ethdev.c:3138
#1  0x00007fa95128565f in rte_eth_dev_configure (port_id=0, nb_rx_q=1, nb_tx_q=1,
    dev_conf=0x7fa94ce338e0 <port_conf>) at ../lib/ethdev/rte_ethdev.c:1110
#2  0x00007fa94cdf74b7 in nfp_flower_init_ctrl_vnic (hw=0x2001e8530)
    at ../drivers/net/nfp/flower/nfp_flower.c:1035
#3  0x00007fa94cdf7b32 in nfp_init_app_fw_flower (pf_dev=0x2001e8c80)
    at ../drivers/net/nfp/flower/nfp_flower.c:1219
#4  0x00007fa94ce30e33 in nfp_pf_init (pci_dev=0xcd22c0, pci_drv=0x7fa94ce44460 <rte_nfp_net_pf_pmd>)
    at ../drivers/net/nfp/nfp_ethdev.c:976
#5  0x00007fa94ce31429 in nfp_pf_pci_probe (pci_drv=0x7fa94ce44460 <rte_nfp_net_pf_pmd>, dev=0xcd22c0)
    at ../drivers/net/nfp/nfp_ethdev.c:1152
#6  0x00007fa94e8f8f34 in rte_pci_probe_one_driver (dr=0x7fa94ce44460 <rte_nfp_net_pf_pmd>, dev=0xcd22c0)
    at ../drivers/bus/pci/pci_common.c:269
#7  0x00007fa94e8f91c0 in pci_probe_all_drivers (dev=0xcd22c0) at ../drivers/bus/pci/pci_common.c:353
#8  0x00007fa94e8f9244 in pci_probe () at ../drivers/bus/pci/pci_common.c:380
#9  0x00007fa951163c0e in rte_bus_probe () at ../lib/eal/common/eal_common_bus.c:72
#10 0x00007fa951193903 in rte_eal_init (argc=10, argv=0xcc5150) at ../lib/eal/linux/eal.c:1279
#11 0x0000000000601531 in dpdk_init__ (ovs_other_config=0xccefd0) at lib/dpdk.c:466
#12 0x0000000000601913 in dpdk_init (ovs_other_config=0xccefd0) at lib/dpdk.c:545
#13 0x00000000004129ed in bridge_run () at vswitchd/bridge.c:3252
#14 0x0000000000418364 in main (argc=11, argv=0x7ffe9c3d49c8) at vswitchd/ovs-vswitchd.c:129

```

Our nfp card use `control message` to exchange message between PMD and firmware when we use flower firmware.
The control message is in the form of pkt and we use a `ctrl vNIC` ehtdev as the agent to send and receive these pkts.
e.g., if we want to create representor port, the PMD must send the corresponding control message to firmware.
To be able to send and receive pkt, we must do some configure steps to this ethdev firstly.
But the framework has not do the assignment step at this moment.
And this is where the problem comes from.

Logically, if our probe process is successful, assignment again won't import any problem.
If our probe process failed, our logic will undo the assignment and the framework will go as its original logic.
So maybe we can keep the logic now?
Or there exist another way more standard?