| Message ID | 20230905213753.697461-1-vinicius.gomes@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [iwl-net,v1] igc: Fix infinite initialization loop with early XDP redirect | expand |
On Tue, Sep 05, 2023 at 02:37:52PM -0700, Vinicius Costa Gomes wrote: > When a XDP redirect happens before the link is ready, that When exactly link was 'ready' in your setup? You said it was enough to launch traffic towards igc iface before running xdp-bench. Was the iface down or up or? > transmission will not finish and will timeout, causing an adapter > reset. If the redirects do not stop, the adapter will not stop > resetting. Please highlight that this driver shares tx resources with netstack. I believe the source of this bug is that the watchdog is responsible to call netif_carrier_on() from a workqueue which happens to be scheduled *after* clearing __IGC_DOWN in igc_up(). > > Wait for the driver to signal that there's a carrier before allowing > transmissions to proceed. > > Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action") > Reported-by: Ferenc Fejes <ferenc.fejes@ericsson.com> > Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@ericsson.com/ > Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> > Tested-by: Ferenc Fejes <ferenc.fejes@ericsson.com> > --- > drivers/net/ethernet/intel/igc/igc_main.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c > index 293b45717683..98de34d0ce07 100644 > --- a/drivers/net/ethernet/intel/igc/igc_main.c > +++ b/drivers/net/ethernet/intel/igc/igc_main.c > @@ -6491,7 +6491,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames, > struct igc_ring *ring; > int i, drops; > > - if (unlikely(test_bit(__IGC_DOWN, &adapter->state))) > + if (unlikely(!netif_carrier_ok(dev))) > return -ENETDOWN; I thought about keeping the bit check as well but given what i wrote above it is probably redundant, so: Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) > -- > 2.41.0 >
Maciej Fijalkowski <maciej.fijalkowski@intel.com> writes: > On Tue, Sep 05, 2023 at 02:37:52PM -0700, Vinicius Costa Gomes wrote: >> When a XDP redirect happens before the link is ready, that > > When exactly link was 'ready' in your setup? You said it was enough to > launch traffic towards igc iface before running xdp-bench. Was the iface > down or up or? In short, the interface was up and it was brought down "externally". I should have explained my test better: A is the system under test, B is the monitor; 1. initially the link between systems A and B is up; 2. I setup the vlans and xdp-bench; 3. I start sending traffic; 4. on system B, I brind the NIC connected to A down; 5. infinite initialization loop. > >> transmission will not finish and will timeout, causing an adapter >> reset. If the redirects do not stop, the adapter will not stop >> resetting. > > Please highlight that this driver shares tx resources with netstack. I > believe the source of this bug is that the watchdog is responsible to call > netif_carrier_on() from a workqueue which happens to be scheduled *after* > clearing __IGC_DOWN in igc_up(). > Sure, will add this information to the commit message and send a v2. >> >> Wait for the driver to signal that there's a carrier before allowing >> transmissions to proceed. >> >> Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action") >> Reported-by: Ferenc Fejes <ferenc.fejes@ericsson.com> >> Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@ericsson.com/ >> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> >> Tested-by: Ferenc Fejes <ferenc.fejes@ericsson.com> >> --- >> drivers/net/ethernet/intel/igc/igc_main.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c >> index 293b45717683..98de34d0ce07 100644 >> --- a/drivers/net/ethernet/intel/igc/igc_main.c >> +++ b/drivers/net/ethernet/intel/igc/igc_main.c >> @@ -6491,7 +6491,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames, >> struct igc_ring *ring; >> int i, drops; >> >> - if (unlikely(test_bit(__IGC_DOWN, &adapter->state))) >> + if (unlikely(!netif_carrier_ok(dev))) >> return -ENETDOWN; > > I thought about keeping the bit check as well but given what i wrote above > it is probably redundant, so: > > Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > >> >> if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK)) >> -- >> 2.41.0 >> Cheers,
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c index 293b45717683..98de34d0ce07 100644 --- a/drivers/net/ethernet/intel/igc/igc_main.c +++ b/drivers/net/ethernet/intel/igc/igc_main.c @@ -6491,7 +6491,7 @@ static int igc_xdp_xmit(struct net_device *dev, int num_frames, struct igc_ring *ring; int i, drops; - if (unlikely(test_bit(__IGC_DOWN, &adapter->state))) + if (unlikely(!netif_carrier_ok(dev))) return -ENETDOWN; if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
When a XDP redirect happens before the link is ready, that transmission will not finish and will timeout, causing an adapter reset. If the redirects do not stop, the adapter will not stop resetting. Wait for the driver to signal that there's a carrier before allowing transmissions to proceed. Fixes: 4ff320361092 ("igc: Add support for XDP_REDIRECT action") Reported-by: Ferenc Fejes <ferenc.fejes@ericsson.com> Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@ericsson.com/ Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> Tested-by: Ferenc Fejes <ferenc.fejes@ericsson.com> --- drivers/net/ethernet/intel/igc/igc_main.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)