netdev - RE: [Intel-wired-lan] [PATCH iwl-next v1 3/5] ixgbe: Add link management support for E610 device

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: <DM6PR11MB4610AD8C2A75BC215FC0621FF3032@DM6PR11MB4610.namprd11.prod.outlook.com>
Date: Fri, 5 Apr 2024 15:10:55 +0000
From: "Kwapulinski, Piotr" <piotr.kwapulinski@...el.com>
To: "Gomes, Vinicius" <vinicius.gomes@...el.com>,
	"intel-wired-lan@...ts.osuosl.org" <intel-wired-lan@...ts.osuosl.org>
CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "Jagielski, Jedrzej"
	<jedrzej.jagielski@...el.com>, "Glaza, Jan" <jan.glaza@...el.com>, "Wegrzyn,
 Stefan" <stefan.wegrzyn@...el.com>
Subject: RE: [Intel-wired-lan] [PATCH iwl-next v1 3/5] ixgbe: Add link
 management support for E610 device

>-----Original Message-----
>From: Gomes, Vinicius <vinicius.gomes@...el.com> 
>Sent: Friday, April 5, 2024 2:15 AM
>To: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>; intel-wired-lan@...ts.osuosl.org
>Cc: Kwapulinski, Piotr <piotr.kwapulinski@...el.com>; netdev@...r.kernel.org; Jagielski, Jedrzej <jedrzej.jagielski@...el.com>; Glaza, Jan <jan.glaza@...el.com>; Wegrzyn, Stefan <stefan.wegrzyn@...el.com>
>Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 3/5] ixgbe: Add link management support for E610 device
>
>Piotr Kwapulinski <piotr.kwapulinski@...el.com> writes:
>
>> Add low level link management support for E610 device. Link management 
>> operations are handled via the Admin Command Interface. Add the 
>> following link management operations:
>> - get link capabilities
>> - set up link
>> - get media type
>> - get link status, link status events
>> - link power management
>>
>> 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 Glaza <jan.glaza@...el.com>
>> Signed-off-by: Piotr Kwapulinski <piotr.kwapulinski@...el.com>
>> ---
>
>[...]
>
>> +/**
>> + * ixgbe_update_link_info - update status of the HW network link
>> + * @hw: pointer to the HW struct
>> + *
>> + * Update the status of the HW network link.
>> + *
>> + * Return: the exit code of the operation.
>> + */
>> +int ixgbe_update_link_info(struct ixgbe_hw *hw) {
>> +	struct ixgbe_link_status *li;
>> +	int err;
>> +
>> +	if (!hw)
>> +		return -EINVAL;
>> +
>> +	li = &hw->link.link_info;
>> +
>> +	err = ixgbe_aci_get_link_info(hw, true, NULL);
>> +	if (err)
>> +		return err;
>> +
>> +	if (li->link_info & IXGBE_ACI_MEDIA_AVAILABLE) {
>> +		struct ixgbe_aci_cmd_get_phy_caps_data __free(kfree) *pcaps;
>> +
>> +		pcaps =	kzalloc(sizeof(*pcaps), GFP_KERNEL);
>> +		if (!pcaps)
>> +			return -ENOMEM;
>> +
>
>Seems that 'pcaps' is leaking here.
Do you think that __free() does not do the job here ?

>
>> +		err = ixgbe_aci_get_phy_caps(hw, false,
>> +					     IXGBE_ACI_REPORT_TOPO_CAP_MEDIA,
>> +					     pcaps);
>> +
>> +		if (!err)
>> +			memcpy(li->module_type, &pcaps->module_type,
>> +			       sizeof(li->module_type));
>> +	}
>> +
>> +	return err;
>> +}
>> +
>[...]
>
>> +/**
>> + * ixgbe_get_media_type_e610 - Gets media type
>> + * @hw: pointer to the HW struct
>> + *
>> + * In order to get the media type, the function gets PHY
>> + * capabilities and later on use them to identify the PHY type
>> + * checking phy_type_high and phy_type_low.
>> + *
>> + * Return: the type of media in form of ixgbe_media_type enum
>> + * or ixgbe_media_type_unknown in case of an error.
>> + */
>> +enum ixgbe_media_type ixgbe_get_media_type_e610(struct ixgbe_hw *hw) 
>> +{
>> +	struct ixgbe_aci_cmd_get_phy_caps_data pcaps;
>> +	int rc;
>> +
>> +	rc = ixgbe_update_link_info(hw);
>> +	if (rc)
>> +		return ixgbe_media_type_unknown;
>> +
>> +	/* If there is no link but PHY (dongle) is available SW should use
>> +	 * Get PHY Caps admin command instead of Get Link Status, find most
>> +	 * significant bit that is set in PHY types reported by the command
>> +	 * and use it to discover media type.
>> +	 */
>> +	if (!(hw->link.link_info.link_info & IXGBE_ACI_LINK_UP) &&
>> +	    (hw->link.link_info.link_info & IXGBE_ACI_MEDIA_AVAILABLE)) {
>> +		u64 phy_mask;
>> +		u8 i;
>> +
>> +		/* Get PHY Capabilities */
>> +		rc = ixgbe_aci_get_phy_caps(hw, false,
>> +					    IXGBE_ACI_REPORT_TOPO_CAP_MEDIA,
>> +					    &pcaps);
>> +		if (rc)
>> +			return ixgbe_media_type_unknown;
>> +
>> +		/* Check if there is some bit set in phy_type_high */
>> +		for (i = 64; i > 0; i--) {
>> +			phy_mask = (u64)((u64)1 << (i - 1));
>> +			if ((pcaps.phy_type_high & phy_mask) != 0) {
>> +				/* If any bit is set treat it as PHY type */
>> +				hw->link.link_info.phy_type_high = phy_mask;
>> +				hw->link.link_info.phy_type_low = 0;
>> +				break;
>> +			}
>> +			phy_mask = 0;
>> +		}
>> +
>> +		/* If nothing found in phy_type_high search in phy_type_low */
>> +		if (phy_mask == 0) {
>> +			for (i = 64; i > 0; i--) {
>> +				phy_mask = (u64)((u64)1 << (i - 1));
>> +				if ((pcaps.phy_type_low & phy_mask) != 0) {
>> +					/* Treat as PHY type is any bit set */
>> +					hw->link.link_info.phy_type_high = 0;
>> +					hw->link.link_info.phy_type_low = phy_mask;
>> +					break;
>> +				}
>> +			}
>> +		}
>
>These two look like they are doing something very similar to fls64().
>Could that work?

Will try to apply.
Thank you for review.
Piotr

>
>> +
>> +		/* Based on search above try to discover media type */
>> +		hw->phy.media_type = ixgbe_get_media_type_from_phy_type(hw);
>> +	}
>> +
>> +	return hw->phy.media_type;
>> +}
>> +
>
>
>--
>Vinicius
>