Re: [PATCH iwl-next v7 2/7] ixgbe: Add support for E610 device capabilities detection

[Simon Horman <horms@kernel.org>]

On Mon, May 27, 2024 at 05:10:18PM +0200, Piotr Kwapulinski wrote:
> Add low level support for E610 device capabilities detection. The
> capabilities are discovered via the Admin Command Interface. Discover the
> following capabilities:
> - function caps: vmdq, dcb, rss, rx/tx qs, msix, nvm, orom, reset
> - device caps: vsi, fdir, 1588
> - phy caps
> 
> Co-developed-by: Stefan Wegrzyn <stefan.wegrzyn@intel.com>
> Signed-off-by: Stefan Wegrzyn <stefan.wegrzyn@intel.com>
> Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@intel.com>
> Reviewed-by: Jan Sokolowski <jan.sokolowski@intel.com>
> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@intel.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c | 517 ++++++++++++++++++
>  drivers/net/ethernet/intel/ixgbe/ixgbe_e610.h |  11 +
>  2 files changed, 528 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_e610.c

...

> +/**
> + * ixgbe_discover_dev_caps - Read and extract device capabilities
> + * @hw: pointer to the hardware structure
> + * @dev_caps: pointer to device capabilities structure
> + *
> + * Read the device capabilities and extract them into the dev_caps structure
> + * for later use.
> + *
> + * Return: the exit code of the operation.
> + */
> +int ixgbe_discover_dev_caps(struct ixgbe_hw *hw,
> +			    struct ixgbe_hw_dev_caps *dev_caps)
> +{
> +	u8 *cbuf __free(kfree);
> +	u32 cap_count;
> +	int err;
> +
> +	cbuf = kzalloc(IXGBE_ACI_MAX_BUFFER_SIZE, GFP_KERNEL);
> +	if (!cbuf)
> +		return -ENOMEM;
> +	/* Although the driver doesn't know the number of capabilities the
> +	 * device will return, we can simply send a 4KB buffer, the maximum
> +	 * possible size that firmware can return.
> +	 */
> +	cap_count = IXGBE_ACI_MAX_BUFFER_SIZE /
> +		    sizeof(struct ixgbe_aci_cmd_list_caps_elem);
> +
> +	err = ixgbe_aci_list_caps(hw, cbuf, IXGBE_ACI_MAX_BUFFER_SIZE,
> +				  &cap_count,
> +				  ixgbe_aci_opc_list_dev_caps);
> +	if (!err)
> +		ixgbe_parse_dev_caps(hw, dev_caps, cbuf, cap_count);
> +
> +	return err;

Hi Piotr, all,

A minor nit from my side.

It would be more idiomatic to write this such that the main flow of
execution is the non-error path, while errors are handled by conditions. In
this case, something like this (completely untested!):

	err = ixgbe_aci_list_caps(hw, cbuf, IXGBE_ACI_MAX_BUFFER_SIZE,
				  &cap_count,
				  ixgbe_aci_opc_list_dev_caps);
	if (err)
		return err;

	ixgbe_parse_dev_caps(hw, dev_caps, cbuf, cap_count);

	return 0;

Likewise in ixgbe_discover_func_caps()

> +}
> +
> +/**
> + * ixgbe_discover_func_caps - Read and extract function capabilities
> + * @hw: pointer to the hardware structure
> + * @func_caps: pointer to function capabilities structure
> + *
> + * Read the function capabilities and extract them into the func_caps structure
> + * for later use.
> + *
> + * Return: the exit code of the operation.
> + */
> +int ixgbe_discover_func_caps(struct ixgbe_hw *hw,
> +			     struct ixgbe_hw_func_caps *func_caps)
> +{
> +	u8 *cbuf __free(kfree);
> +	u32 cap_count;
> +	int err;
> +
> +	cbuf = kzalloc(IXGBE_ACI_MAX_BUFFER_SIZE, GFP_KERNEL);
> +	if (!cbuf)
> +		return -ENOMEM;
> +
> +	/* Although the driver doesn't know the number of capabilities the
> +	 * device will return, we can simply send a 4KB buffer, the maximum
> +	 * possible size that firmware can return.
> +	 */
> +	cap_count = IXGBE_ACI_MAX_BUFFER_SIZE /
> +		    sizeof(struct ixgbe_aci_cmd_list_caps_elem);
> +
> +	err = ixgbe_aci_list_caps(hw, cbuf, IXGBE_ACI_MAX_BUFFER_SIZE,
> +				  &cap_count,
> +				  ixgbe_aci_opc_list_func_caps);
> +	if (!err)
> +		ixgbe_parse_func_caps(hw, func_caps, cbuf, cap_count);
> +
> +	return err;
> +}

...