Re: [PATCH net v2] eth: fbnic: Keep real queue counts synchronized

[Björn Töpel]

netdev-bot+sashiko@xxxxxxxxxx writes:

> Thank you for your contribution! Sashiko AI review found 1 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 0
>
> If you believe none of the issues are legitimate, please reply to each
> issue of Medium severity or higher and explain why the AI is wrong.
> If any of them are legitimate and you plan to respin, please let patchwork
> know by including "pw-bot: cr" as a separate line at the end of your reply
> (one such reply per series is enough).
>
> - [High] The patch adds a hard `-ENOSPC` exit to `__fbnic_pm_resume()`
>   (drivers/net/ethernet/meta/fbnic/fbnic_pci.c:523-529).
>
> --
>
> eth: fbnic: Keep real queue counts synchronized
>
> The patch publishes the default channel counts, and the counts set while
> the device is down, to the core with netif_set_real_num_queues() right
> away. It adds num_napi_cfg so the configured NAPI count survives suspend.
> Resume now fails with -ENOSPC if fewer vectors come back than that layout
> needs.
>
> Link: https://lore.kernel.org/netdev/178915061000.219967.7726187707862333281@xxxxxxxxxx/
>
>> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>> index c6698e3002a13..48279a5214c8a 100644
>> --- a/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_pci.c
>
> [ ... ]
>
>> @@ -519,8 +520,16 @@ static int __fbnic_pm_resume(struct device *dev)
>>  
>>  	fbn = netdev_priv(netdev);
>>  
>> -	/* Reset the queues if needed */
>> -	fbnic_reset_queues(fbn, fbn->num_tx_queues, fbn->num_rx_queues);
>> +	max_napis = fbd->num_irqs - FBNIC_NON_NAPI_VECTORS;
>> +	if (fbn->num_napi_cfg > max_napis) {
>> +		netdev_err(netdev,
>> +			   "Unable to restore channel configuration: %u NAPI vectors required, only %u available\n",
>> +			   fbn->num_napi_cfg, max_napis);
>> +		err = -ENOSPC;
>> +		goto unlock;
>> +	}
>
> [Severity: High]
> Can this leave the NIC unusable after a resume or an AER slot reset that
> used to recover?

Thanks, this is valid. The resume path limits the IRQ request using the
online CPU count, then rejects the resulting allocation against the
saved configuration. It can therefore fail without ever asking PCI for
the required number of vectors. The administratively-down and AER cases
are valid as well.

I'll rework the IRQ allocation and resume handling.

pw-bot: cr