Re: [PATCH iwl-net v1] igc: Fix infinite initialization loop with early XDP redirect

[Vinicius Costa Gomes]

Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> 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@xxxxxxxxxxxx>
>> Closes: https://lore.kernel.org/netdev/0caf33cf6adb3a5bf137eeaa20e89b167c9986d5.camel@xxxxxxxxxxxx/
>> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@xxxxxxxxx>
>> Tested-by: Ferenc Fejes <ferenc.fejes@xxxxxxxxxxxx>
>> ---
>>  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@xxxxxxxxx>
>
>>  
>>  	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>> -- 
>> 2.41.0
>> 


Cheers,
-- 
Vinicius