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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DM6PR11MB46106D8FD747CC3D975524C1F3FF2@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Mon, 3 Jun 2024 12:58:36 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: Simon Horman <horms@...nel.org>
CC: "intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Keller, Jacob E"
	<jacob.e.keller@...el.com>, "Wegrzyn, Stefan" <stefan.wegrzyn@...el.com>,
	"Jagielski, Jedrzej" <jedrzej.jagielski@...el.com>, "Sokolowski, Jan"
	<jan.sokolowski@...el.com>
Subject: RE: [PATCH iwl-next v7 2/7] ixgbe: Add support for E610 device
 capabilities detection

>-----Original Message-----
>From: Simon Horman <horms@...nel.org> 
>Sent: Friday, May 31, 2024 4:50 PM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>
>Cc: intel-wired-lan@...ts.osuosl.org; netdev@...r.kernel.org; Keller, Jacob E <jacob.e.keller@...el.com>; Wegrzyn, Stefan <stefan.wegrzyn@...el.com>; Jagielski, Jedrzej <jedrzej.jagielski@...el.com>; Sokolowski, Jan <jan.sokolowski@...el.com>
>Subject: Re: [PATCH iwl-next v7 2/7] ixgbe: Add support for E610 device capabilities detection
>
>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@...el.com>
>> Signed-off-by: Stefan Wegrzyn <stefan.wegrzyn@...el.com>
>> Co-developed-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Signed-off-by: Jedrzej Jagielski <jedrzej.jagielski@...el.com>
>> Reviewed-by: Jan Sokolowski <jan.sokolowski@...el.com>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.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()
Hello Simon
Will do
Thank you for feedback

>
>> +}
>> +
>> +/**
>> + * 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;
>> +}
>