On Thu, Feb 25, 2016 at 01:32:33AM +0000, Lu, Wenzhuo wrote:

Hi Bruce,

quoted

-----Original Message-----
From: Richardson, Bruce
Sent: Wednesday, February 24, 2016 10:26 PM
To: He, Shaopeng
Cc: Lu, Wenzhuo; dev@dpdk.org
Subject: Re: [dpdk-dev] [PATCH v2] ixgbe: fix link down issue on x550em_x

On Thu, Feb 04, 2016 at 06:21:04AM +0000, He, Shaopeng wrote:

quoted

quoted

-----Original Message-----
From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Wenzhuo Lu
Sent: Monday, February 01, 2016 4:43 PM
To: dev@dpdk.org
Subject: [dpdk-dev] [PATCH v2] ixgbe: fix link down issue on
x550em_x

Normally the auto-negotiation is supported by FW. But on
X550EM_X_10G_T it's not supported by FW. As the port of
X550EM_X_10G_T is 10G. If we connect the port with a peer which is
1G. The link is always down.
We have to supprted auto-neg by SW to avoid such link down issue.

Signed-off-by: Wenzhuo Lu <redacted>

Acked-by: Shaopeng He <redacted>

I'm a bit confused regarding the commit message and the code in the patch.
The commit message talks about enabling speed auto-negotiation, while the
code never refers to any such thing. Instead all we have are settings for
manipulating interrupt masks to enable PHY interrupts. I think some additional
information is needed to connect A and B together here.

The reason is that the handler of the link speed auto-neg is already in the base code. It's ixgbe_handle_lasi.
What we need is a trigger. When the advertised link speed changes, a PHY interruption will be triggered. So, we should handle this interruption and call ixgbe_handle_lasi to set the link speed correct.
Let me add more explanation in the commit log.

Yes, please do. What you have just explained makes much more sense and should
be included in the log.

quoted

A second, more minor nit is that the commit title never refers to link auto-
negotiation, but refers to this as a bug fix - which is also correct. If this is
primarily a bug fix, please include a fixes line if possible, but please also refer to
auto-neg in the title if possible too.

I didn't add a fixes line because it doesn't fix an issue introduced by SW, or even FW, HW. As said in commit log, we hit a link down issue, but the root cause is the link speed auto-neg is not supported on this specific NIC.
On the other NICs, link speed auto-neg is supported by FW, SW need no do anything. So we didn't implement the link speed auto-neg. But we have to implement this feature on this NIC for FW doesn't implement it.
Should I change the tittle to "support link speed auto-neg on x550em_x"?

Yes, that is probably better.

Thanks,
/Bruce