[(RFC) PATCH ] be2net: Allow a VF to use physical link state.

[(RFC) PATCH ] be2net: Allow a VF to use physical link state.

[David Wilder]

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(-)

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.
  */
-- 
1.8.3.1

Re: [(RFC) PATCH ] be2net: Allow a VF to use physical link state.

[Jakub Kicinski]

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.

RE: [(RFC) PATCH ] be2net: Allow a VF to use physical link state.

[dwilder]

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.

Re: [(RFC) PATCH ] be2net: Allow a VF to use physical link state.

[Jakub Kicinski]

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.