netdev - Re: i40e - Is i40e_force_link_state doing the right thing ?

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]
Date:   Tue, 15 May 2018 14:47:17 -0700
From:   Chaitanya Lala <chaitanya.lala@...il.com>
To:     "Stachura, Mariusz" <mariusz.stachura@...el.com>
Cc:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Williams, Mitch A" <mitch.a.williams@...el.com>,
        "Bowers, AndrewX" <andrewx.bowers@...el.com>,
        "Kirsher, Jeffrey T" <jeffrey.t.kirsher@...el.com>
Subject: Re: i40e - Is i40e_force_link_state doing the right thing ?

Hi Mariusz, ...

On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz
<mariusz.stachura@...el.com> wrote:
> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@...il.com> wrote:
>> Hi,
>>
>> I am trying to bring up a Intel XL710 4x10G Intel card using the
>> latest mainline top-of-tree.
>> The problem is that "ifconfig up" and "ifconfig down" do not take
>> effect at the link state level.
>> I tracked the problem down to i40e_force_link_state() when it is
>> called from i40e_down().
>> It calls i40e_force_link_state with "is_up" == false. In-turn it
>> calls, i40e_aq_set_link_restart_an(hw, true, NULL).
>>
>> Should the second argument of  i40e_aq_set_link_restart_an be "is_up"
>> vs the current "true"
>> i.e. i40e_aq_set_link_restart_an(hw, is_up, NULL). ? When I make this
>> change, the link state syncs-up with the interface administrative
>> state.
>>
>> Is this a bug ?
>>
>> Thanks,
>>  Chaitanya
>
> Hello Chaitanya,
>
> i40e_down() calls i40e_force_link_state with "is_up" == false only if interface's private flag "link-down-on-close" is set. By default the link is left up for manageability and VF traffic, user can use this flag to power down the interface on the link level. Does that work for you?
> The command is:
> "ethtool --set-priv-flags IFNAME link-down-on-close on" and then

This flag is _on_ in my setup and hencet i40e_force_link_state is
being called with is_up == false in my setup. The problem is that
irrespective of
value of "is_up" flag, i40e_force_link_state invokes
i40e_aq_set_link_restart_an with second argument (enable_link) as
"true". So i40e_aq_set_link_restart_an
is always trying to enable link even if is_up was false. Is that
correct behavior ?

I have pasted code with my annotations below marked with "//XXX".

/**
 * i40e_force_link_state - Force the link status
 * @pf: board private structure
 * @is_up: whether the link state should be forced up or down
 **/
static i40e_status i40e_force_link_state(struct i40e_pf *pf, bool
is_up) // XXX is_up was _false_ in my test case because I have
link-down-on-close "on"
{
struct i40e_aq_get_phy_abilities_resp abilities;
struct i40e_aq_set_phy_config config = {0};
struct i40e_hw *hw = &pf->hw;
i40e_status err;
u64 mask;

/* Get the current phy config */
err = i40e_aq_get_phy_capabilities(hw, false, false, &abilities,
  NULL);
if (err) {
dev_err(&pf->pdev->dev,
"failed to get phy cap., ret =  %s last_status =  %s\n",
i40e_stat_str(hw, err),
i40e_aq_str(hw, hw->aq.asq_last_status));
return err;
}

/* If link needs to go up, but was not forced to go down,
* no need for a flap
*/
if (is_up && abilities.phy_type != 0)
return I40E_SUCCESS;

/* To force link we need to set bits for all supported PHY types,
* but there are now more than 32, so we need to split the bitmap
* across two fields.
*/
mask = I40E_PHY_TYPES_BITMASK;
config.phy_type = is_up ? cpu_to_le32((u32)(mask & 0xffffffff)) : 0;
config.phy_type_ext = is_up ? (u8)((mask >> 32) & 0xff) : 0;
/* Copy the old settings, except of phy_type */
config.abilities = abilities.abilities;
config.link_speed = abilities.link_speed;
config.eee_capability = abilities.eee_capability;
config.eeer = abilities.eeer_val;
config.low_power_ctrl = abilities.d3_lpan;
err = i40e_aq_set_phy_config(hw, &config, NULL);

if (err) {
dev_err(&pf->pdev->dev,
"set phy config ret =  %s last_status =  %s\n",
i40e_stat_str(&pf->hw, err),
i40e_aq_str(&pf->hw, pf->hw.aq.asq_last_status));
return err;
}

/* Update the link info */
err = i40e_update_link_info(hw);
if (err) {
/* Wait a little bit (on 40G cards it sometimes takes a really
* long time for link to come back from the atomic reset)
* and try once more
*/
msleep(1000);
i40e_update_link_info(hw);
}

i40e_aq_set_link_restart_an(hw, true, NULL); // XXX Second argument is
unconditionally true. Should it not be is_up  instead of true ?

return I40E_SUCCESS;
}

Thanks,
Chaitanya


> "ifconfig IFNAME down"
>
> Best regards!
> Mariusz
> --------------------------------------------------------------------
>
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-316 | Kapital zakladowy 200.000 PLN.
>
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek
> przegladanie lub rozpowszechnianie jest zabronione.
> This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by
> others is strictly prohibited.