[PATCH 0/2] net: qcom/emac: fixes for pause frame floods

[PATCH 0/2] net: qcom/emac: fixes for pause frame floods

[Timur Tabi]

The first patch is for 4.13.  It's changes the default behavior of the
EMAC driver so that it doesn't send pause frames unless the user 
enables them.

The second patch is for 4.14, but it can be applied to 4.13 if you
want.  It adds the ability for the user to enable a special "single
pause frame" mode that could be useful in some situations.

Timur Tabi (2):
  [for 4.13] net: qcom/emac: disable flow control autonegotiation by
    default
  net: qcom/emac: add software control for pause frame mode

 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++++++++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c     | 22 +++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac.c         | 12 +++++++--
 drivers/net/ethernet/qualcomm/emac/emac.h         |  3 +++
 4 files changed, 65 insertions(+), 2 deletions(-)

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

The EMAC has a curious qwirk when RX flow control is enabled and the
kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
If RX flow control is enabled, the EMAC will then send a non-stop
stream of pause frames until the system is reset.  The EMAC does not
have a built-in watchdog.

In various tests, the pause frame stream sometimes overloads nearby
switches, effectively disabling the network.  Since the RX queue is
large and the host processor is more than capable of handling incoming
packets quickly, the only time the EMAC will send any pause frames is
when the kernel is hung and unrecoverable.

To avoid all these problems, we disable flow control autonegotiation
by default, and only enable receiving pause frames.

Cc: stable@vger.kernel.org # 4.11.x
Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 60850bfa3d32..475c0ea29235 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -441,8 +441,13 @@ static void emac_init_adapter(struct emac_adapter *adpt)
 	/* others */
 	adpt->preamble = EMAC_PREAMBLE_DEF;
 
-	/* default to automatic flow control */
-	adpt->automatic = true;
+	/* Disable transmission of pause frames by default, to avoid the
+	 * risk of a pause frame flood that can occur if the kernel hangs.
+	 * We still want to be able to respond to them, however.
+	 */
+	adpt->automatic = false;
+	adpt->tx_flow_control = false;
+	adpt->rx_flow_control = true;
 }
 
 /* Get the clock */
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

[PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

The EMAC has the option of sending only a single pause frame when
flow control is enabled and the RX queue is full.  Although sending
only one pause frame has little value, this would allow admins to
enable automatic flow control without having to worry about the EMAC
flooding nearby switches with pause frames if the kernel hangs.

The option is enabled by using the single-pause-mode private flag.

Signed-off-by: Timur Tabi <timur@codeaurora.org>
---
 drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++++++++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac-mac.c     | 22 +++++++++++++++++
 drivers/net/ethernet/qualcomm/emac/emac.c         |  3 +++
 drivers/net/ethernet/qualcomm/emac/emac.h         |  3 +++
 4 files changed, 58 insertions(+)

diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
index bbe24639aa5a..c8c6231b87f3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
@@ -88,6 +88,8 @@ static void emac_set_msglevel(struct net_device *netdev, u32 data)
 static int emac_get_sset_count(struct net_device *netdev, int sset)
 {
 	switch (sset) {
+	case ETH_SS_PRIV_FLAGS:
+		return 1;
 	case ETH_SS_STATS:
 		return EMAC_STATS_LEN;
 	default:
@@ -100,6 +102,10 @@ static void emac_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
 	unsigned int i;
 
 	switch (stringset) {
+	case ETH_SS_PRIV_FLAGS:
+		strcpy(data, "single-pause-mode");
+		break;
+
 	case ETH_SS_STATS:
 		for (i = 0; i < EMAC_STATS_LEN; i++) {
 			strlcpy(data, emac_ethtool_stat_strings[i],
@@ -230,6 +236,27 @@ static int emac_get_regs_len(struct net_device *netdev)
 	return EMAC_MAX_REG_SIZE * sizeof(u32);
 }
 
+#define EMAC_PRIV_ENABLE_SINGLE_PAUSE	BIT(0)
+
+static int emac_set_priv_flags(struct net_device *netdev, u32 flags)
+{
+	struct emac_adapter *adpt = netdev_priv(netdev);
+
+	adpt->single_pause_mode = !!(flags & EMAC_PRIV_ENABLE_SINGLE_PAUSE);
+
+	if (netif_running(netdev))
+		return emac_reinit_locked(adpt);
+
+	return 0;
+}
+
+static u32 emac_get_priv_flags(struct net_device *netdev)
+{
+	struct emac_adapter *adpt = netdev_priv(netdev);
+
+	return adpt->single_pause_mode ? EMAC_PRIV_ENABLE_SINGLE_PAUSE : 0;
+}
+
 static const struct ethtool_ops emac_ethtool_ops = {
 	.get_link_ksettings = phy_ethtool_get_link_ksettings,
 	.set_link_ksettings = phy_ethtool_set_link_ksettings,
@@ -253,6 +280,9 @@ static int emac_get_regs_len(struct net_device *netdev)
 
 	.get_regs_len    = emac_get_regs_len,
 	.get_regs        = emac_get_regs,
+
+	.set_priv_flags = emac_set_priv_flags,
+	.get_priv_flags = emac_get_priv_flags,
 };
 
 void emac_set_ethtool_ops(struct net_device *netdev)
diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
index bcd4708b3745..0ea3ca09c689 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
@@ -551,6 +551,28 @@ static void emac_mac_start(struct emac_adapter *adpt)
 	mac &= ~(HUGEN | VLAN_STRIP | TPAUSE | SIMR | HUGE | MULTI_ALL |
 		 DEBUG_MODE | SINGLE_PAUSE_MODE);
 
+	/* Enable single-pause-frame mode if requested.
+	 *
+	 * If enabled, the EMAC will send a single pause frame when the RX
+	 * queue is full.  This normally leads to packet loss because
+	 * the pause frame disables the remote MAC only for 33ms (the quanta),
+	 * and then the remote MAC continues sending packets even though
+	 * the RX queue is still full.
+	 *
+	 * If disabled, the EMAC sends a pause frame every 31ms until the RX
+	 * queue is no longer full.  Normally, this is the preferred
+	 * method of operation.  However, when the system is hung (e.g.
+	 * cores are halted), the EMAC interrupt handler is never called
+	 * and so the RX queue fills up quickly and stays full.  The resuling
+	 * non-stop "flood" of pause frames sometimes has the effect of
+	 * disabling nearby switches.  In some cases, other nearby switches
+	 * are also affected, shutting down the entire network.
+	 *
+	 * The user can enable or disable single-pause-frame mode
+	 * via ethtool.
+	 */
+	mac |= adpt->single_pause_mode ? SINGLE_PAUSE_MODE : 0;
+
 	writel_relaxed(csr1, adpt->csr + EMAC_EMAC_WRAPPER_CSR1);
 
 	writel_relaxed(mac, adpt->base + EMAC_MAC_CTRL);
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
index 475c0ea29235..6f5e858ffbf3 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.c
+++ b/drivers/net/ethernet/qualcomm/emac/emac.c
@@ -448,6 +448,9 @@ static void emac_init_adapter(struct emac_adapter *adpt)
 	adpt->automatic = false;
 	adpt->tx_flow_control = false;
 	adpt->rx_flow_control = true;
+
+	/* Disable single-pause-frame mode by default */
+	adpt->single_pause_mode = false;
 }
 
 /* Get the clock */
diff --git a/drivers/net/ethernet/qualcomm/emac/emac.h b/drivers/net/ethernet/qualcomm/emac/emac.h
index 8ee4ec6aef2e..d7c9f44209d4 100644
--- a/drivers/net/ethernet/qualcomm/emac/emac.h
+++ b/drivers/net/ethernet/qualcomm/emac/emac.h
@@ -363,6 +363,9 @@ struct emac_adapter {
 	bool				tx_flow_control;
 	bool				rx_flow_control;
 
+	/* True == use single-pause-frame mode. */
+	bool				single_pause_mode;
+
 	/* Ring parameter */
 	u8				tpd_burst;
 	u8				rfd_burst;
-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Florian Fainelli]

On 08/01/2017 02:37 PM, Timur Tabi wrote:
> The EMAC has the option of sending only a single pause frame when
> flow control is enabled and the RX queue is full.  Although sending
> only one pause frame has little value, this would allow admins to
> enable automatic flow control without having to worry about the EMAC
> flooding nearby switches with pause frames if the kernel hangs.
> 
> The option is enabled by using the single-pause-mode private flag.

A few adapters (bcmgenet, bcmsysport) support configuring the pause
quanta so it would not be inconceivable to try to update
ethtool_pauseparam to include additional fields such as:

- number of pause frames to send where we define an arbitrary high
default value (e.g: 0xffff), N < 0xffff is something drivers can test
for whether they support it, and 0 is only valid if pause is already
disabled

- pause quanta (16-bits)

Private flags are not usually that great and there could be more
adapters capable of doing the same pause frame number configuration, but
since there is no available knob it's hard to know.

> 
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/emac/emac-ethtool.c | 30 +++++++++++++++++++++++
>  drivers/net/ethernet/qualcomm/emac/emac-mac.c     | 22 +++++++++++++++++
>  drivers/net/ethernet/qualcomm/emac/emac.c         |  3 +++
>  drivers/net/ethernet/qualcomm/emac/emac.h         |  3 +++
>  4 files changed, 58 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> index bbe24639aa5a..c8c6231b87f3 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-ethtool.c
> @@ -88,6 +88,8 @@ static void emac_set_msglevel(struct net_device *netdev, u32 data)
>  static int emac_get_sset_count(struct net_device *netdev, int sset)
>  {
>  	switch (sset) {
> +	case ETH_SS_PRIV_FLAGS:
> +		return 1;
>  	case ETH_SS_STATS:
>  		return EMAC_STATS_LEN;
>  	default:
> @@ -100,6 +102,10 @@ static void emac_get_strings(struct net_device *netdev, u32 stringset, u8 *data)
>  	unsigned int i;
>  
>  	switch (stringset) {
> +	case ETH_SS_PRIV_FLAGS:
> +		strcpy(data, "single-pause-mode");
> +		break;
> +
>  	case ETH_SS_STATS:
>  		for (i = 0; i < EMAC_STATS_LEN; i++) {
>  			strlcpy(data, emac_ethtool_stat_strings[i],
> @@ -230,6 +236,27 @@ static int emac_get_regs_len(struct net_device *netdev)
>  	return EMAC_MAX_REG_SIZE * sizeof(u32);
>  }
>  
> +#define EMAC_PRIV_ENABLE_SINGLE_PAUSE	BIT(0)
> +
> +static int emac_set_priv_flags(struct net_device *netdev, u32 flags)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +
> +	adpt->single_pause_mode = !!(flags & EMAC_PRIV_ENABLE_SINGLE_PAUSE);
> +
> +	if (netif_running(netdev))
> +		return emac_reinit_locked(adpt);
> +
> +	return 0;
> +}
> +
> +static u32 emac_get_priv_flags(struct net_device *netdev)
> +{
> +	struct emac_adapter *adpt = netdev_priv(netdev);
> +
> +	return adpt->single_pause_mode ? EMAC_PRIV_ENABLE_SINGLE_PAUSE : 0;
> +}
> +
>  static const struct ethtool_ops emac_ethtool_ops = {
>  	.get_link_ksettings = phy_ethtool_get_link_ksettings,
>  	.set_link_ksettings = phy_ethtool_set_link_ksettings,
> @@ -253,6 +280,9 @@ static int emac_get_regs_len(struct net_device *netdev)
>  
>  	.get_regs_len    = emac_get_regs_len,
>  	.get_regs        = emac_get_regs,
> +
> +	.set_priv_flags = emac_set_priv_flags,
> +	.get_priv_flags = emac_get_priv_flags,
>  };
>  
>  void emac_set_ethtool_ops(struct net_device *netdev)
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac-mac.c b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
> index bcd4708b3745..0ea3ca09c689 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac-mac.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac-mac.c
> @@ -551,6 +551,28 @@ static void emac_mac_start(struct emac_adapter *adpt)
>  	mac &= ~(HUGEN | VLAN_STRIP | TPAUSE | SIMR | HUGE | MULTI_ALL |
>  		 DEBUG_MODE | SINGLE_PAUSE_MODE);
>  
> +	/* Enable single-pause-frame mode if requested.
> +	 *
> +	 * If enabled, the EMAC will send a single pause frame when the RX
> +	 * queue is full.  This normally leads to packet loss because
> +	 * the pause frame disables the remote MAC only for 33ms (the quanta),
> +	 * and then the remote MAC continues sending packets even though
> +	 * the RX queue is still full.
> +	 *
> +	 * If disabled, the EMAC sends a pause frame every 31ms until the RX
> +	 * queue is no longer full.  Normally, this is the preferred
> +	 * method of operation.  However, when the system is hung (e.g.
> +	 * cores are halted), the EMAC interrupt handler is never called
> +	 * and so the RX queue fills up quickly and stays full.  The resuling
> +	 * non-stop "flood" of pause frames sometimes has the effect of
> +	 * disabling nearby switches.  In some cases, other nearby switches
> +	 * are also affected, shutting down the entire network.

This explanation would have more value in your previous commit message,
especially since you are targeting it as a bugfix.

> +	 *
> +	 * The user can enable or disable single-pause-frame mode
> +	 * via ethtool.
> +	 */
> +	mac |= adpt->single_pause_mode ? SINGLE_PAUSE_MODE : 0;
> +
>  	writel_relaxed(csr1, adpt->csr + EMAC_EMAC_WRAPPER_CSR1);
>  
>  	writel_relaxed(mac, adpt->base + EMAC_MAC_CTRL);
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
> index 475c0ea29235..6f5e858ffbf3 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac.c
> @@ -448,6 +448,9 @@ static void emac_init_adapter(struct emac_adapter *adpt)
>  	adpt->automatic = false;
>  	adpt->tx_flow_control = false;
>  	adpt->rx_flow_control = true;
> +
> +	/* Disable single-pause-frame mode by default */
> +	adpt->single_pause_mode = false;
>  }
>  
>  /* Get the clock */
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac.h b/drivers/net/ethernet/qualcomm/emac/emac.h
> index 8ee4ec6aef2e..d7c9f44209d4 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac.h
> +++ b/drivers/net/ethernet/qualcomm/emac/emac.h
> @@ -363,6 +363,9 @@ struct emac_adapter {
>  	bool				tx_flow_control;
>  	bool				rx_flow_control;
>  
> +	/* True == use single-pause-frame mode. */
> +	bool				single_pause_mode;
> +
>  	/* Ring parameter */
>  	u8				tpd_burst;
>  	u8				rfd_burst;
> 


-- 
Florian

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Florian Fainelli]

On 08/01/2017 02:37 PM, Timur Tabi wrote:
> The EMAC has a curious qwirk when RX flow control is enabled and the
> kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
> If RX flow control is enabled, the EMAC will then send a non-stop
> stream of pause frames until the system is reset.  The EMAC does not
> have a built-in watchdog.
> 
> In various tests, the pause frame stream sometimes overloads nearby
> switches, effectively disabling the network.  Since the RX queue is
> large and the host processor is more than capable of handling incoming
> packets quickly, the only time the EMAC will send any pause frames is
> when the kernel is hung and unrecoverable.

This is not specific to your EMAC, a lot of adapters have this problem
actually.

I wonder if it would make sense to reach for a broader solution where we
could have a networking stack panic/oops notifier which will actively
clean up the active network devices' RX queue(s) and if tx_pause was
enabled, disable it. We could have drivers announce themselves as
needing this either via NETIF_F_* feature bit or some other private flag.

> 
> To avoid all these problems, we disable flow control autonegotiation
> by default, and only enable receiving pause frames.
> 
> Cc: stable@vger.kernel.org # 4.11.x
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/emac/emac.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
> index 60850bfa3d32..475c0ea29235 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac.c
> @@ -441,8 +441,13 @@ static void emac_init_adapter(struct emac_adapter *adpt)
>  	/* others */
>  	adpt->preamble = EMAC_PREAMBLE_DEF;
>  
> -	/* default to automatic flow control */
> -	adpt->automatic = true;
> +	/* Disable transmission of pause frames by default, to avoid the
> +	 * risk of a pause frame flood that can occur if the kernel hangs.
> +	 * We still want to be able to respond to them, however.
> +	 */
> +	adpt->automatic = false;
> +	adpt->tx_flow_control = false;
> +	adpt->rx_flow_control = true;
>  }
>  
>  /* Get the clock */
> 


-- 
Florian

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

On 08/01/2017 04:51 PM, Florian Fainelli wrote:

> A few adapters (bcmgenet, bcmsysport) support configuring the pause
> quanta so it would not be inconceivable to try to update
> ethtool_pauseparam to include additional fields such as:

Wouldn't this require a change to the user space tool?

> - number of pause frames to send where we define an arbitrary high
> default value (e.g: 0xffff), N < 0xffff is something drivers can test
> for whether they support it, and 0 is only valid if pause is already
> disabled
> 
> - pause quanta (16-bits)
> 
> Private flags are not usually that great and there could be more
> adapters capable of doing the same pause frame number configuration, but
> since there is no available knob it's hard to know.

Well, for the EMAC, the quanta in this case would be either 1 or 
infinite.  For other devices, it could be any combination of values.  In 
a future revision of the hardware, we might support a variable quanta. 
And I suspect that some devices measure the quanta in time, not count.

How would the user know what the acceptable values are?  If I set the 
quanta to 10 via user space, and my driver truncates that to 1, I don't 
think that would be acceptable.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/01/2017 04:55 PM, Florian Fainelli wrote:
> This is not specific to your EMAC, a lot of adapters have this problem
> actually.
> 
> I wonder if it would make sense to reach for a broader solution where we
> could have a networking stack panic/oops notifier which will actively
> clean up the active network devices' RX queue(s) and if tx_pause was
> enabled, disable it. We could have drivers announce themselves as
> needing this either via NETIF_F_* feature bit or some other private flag.

Unfortunately, the problem occurs only when Linux hangs, to the point 
where the driver's interrupt handlers are blocked.  The RX queue is 256 
entries, and the processor has 48 cores, so the EMAC is never going to 
send pause frames in any real-world situation.

The only time I've seen pause frames sent out is in the lab when I halt 
the cores with a hardware debugger, and only if I have enough network 
traffic that the EMAC picks up.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Florian Fainelli]

On 08/01/2017 03:00 PM, Timur Tabi wrote:
> On 08/01/2017 04:51 PM, Florian Fainelli wrote:
> 
>> A few adapters (bcmgenet, bcmsysport) support configuring the pause
>> quanta so it would not be inconceivable to try to update
>> ethtool_pauseparam to include additional fields such as:
> 
> Wouldn't this require a change to the user space tool?

It would yes.

> 
>> - number of pause frames to send where we define an arbitrary high
>> default value (e.g: 0xffff), N < 0xffff is something drivers can test
>> for whether they support it, and 0 is only valid if pause is already
>> disabled
>>
>> - pause quanta (16-bits)
>>
>> Private flags are not usually that great and there could be more
>> adapters capable of doing the same pause frame number configuration, but
>> since there is no available knob it's hard to know.
> 
> Well, for the EMAC, the quanta in this case would be either 1 or
> infinite.  For other devices, it could be any combination of values.  In
> a future revision of the hardware, we might support a variable quanta.
> And I suspect that some devices measure the quanta in time, not count.

OK, either way is fine and there should be ways to convert back and
forth without too much trouble.

> 
> How would the user know what the acceptable values are?  If I set the
> quanta to 10 via user space, and my driver truncates that to 1, I don't
> think that would be acceptable.

There can be several ways to deal with that, we can either ask for a
strict rejection before committing the changes to the HW, or we can have
the HW round up/down to the nearest supported values and an user would
call get_pauseparam() to see which value ended-up being used.

Anyway food for thought, David decides what gets merged ;)
-- 
Florian

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Florian Fainelli]

On 08/01/2017 03:02 PM, Timur Tabi wrote:
> On 08/01/2017 04:55 PM, Florian Fainelli wrote:
>> This is not specific to your EMAC, a lot of adapters have this problem
>> actually.
>>
>> I wonder if it would make sense to reach for a broader solution where we
>> could have a networking stack panic/oops notifier which will actively
>> clean up the active network devices' RX queue(s) and if tx_pause was
>> enabled, disable it. We could have drivers announce themselves as
>> needing this either via NETIF_F_* feature bit or some other private flag.
> 
> Unfortunately, the problem occurs only when Linux hangs, to the point
> where the driver's interrupt handlers are blocked.  The RX queue is 256
> entries, and the processor has 48 cores, so the EMAC is never going to
> send pause frames in any real-world situation.
> 
> The only time I've seen pause frames sent out is in the lab when I halt
> the cores with a hardware debugger, and only if I have enough network
> traffic that the EMAC picks up.

The size and scale of your system makes it so but imagine e.g: a single
core ~ 1Ghz @ 1Gbits/sec system having the same problems, here you are
quite likely to see the system under panic flooding the network.

Then again your patch is fine and can be revised at any time a broader
facility is offered, I just felt like we actually have a good way with
reasonably driver-agnostic code to possibly deal with that problem.

Implementing such a solution would not be a -stable backport candidate
though....
-- 
Florian

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Andrew Lunn]

On Tue, Aug 01, 2017 at 04:37:39PM -0500, Timur Tabi wrote:
> The EMAC has a curious qwirk when RX flow control is enabled and the
> kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
> If RX flow control is enabled, the EMAC will then send a non-stop
> stream of pause frames until the system is reset.  The EMAC does not
> have a built-in watchdog.
> 
> In various tests, the pause frame stream sometimes overloads nearby
> switches, effectively disabling the network.  Since the RX queue is
> large and the host processor is more than capable of handling incoming
> packets quickly, the only time the EMAC will send any pause frames is
> when the kernel is hung and unrecoverable.
> 
> To avoid all these problems, we disable flow control autonegotiation
> by default, and only enable receiving pause frames.
> 
> Cc: stable@vger.kernel.org # 4.11.x
> Signed-off-by: Timur Tabi <timur@codeaurora.org>
> ---
>  drivers/net/ethernet/qualcomm/emac/emac.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/qualcomm/emac/emac.c b/drivers/net/ethernet/qualcomm/emac/emac.c
> index 60850bfa3d32..475c0ea29235 100644
> --- a/drivers/net/ethernet/qualcomm/emac/emac.c
> +++ b/drivers/net/ethernet/qualcomm/emac/emac.c
> @@ -441,8 +441,13 @@ static void emac_init_adapter(struct emac_adapter *adpt)
>  	/* others */
>  	adpt->preamble = EMAC_PREAMBLE_DEF;
>  
> -	/* default to automatic flow control */
> -	adpt->automatic = true;
> +	/* Disable transmission of pause frames by default, to avoid the
> +	 * risk of a pause frame flood that can occur if the kernel hangs.
> +	 * We still want to be able to respond to them, however.
> +	 */
> +	adpt->automatic = false;
> +	adpt->tx_flow_control = false;
> +	adpt->rx_flow_control = true;

Hi Timur

Pause frames are something you can auto-negotiate at the PHY
level. Should you also be clearing some bits in the phydev, so the
peer knows pause frames are not supported?

     Andrew

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/1/17 6:15 PM, Andrew Lunn wrote:
> Pause frames are something you can auto-negotiate at the PHY
> level. Should you also be clearing some bits in the phydev, so the
> peer knows pause frames are not supported?

When pause frame autonegotiation is enabled in the driver, that only 
means that the driver looks at what the PHY has autonegotiated, and then 
configures the MAC to match that.

The driver doesn't touch the PHY at all.  It leaves all that to phylib.

Now if autonegotiation is disabled in the driver, then it just 
hard-codes those TX/RX settings in the driver.  Are you saying I should 
program the PHY at the point to disable autonegotiation on the PHY 
level?  If so, then I don't know how to do that.  I just assumed that 
the MAC never tells the PHY what to do.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Andrew Lunn]

On Tue, Aug 01, 2017 at 07:56:31PM -0500, Timur Tabi wrote:
> On 8/1/17 6:15 PM, Andrew Lunn wrote:
> >Pause frames are something you can auto-negotiate at the PHY
> >level. Should you also be clearing some bits in the phydev, so the
> >peer knows pause frames are not supported?
> 
> When pause frame autonegotiation is enabled in the driver, that only means
> that the driver looks at what the PHY has autonegotiated, and then
> configures the MAC to match that.
> 
> The driver doesn't touch the PHY at all.  It leaves all that to phylib.
> 
> Now if autonegotiation is disabled in the driver, then it just hard-codes
> those TX/RX settings in the driver.  Are you saying I should program the PHY
> at the point to disable autonegotiation on the PHY level?  If so, then I
> don't know how to do that.  I just assumed that the MAC never tells the PHY
> what to do.

Documentation/networking/phy.txt says:

Pause frames / flow control

 The PHY does not participate directly in flow control/pause frames except by
 making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
 MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
 controller supports such a thing. Since flow control/pause frames generation
 involves the Ethernet MAC driver, it is recommended that this driver takes care
 of properly indicating advertisement and support for such features by setting
 the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
 either before or after phy_connect() and/or as a result of implementing the
 ethtool::set_pauseparam feature.

So just check if the MAC driver is setting SUPPORTED_Pause and
SUPPORTED_AsymPause.

	Andrew

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/1/17 9:58 PM, Andrew Lunn wrote:
>   The PHY does not participate directly in flow control/pause frames except by
>   making sure that the SUPPORTED_Pause and SUPPORTED_AsymPause bits are set in
>   MII_ADVERTISE to indicate towards the link partner that the Ethernet MAC
>   controller supports such a thing. Since flow control/pause frames generation
>   involves the Ethernet MAC driver, it is recommended that this driver takes care
>   of properly indicating advertisement and support for such features by setting
>   the SUPPORTED_Pause and SUPPORTED_AsymPause bits accordingly. This can be done
>   either before or after phy_connect() and/or as a result of implementing the
>   ethtool::set_pauseparam feature.
> 
> So just check if the MAC driver is setting SUPPORTED_Pause and
> SUPPORTED_AsymPause.

I think this was covered in this thread: 
http://www.spinics.net/lists/netdev/msg408978.html

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

RE: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Laight]

From: Timur Tabi
> Sent: 01 August 2017 22:38
> The EMAC has a curious qwirk when RX flow control is enabled and the
> kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
> If RX flow control is enabled, the EMAC will then send a non-stop
> stream of pause frames until the system is reset.  The EMAC does not
> have a built-in watchdog.
> 
> In various tests, the pause frame stream sometimes overloads nearby
> switches, effectively disabling the network.
...

If the nearby switches cannot handle pause frames, then the MAC shouldn't
be sending them at all.
They

I suspect that they should only ever be sent if the phy autonegotiation
indicates that they are supported.
You might want to avoid sending them even if allowed, or advertise
non-support on hardware that could support them, but sending them
anyway is likely to cause grief.

This is similar to the problems that arise when the 'speed' is forced
instead of limiting the advertised speed to one value.

	David

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/2/17 8:48 AM, David Laight wrote:
> If the nearby switches cannot handle pause frames, then the MAC shouldn't
> be sending them at all.

There's no way for me to know whether the switches can handle the pause 
frames or not.  You would think that sending one multicast pause frame 
ever 33ms would not overload a switch, but in our lab it does.

This is why I changed the default to disable sending pause frames.

> They
> 
> I suspect that they should only ever be sent if the phy autonegotiation
> indicates that they are supported.

Or if you manually enable them.  That's why I changed to default: the 
MAC is now configured by default to accept pause frames but not send them.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

RE: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Laight]

From: Timur Tabi
> Sent: 02 August 2017 15:22
> On 8/2/17 8:48 AM, David Laight wrote:
> > If the nearby switches cannot handle pause frames, then the MAC shouldn't
> > be sending them at all.
> 
> There's no way for me to know whether the switches can handle the pause
> frames or not.  You would think that sending one multicast pause frame
> ever 33ms would not overload a switch, but in our lab it does.
> 
> This is why I changed the default to disable sending pause frames.
...

Thinks ...
Sending pause frames just tells the adjacent switch not to send you packets
(because you'll discard them).
Since the idea is to avoid the discards, the switch will buffer the
packets it would have sent.
The buffers in the switch then fill up with packets it isn't sending you.
The switch then runs out of buffers, it has 2 choices:
1) Throw the packets away.
2) Send 'pause' frames to the sources.
If it sends 'pause' frames the entire network will very quickly lock up.
If it discards the packets they might as well have been discarded by the
receiving MAC.

Doesn't this mean that pause frames are 99.999% useless??

	David

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 09:51 AM, David Laight wrote:
> Sending pause frames just tells the adjacent switch not to send you packets
> (because you'll discard them).
> Since the idea is to avoid the discards, the switch will buffer the
> packets it would have sent.
> The buffers in the switch then fill up with packets it isn't sending you.

I was under the impression that the switch forwards the pause frames to 
other devices, so that the transmitting NIC can stop sending the data, 
but your explanation makes a lot more sense.  If the EMAC never stops 
sending pause frames, then the switch's buffers will fill up, disabling 
all other devices.  If the switch does not have per-port buffers, then 
it makes sense when the buffer is full, it blocks all ports.

> The switch then runs out of buffers, it has 2 choices:
> 1) Throw the packets away.
> 2) Send 'pause' frames to the sources.
> If it sends 'pause' frames the entire network will very quickly lock up.
> If it discards the packets they might as well have been discarded by the
> receiving MAC.
> 
> Doesn't this mean that pause frames are 99.999% useless??

Pause frames are intended for situations where the receiving CPU is 
temporarily overwhelmed and just needs a second or two to resume 
processing incoming packets.  That makes sense on a dinky single-core 
32-bit CPU.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

RE: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Laight]

From: Timur Tabi
> Sent: 02 August 2017 16:09
> On 08/02/2017 09:51 AM, David Laight wrote:
> > Sending pause frames just tells the adjacent switch not to send you packets
> > (because you'll discard them).
> > Since the idea is to avoid the discards, the switch will buffer the
> > packets it would have sent.
> > The buffers in the switch then fill up with packets it isn't sending you.
> 
> I was under the impression that the switch forwards the pause frames to
> other devices, so that the transmitting NIC can stop sending the data,

Most of my ethernet knowledge predates FDX :-)
It wouldn't make any sense to try to use the source address of a pause
frame to suppress traffic to a specific MAC - that would have to go way down
into IP on all the receiving systems.
Also ISTR that pause frames are very short and don't even contain MAC
addresses.

> but your explanation makes a lot more sense.  If the EMAC never stops
> sending pause frames, then the switch's buffers will fill up, disabling
> all other devices.  If the switch does not have per-port buffers, then
> it makes sense when the buffer is full, it blocks all ports.

A switch will (probably) either have buffers for each input port, or for
each output port (or maybe both).
Output port buffers are less likely to cause grief when an output port is
running at a slower speed than the input port.

But is a switch is going to send pause frames it doesn't really matter
how the buffers are arranged. The cascade will still happen.

It would take very careful heuristics in a switch to manage pause
frames properly.

Of course, once the kernel has crashed, even multicast packets will
eventually run the MAC out of buffers.
(Unless you run on private network and manage to reinstall and reboot
while the MAC is still active and then eventually die when a receive
frame overwrites what used to be a receive buffer.)

	David

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Tue,  1 Aug 2017 16:37:39 -0500

> The EMAC has a curious qwirk when RX flow control is enabled and the
> kernel hangs.  With the kernel hung, the EMAC's RX queue soon fills.
> If RX flow control is enabled, the EMAC will then send a non-stop
> stream of pause frames until the system is reset.  The EMAC does not
> have a built-in watchdog.
> 
> In various tests, the pause frame stream sometimes overloads nearby
> switches, effectively disabling the network.  Since the RX queue is
> large and the host processor is more than capable of handling incoming
> packets quickly, the only time the EMAC will send any pause frames is
> when the kernel is hung and unrecoverable.
> 
> To avoid all these problems, we disable flow control autonegotiation
> by default, and only enable receiving pause frames.
> 
> Cc: stable@vger.kernel.org # 4.11.x
> Signed-off-by: Timur Tabi <timur@codeaurora.org>

I've thought about this a lot and I don't like it for many reasons.

First of all, this hung kernel scenerio is completely bogus.

The ethernet chip may not have a proper watchdog for the pause frame
generation logic, but the whole kernel sure as hell does.

And if this kind of thing matters to the user, they will have a
software or hardware watchdog driver enabled to break out of this
situation.

Turning off flow control by default has so many negative ramifications
and don't try to convince me that users will be "aware" of this and
turn it back on.

They largely won't.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 12:54 PM, David Miller wrote:
> And if this kind of thing matters to the user, they will have a
> software or hardware watchdog driver enabled to break out of this
> situation.

The problem is that the user is not going to expect that the EMAC can 
disable the nearby switch(es) when the kernel is hung and not rebooted 
quickly enough.  Internally, this bug/feature has caused quite a bit of 
mayhem, so the problem is real.  No cares about enabling flow control -- 
it just happens to be enabled on some systems where the switch agrees to 
it.  So random individuals can't debug the hardware because suddenly the 
EMAC has gone haywire and disabled the local network.

> Turning off flow control by default has so many negative ramifications
> and don't try to convince me that users will be "aware" of this and
> turn it back on.

What are the negative ramifications?  It's practically impossible to 
overload the chip such that it can't process the incoming packets fast 
enough.  I don't know of any real-world situation where the EMAC needs 
to transmit pause frames.

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Wed, 2 Aug 2017 13:23:18 -0500

> On 08/02/2017 12:54 PM, David Miller wrote:
>> And if this kind of thing matters to the user, they will have a
>> software or hardware watchdog driver enabled to break out of this
>> situation.
> 
> The problem is that the user is not going to expect that the EMAC can
> disable the nearby switch(es) when the kernel is hung and not rebooted
> quickly enough.  Internally, this bug/feature has caused quite a bit
> of mayhem, so the problem is real.  No cares about enabling flow
> control -- it just happens to be enabled on some systems where the
> switch agrees to it.  So random individuals can't debug the hardware
> because suddenly the EMAC has gone haywire and disabled the local
> network.

Again, any serious installation will have a system watchdog enabled
which will break the pause frame bomb.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Wed, 2 Aug 2017 13:23:18 -0500

> It's practically impossible to overload the chip such that it can't
> process the incoming packets fast enough.  I don't know of any
> real-world situation where the EMAC needs to transmit pause frames.

Slow cpus or very expensive stack operations can cause this, it's not
a property of the EMAC chip at all.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 08/02/2017 01:35 PM, David Miller wrote:
> Again, any serious installation will have a system watchdog enabled
> which will break the pause frame bomb.

Oh well.  I guess I'll have to carry this patch internally.

What about patch #2?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[David Miller]

From: Timur Tabi <timur@codeaurora.org>
Date: Wed, 2 Aug 2017 13:39:34 -0500

> On 08/02/2017 01:35 PM, David Miller wrote:
>> Again, any serious installation will have a system watchdog enabled
>> which will break the pause frame bomb.
> 
> Oh well.  I guess I'll have to carry this patch internally.

So you don't want to run a proper watchdog on your systems?
You want them to just hang there and wait for someone to
notice instead?

> What about patch #2?

No real objection.

Re: [PATCH 1/2] [for 4.13] net: qcom/emac: disable flow control autonegotiation by default

[Timur Tabi]

On 8/2/17 6:15 PM, David Miller wrote:

> So you don't want to run a proper watchdog on your systems?
> You want them to just hang there and wait for someone to
> notice instead?

This is for internal development.  We noticed the problem first during 
debugging, when we would halt a core for more than a couple minutes. 
Then the entire lab network would go down.

-- 
Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the
Code Aurora Forum, hosted by The Linux Foundation.

Re: [PATCH 2/2] net: qcom/emac: add software control for pause frame mode

[Timur Tabi]

On 08/01/2017 04:37 PM, Timur Tabi wrote:
> The EMAC has the option of sending only a single pause frame when
> flow control is enabled and the RX queue is full.  Although sending
> only one pause frame has little value, this would allow admins to
> enable automatic flow control without having to worry about the EMAC
> flooding nearby switches with pause frames if the kernel hangs.
> 
> The option is enabled by using the single-pause-mode private flag.
> 
> Signed-off-by: Timur Tabi<timur@codeaurora.org>

Dave,

I don't see this patch in net-next.  Can you pick it up for 4.14?

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc.  Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.