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

[Simon Horman <horms@kernel.org>]

On Mon, Apr 15, 2024 at 12:34:32PM +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>

Hi Pitor,

A few minor nits from my side.
No need to respin just because of these.

> ---
>  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_get_num_per_func - determine number of resources per PF
> + * @hw: pointer to the HW structure
> + * @max: value to be evenly split between each PF
> + *
> + * Determine the number of valid functions by going through the bitmap returned
> + * from parsing capabilities and use this to calculate the number of resources
> + * per PF based on the max value passed in.
> + *
> + * Return: the number of resources per PF or 0, if no PH are available.
> + */
> +static u32 ixgbe_get_num_per_func(struct ixgbe_hw *hw, u32 max)
> +{
> +	const u32 IXGBE_CAPS_VALID_FUNCS_M = 0xFF;

nit: Maybe this could simply be a #define?

> +	u8 funcs = hweight8(hw->dev_caps.common_cap.valid_functions &
> +			    IXGBE_CAPS_VALID_FUNCS_M);

nit: Please consider using reverse xmas tree order - longest line to shortest -
     for local variables in new Networking code

> +
> +	return funcs ? (max / funcs) : 0;
> +}

...

> +/**
> + * ixgbe_aci_disable_rxen - disable RX
> + * @hw: pointer to the HW struct
> + *
> + * Request a safe disable of Receive Enable using ACI command (0x000C).
> + *
> + * Return: the exit code of the operation.
> + */
> +int ixgbe_aci_disable_rxen(struct ixgbe_hw *hw)
> +{
> +	struct ixgbe_aci_cmd_disable_rxen *cmd;
> +	struct ixgbe_aci_desc desc;
> +
> +	cmd = &desc.params.disable_rxen;
> +
> +	ixgbe_fill_dflt_direct_cmd_desc(&desc, ixgbe_aci_opc_disable_rxen);
> +
> +	cmd->lport_num = (u8)hw->bus.func;

nit: This cast seems unnecessary.
     AFAICT the type of hw->bus.func is u8.

> +
> +	return ixgbe_aci_send_cmd(hw, &desc, NULL, 0);
> +}

...