On Tue, Jun 22, 2021 at 04:49:51PM +0900, Benjamin Poirier wrote:

On 2021-06-21 21:48 +0800, Coiby Xu wrote:

quoted

According to the TODO item,

quoted

* the flow control implementation in firmware is buggy (sends a flood of pause
  frames, resets the link, device and driver buffer queues become
  desynchronized), disable it by default

Currently, qlge_mpi_port_cfg_work calls qlge_mb_get_port_cfg which gets
the link config from the firmware and saves it to qdev->link_config. By
default, flow control is enabled. This commit writes the
save the pause parameter of qdev->link_config and don't let it
overwritten by link settings of current port. Since qdev->link_config=0
when qdev is initialized, this could disable flow control by default and
the pause parameter value could also survive MPI resetting,
    $ ethtool -a enp94s0f0
    Pause parameters for enp94s0f0:
    Autonegotiate:  off
    RX:             off
    TX:             off

The follow control can be enabled manually,

    $ ethtool -A enp94s0f0 rx on tx on
    $ ethtool -a enp94s0f0
    Pause parameters for enp94s0f0:
    Autonegotiate:  off
    RX:             on
    TX:             on

Signed-off-by: Coiby Xu <redacted>
---
 drivers/staging/qlge/TODO       |  3 ---
 drivers/staging/qlge/qlge_mpi.c | 11 ++++++++++-
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/qlge/TODO b/drivers/staging/qlge/TODO
index b7a60425fcd2..8c84160b5993 100644
--- a/drivers/staging/qlge/TODO
+++ b/drivers/staging/qlge/TODO

@@ -4,9 +4,6 @@
   ql_build_rx_skb(). That function is now used exclusively to handle packets
   that underwent header splitting but it still contains code to handle non
   split cases.
-* the flow control implementation in firmware is buggy (sends a flood of pause
-  frames, resets the link, device and driver buffer queues become
-  desynchronized), disable it by default
 * some structures are initialized redundantly (ex. memset 0 after
   alloc_etherdev())
 * the driver has a habit of using runtime checks where compile time checks are

diff --git a/drivers/staging/qlge/qlge_mpi.c b/drivers/staging/qlge/qlge_mpi.c
index 2630ebf50341..0f1c7da80413 100644
--- a/drivers/staging/qlge/qlge_mpi.c
+++ b/drivers/staging/qlge/qlge_mpi.c

@@ -806,6 +806,7 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
 {
 	struct mbox_params mbc;
 	struct mbox_params *mbcp = &mbc;
+	u32 saved_pause_link_config = 0;

Initialization is not needed given the code below, 

Thanks for the spotting this issue!

in fact the
declaration can be moved to the block below.

I thought I need to put the declaration in the beginning of the
function. But it seems Linux kernel coding style doesn't require it.
I'll move it to the else block below then.

quoted

 	int status = 0;

 	memset(mbcp, 0, sizeof(struct mbox_params));

@@ -826,7 +827,15 @@ int qlge_mb_get_port_cfg(struct qlge_adapter *qdev)
 	} else	{
 		netif_printk(qdev, drv, KERN_DEBUG, qdev->ndev,
 			     "Passed Get Port Configuration.\n");
-		qdev->link_config = mbcp->mbox_out[1];
+		/*
+		 * Don't let the pause parameter be overwritten by
+		 *
+		 * In this way, follow control can be disabled by default
+		 * and the setting could also survive the MPI reset
+		 */

It seems this comment is incomplete. Also, it's "flow control", not
"follow control".

Ah, yes. I should state it as "Don't let the pause parameter be 
overwritten by be overwritten by the firmware.". And thanks for
correcting the typo.

quoted

+		saved_pause_link_config = qdev->link_config & CFG_PAUSE_STD;
+		qdev->link_config = ~CFG_PAUSE_STD & mbcp->mbox_out[1];
+		qdev->link_config |= saved_pause_link_config;
 		qdev->max_frame_size = mbcp->mbox_out[2];
 	}
 	return status;
--
2.32.0

-- 
Best regards,
Coiby