Message ID | 20200609000059.12924-1-dwilder@us.ibm.com |
---|---|
State | RFC |
Delegated to: | David Miller |
Headers | show |
Series | [(RFC)] be2net: Allow a VF to use physical link state. | expand |
On Mon, 8 Jun 2020 17:00:59 -0700 David Wilder wrote: > Hyper-visors owning a PF are allowed by Emulex specification to provide > a VF with separate physical and/or logical link states. However, on > linux, a VF driver must chose one or the other. > > My scenario is a proprietary driver controlling the PF, be2net is the VF. > When I do a physical cable pull test the PF sends a link event > notification to the VF with the "physical" link status but this is > ignored in be2net (see be_async_link_state_process() ). > > The PF is reporting the adapter type as: > 0xe228 /* Device id for VF in Lancer */ > > I added a module parameter "use_pf_link_state". When set the VF should > ignore logical link state and use the physical link state. > > However I have an issue making this work. When the cable is pulled I > see two link statuses reported: > [1706100.767718] be2net 8002:01:00.0 eth1: Link is Down > [1706101.189298] be2net 8002:01:00.0 eth1: Link is Up > > be_link_status_update() is called twice, the first with the physical link > status called from be_async_link_state_process(), and the second with the > logical link status from be_get_link_ksettings(). > > I am unsure why be_async_link_state_process() is called from > be_get_link_ksettings(), it results in multiple link state changes > (even in the un-patched case). If I eliminate this call then it works. > But I am un-sure of this change. > > Signed-off-by: David Wilder <dwilder@us.ibm.com> Hm. Just looking at the code in __be_cmd_set_logical_link_config(): if (link_state == IFLA_VF_LINK_STATE_ENABLE || link_state == IFLA_VF_LINK_STATE_AUTO) link_config |= PLINK_ENABLE; if (link_state == IFLA_VF_LINK_STATE_AUTO) link_config |= PLINK_TRACK; Maybe we shouldn't set ENABLE for AUTO? The module parameter is definitely not a good idea, what you're asking for seems to be well within the expectation from the .ndo_set_vf_link_state config, so it seems the driver / firmware is just not implementing that right.
On 2020-06-09 14:58, Jakub Kicinski wrote: > On Mon, 8 Jun 2020 17:00:59 -0700 David Wilder wrote: >> Hyper-visors owning a PF are allowed by Emulex specification to >> provide >> a VF with separate physical and/or logical link states. However, on >> linux, a VF driver must chose one or the other. >> >> My scenario is a proprietary driver controlling the PF, be2net is the >> VF. >> When I do a physical cable pull test the PF sends a link event >> notification to the VF with the "physical" link status but this is >> ignored in be2net (see be_async_link_state_process() ). >> >> The PF is reporting the adapter type as: >> 0xe228 /* Device id for VF in Lancer */ >> >> I added a module parameter "use_pf_link_state". When set the VF should >> ignore logical link state and use the physical link state. >> >> However I have an issue making this work. When the cable is pulled I >> see two link statuses reported: >> [1706100.767718] be2net 8002:01:00.0 eth1: Link is Down >> [1706101.189298] be2net 8002:01:00.0 eth1: Link is Up >> >> be_link_status_update() is called twice, the first with the physical >> link >> status called from be_async_link_state_process(), and the second with >> the >> logical link status from be_get_link_ksettings(). >> >> I am unsure why be_async_link_state_process() is called from >> be_get_link_ksettings(), it results in multiple link state changes >> (even in the un-patched case). If I eliminate this call then it works. >> But I am un-sure of this change. >> >> Signed-off-by: David Wilder <dwilder@us.ibm.com> > > Hm. Just looking at the code in __be_cmd_set_logical_link_config(): > > > if (link_state == IFLA_VF_LINK_STATE_ENABLE || > link_state == IFLA_VF_LINK_STATE_AUTO) > link_config |= PLINK_ENABLE; > > if (link_state == IFLA_VF_LINK_STATE_AUTO) > link_config |= PLINK_TRACK; > > Maybe we shouldn't set ENABLE for AUTO? If I am understanding this correctly, this is used by the linux PF driver to configure how link status is delivered to a VF. My problem is one of interoperability between the PF (not linux) and the VF is running on linux. The PF driver is implemented to the Emulex/Broadcom spec, which allows a PF driver to be configured such that the VF can be notified of both physical and logical link status, separately. > > The module parameter is definitely not a good idea, what you're asking > for seems to be well within the expectation from the > .ndo_set_vf_link_state config, so it seems the driver / firmware is > just > not implementing that right. I am attempting to resolve an issue that the linux implementation cant conform to the the Emulex specification due to the implementation on linux.
On Wed, 10 Jun 2020 10:22:22 -0700 dwilder wrote: > On 2020-06-09 14:58, Jakub Kicinski wrote: > > On Mon, 8 Jun 2020 17:00:59 -0700 David Wilder wrote: > >> Hyper-visors owning a PF are allowed by Emulex specification to > >> provide > >> a VF with separate physical and/or logical link states. However, on > >> linux, a VF driver must chose one or the other. > >> > >> My scenario is a proprietary driver controlling the PF, be2net is the > >> VF. > >> When I do a physical cable pull test the PF sends a link event > >> notification to the VF with the "physical" link status but this is > >> ignored in be2net (see be_async_link_state_process() ). > >> > >> The PF is reporting the adapter type as: > >> 0xe228 /* Device id for VF in Lancer */ > >> > >> I added a module parameter "use_pf_link_state". When set the VF should > >> ignore logical link state and use the physical link state. > >> > >> However I have an issue making this work. When the cable is pulled I > >> see two link statuses reported: > >> [1706100.767718] be2net 8002:01:00.0 eth1: Link is Down > >> [1706101.189298] be2net 8002:01:00.0 eth1: Link is Up > >> > >> be_link_status_update() is called twice, the first with the physical > >> link > >> status called from be_async_link_state_process(), and the second with > >> the > >> logical link status from be_get_link_ksettings(). > >> > >> I am unsure why be_async_link_state_process() is called from > >> be_get_link_ksettings(), it results in multiple link state changes > >> (even in the un-patched case). If I eliminate this call then it works. > >> But I am un-sure of this change. > >> > >> Signed-off-by: David Wilder <dwilder@us.ibm.com> > > > > Hm. Just looking at the code in __be_cmd_set_logical_link_config(): > > > > > > if (link_state == IFLA_VF_LINK_STATE_ENABLE || > > link_state == IFLA_VF_LINK_STATE_AUTO) > > link_config |= PLINK_ENABLE; > > > > if (link_state == IFLA_VF_LINK_STATE_AUTO) > > link_config |= PLINK_TRACK; > > > > Maybe we shouldn't set ENABLE for AUTO? > > If I am understanding this correctly, this is used by the linux PF > driver to configure how link status is delivered to a VF. > > My problem is one of interoperability between the PF (not linux) and the > VF is running on linux. I see. > The PF driver is implemented to the Emulex/Broadcom spec, which allows a > PF driver to be configured such that the VF can be notified of both > physical and logical link status, separately. We'd need a first-class support for this behavior both on PF and VF sides. A module parameter in one vendor driver to enable support for this behavior when running with a non-Linux PF driver is unlikely to succeed. > > The module parameter is definitely not a good idea, what you're asking > > for seems to be well within the expectation from the > > .ndo_set_vf_link_state config, so it seems the driver / firmware is > > just > > not implementing that right. > > I am attempting to resolve an issue that the linux implementation cant > conform to the the Emulex specification due to the implementation on > linux. Sadly I'd think that it's Emulex that needs to conform to Linux APIs, not the other way around.
diff --git a/drivers/net/ethernet/emulex/benet/be_cmds.c b/drivers/net/ethernet/emulex/benet/be_cmds.c index 701c12c..52934b2 100644 --- a/drivers/net/ethernet/emulex/benet/be_cmds.c +++ b/drivers/net/ethernet/emulex/benet/be_cmds.c @@ -278,6 +278,8 @@ static int be_mcc_compl_process(struct be_adapter *adapter, return compl->status; } +extern int use_pf_link_state; + /* Link state evt is a string of bytes; no need for endian swapping */ static void be_async_link_state_process(struct be_adapter *adapter, struct be_mcc_compl *compl) @@ -288,13 +290,18 @@ static void be_async_link_state_process(struct be_adapter *adapter, /* When link status changes, link speed must be re-queried from FW */ adapter->phy.link_speed = -1; + if (use_pf_link_state && + evt->port_link_status & LOGICAL_LINK_STATUS_MASK) + return; + /* On BEx the FW does not send a separate link status * notification for physical and logical link. * On other chips just process the logical link * status notification */ if (!BEx_chip(adapter) && - !(evt->port_link_status & LOGICAL_LINK_STATUS_MASK)) + !(evt->port_link_status & LOGICAL_LINK_STATUS_MASK) && + !use_pf_link_state) return; /* For the initial link status do not rely on the ASYNC event as diff --git a/drivers/net/ethernet/emulex/benet/be_ethtool.c b/drivers/net/ethernet/emulex/benet/be_ethtool.c index d6ed1d9..fd91d63 100644 --- a/drivers/net/ethernet/emulex/benet/be_ethtool.c +++ b/drivers/net/ethernet/emulex/benet/be_ethtool.c @@ -618,8 +618,6 @@ static int be_get_link_ksettings(struct net_device *netdev, if (adapter->phy.link_speed < 0) { status = be_cmd_link_status_query(adapter, &link_speed, &link_status, 0); - if (!status) - be_link_status_update(adapter, link_status); cmd->base.speed = link_speed; status = be_cmd_get_phy_info(adapter); diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c index a7ac23a..1020be6 100644 --- a/drivers/net/ethernet/emulex/benet/be_main.c +++ b/drivers/net/ethernet/emulex/benet/be_main.c @@ -36,6 +36,10 @@ module_param(rx_frag_size, ushort, 0444); MODULE_PARM_DESC(rx_frag_size, "Size of a fragment that holds rcvd data."); +unsigned int use_pf_link_state; +module_param(use_pf_link_state, uint, 0444); +MODULE_PARM_DESC(use_pf_link_state, "Use physical link state"); + /* Per-module error detection/recovery workq shared across all functions. * Each function schedules its own work request on this shared workq. */
Hyper-visors owning a PF are allowed by Emulex specification to provide a VF with separate physical and/or logical link states. However, on linux, a VF driver must chose one or the other. My scenario is a proprietary driver controlling the PF, be2net is the VF. When I do a physical cable pull test the PF sends a link event notification to the VF with the "physical" link status but this is ignored in be2net (see be_async_link_state_process() ). The PF is reporting the adapter type as: 0xe228 /* Device id for VF in Lancer */ I added a module parameter "use_pf_link_state". When set the VF should ignore logical link state and use the physical link state. However I have an issue making this work. When the cable is pulled I see two link statuses reported: [1706100.767718] be2net 8002:01:00.0 eth1: Link is Down [1706101.189298] be2net 8002:01:00.0 eth1: Link is Up be_link_status_update() is called twice, the first with the physical link status called from be_async_link_state_process(), and the second with the logical link status from be_get_link_ksettings(). I am unsure why be_async_link_state_process() is called from be_get_link_ksettings(), it results in multiple link state changes (even in the un-patched case). If I eliminate this call then it works. But I am un-sure of this change. Signed-off-by: David Wilder <dwilder@us.ibm.com> --- drivers/net/ethernet/emulex/benet/be_cmds.c | 9 ++++++++- drivers/net/ethernet/emulex/benet/be_ethtool.c | 2 -- drivers/net/ethernet/emulex/benet/be_main.c | 4 ++++ 3 files changed, 12 insertions(+), 3 deletions(-)