On Tue, Jun 22, 2021 at 04:49:51PM +0900, Benjamin Poirier wrote:
On 2021-06-21 21:48 +0800, Coiby Xu wrote:According to the TODO item, > * 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 <coiby.xu@xxxxxxxxx> --- 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.
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.
+ 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
