Re: [Issue report] drivers/ftgmac100: DHCP occasionally fails during boot up or link down/up

On Tue, Feb 15, 2022 at 02:38:51PM +0800, Heyi Guo wrote:

Hi,

We are using Aspeed 2600 and found DHCP occasionally fails during boot up or
link down/up. The DHCP client is systemd 247.6 networkd. Our network device
is 2600 MAC4 connected to a RGMII PHY module.

Current investigation shows the first DHCP discovery packet sent by
systemd-networkd might be corrupted, and sysmtemd-networkd will continue to
send DHCP discovery packets with the same XID, but no other packets, as
there is no IP obtained at the moment. However the server side will not
respond with this serial of DHCP requests, until it receives some other
packets. This situation can be recovered by another link down/up, or a "ping
-I eth0 xxx.xxx.xxx.xxx" command to insert some other TX packets.

Navigating the driver code ftgmac.c, I've some question about the work flow
from link down to link up. I think the flow is as below:

1. ftgmac100_open() will enable net interface with ftgmac100_init_all(), and
then call phy_start()

2. When PHY is link up, it will call netif_carrier_on() and then adjust_link
interface, which is ftgmac100_adjust_link() for ftgmac100

The order there is questionable. Maybe it should first call the adjust
link callback, and then the netif_carrier_on(). However...

3. In ftgmac100_adjust_link(), it will schedule the reset work
(ftgmac100_reset_task)

4. ftgmac100_reset_task() will then reset the MAC

Because of this delayed reset, changing the order will not help this
driver.

I found networkd will start to send DHCP request immediately after
netif_carrier_on() called in step 2, but step 4 will reset the MAC, which
may potentially corrupt the sending packet.

What is not clear to my is why it is scheduling the work rather than
just doing it. At least for adjust_link, it is in a context it can
sleep. ftgmac100_set_ringparam() should also be able to
sleep. ftgmac100_interrupt() cannot sleep, so it does need to schedule
work.

I would suggest you refactor ftgmac100_reset_task() into a function
that actually does the reset, and a wrapper which takes a
work_struct. adjust_link can then directly do the reset, which
probably solves your problem.

	 Andrew