Re: [PATCH net-next v1 2/5] net: lan743x: start phylink last and stop it first

[Chris Babroski <cbabroski@nvidia.com>]

> > @@ -3334,6 +3334,8 @@ static int lan743x_netdev_close(struct net_device *netdev)
> >  	struct lan743x_adapter *adapter = netdev_priv(netdev);
> >  	int index;
> >  
> > +	lan743x_phylink_disconnect(adapter);
> > +
> >  	for (index = 0; index < adapter->used_tx_channels; index++)
> >  		lan743x_tx_close(&adapter->tx[index]);
>
> [Severity: High]
> Can a TX queue get woken again after this point and stay awake while the
> rings are freed?
> ...
> This looks to be fixed later in the series by "net: lan743x: quiesce TX
> before freeing the rings". That commit adds a netif_device_present() check
> before the NAPI wake, and adds synchronize_net() plus netif_tx_disable()
> after lan743x_phylink_disconnect() in lan743x_netdev_close().

As mentioned by Sashiko, this issue is resolved in patch 3/5. For v2 I
will rearrange the commit order so that patch 3/5 comes before this one.

> [Severity: High]
> Can a transmit that is already in flight also race with the ring
> teardown here?
> ...
> This also appears to be fixed later in the series by "net: lan743x:
> quiesce TX before freeing the rings", which adds synchronize_net() and
> netif_tx_disable() right after lan743x_phylink_disconnect().
> netif_tx_disable() takes each txq lock, so it waits for in-flight
> transmits.

Same as above.

> > @@ -3387,6 +3383,10 @@ static int lan743x_netdev_open(struct net_device *netdev)
> >  			goto close_tx;
> >  	}
> >  
> > +	ret = lan743x_phylink_connect(adapter);
> > +	if (ret)
> > +		goto close_tx;
> > +
>
> [Severity: High]
> This is a pre-existing issue, but does every open after the first one
> fail on LAN7431/PCI11x1x boards that have no DT node and no PHY on the
> MDIO bus?

This is a pre-existing issue and doesn't occur on the HW I have
available to test with so I'm hesitant to change as part of this patch
series. It feels out of scope, but if the maintainers can provide an
appropriate fix and want it included in this patch series I can make the
change as part of a later revision.

> [Severity: High]
> This isn't a bug introduced by this patch, but what happens on resume
> when this goto close_tx is taken?
> ...
> A transmit path that bypasses the qdisc and does not check carrier, such
> as netpoll, could also reach lan743x_tx_xmit_frame() while ring_cpu_ptr
> and buffer_info are NULL.
> 
> Together with the fixed-link -EINVAL above, every resume on fixed-link
> boards without a DT node would hit this. It still seems to be present at
> the end of the series. The lan743x_netdev_stop() wrapper added later only
> skips close when !netif_device_present(), and resume has already
> re-attached the device by then.

Same as above.

Thanks,
Chris