i40e - Is i40e_force_link_state doing the right thing ?

i40e - Is i40e_force_link_state doing the right thing ?

[Chaitanya Lala]

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

Re: i40e - Is i40e_force_link_state doing the right thing ?

[Alexander Duyck]

On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala
<chaitanya.lala@gmail.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

If you are calling i40e_down the assumption is you are bringing the
interface down, so as not to pass traffic. That is why the "is_up" is
set to false.

Could you provide the dmesg results for when you load the driver and
execute the "ifconfig up" and "ifconfig down" commands? That would
help us to understand what might be going on.

Thanks.

- Alex

RE: i40e - Is i40e_force_link_state doing the right thing ?

[Stachura, Mariusz]

On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.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
"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.

Re: i40e - Is i40e_force_link_state doing the right thing ?

[Chaitanya Lala]

Hi Mariusz, ...

On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz
<mariusz.stachura@intel.com> wrote:
> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.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.

RE: i40e - Is i40e_force_link_state doing the right thing ?

[Stachura, Mariusz]

> Hi Mariusz, ...
>
> On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz <mariusz.stachura@intel.com> wrote:
>> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.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".
> (...)
> Thanks,
> Chaitanya

Hey,
i40e_aq_set_link_restart_an has second argument set to "true" intentionally, as I understand the "link-down-on-close" does not work for you, right? I will double check if this feature works for me and get back to you, thank you again.
--------------------------------------------------------------------

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.

Re: i40e - Is i40e_force_link_state doing the right thing ?

[Chaitanya Lala]

Hi Mariusz, ...

On Wed, May 16, 2018 at 12:49 AM, Stachura, Mariusz
<mariusz.stachura@intel.com> wrote:
>> Hi Mariusz, ...
>>
>> On Tue, May 15, 2018 at 2:24 PM, Stachura, Mariusz <mariusz.stachura@intel.com> wrote:
>>> On Tue, May 15, 2018 at 1:15 PM, Chaitanya Lala <chaitanya.lala@gmail.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".
>> (...)
>> Thanks,
>> Chaitanya
>
> Hey,
> i40e_aq_set_link_restart_an has second argument set to "true" intentionally, as I understand the "link-down-on-close" does not work for you, right? I will double check if this feature works for me and get back to you, thank you again.


That's correct. Even with the flag set, if-down does not result in a
link down for me. Thanks for checking!

-
Chaitanya

> --------------------------------------------------------------------
>
> 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.