>-----Original Message-----
>From: Gomes, Vinicius <[email protected]>
>Sent: Friday, April 5, 2024 2:15 AM
>To: Kwapulinski, Piotr <[email protected]>;
>[email protected]
>Cc: Kwapulinski, Piotr <[email protected]>; [email protected];
>Jagielski, Jedrzej <[email protected]>; Glaza, Jan
><[email protected]>; Wegrzyn, Stefan <[email protected]>
>Subject: Re: [Intel-wired-lan] [PATCH iwl-next v1 3/5] ixgbe: Add link
>management support for E610 device
>
>Piotr Kwapulinski <[email protected]> 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 <[email protected]>
>> Signed-off-by: Stefan Wegrzyn <[email protected]>
>> Co-developed-by: Jedrzej Jagielski <[email protected]>
>> Signed-off-by: Jedrzej Jagielski <[email protected]>
>> Reviewed-by: Jan Glaza <[email protected]>
>> Signed-off-by: Piotr Kwapulinski <[email protected]>
>> ---
>
>[...]
>
>> +/**
>> + * 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
>
