18/01/2022 13:12, Ferruh Yigit: > On 1/18/2022 11:21 AM, kumaraparameshwaran rathinavel wrote: > > Comment moved down. > > Please don't top post, it makes very hard to follow the discussion and bad > for archives to visit discussion later. > > > > > On Tue, Jan 18, 2022 at 3:17 PM Ferruh Yigit <ferruh.yi...@intel.com > > <mailto:ferruh.yi...@intel.com>> wrote: > > > > On 1/17/2022 6:33 PM, Thomas Monjalon wrote: > > > 17/01/2022 19:28, Ferruh Yigit: > > >>> + ret = rte_eth_dev_get_port_by_name(request_param->port_name, > > &port_id); > > >>> + if (ret) { > > >>> + TAP_LOG(ERR, "Failed to get port id for %s", > > >>> + request_param->port_name); > > >>> + return -1; > > >>> + } > > >>> + dev = &rte_eth_devices[port_id]; > > >> > > >> Since this is not really related with your patch, I want to have a > > separate thread for it. > > >> > > >> It is not good to access the 'rte_eth_devices' global variable > > directly from a driver, that > > >> is error prone. > > >> > > >> Btw, what 'peer' supposed to contain? > > >> > > >> It can be solved by adding an internal API, only for drivers to get > > eth_dev from the name, > > >> like: 'rte_eth_dev_get_by_name()'. > > >> This way a few other usage can be converted to this API. > > >> > > >> @Thomas and @Andrew what do you think about the new API proposal? > > > > > > It looks similar to rte_eth_dev_get_port_by_name() which returns a > > port_id. > > > > Exactly, but get eth_dev directly for drivers. For drivers no need to > > work with port_id > > handler, they can use eth_dev directly. > > > > Another solution can be an getter function for drivers, which gets > > port_id and returns > > the eth_dev. > > > > > It is a bit strange for an ethdev driver to not have access to its > > own ethdev struct. > > > Isn't there something broken in the logic? > > > > > > > This is callback function between primary and secondary applications > > sync. So port name > > will be same for both, but eth_dev will be different and port_id may be > > different. > > Driver finds its own eth_dev from the shared port name. > > > > > Just wanted to bring it to your attention, > > > > In Mellanox driver there is a requirement to exchange fds between primary > > and secondary and similar usage is seen, the primary sends the port_id and > > the secondary refers to the rte_eth_devices in the driver, > > The functions are > > - mlx5_mp_secondary_handle in secondary > > - mlx5_mp_req_start_rxtx in primary which is invoked from > > mlx5_dev_start. > > > > In my implementation I have used the name and invoked get_port_by_name, I > > can also pass the port_id from the primary to make it uniform. So with > > similar usage in Mellanox is there a problem there as well on referring to > > the rte_eth_devices from the PMD ? > > > > It would be same, still will be accessing to the 'rte_eth_devices'. > That is why a new API for drivers may help.
I agree to add a new API if needed to remove those direct access to rte_eth_devices.
