Re: [PATCH net 2/2] net: ngbe: clear DRV_LOAD bit when ngbe_open() fails

Hi,

Thanks for the review. Point by point:

[Medium] the unwind frees DMA memory the hardware is still programmed
to use / DRV_LOAD clearing order

The hygiene point is fair: wx_configure() has already enabled the Rx
queues (WX_PX_RR_CFG_RR_EN), programmed the ring base addresses and
the ISB address when these failure paths run, so the current unwind
releases DMA memory the hardware is still programmed to use, and
wx_control_hw(wx, false) hands the port back to the management
firmware with the Rx unit still enabled -- a different ordering from
ngbe_close(), which quiesces first.

On the practical question: in this window nothing writes into the
freed memory. phylink_start() and ngbe_irq_enable() both live in
ngbe_up_complete(), which is never reached on the failure paths, so
the link stays down and the hardware interrupt sources are still
masked; no frames arrive and no descriptor or ISB writeback lands in
the freed coherent memory. Still, the unwind should not rely on that.

I also looked at routing the error paths through ngbe_down(), and it
does not work as-is: ngbe_disable_device() begins with
test_and_set_bit(WX_STATE_DOWN, wx->state), and on an open failure
that bit is already set (the suspend-time ngbe_close() set it, and
only ngbe_up_complete() -- never reached -- clears it), so the whole
register quiesce (wx_disable_rx_queue()/wx_disable_rx()/
wx_irq_disable()/TX SWFLSH) would be skipped; what is left of
ngbe_down() does not stop the Rx unit either (ngbe_reset() is only a
software MAC-table/PTP tidy-up). In addition, phylink_stop()
unconditionally calls phy_stop(), which WARNs on a PHY that is
attached but never started (PHY_READY). So a correct fix needs a
guard-free quiesce or a per-path sequencing; I will follow up with a
separate patch rather than respinning this series.

[High, pre-existing] failed open leaves netif_running() true with
resources freed / WX_STATE_RES_FREED

Acknowledged. As noted in my 1/2 reply, that bit is only set through
wx->down_suspend(), which is only called from the PCI error-recovery
path, so ngbe_close()'s guard does not protect this scenario at all
today. Pre-existing; I plan to address it together with the
failed-open unwind.

[High, pre-existing] ngbe_setup_tc() discards errors

Acknowledged -- both the wx_init_interrupt_scheme() and ngbe_open()
results are dropped and 0 is returned to wx_set_channels(). Agree it
should propagate; separate patch.

[Low] unlocked wr32m() on WX_CFG_PORT_CTL

Agreed with your own conclusion: the same unlocked update already
exists on every ifup, in ngbe_close() and in ngbe_dev_shutdown(), so
this adds no new race class. Serializing that register would be a
separate libwx cleanup.

Thanks,
Zhang Yunfei