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

[Simon Horman <horms@kernel.org>]

On Mon, Apr 22, 2024 at 10:41:47AM +0000, Kwapulinski, Piotr wrote:
> >-----Original Message-----
> >From: Simon Horman <horms@kernel.org> 
> >Sent: Saturday, April 20, 2024 8:18 PM
> >To: Kwapulinski, Piotr <piotr.kwapulinski@intel.com>
> >Cc: intel-wired-lan@lists.osuosl.org; netdev@vger.kernel.org; Gomes, Vinicius <vinicius.gomes@intel.com>; Wegrzyn, Stefan <stefan.wegrzyn@intel.com>; Jagielski, Jedrzej <jedrzej.jagielski@intel.com>; Sokolowski, Jan <jan.sokolowski@intel.com>
> >Subject: Re: [PATCH iwl-next v2 2/5] ixgbe: Add support for E610 device capabilities detection
> >
> >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?
> Hello,
> will do
> 
> >
> >> +	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
> Will do
> 
> >
> >> +
> >> +	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.
> Will do

Thanks. FWIIW, I think I noticed a similar cast at least once more
elsewhere in the patchset

> 
> >
> >> +
> >> +	return ixgbe_aci_send_cmd(hw, &desc, NULL, 0); }
> >
> >...
> Thank you for review
> Piotr
>