Re: [PATCH net-next v1 4/5] net: lan743x: reset the device on TX timeout

[Chris Babroski <cbabroski@nvidia.com>]

> >  static int lan743x_dmac_init(struct lan743x_adapter *adapter)
> >  {
> >  	u32 data = 0;
> > +	int ret;
> >  
> >  	lan743x_csr_write(adapter, DMAC_CMD, DMAC_CMD_SWR_);
> > -	lan743x_csr_wait_for_bit(adapter, DMAC_CMD, DMAC_CMD_SWR_,
> > -				 0, 1000, 20000, 100);
> > +	ret = lan743x_csr_wait_for_bit(adapter, DMAC_CMD, DMAC_CMD_SWR_,
> > +				       0, 1000, 20000, 100);
> > +	if (ret)
> > +		return ret;
> 
> [Severity: Medium]
> The commit message only talks about probe here. However,
> lan743x_hardware_init() is also called from lan743x_pm_resume().
> 
> Before this change lan743x_hardware_init() could not fail in practice,
> so the resume error branch was dead code. Does this new return make that
> branch reachable?
> 
> lan743x_pm_resume() {
>     ...
> 	ret = lan743x_hardware_init(adapter, pdev);
> 	if (ret) {
> 		netif_err(adapter, probe, adapter->netdev,
> 			  "lan743x_hardware_init returned %d\n", ret);
> 		lan743x_pci_cleanup(adapter);
> 		return ret;
> 	}
>     ...
> }
> 
> If that branch runs, the netdev stays registered and detached. A later
> unbind then runs:
> 
> lan743x_pcidev_remove()
>   lan743x_full_cleanup()
>     lan743x_hardware_cleanup()   <- INT_EN_CLR write to a disabled device
>     lan743x_pci_cleanup()        <- second region release and disable
> 
> Would that release the PCI regions twice and trigger the "disabling
> already-disabled device" warning?

Yes, and this issue can be resolved by removing the call to
lan743x_pci_cleanup() on lan743x_hardware_init() error and continuing to
return early from resume, leaving the device in a detached state.
lan743x_pci_cleanup() would then only be called in remove which matches
how lan743x_pci_init() is called in probe and not resume.

Will fix this as part of v2.

> > +static void lan743x_reset_work(struct work_struct *work)
> > +{
> > +	struct lan743x_adapter *adapter =
> > +		container_of(work, struct lan743x_adapter, reset_work);
> > +	struct net_device *netdev = adapter->netdev;
> > +	int ret;
> > +
> > +	rtnl_lock();
> > +
> > +	if (!netif_running(netdev) || !netif_device_present(netdev))
> > +		goto unlock;
> > +
> > +	netdev_warn(netdev, "resetting after TX timeout\n");
> > +
> > +	netif_device_detach(netdev);
> > +	lan743x_netdev_close(netdev);
> 
> [Severity: Medium]
> Here lan743x_netdev_close() runs on a controller that is already assumed
> to be wedged. The controller-wide DMAC_CMD_SWR_ reset only happens later,
> in lan743x_dmac_init().
> 
> In lan743x_tx_close(), suppose the channel does not stop and the
> per-channel reset also times out. The error is only logged, and the ring
> is freed anyway:
> 
> lan743x_tx_close() {
>     ...
> 		ret = lan743x_dmac_tx_reset(adapter, tx->channel_number);
> 		if (ret)
> 			netdev_err(adapter->netdev,
> 				   "TX channel %d reset timed out\n",
> 				   tx->channel_number);
>     ...
> 	lan743x_tx_release_all_descriptors(tx);
>     ...
> 	lan743x_tx_ring_cleanup(tx);
> }
> 
> lan743x_rx_close() does the same for the RX ring, the head write-back
> buffer and the RX skbs.

In another thread we discussed removing the call to
lan743x_dmac_tx_reset() inside lan743x_tx_close() as part of v2 because
recovery happens in the following lan743x_tx_open(), but this still does
not solve the issue flagged here that the descriptor ring is freed while
the DMAC is still started.

Woojung already clarified that TX reset should occur after FIFO disable,
so we could fix this by adding a conditional "stop_failed" flag that is
used to determine if lan743x_dmac_tx_reset() should be called after
lan743x_tx_fifo_disable().

The conditional would make it so that we don't have to re-order the
lan743x_tx_close() flow (which has higher change to introduce
regressions on a common code path).

I can add as part of v2.

> [Severity: Medium]
> This is a pre-existing issue, not one introduced by this patch. The same
> close/open sequence leaves the TX one-step state inconsistent, and with
> this patch the watchdog can trigger that sequence automatically.
> 
> lan743x_ptp_open() calls lan743x_ptp_reset() and sets the RX filter back
> to HWTSTAMP_FILTER_NONE. However, tx->ts_flags still has
> TX_TS_FLAG_ONE_STEP_SYNC, and nothing turns one-step Sync insertion back
> on in the hardware.
> 
> After the reset, lan743x_ptp_hwtstamp_get() still reports:
> 
> 	if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
> 		config->tx_type = HWTSTAMP_TX_ONESTEP_SYNC;
> 
> and lan743x_tx_xmit_frame() still sets ignore_sync for Sync frames:
> 
> 		if (tx->ts_flags & TX_TS_FLAG_ONE_STEP_SYNC)
> 			ignore_sync = true;
> 
> Can Sync frames go out without a hardware-inserted origin timestamp until
> the application sets up timestamping again?
> 
> Admin down/up and suspend/resume already have the same mismatch. The
> commit message also documents it ("one-step sync insertion is disabled,
> while the driver keeps its TX timestamping flags"). The PHC is
> unregistered and registered again, so applications have to reconfigure.
> Reconfiguring applies one-step mode again through
> lan743x_ptp_hwtstamp_set().

As mentioned by Sashiko, this is a pre-existing issue and already the
behavior that happens with admin down/up and suspend/resume. I was not
planning to change this behavior as part of this patch series.

We have a discussion going in a separate thread for this patch about how
PTP recovery should be handled across reset (if at all as part of this
series).

This is an issue that could get worse if we register the PHC at probe,
as discussed in the other thread, so ptp4l is no longer forced to
restart. It would then require restoring the hwtstamp config in the
driver after reset.

> > +
> > +	ret = lan743x_dmac_init(adapter);
> > +	if (ret) {
> > +		netdev_err(netdev, "DMA controller reset failed: %d\n", ret);
> > +		goto close;
> > +	}
> 
> [Severity: Medium]
> According to the commit message, lan743x_dmac_init() now returns the poll
> error so the work does not reopen a controller whose reset timed out. It
> also says the interface is taken down "so it can be brought back up
> later".
> 
> lan743x_netdev_open() never calls lan743x_dmac_init(). That function is
> only called from lan743x_hardware_init() (probe and resume) and from this
> work.
> 
> So if the user runs "ip link set up" after this failure, does
> lan743x_netdev_open() reopen the same controller that never came out of
> reset?
> 
> The early return in lan743x_dmac_init() also skips the DMAC_CFG,
> DMAC_COAL_CFG and DMAC_OBFF_CFG writes. Nothing on the open path writes
> them again.
> 
> Should the open path retry the DMAC reset, or should the device be kept
> from coming back up in this case?

I think adding DMAC reset to the open path increases regression risk on
a common code path, but the alternative of keeping the device detached
after lan743x_dmac_init() failure would work to prevent future "ip link
set up" from opening the device after a failed reset.

In v2 instead of "goto close" which includes device attach we could call
dev_close() without netif_device_attach() in this error case and leave
the device detached.

> > +
> > +	ret = lan743x_netdev_open(netdev);
> > +	if (ret) {
> > +		netdev_err(netdev, "failed to reopen after reset: %d\n", ret);
> > +		goto close;
> > +	}
> 
> [Severity: Medium]
> Some boards use the fixed-link fallback in lan743x_phylink_connect():
> LAN7431 or PCI11x1x with no DT node and no PHY on the MDIO bus. Can this
> reopen ever succeed on those boards?
> ... 
> The fixed-link handling is existing code. On these boards, though, the
> new recovery turns a stalled TX queue into an interface that cannot come
> back up until the driver is probed again.

This same issue was flagged in another thread as well and I wasn't
planning to make changes because they seemed out of scope.

It sounds like the fix would not be complicated though. I can attempt to
fix this as part of v2 and then may need some help testing on HW that
uses fixed-link mode.

> > +
> > +	goto attach;
> > +
> > +close:
> > +	dev_close(netdev);
> > +attach:
> > +	netif_device_attach(netdev);
> 
> [Severity: Medium]
> While the device is detached under rtnl here, multicast list writers that
> do not take rtnl can still change dev->mc. The IPv6 socket join path is
> one example:
> 
> ipv6_sock_mc_join()
>   igmp6_group_added()
>     dev_mc_add()
>       __dev_set_rx_mode()
> 	if (!netif_device_present(dev))
> 		return;
> 
> These changes never reach the hardware. After netif_device_attach(),
> nothing reprograms the filter. lan743x_rfe_open() only programs RSS, and
> lan743x_rfe_set_multicast() is only called from ndo_set_rx_mode.
> 
> The normal open path resyncs because __dev_open() calls dev_set_rx_mode()
> after ndo_open. This path has nothing equivalent.
> 
> Can the RFE filter get out of sync with dev->mc? Groups joined during the
> reset would not be received, and groups left would stay in the filter,
> until the next rx_mode change. Would a call to dev_set_rx_mode() after
> the reopen fix this?

It sounds like this issue can also already happen in resume when
lan743x_netdev_open() and netif_device_attach() are called.

If I understand correctly, it could potentially be fixed by calling
lan743x_rfe_set_multicast() after netif_device_attach() in the reset
and resume paths, but dev_set_rx_mode() cannot be called directly.

I can add this to v2 and keep or remove based on review feedback.

Thanks,
Chris

pw-bot: cr