Re: [dpdk-dev] [PATCH v4 07/22] net/atlantic: rte device start, stop, initial configuration

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/9/2018 10:31 AM, Igor Russkikh wrote:
> From: Pavel Belous <Pavel.Belous@aquantia.com>
> 
> Start, stop and reset are all done via hw_atl layer.
> Link interrupt configuration is also done here.
> 
> Signed-off-by: Igor Russkikh <igor.russkikh@aquantia.com>
> Signed-off-by: Pavel Belous <pavel.belous@aquantia.com>
> Signed-off-by: Pavel Belous <Pavel.Belous@aquantia.com>
<...>

> +static void
> +atl_print_adapter_info(struct aq_hw_s *hw __rte_unused)

"hw" is used below, you can drop __rte_unused

<...>

> +	/* For secondary processes, the primary process has done all the work */
> +	if (rte_eal_process_type() != RTE_PROC_PRIMARY)
> +		return 0;
> +
> +	rte_eth_copy_pci_info(eth_dev, pci_dev);

This shouldn't be required, done in rte_eth_dev_pci_allocate() just before
calling this function, can you please double check?

<...>

> +	/* Copy the permanent MAC address */
> +	if (hw->aq_fw_ops->get_mac_permanent(hw,
> +			(u8 *)&eth_dev->data->mac_addrs[0]) != 0)

You can use "eth_dev->data->mac_addrs->addr_bytes" to prevent casting but both
same eventually

<...>

> +#ifdef RTE_LIBRTE_SECURITY
> +	rte_free(eth_dev->security_ctx);
> +#endif

I think "eth_dev->security_ctx" should be allocated in the driver, if you are
not allocating it, you don't need to free it.

<...>

>  static void
> -atl_dev_stop(struct rte_eth_dev *dev __rte_unused)
> +atl_dev_stop(struct rte_eth_dev *dev)
>  {
> +	struct aq_hw_s *hw =
> +		ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +
> +	/* reset the NIC */
> +	atl_reset_hw(hw);
> +	hw->adapter_stopped = 0;

Just to double check, below in close() there is an "atl_dev_stop(dev);" called,
which look like for stop, should it be called here too?

And the state "hw->adapter_stopped", should it be "0" or "1" when device stopped?

<...>

> +static int
> +atl_fw_version_get(struct rte_eth_dev *dev, char *fw_version, size_t fw_size)
> +{
> +	struct aq_hw_s *hw = ATL_DEV_PRIVATE_TO_HW(dev->data->dev_private);
> +	uint32_t fw_ver = 0;
> +	unsigned int ret = 0;
> +
> +	ret = hw_atl_utils_get_fw_version(hw, &fw_ver);
> +	if (ret)
> +		return 0;

Why not return error for this case?